Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[Reporting] Add deprecation messages for roles mapped to reporting_user #115125

Merged

Conversation

tsullivan
Copy link
Member

@tsullivan tsullivan commented Oct 15, 2021

Closes #115096

Follows: #100427, #94966

Summary

  1. This fixes the deprecations for various edge cases:
  • when security was disabled, users saw a meaningless error message
  • when users did not have manage_security privilege they saw a meaningless error message
  1. Adds support for checking deprecations related to xpack.reporting.roles.allow

Screenshots

  • Listing of Reporting deprecations. The highlighted ones are affected in this PR.
    image
  • Details for users found with deprecated reporting roles:
    image
  • Details for roles found mapped to deprecated reporting roles:
    image
  • If current user does not have manage_security privileges, they will see this error state:
    image
    and these messages in the Kibana server log:
    image

Checklist

  • Log users that have reporting privileges granted, from a customized xpack.reporting.roles.allow
  • Log roles that are mapped to a deprecated reporting role
  • Any text added follows EUI's writing guidelines, uses sentence case text and includes i18n support
  • Documentation was added for features that require explanation or tutorials
  • Unit or functional tests were updated or added to match the most common scenarios

Testing

Security disabled

  1. Test the Upgrade Assistant with Reporting enabled and Security disabled

Default deprecation

  1. Create a customer user and assign the reporting_user role
  2. Test the Upgrade Assistant with a superuser

Customized xpack.reporting.roles.allow

  1. Add a custom role for enabling reporting access (my_test_reporting_role)
  2. Update kibana.yml to allow the role to access reporting: xpack.reporting.roles.allow: ['my_test_reporting_role']
  3. Create a customer user and assign the my_test_reporting_user role
  4. Test the Upgrade Assistant with a superuser

Mapped roles

  1. Add role mappings that map a custom role to reporting_user in Stack Management > Role Mappings

Invalid privileges for Reporting deprecations

  1. Create a custom superuser role that provides the manage cluster privilege, and then some privileges to Kibana (so they can login)
  2. Test the Upgrade Assistant with the custom user that has the custom superuser role

@tsullivan tsullivan marked this pull request as draft October 15, 2021 01:54
@tsullivan tsullivan added deprecation_warnings (Deprecated) Feature:Reporting Use Reporting:Screenshot, Reporting:CSV, or Reporting:Framework instead Team:AppServicesUx v7.16.0 v8.0.0 backport:skip This commit does not require backporting release_note:skip Skip the PR/issue when compiling release notes labels Oct 15, 2021
@tsullivan tsullivan force-pushed the reporting/roles-deprecation-errorhandling-7.x branch 2 times, most recently from a14b4ad to e2f4f7d Compare October 15, 2021 01:59
@tsullivan tsullivan force-pushed the reporting/roles-deprecation-errorhandling-7.x branch 4 times, most recently from 52e9b8b to a8865bd Compare October 19, 2021 22:49
@tsullivan tsullivan requested review from jloleysens, dokmic and a team October 19, 2021 22:49
@@ -64,7 +64,7 @@ export const config: PluginConfigDescriptor<ReportingConfigType> = {
defaultMessage:
`Use Kibana application privileges to grant reporting privileges.` +
` Using "{fromPath}.roles.allow" to grant reporting privileges` +
` prevents users from using API Keys to create reports.` +
` is deprecated.` +
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is unrelated to this PR, but the existing text in this config deprecation was really hard to read on the page.

@@ -72,6 +72,7 @@ export class ReportingCore {
public getContract: () => ReportingSetup;

constructor(private logger: LevelLogger, context: PluginInitializerContext<ReportingConfigType>) {
// TODO: capture the entire packageInfo so we can form documentation links on the server
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This applies this this PR, as the doc links are hardcoded to the current version, which is an issue.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

+1 would be a good idea to address it in this PR

This fixes the deprecations for various edge cases:
 - when security was disabled, users saw a meaningless error message
 - when users did not have manage_security privilege they saw a meaningless error message

Adds support for checking deprecations related to xpack.reporting.roles.allow
@tsullivan tsullivan force-pushed the reporting/roles-deprecation-errorhandling-7.x branch from a8865bd to 4847e72 Compare October 19, 2021 22:56
config,
createMockPluginSetup({
router: httpSetup.createRouter(''),
security: null,
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is a side effect of not having the security dependency not mocked well in createMockReportingCore before this PR.

config,
createMockPluginSetup({
router: httpSetup.createRouter(''),
security: null,
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is a side effect of not having the security dependency not mocked well in createMockReportingCore before this PR.

@@ -39,9 +41,9 @@ export const createMockPluginSetup = (setupMock?: any): ReportingInternalSetup =
features: featuresPluginMock.createSetup(),
basePath: { set: jest.fn() },
router: setupMock.router,
security: setupMock.security,
security: securityMock.createSetup(),
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The tests in this PR needed a better security mock from the reporting test utilities

licensing: { license$: Rx.of({ isAvailable: true, isActive: true, type: 'basic' }) } as any,
taskManager: { registerTaskDefinitions: jest.fn() } as any,
taskManager: taskManagerMock.createSetup(),
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

opportunistic cleanup

@tsullivan tsullivan requested a review from vadimkibana October 19, 2021 23:00
@tsullivan tsullivan marked this pull request as ready for review October 19, 2021 23:01
@elasticmachine
Copy link
Contributor

Pinging @elastic/kibana-reporting-services (Team:Reporting Services)

@tsullivan tsullivan changed the title [Reporting] Fix deprecation messages [Reporting] Add deprecation messages for roles mapped to reporting_user Oct 19, 2021
Copy link
Contributor

@dokmic dokmic left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code changes LGTM, but I haven't tested that.

Copy link
Contributor

@jloleysens jloleysens left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is looking great @tsullivan, I did a few tests locally and it seemed to be working as expected (I didn't test the role map deprecation manually).

I left a few non-blocker comments but I think it'd be good to address the formatting of markdown before merging.

describe('roles mapped to a deprecated role', () => {
test('logs a deprecation when a role was found that maps to the deprecated reporting_user role', async () => {
esClient.asCurrentUser.security.getRoleMapping = jest.fn().mockResolvedValue({
body: { dungeon_master: { roles: ['reporting_user'] } },
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

😄

return [
{
title,
level: 'fetch_error', // NOTE: is fetch_error not shown in the Upgrade Assistant UI?
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should we follow up on this with the Stack Management team?

@jportner jportner self-requested a review October 20, 2021 13:39
Copy link
Contributor

@jportner jportner left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I added some suggestions below!

A general suggestion/comment: we don't need to use template strings to prevent things from being translated. For example, instead of this:

'Set "{upgradableConfig}" in kibana.yml'

you can write thisL

'Set "xpack.reporting.roles.enabled: false" in kibana.yml'

I confirmed this with @Bamieh

@tylersmalley tylersmalley deleted the branch elastic:7.16 October 20, 2021 16:09
@tylersmalley tylersmalley reopened this Oct 20, 2021
@kibanamachine
Copy link
Contributor

💚 Build Succeeded

Metrics [docs]

✅ unchanged

History

To update your PR or re-run it, just comment with:
@elasticmachine merge upstream

@@ -85,8 +86,8 @@ export class ReportingCore {
this.executing = new Set();
}

public getKibanaVersion() {
return this.kibanaVersion;
public getKibanaPackageInfo() {
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This change is to allow documentation links generated on the server to use the current branch.

@tsullivan tsullivan enabled auto-merge (squash) October 20, 2021 20:17
@tsullivan tsullivan merged commit 9231d80 into elastic:7.16 Oct 20, 2021
@kibanamachine
Copy link
Contributor

💚 Build Succeeded

Metrics [docs]

✅ unchanged

To update your PR or re-run it, just comment with:
@elasticmachine merge upstream

tsullivan added a commit to tsullivan/kibana that referenced this pull request Oct 20, 2021
…er (elastic#115125)

* [Reporting] Add deprecation messages for roles mapped to reporting_user

This fixes the deprecations for various edge cases:
 - when security was disabled, users saw a meaningless error message
 - when users did not have manage_security privilege they saw a meaningless error message

Adds support for checking deprecations related to xpack.reporting.roles.allow

* updates to content per feedback

* updates per feedback

* store packageInfo in reportingCore

* use branch in the documentation links generated in the server

* add tests for scenario where config does not need to be changed
tsullivan added a commit that referenced this pull request Oct 21, 2021
…er (#115125) (#115875)

* [Reporting] Add deprecation messages for roles mapped to reporting_user

This fixes the deprecations for various edge cases:
 - when security was disabled, users saw a meaningless error message
 - when users did not have manage_security privilege they saw a meaningless error message

Adds support for checking deprecations related to xpack.reporting.roles.allow

* updates to content per feedback

* updates per feedback

* store packageInfo in reportingCore

* use branch in the documentation links generated in the server

* add tests for scenario where config does not need to be changed
@tsullivan tsullivan deleted the reporting/roles-deprecation-errorhandling-7.x branch October 21, 2021 02:12
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backport:skip This commit does not require backporting (Deprecated) Feature:Reporting Use Reporting:Screenshot, Reporting:CSV, or Reporting:Framework instead deprecation_warnings release_note:skip Skip the PR/issue when compiling release notes v7.16.0 v8.0.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants