Only add cloud-specific links for superusers by legrego · Pull Request #97870 · elastic/kibana

legrego · 2021-04-21T18:28:00Z

Summary

Adds cloud specific links for users who are likely authorized to view them. In this first iteration, we are checking if the current user is a superuser.

Link examples

Manage this deployment for Cloud users with superuser privileges

Profile and Account & Billing with Cloud links for superuser privileges

Profile link for self managed, or Cloud w/o superuser privileges

Resolves #97308

legrego · 2021-04-22T14:09:42Z

@elasticmachine merge upstream

pgayvallet · 2021-04-23T07:04:56Z

x-pack/plugins/cloud/public/plugin.ts


 interface CloudSetupDependencies {
  home?: HomePublicPluginSetup;
+  security?: Pick<SecurityPluginSetup, 'authc'>;


NIT: as you're using the whole type in the tests via securityMock anyhow, any reason to use a Pick here?

I leave that to Larry, but I personally like the fact that we explicitly outline the part of the dependency contract we really need (too bad we don't have a more expressive syntax to do that). This can make the life much easier if you're changing/fixing the behavior of a certain portion of your public contract and want to quickly know what plugins may be affected without following all the paths where the contract is passed to.

Yeah I did this mostly out of habit for the reasons Aleh mentioned. I don't mind reverting if you'd prefer the original approach

Na, this is fine

pgayvallet · 2021-04-23T07:07:59Z

x-pack/plugins/cloud/public/plugin.ts

+    this.checkIfAuthorizedForLinks().then((authorized) => {
+      if (authorized && baseUrl && deploymentUrl) {
+        coreStart.chrome.setCustomNavLink({
+          title: i18n.translate('xpack.cloud.deploymentLinkLabel', {


NIT: I know checkIfAuthorizedForLinks cannot likely throw, but we may still want to add a catch handler, just to follow the best practices against UnhandledPromiseRejection, wdyt?

Good idea, will do!

azasypkin

Test locally, LGTM, thanks!

azasypkin · 2021-04-23T08:07:17Z

x-pack/plugins/cloud/public/plugin.test.ts

+      const coreSetup = coreMock.createSetup();
+      const homeSetup = homePluginMock.createSetupContract();
+      const securitySetup = securityMock.createSetup();
+      securitySetup.authc.getCurrentUser.mockResolvedValue({


optional nit: wondering if it's time to expose existing mockAuthenticatedUser from security/common/mocks? Not a big deal, up to you.

Good idea, will do! I forgot about this mock

azasypkin · 2021-04-23T08:18:26Z

x-pack/plugins/cloud/public/plugin.ts

+    // Otherwise check roles. If user is not defined due to an unexpected error, then fail *open*.
+    // Cloud admin console will always perform the actual authorization checks.
+    const user = await this.authenticatedUserPromise;
+    return user?.roles.includes('superuser') ?? true;


question: sorry if I missed that in #97308, but for how long we expect this workaround to exist? If we're not sure, I'm wondering if we may want to leave an escape hatch and expose something like xpack.cloud.roles: schema.arrayOf(schema.string(), { defaultValue: ['superuser'] }) config value, so that in the worst case users could create an empty "marker" role if they need to.... I don't have a strong opinion on that, so feel free to ignore.

This workaround will likely exist for a while - it's really up to the cloud team's roadmap to prioritize an API or similar mechanism for us to leverage.

I think it's premature to add an escape hatch, because we don't know what it would look like just yet. It's possible that we would instead want something under cloud's control, such as inspecting the user's metadata that their SSO realm attaches

I think it's premature to add an escape hatch, because we don't know what it would look like just yet. It's possible that we would instead want something under cloud's control, such as inspecting the user's metadata that their SSO realm attaches

That makes sense, thanks. Works for me as long as it works for Cloud.

azasypkin · 2021-04-23T08:29:48Z

x-pack/plugins/cloud/public/plugin.ts


 interface CloudSetupDependencies {
  home?: HomePublicPluginSetup;
+  security?: Pick<SecurityPluginSetup, 'authc'>;


I leave that to Larry, but I personally like the fact that we explicitly outline the part of the dependency contract we really need (too bad we don't have a more expressive syntax to do that). This can make the life much easier if you're changing/fixing the behavior of a certain portion of your public contract and want to quickly know what plugins may be affected without following all the paths where the contract is passed to.

azasypkin · 2021-04-23T08:34:31Z

x-pack/plugins/cloud/public/plugin.test.ts

+import { securityMock } from '../../security/public/mocks';
+import { CloudPlugin } from './plugin';
+
+const nextTick = async () => await new Promise<void>((resolve) => setTimeout(() => resolve(), 1));


question: can we use import { nextTick } from '@kbn/test/jest'; instead?

azasypkin · 2021-04-23T08:35:22Z

x-pack/plugins/cloud/public/plugin.ts

+    this.checkIfAuthorizedForLinks().then((authorized) => {
+      if (authorized && baseUrl && deploymentUrl) {
+        coreStart.chrome.setCustomNavLink({
+          title: i18n.translate('xpack.cloud.deploymentLinkLabel', {


…nks-for-superusers

…kibana into cloud/user-links-for-superusers

legrego · 2021-04-23T13:39:13Z

x-pack/plugins/security/server/mocks.ts

  createSetup: createSetupMock,
  createStart: createStartMock,
  createApiResponse: createApiResponseMock,
+  createMockAuthenticatedUser: (props: MockAuthenticatedUserProps = {}) =>


I didn't have to expose this from the server mock, but I felt like this was a good way to ensure consistency between the client and server mock contracts

Any reason why not just createMockAuthenticatedUser: mockAuthenticatedUser? No need to change anything, just curious.

I debated exposing mockAuthenticatedUser, but prefixing it with create was more consistent with the other mock functions (createSetup, createStart, ...), and it felt more correct. When I first imported mockAuthenticatedUser, I assumed based on the name that it was an object with a create property or similar on it (mockAuthenticatedUser.create())

Yeah, I like the createMockAuthenticatedUser name, I was mostly curious if we need this additional arrow function or we can just assign mockAuthenticatedUser driectly to createMockAuthenticatedUser.

Ah sorry I should have read your comment a little closer 🙈

We don't need the additional arrow function, I added that so my IDE would give me type information as I wrote it out 😬

azasypkin

Still LGTM, thanks!

x-pack/plugins/cloud/public/plugin.ts

Co-authored-by: Aleh Zasypkin <aleh.zasypkin@gmail.com>

kibanamachine · 2021-04-23T15:59:08Z

💚 Build Succeeded

Metrics [docs]

Page load bundle

Size of the bundles that are downloaded on every page load. Target size is below 100kb

id	before	after	diff
`cloud`	4.9KB	5.5KB	+569.0B

History

💚 Build #121989 succeeded 8d5703e
💚 Build #121741 succeeded 57dad48
💔 Build #121483 failed 8afbfa0

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

Co-authored-by: Aleh Zasypkin <aleh.zasypkin@gmail.com> Co-authored-by: Kibana Machine <42973632+kibanamachine@users.noreply.github.com>

kibanamachine · 2021-04-23T16:02:26Z

💚 Backport successful

Status	Branch	Result
✅	7.13
✅	7.x

The backport PRs will be merged automatically after passing CI.

Co-authored-by: Aleh Zasypkin <aleh.zasypkin@gmail.com> Co-authored-by: Kibana Machine <42973632+kibanamachine@users.noreply.github.com> Co-authored-by: Larry Gregory <larry.gregory@elastic.co> Co-authored-by: Aleh Zasypkin <aleh.zasypkin@gmail.com>

Only add cloud-specific links for superusers

8afbfa0

Merge branch 'master' into cloud/user-links-for-superusers

57dad48

legrego added auto-backport Deprecated - use backport:version if exact versions are needed release_note:skip Skip the PR/issue when compiling release notes v7.13.0 v7.14.0 v8.0.0 labels Apr 22, 2021

legrego marked this pull request as ready for review April 22, 2021 16:25

legrego requested a review from a team as a code owner April 22, 2021 16:25

legrego requested a review from azasypkin April 22, 2021 17:47

pgayvallet approved these changes Apr 23, 2021

View reviewed changes

azasypkin approved these changes Apr 23, 2021

View reviewed changes

legrego added 4 commits April 23, 2021 07:33

expose mock authenticated user from security mocks

f19f6c3

address PR feedback

8e5db49

Merge branch 'master' of github.com:elastic/kibana into cloud/user-li…

f1f2d9f

…nks-for-superusers

Merge branch 'cloud/user-links-for-superusers' of github.com:legrego/…

8d5703e

…kibana into cloud/user-links-for-superusers

legrego requested a review from a team as a code owner April 23, 2021 11:39

legrego commented Apr 23, 2021

View reviewed changes

legrego requested a review from azasypkin April 23, 2021 13:39

azasypkin approved these changes Apr 23, 2021

View reviewed changes

x-pack/plugins/cloud/public/plugin.ts Outdated Show resolved Hide resolved

Update x-pack/plugins/cloud/public/plugin.ts

6f9e0fc

Co-authored-by: Aleh Zasypkin <aleh.zasypkin@gmail.com>

legrego enabled auto-merge (squash) April 23, 2021 14:07

legrego mentioned this pull request Apr 23, 2021

Adding RBAC to new Cloud links #97308

Closed

legrego merged commit ec8ff3a into elastic:master Apr 23, 2021

kibanamachine mentioned this pull request Apr 23, 2021

[7.13] Only add cloud-specific links for superusers (#97870) #98170

Merged

kibanamachine mentioned this pull request Apr 23, 2021

[7.x] Only add cloud-specific links for superusers (#97870) #98171

Merged

legrego deleted the cloud/user-links-for-superusers branch April 23, 2021 19:00

legrego mentioned this pull request May 14, 2021

[Workplace Search] Add Account Settings page imported from Security plugin #99791

Merged

2 tasks

azasypkin mentioned this pull request Jul 19, 2022

Adding check to show/hide Avatar form based on whether the user is a … #135743

Merged

Conversation

legrego commented Apr 21, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

Link examples

Uh oh!

legrego commented Apr 22, 2021

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

azasypkin left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

azasypkin left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

kibanamachine commented Apr 23, 2021

💚 Build Succeeded

Metrics [docs]

Page load bundle

History

Uh oh!

kibanamachine commented Apr 23, 2021

💚 Backport successful

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

legrego commented Apr 21, 2021 •

edited

Loading