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.

Sign up for GitHub

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

Jump to bottom

Feature Controls: addressing bugs for enterprise search #70538

Merged
merged 8 commits into from
Jul 6, 2020
Merged

Feature Controls: addressing bugs for enterprise search #70538

merged 8 commits into from
Jul 6, 2020

Conversation

legrego
Copy link
Member

@legrego legrego commented Jul 2, 2020 •
edited
Loading

Summary

Fixes two issues related to feature visibility, which only surfaced when Enterprise Search attempted to integrate with Feature Controls in unique ways that haven't been used up until now:

  1. Resolves Features without privileges should not appear in role management #70501
    Features without privileges should not appear in the role management screen. This fixes that behavior.

  2. Updates the way we toggle visible applications for Spaces and Security by also inspecting the app array that each feature specifies. App nav links in the Kibana Platform have an id which matches the application itself, so we really should have been doing this for a while now. We got a way without this fix because Features also required a distinct navLinkId, which happened to correspond to the application id. Note that this navLinkId was required for the legacy platform world...it wasn't always broken.

This is also required for the security solution as well, who has a similar bug to work through: #69798
cc @XavierM

@legrego legrego mentioned this pull request Jul 2, 2020
Closed
@legrego legrego added Feature:Security/Feature Controls Platform Security - Spaces & Role Mgmt feature controls release_note:skip Skip the PR/issue when compiling release notes Team:Security Team focused on: Auth, Users, Roles, Spaces, Audit Logging, and more! v7.9.0 v8.0.0 labels Jul 2, 2020
@legrego legrego marked this pull request as ready for review July 2, 2020 10:06
@legrego legrego requested a review from a team as a code owner July 2, 2020 10:06
@elasticmachine
Copy link
Contributor

Pinging @elastic/kibana-security (Team:Security)

@legrego legrego requested a review from cee-chen July 2, 2020 10:07
@legrego
Copy link
Member Author

legrego commented Jul 2, 2020

@elasticmachine merge upstream

@XavierM XavierM mentioned this pull request Jul 2, 2020
Merged
@XavierM
Copy link
Contributor

XavierM commented Jul 2, 2020

@elasticmachine merge upstream

@cee-chen
Copy link
Contributor

cee-chen commented Jul 2, 2020 •
edited
Loading

👋 Just to check - this PR doesn't yet remove navLinkId from being required by features.registerFeature(), correct? All good if not, I'll probably just set it to something like '_' in the meanwhile if that's not an issue!

EDIT: Oh wait, I think I misunderstood my type errors - it's navLinkId: '' that was throwing an error, not navLinkId being removed entirely. I think I should be good then!

@cee-chen cee-chen mentioned this pull request Jul 2, 2020
Merged
28 tasks
@legrego
Copy link
Member Author

legrego commented Jul 2, 2020

👋 Just to check - this PR doesn't yet remove navLinkId from being required by features.registerFeature(), correct? All good if not, I'll probably just set it to something like '_' in the meanwhile if that's not an issue!

EDIT: Oh wait, I think I misunderstood my type errors - it's navLinkId: '' that was throwing an error, not navLinkId being removed entirely. I think I should be good then!

Correct this does not remove navLinkId yet. For the time being, I'd set this to enterprise_search like you had it, even though it's duplicative with the app id.

@azasypkin
Copy link
Member

ACK: will review first thing Monday morning.

@azasypkin azasypkin self-requested a review July 3, 2020 17:21
@legrego
Copy link
Member Author

legrego commented Jul 5, 2020

@elasticmachine merge upstream

@azasypkin
Copy link
Member

@elasticmachine merge upstream

@legrego
Copy link
Member Author

legrego commented Jul 6, 2020

@elasticmachine merge upstream

azasypkin
azasypkin approved these changes Jul 6, 2020
View reviewed changes
Copy link
Member

@azasypkin azasypkin left a comment

Choose a reason for hiding this comment

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

LGTM!

x-pack/plugins/security/server/authorization/disable_ui_capabilities.ts
@@ -19,7 +19,7 @@ export function disableUICapabilitiesFactory(
authz: AuthorizationServiceSetup
) {
const featureNavLinkIds = features
.map((feature) => feature.navLinkId)
.flatMap((feature) => [feature.navLinkId, ...feature.app])
Copy link
Member

Choose a reason for hiding this comment

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

optional nit: would you mind adding a comment here that explains why we use nav link IDs and app IDs? I took a look at the code before I read PR description I got confused a bit app IDs being a part of featureNavLinkIds 🙂

@kibanamachine
Copy link
Contributor

💚 Build Succeeded

Build metrics

✅ unchanged

History

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

@legrego legrego merged commit 0673dbd into elastic:master Jul 6, 2020
@legrego legrego deleted the fc/fixing-misc-bugs branch July 6, 2020 14:23
@legrego legrego mentioned this pull request Jul 6, 2020
Merged
legrego added a commit to legrego/kibana that referenced this pull request Jul 6, 2020
@legrego @elasticmachine
Feature Controls: addressing bugs for enterprise search (elastic#70538)
e6961ac 
Co-authored-by: Elastic Machine <elasticmachine@users.noreply.github.com>
legrego added a commit that referenced this pull request Jul 6, 2020
@legrego @elasticmachine
[7.x] Feature Controls: addressing bugs for enterprise search (#70538) (
b9c5bbd 
#70808)

Co-authored-by: Elastic Machine <elasticmachine@users.noreply.github.com>

Co-authored-by: Elastic Machine <elasticmachine@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@azasypkin azasypkin azasypkin approved these changes

@cee-chen cee-chen Awaiting requested review from cee-chen

Assignees
No one assigned
Labels
Feature:Security/Feature Controls Platform Security - Spaces & Role Mgmt feature controls release_note:skip Skip the PR/issue when compiling release notes Team:Security Team focused on: Auth, Users, Roles, Spaces, Audit Logging, and more! v7.9.0 v8.0.0
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Features without privileges should not appear in role management
6 participants
@legrego @elasticmachine @XavierM @cee-chen @azasypkin @kibanamachine