-
Notifications
You must be signed in to change notification settings - Fork 8.2k
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
Register kibana_dashboard_only_user
and kibana_user
roles deprecations in UA.
#110960
Register kibana_dashboard_only_user
and kibana_user
roles deprecations in UA.
#110960
Conversation
a791bca
to
91ff082
Compare
91ff082
to
36634ff
Compare
36634ff
to
3d508ea
Compare
NOTE to myself: We all should agree how we use quotes/brackets in UA messages (always use
cc @gchaps (Sorry for the noise, just noticed we already mandate using |
f07374f
to
e686c81
Compare
e686c81
to
19267a9
Compare
@@ -0,0 +1,244 @@ | |||
/* |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
note: I don't try to analyze Advanced Settings to figure out if users change default role for the Dashboard only mode as it turned out to be a non-trivial task: as far as I'm aware we cannot use UI Settings client to retrieve settings from all spaces, and at the same time we cannot use SO Client directly since we need to account for UI Settings overrides coming from predefined defaults and kibana.yml
. It feels like the amount of complexity wouldn't be justified for this deprecation.
const title = i18n.translate( | ||
'xpack.security.deprecations.kibanaDashboardOnlyUser.deprecationTitle', | ||
{ | ||
defaultMessage: 'The "{roleName}" role is removed', |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
note: I use the same title for all cases, including fetch_error
. It feels reasonable to me since we know what deprecation exactly we want to handled, but I'm not sure if we have any best practices here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think using the same title for all cases is fine. The title isn't even shown for the fetch error anyway.
I'd personally recommend moving the translation to the top level scope so you're only defining it once.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd personally recommend moving the translation to the top level scope so you're only defining it once.
IIRC i18n might not work well with any label you define at the file level (it will always be in English) since file require may happen before we asynchronously initialize translations in i18n (it's OK for the client side though). I haven't heard of it being improved, but my knowledge may be outdated.
But we can have a global function to return this label, I'll go this route.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
IIRC i18n might not work well with any label you define at the file level (it will always be in English) since file require may happen before we asynchronously initialize translations in i18n (it's OK for the client side though). I haven't heard of it being improved, but my knowledge may be outdated.
TIL!
}, | ||
} | ||
), | ||
level: 'warning', |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
question: if I understand correctly we should not use critical
here since it doesn't really blocks upgrade?
return []; | ||
} | ||
|
||
return [ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
note: I split role mappings and users related cases in two separate deprecation entries. I don't like that it has the same name in the UA table, but apart from that the finer granularity fits nicely here (since user needs to use different screens during manual resolution).
values: { userRoleName: KIBANA_USER_ROLE_NAME, adminRoleName: KIBANA_ADMIN_ROLE_NAME }, | ||
}), | ||
message: i18n.translate('xpack.security.deprecations.kibanaUser.usersDeprecationMessage', { | ||
defaultMessage: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
note: I'd better suggest using custom roles with more precise Kibana privileges set, and only use kibana_admin
as the last resort, but I couldn't figure out how to keep it short, concise and not confusing.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree, let's keep it simple
Hey @jportner , would you mind giving a preliminary feedback here? Handling of these two deprecations cover quite a few use cases (e.g. failure to perform a deprecation check, |
if (usersWithKibanaUserRole.length === 0) { | ||
logger.debug(`No users with "${KIBANA_USER_ROLE_NAME}" role found.`); | ||
} else { | ||
logger.debug( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
question: are we concerned with mentioning user names in the logs in this case?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Seeing as these are debug logs, and they will only be generated by authorized users who use the upgrade assistant, I think this is OK.
I'm planning to review this when I return on Sep 27 👍 |
Pinging @elastic/kibana-security (Team:Security) |
Time got away from me today, will plan to review first thing tomorrow 👍 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A couple of nits on wording and docs links, other than that I think this is ready!
defaultMessage: | ||
'The "{userRoleName}" role is deprecated. Use "{adminRoleName}" role instead.', | ||
values: { userRoleName: KIBANA_USER_ROLE_NAME, adminRoleName: KIBANA_ADMIN_ROLE_NAME }, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I suggest shortening this
defaultMessage: | |
'The "{userRoleName}" role is deprecated. Use "{adminRoleName}" role instead.', | |
values: { userRoleName: KIBANA_USER_ROLE_NAME, adminRoleName: KIBANA_ADMIN_ROLE_NAME }, | |
defaultMessage: 'The "{userRoleName}" role is deprecated', | |
values: { userRoleName: KIBANA_USER_ROLE_NAME }, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It was actually inspired by the example from deprecation guide: The timezone Xyz used by Rollup job Abc is deprecated. Use Tz instead.
. But I'm fine with either honestly - will change.
}), | ||
message: i18n.translate('xpack.security.deprecations.kibanaUser.usersDeprecationMessage', { | ||
defaultMessage: | ||
'Use a "{adminRoleName}" role to grant access to all Kibana features in all spaces. The "{userRoleName}" role will be removed in 8.0.', |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
s/a/the/
'Use a "{adminRoleName}" role to grant access to all Kibana features in all spaces. The "{userRoleName}" role will be removed in 8.0.', | |
'Use the "{adminRoleName}" role to grant access to all Kibana features in all spaces. The "{userRoleName}" role will be removed in 8.0.', |
}), | ||
level: 'warning', | ||
deprecationType: 'feature', | ||
documentationUrl: `https://www.elastic.co/guide/en/kibana/${packageInfo.branch}/xpack-security-authorization.html`, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it would be better to direct users to the "Built-in roles" page, which describes both the deprecated role and the new one:
documentationUrl: `https://www.elastic.co/guide/en/kibana/${packageInfo.branch}/xpack-security-authorization.html`, | |
documentationUrl: `https://www.elastic.co/guide/en/elasticsearch/reference/${packageInfo.branch}/built-in-roles.html`, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Agree!
defaultMessage: | ||
'The "{userRoleName}" role is deprecated. Use "{adminRoleName}" role instead.', | ||
values: { userRoleName: KIBANA_USER_ROLE_NAME, adminRoleName: KIBANA_ADMIN_ROLE_NAME }, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same as above
defaultMessage: | |
'The "{userRoleName}" role is deprecated. Use "{adminRoleName}" role instead.', | |
values: { userRoleName: KIBANA_USER_ROLE_NAME, adminRoleName: KIBANA_ADMIN_ROLE_NAME }, | |
defaultMessage: 'The "{userRoleName}" role is deprecated', | |
values: { userRoleName: KIBANA_USER_ROLE_NAME }, |
'xpack.security.deprecations.kibanaUser.roleMappingsDeprecationMessage', | ||
{ | ||
defaultMessage: | ||
'Use a "{adminRoleName}" role to grant access to all Kibana features in all spaces. The "{userRoleName}" role will be removed in 8.0.', |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same as above
'Use a "{adminRoleName}" role to grant access to all Kibana features in all spaces. The "{userRoleName}" role will be removed in 8.0.', | |
'Use the "{adminRoleName}" role to grant access to all Kibana features in all spaces. The "{userRoleName}" role will be removed in 8.0.', |
), | ||
level: 'warning', | ||
deprecationType: 'feature', | ||
documentationUrl: `https://www.elastic.co/guide/en/kibana/${packageInfo.branch}/xpack-security-authorization.html`, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same as above
documentationUrl: `https://www.elastic.co/guide/en/kibana/${packageInfo.branch}/xpack-security-authorization.html`, | |
documentationUrl: `https://www.elastic.co/guide/en/elasticsearch/reference/${packageInfo.branch}/built-in-roles.html`, |
), | ||
level: 'warning', | ||
deprecationType: 'feature', | ||
documentationUrl: `https://www.elastic.co/guide/en/kibana/${packageInfo.branch}/xpack-dashboard-only-mode.html`, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry to ask you to change this again:
documentationUrl: `https://www.elastic.co/guide/en/kibana/${packageInfo.branch}/xpack-dashboard-only-mode.html`, | |
documentationUrl: `https://www.elastic.co/guide/en/elasticsearch/reference/${packageInfo.branch}/built-in-roles.html`, |
), | ||
level: 'warning', | ||
deprecationType: 'feature', | ||
documentationUrl: `https://www.elastic.co/guide/en/kibana/${packageInfo.branch}/xpack-dashboard-only-mode.html`, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same as above
documentationUrl: `https://www.elastic.co/guide/en/kibana/${packageInfo.branch}/xpack-dashboard-only-mode.html`, | |
documentationUrl: `https://www.elastic.co/guide/en/elasticsearch/reference/${packageInfo.branch}/built-in-roles.html`, |
defaultMessage: 'The "{userRoleName}" role is deprecated. Use "{adminRoleName}" role instead.', | ||
values: { userRoleName: KIBANA_USER_ROLE_NAME, adminRoleName: KIBANA_ADMIN_ROLE_NAME }, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I missed this in my review! Since it uses the same i18n key, the defaultMessage needs to be the same:
defaultMessage: 'The "{userRoleName}" role is deprecated. Use "{adminRoleName}" role instead.', | |
values: { userRoleName: KIBANA_USER_ROLE_NAME, adminRoleName: KIBANA_ADMIN_ROLE_NAME }, | |
defaultMessage: 'The "{userRoleName}" role is deprecated', | |
values: { userRoleName: KIBANA_USER_ROLE_NAME }, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry, I messed up a couple things in my previous review, see below 🙈
title: getDeprecationTitle(), | ||
message: i18n.translate('xpack.security.deprecations.kibanaUser.usersDeprecationMessage', { | ||
defaultMessage: | ||
'Use the "{adminRoleName}" role to grant access to all Kibana features in all spaces. The "{userRoleName}" role will be removed in 8.0.', |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm so sorry, I messed up. I forgot that we aren't actually removing the kibana_user
role in 8.0 (see #111160).
So we should probably change this message accordingly:
'Use the "{adminRoleName}" role to grant access to all Kibana features in all spaces. The "{userRoleName}" role will be removed in 8.0.', | |
'Use the "{adminRoleName}" role to grant access to all Kibana features in all spaces. The "{userRoleName}" role will be removed in a future release.', |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great catch, thanks! I should have read this table more attentively.
'xpack.security.deprecations.kibanaUser.roleMappingsDeprecationMessage', | ||
{ | ||
defaultMessage: | ||
'Use the "{adminRoleName}" role to grant access to all Kibana features in all spaces. The "{userRoleName}" role will be removed in 8.0.', |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same as above
'Use the "{adminRoleName}" role to grant access to all Kibana features in all spaces. The "{userRoleName}" role will be removed in 8.0.', | |
'Use the "{adminRoleName}" role to grant access to all Kibana features in all spaces. The "{userRoleName}" role will be removed in a future release.', |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed and updated screenshots
With help from @jportner, here are some edits: (No 1) The "kibana_dashboard_only_user" role is deprecated Users with the "kibana_dashboard_only_user" role will not be able to access the Dashboard app. Use Kibana privileges instead.
(No 2) The "kibana_dashboard_only_user" role is deprecated Users with the "kibana_dashboard_only_user" role will not be able to access the Dashboard app. Use Kibana privileges instead.
(No 3) The "kibana_user" role is deprecated Use the "kibana_admin" role to grant access to all Kibana features in all spaces. Remove the "kibana_user" role from all users and add the "kibana_admin" role. The affected users are: (No 4) The "kibana_user" role is deprecated Use the "kibana_admin" role to grant access to all Kibana features in all spaces. Remove the "kibana_user" role from all role mappings and add the "kibana_admin" role. The affected role mappings are: Note I removed the line "xxx will be removed in a future release." Our guidelines say "You do not need to include any form of "and will be removed in a future release". If an item is deprecated, but won’t be removed in the next major version, the message should indicate that: Note: Support for the "kibana_user" role will be removed following the release of n.0. |
Looks unrelated. |
@elasticmachine merge upstream |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, sorry for all the back-and-forth, we had some confusion regarding the deprecation message guidance!
💚 Build Succeeded
Metrics [docs]Public APIs missing comments
Public APIs missing exports
History
To update your PR or re-run it, just comment with: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
updated screenshots lgtm
Summary
Register
kibana_dashboard_only_user
andkibana_user
roles deprecations in UA.Screenshots
There are 2 different deprecations, but each affects 2 slightly different parts of the Kibana (users and role mappings) and hence will be handled differently. That's why we see 2 pairs of entries with the same title in the main Upgrade Assistant list view. The detailed description, as you'll see below, is different for every entry though:
Detailed view for list entry №1:
Detailed view for list entry №2:
Detailed view for list entry №3:
Detailed view for list entry №4:
**Previous screenshots**
Detailed view for list entry №1:
Detailed view for list entry №2:
Detailed view for list entry №3:
Detailed view for list entry №4:
Fixes #81690
Fixes #54755
Fixes #42853