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

Migrate authorization subsystem to the new platform. #46145

Merged
merged 17 commits into from
Nov 12, 2019
Merged

Migrate authorization subsystem to the new platform. #46145

merged 17 commits into from
Nov 12, 2019

Conversation

azasypkin
Copy link
Member

@azasypkin azasypkin commented Sep 19, 2019 •
edited
Loading

In this PR we migrate authorization subsystem to the new platform. Authorization source code will now live within a new platform plugin that will coexist with the legacy Security plugin until we fully migrate to the new platform.

NOTE: It'd be much easier to review PR commit by commit.

Blocked by: #44513, #49457
Related to: #33775

@azasypkin azasypkin added Feature:New Platform Team:Security Team focused on: Auth, Users, Roles, Spaces, Audit Logging, and more! release_note:skip Skip the PR/issue when compiling release notes Feature:Security/Authorization Platform Security - Authorization v7.5.0 labels Sep 19, 2019
@elasticmachine
Copy link
Contributor

Pinging @elastic/kibana-security

@legrego legrego mentioned this pull request Sep 23, 2019
Merged
7 tasks
@mshustov mshustov mentioned this pull request Oct 16, 2019
Merged
7 tasks
@azasypkin azasypkin force-pushed the issue-xxx-np-authz branch 4 times, most recently from 1518b47 to 0626e7a Compare October 22, 2019 09:18
@azasypkin azasypkin force-pushed the issue-xxx-np-authz branch 2 times, most recently from 22ba245 to 0f6cfe5 Compare October 28, 2019 09:10
@azasypkin azasypkin mentioned this pull request Oct 28, 2019
Merged
@azasypkin azasypkin force-pushed the issue-xxx-np-authz branch 2 times, most recently from 5dc7201 to bb13b13 Compare October 28, 2019 13:41
@elastic elastic deleted a comment from elasticmachine Oct 28, 2019
@azasypkin azasypkin removed the blocked label Oct 28, 2019
@elasticmachine
Copy link
Contributor

💚 Build Succeeded

@azasypkin azasypkin marked this pull request as ready for review October 29, 2019 06:58
@azasypkin azasypkin requested review from a team as code owners October 29, 2019 06:58
@azasypkin azasypkin requested a review from a team October 29, 2019 06:58
@azasypkin azasypkin requested a review from kobelb November 1, 2019 16:32
kobelb
kobelb approved these changes Nov 1, 2019
View reviewed changes
Copy link
Contributor

@kobelb kobelb left a comment •
edited
Loading

Choose a reason for hiding this comment

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

The additional refactoring which you've done LGTM! We currently have a lot of duplicated tests in x-pack/plugins/security/server/routes/authorization/roles/get.test.ts and x-pack/plugins/security/server/routes/authorization/roles/get_all.test.ts which we could potentially remove now that we have a dedicated transformElasticsearchRoleToRole. This isn't a requirement by any means, what you've done has already made this much cleaner.

mikecote
mikecote approved these changes Nov 1, 2019
View reviewed changes
Copy link
Contributor

@mikecote mikecote left a comment

Choose a reason for hiding this comment

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

Alerting changes LGTM!

@azasypkin
Copy link
Member Author

@elastic/kibana-platform would you mind reviewing core related changes in Core fixes. commit?

@tsullivan @joelgriffith this PR includes a couple of minor changes in reporting plugin as well, could you please take a look (this is a workaround, but I left a comment explaining why we need it)? These are included into Changes in other plugins. commit.

@azasypkin
Copy link
Member Author

We currently have a lot of duplicated tests in x-pack/plugins/security/server/routes/authorization/roles/get.test.ts and x-pack/plugins/security/server/routes/authorization/roles/get_all.test.ts which we could potentially remove now that we have a dedicated transformElasticsearchRoleToRole.

++, put that to my short-term ToDo :)

@elasticmachine
Copy link
Contributor

💚 Build Succeeded

gmmorris
gmmorris approved these changes Nov 11, 2019
View reviewed changes
Copy link
Contributor

@gmmorris gmmorris left a comment

Choose a reason for hiding this comment

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

The Reporting changes look reasonable to me, as we're repeating the approach that has been taken in Actions.

It would be nice if we could merge the creation of fakeRequest int oone single API call somewhere, so that we have only one place to clean up, but I know shared code is tricky in Kibana, so not a blocker from my perspective.

joshdover
joshdover approved these changes Nov 11, 2019
View reviewed changes
Copy link
Contributor

@joshdover joshdover left a comment

Choose a reason for hiding this comment

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

Platform changes LGTM

@azasypkin
Copy link
Member Author

Thanks everyone for review! Let's see if it sticks.

@azasypkin azasypkin added v7.6.0 and removed v7.5.0 labels Nov 11, 2019
@azasypkin
Copy link
Member Author

@elasticmachine merge upstream

@elasticmachine
Copy link
Contributor

💚 Build Succeeded

@azasypkin azasypkin merged commit ad356f5 into elastic:master Nov 12, 2019
@azasypkin azasypkin deleted the issue-xxx-np-authz branch November 12, 2019 10:31
@azasypkin azasypkin mentioned this pull request Nov 12, 2019
Merged
jloleysens added a commit to jloleysens/kibana that referenced this pull request Nov 12, 2019
@jloleysens
Merge remote-tracking branch 'upstream/master' into test-1
37d4836 
* upstream/master:
  [ML] Make navigation in tests more stable (elastic#50132)
  Migrate authorization subsystem to the new platform.  (elastic#46145)
  Bugfix: Interpreter conversion of string to number should throw on NaN elastic#27788 (elastic#50063)
  Update dependency @elastic/charts to v14 (elastic#49947)
  [ML] Adding cloud specific ML node warning (elastic#50139)
  Fixing bugs in the Shareable Runtime (elastic#49965)
  Revert router base name for Uptime plugin to use hash in default path. (elastic#50095)
  Ability to have telemetry always opted in (elastic#49798)
  Add "Get Help" and "Kibana Feedback" links to the help popover (elastic#49797)
  Removes references to Elasticsearch mapping types (elastic#47610)
  [skip-ci] Replace coordinate map in Kibana getting started docs with Maps (elastic#50167)
  [ML] Indicate missing required privileges for import in File Data Viz (elastic#50147)
  [SIEM][Detection Engine] Removes technical debt and minor bug fixes (elastic#50111)
azasypkin added a commit that referenced this pull request Nov 12, 2019
@azasypkin
[7.x] Migrate authorization subsystem to the new platform. (#46145) (#…
00d4866 
…50289)
@azasypkin
Copy link
Member Author

7.x/7.6.0: 00d4866

jloleysens added a commit to jloleysens/kibana that referenced this pull request Nov 12, 2019
@jloleysens
Merge remote-tracking branch 'upstream/master' into test-3
7b512ad 
* upstream/master:
  Move table-list-view to kibana-react (elastic#50046)
  [ML] Stats bar for data frame analytics (elastic#49464)
  [ML] Make navigation in tests more stable (elastic#50132)
  Migrate authorization subsystem to the new platform.  (elastic#46145)
  Bugfix: Interpreter conversion of string to number should throw on NaN elastic#27788 (elastic#50063)
  Update dependency @elastic/charts to v14 (elastic#49947)
  [ML] Adding cloud specific ML node warning (elastic#50139)
  Fixing bugs in the Shareable Runtime (elastic#49965)
  Revert router base name for Uptime plugin to use hash in default path. (elastic#50095)
  Ability to have telemetry always opted in (elastic#49798)
  Add "Get Help" and "Kibana Feedback" links to the help popover (elastic#49797)
  Removes references to Elasticsearch mapping types (elastic#47610)
  [skip-ci] Replace coordinate map in Kibana getting started docs with Maps (elastic#50167)
  [ML] Indicate missing required privileges for import in File Data Viz (elastic#50147)
  [SIEM][Detection Engine] Removes technical debt and minor bug fixes (elastic#50111)
jloleysens added a commit to jloleysens/kibana that referenced this pull request Nov 12, 2019
@jloleysens
Merge remote-tracking branch 'upstream/master' into np-migration/ua […
faa2676 
…skip ci]

* upstream/master:
  Move table-list-view to kibana-react (elastic#50046)
  [ML] Stats bar for data frame analytics (elastic#49464)
  [ML] Make navigation in tests more stable (elastic#50132)
  Migrate authorization subsystem to the new platform.  (elastic#46145)
  Bugfix: Interpreter conversion of string to number should throw on NaN elastic#27788 (elastic#50063)
  Update dependency @elastic/charts to v14 (elastic#49947)
  [ML] Adding cloud specific ML node warning (elastic#50139)
  Fixing bugs in the Shareable Runtime (elastic#49965)
  Revert router base name for Uptime plugin to use hash in default path. (elastic#50095)
  Ability to have telemetry always opted in (elastic#49798)
  Add "Get Help" and "Kibana Feedback" links to the help popover (elastic#49797)
  Removes references to Elasticsearch mapping types (elastic#47610)
  [skip-ci] Replace coordinate map in Kibana getting started docs with Maps (elastic#50167)
  [ML] Indicate missing required privileges for import in File Data Viz (elastic#50147)
  [SIEM][Detection Engine] Removes technical debt and minor bug fixes (elastic#50111)
jloleysens added a commit to jloleysens/kibana that referenced this pull request Nov 13, 2019
@jloleysens
Merge branch 'master' of github.com:elastic/kibana into fix/grokdebug…
0c102d9 
…ger-ace-theme

* 'master' of github.com:elastic/kibana: (56 commits)
  [ML] Server info service refactor (elastic#50302)
  Remove internal platform types exports (elastic#50427)
  [APM] Document `apm_oss.metricsIndices` and `apm_oss.sourcemap… (elastic#50312)
  [Telemetry] Server side fetcher (elastic#50015)
  [SIEM] Detection engine placeholders (elastic#50220)
  [Uptime] Donut chart loader position centered vertically  (elastic#50219)
  update telemetry banner notice text (elastic#50403)
  Fix aborting when searching without batching (elastic#49966)
  [Telemetry] Remove telemetry splash page and add conditional messaging (elastic#50189)
  Revert chromedriver update (elastic#50324)
  Remove deprecated argument include_type_name from ES calls (elastic#50285)
  [Maps] add settings to maps telemetry (elastic#50161)
  remove visualize loader (elastic#46910)
  Fix misuse of react-router and react-router-dom (elastic#50120)
  Move table-list-view to kibana-react (elastic#50046)
  [ML] Stats bar for data frame analytics (elastic#49464)
  [ML] Make navigation in tests more stable (elastic#50132)
  Migrate authorization subsystem to the new platform.  (elastic#46145)
  Bugfix: Interpreter conversion of string to number should throw on NaN elastic#27788 (elastic#50063)
  Update dependency @elastic/charts to v14 (elastic#49947)
  ...
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mikecote mikecote mikecote approved these changes

@joshdover joshdover joshdover approved these changes

@kobelb kobelb kobelb approved these changes

@gmmorris gmmorris gmmorris approved these changes

Assignees
No one assigned
Labels
backported Feature:NP Migration Feature:Security/Authorization Platform Security - Authorization 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.6.0
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@azasypkin @elasticmachine @kobelb @gmmorris @joshdover @mikecote