Skip to content

Extract authorization logic and it's peripherals into packages#190028

Merged
eokoneyo merged 8 commits intoelastic:mainfrom
eokoneyo:chore/extract-role-privilege-model-into-package
Aug 22, 2024
Merged

Extract authorization logic and it's peripherals into packages#190028
eokoneyo merged 8 commits intoelastic:mainfrom
eokoneyo:chore/extract-role-privilege-model-into-package

Conversation

@eokoneyo
Copy link
Contributor

@eokoneyo eokoneyo commented Aug 7, 2024

Summary

This PR is a precursor to #189871, as part of the spaces improvement initiative there's a need to be able to share the user privilege assignment component between the roles experience and the new spaces experience to prevent duplication of business logic and cohesiveness in the privilege assignment experience.

The aforementioned PR extracts the required component into it's own package so it might be consumed as needed, this PR is particularly concerned with extracting business logic said UI component depends on that exists still within the security plugin. For context; the security plugin already depends on the spaces plugin, so having the spaces plugin in turn statically depend on the security plugin creates a cyclic dependency. That being said to complement the eventual state of said component so it might be imported elsewhere outside of the security plugin there's a need to extract further logic into standalone packages, so that the spaces plugin can consume this plugin without the afore mentioned cyclic dependency problem.

Visually;

Problem;

image

Proposal

image1

Footnotes

  1. items marked in blue are the packages created in this PR, whilst the entire diagram is the proposed future state

@eokoneyo eokoneyo self-assigned this Aug 7, 2024
@eokoneyo eokoneyo changed the title Chore/extract role privilege model into package Extract authorization logic and it's peripherals into packages Aug 7, 2024
@eokoneyo eokoneyo force-pushed the chore/extract-role-privilege-model-into-package branch 3 times, most recently from 64cc29d to 45d35dd Compare August 8, 2024 21:39
@eokoneyo eokoneyo added the Team:Security Platform Security: Auth, Users, Roles, Spaces, Audit Logging, etc t// label Aug 9, 2024
@eokoneyo eokoneyo force-pushed the chore/extract-role-privilege-model-into-package branch from 1d9ea5c to 8ed5c15 Compare August 12, 2024 08:04
@eokoneyo
Copy link
Contributor Author

Note for reviewers;

The @kbn/security-authorization-core package consolidates the server logic that existed within the security plugin, and the business logic that the UI depends on for role/privilege management; the reasoning behind this is to consolidate all role . privilege management logic in one specific location, and more so as noted above, this also helps facilitate extracting the UI for assigning privileges of a role, so it might be reused.

@eokoneyo
Copy link
Contributor Author

/ci

@eokoneyo eokoneyo added the release_note:skip Skip the PR/issue when compiling release notes label Aug 13, 2024
@eokoneyo eokoneyo marked this pull request as ready for review August 19, 2024 13:37
@eokoneyo eokoneyo requested a review from a team as a code owner August 19, 2024 13:37
@elasticmachine
Copy link
Contributor

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

@eokoneyo
Copy link
Contributor Author

@elasticmachine merge upstream

@SiddharthMantri SiddharthMantri self-requested a review August 21, 2024 07:25
@kc13greiner kc13greiner self-requested a review August 21, 2024 12:07
Copy link
Contributor

@kc13greiner kc13greiner left a comment

Choose a reason for hiding this comment

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

Overall LGTM! Just one question below

Copy link
Contributor

Choose a reason for hiding this comment

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

Question: Is this being used anywhere?

Copy link
Contributor

@SiddharthMantri SiddharthMantri Aug 21, 2024

Choose a reason for hiding this comment

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

Nit: If this is intended for a future change, we recommend not introducing a dependency where a package depends on a plugins exported types. Ideally, such a dependency would be moved to a common package from where it'd be imported in all places where it's used.

Copy link
Contributor Author

@eokoneyo eokoneyo Aug 22, 2024

Choose a reason for hiding this comment

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

Hey @kc13greiner thanks for pointing this out, the type was actually not in use. I've removed it, and @SiddharthMantri thanks for the guidance there's a chance I'd actually need the type in the follow PR to this one.

@eokoneyo eokoneyo force-pushed the chore/extract-role-privilege-model-into-package branch from 47218a6 to 42d4579 Compare August 22, 2024 10:26
Copy link
Contributor

@SiddharthMantri SiddharthMantri left a comment

Choose a reason for hiding this comment

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

LGTM!

@kibana-ci
Copy link

💛 Build succeeded, but was flaky

Failed CI Steps

Metrics [docs]

Module Count

Fewer modules leads to a faster build time

id before after diff
security 524 523 -1

Public APIs missing comments

Total count of every public API that lacks a comment. Target amount is 0. Run node scripts/build_api_docs --plugin [yourplugin] --stats comments for more detailed information.

id before after diff
@kbn/security-authorization-core - 24 +24
@kbn/security-role-management-model - 74 +74
total +98

Async chunks

Total size of all lazy-loaded chunks that will be downloaded as the user navigates the app

id before after diff
security 589.4KB 589.4KB +4.0B

Public APIs missing exports

Total count of every type that is part of your API that should be exported but is not. This will cause broken links in the API documentation system. Target amount is 0. Run node scripts/build_api_docs --plugin [yourplugin] --stats exports for more detailed information.

id before after diff
@kbn/security-authorization-core - 7 +7
security 1 0 -1
total +6
Unknown metric groups

API count

id before after diff
@kbn/security-authorization-core - 25 +25
@kbn/security-role-management-model - 75 +75
total +100

References to deprecated APIs

id before after diff
@kbn/security-authorization-core - 30 +30
@kbn/security-role-management-model - 3 +3
security 53 22 -31
total +2

History

  • 💚 Build #228246 succeeded 47218a6436486c71c73f310b52c13dc51cf5f1f1
  • 💚 Build #226994 succeeded 2100272d864c3f2f61d3080359a5f1e9a85d828a
  • 💛 Build #226724 was flaky 1d9ea5c5e4d198c1525851cda67ef750840b5b85
  • 💔 Build #226677 failed 45d35dde6567c3de8f5edfb7cd03107866fcd448
  • 💔 Build #226363 failed 64cc29d4d173e90e4ddd6aef6f200ea2e4f890ea
  • 💔 Build #226226 failed 1b93a04e434f3051e5492873372b0cccd2e1b464

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

cc @eokoneyo

Copy link
Contributor

@kc13greiner kc13greiner left a comment

Choose a reason for hiding this comment

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

Thanks for the updates - LGTM!

@eokoneyo eokoneyo merged commit 44fafb8 into elastic:main Aug 22, 2024
@eokoneyo eokoneyo deleted the chore/extract-role-privilege-model-into-package branch August 22, 2024 13:44
@kibanamachine kibanamachine added v8.16.0 backport:skip This PR does not require backporting labels Aug 22, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

backport:skip This PR does not require backporting release_note:skip Skip the PR/issue when compiling release notes Team:Security Platform Security: Auth, Users, Roles, Spaces, Audit Logging, etc t// v8.16.0

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants