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

Get rid of implicit circular dependency between Security and Spaces plugins. #80496

Closed
azasypkin opened this issue Oct 14, 2020 · 5 comments · Fixed by #81891
Closed

Get rid of implicit circular dependency between Security and Spaces plugins. #80496

azasypkin opened this issue Oct 14, 2020 · 5 comments · Fixed by #81891
Labels
chore Feature:Security/Authorization Platform Security - Authorization Feature:Security/Spaces Platform Security - Spaces feature Team:Security Team focused on: Auth, Users, Roles, Spaces, Audit Logging, and more!

Comments

@azasypkin
Copy link
Member

Currently Security plugin exposes registerSpacesService method as a part of its setup contract basically introducing an implicit dependency to Spaces plugin. We had to do that because Spaces already depends on Security and circular dependencies aren't allowed by the core plugin system.

The current approach is not only ugly, but is also a clear sign of a broken separation of concerns between these two plugins. We should find a better way to manage this cross-dependency.

@azasypkin azasypkin added chore Team:Security Team focused on: Auth, Users, Roles, Spaces, Audit Logging, and more! Feature:Security/Spaces Platform Security - Spaces feature Feature:Security/Authorization Platform Security - Authorization labels Oct 14, 2020
@elasticmachine
Copy link
Contributor

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

@legrego
Copy link
Member

legrego commented Oct 14, 2020

I feel like removing Space's dependency on security is probably the right way to go. Spaces relies on security primarily to authorize access to spaces.

I don't like any of my proposals, but I'll list them anyway. I'm open to any and all ideas!

  1. A naive approach would be to extract the authorization service into a dedicated authorization plugin, which both Security and Spaces could reference.

  2. Allow the SpacesClient to be augmented via a security wrapper (like we do for saved objects) which can then authorize access to spaces.

  3. Update our authorization model to allow for more granular authorization checks (similar/related: Saved-objects authorization more granular than type #47503), so that we no longer need a dedicated SpacesClient. This is probably the most correct solution, but is also a large amount of work that will take time to get right.

@azasypkin
Copy link
Member Author

Update our authorization model to allow for more granular authorization checks (similar/related: #47503), so that we no longer need a dedicated SpacesClient. This is probably the most correct solution, but is also a large amount of work that will take time to get right.

I admit I don't fully understand how that would work, but would that also allow us to record audit events without Spaces being dependent on Security somehow (as far as I understand any plugin will have to depend on Security to deal with audit log now)?

It seems the first option also may be tricky because of audit log dependency, the second one doesn't seem to have this problem.

@legrego
Copy link
Member

legrego commented Oct 15, 2020

I admit I don't fully understand how that would work, but would that also allow us to record audit events without Spaces being dependent on Security somehow (as far as I understand any plugin will have to depend on Security to deal with audit log now)?

I don't have a specific implementation in mind, but I was effectively proposing to update our authorization model so that the security plugin would know how to secure access to Spaces, without spaces being a unique one-off. In this scenario, the security plugin could perform the audit logging, since it would be the one performing the authorization checks for spaces.

It seems the first option also may be tricky because of audit log dependency, the second one doesn't seem to have this problem.

Agree, I hadn't considered the audit log dependency here.

@azasypkin
Copy link
Member Author

I don't have a specific implementation in mind, but I was effectively proposing to update our authorization model so that the security plugin would know how to secure access to Spaces, without spaces being a unique one-off. In this scenario, the security plugin could perform the audit logging, since it would be the one performing the authorization checks for spaces.

Thanks for clarifying, sounds like a great approach to me. Maybe we can try to estimate how expensive it'd be and how long it'd take us to implement, there is a chance we'll have enough time before #80508 is completely unblocked.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
chore Feature:Security/Authorization Platform Security - Authorization Feature:Security/Spaces Platform Security - Spaces feature Team:Security Team focused on: Auth, Users, Roles, Spaces, Audit Logging, and more!
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants