UI Hide Secrets Sync from nav if not on license and/or no policy permissions #27262

Monkeychip · 2024-05-29T16:32:14Z

Adding Do not merge label so this is target for 1.17.1 and does not get into GA.

ent test pass

This PR hides Secrets Sync from the sidebar navigation under the following circumstances. The decisions was made to remove the upsell badges because showing Secrets Sync to users who do not have this feature or have the feature but do not have any access to the sys/sync endpoint is an anti-pattern to how the UI currently handles showing/hiding features.

Notes:

I do not do any type of redirect or navigation prevention if a user manually finds the Secrets Sync page.
I do very little to client counts: I change the name of a getter and use the same flag service getter to show or hide the link to Secrets Sync overview.
I do not address the situation where a user—with policy permissions—still cannot activate Secrets Sync from within a namespace. That will be addressed in another PR and targeted for 1.17.1.
The permissions paths I gathered from scanning through the sync api.

These changes do the following:

Hides Secrets Sync nav from all OSS users.
Displays only one upsell badge for HVD managed clusters. We cannot tell which tier they are on. In the future this will likely change.
If you're an Enterprise user without Secrets Sync on your license, you will not see it in the sidebar nav.
If you're an Enterprise user with Secrets Sync on your license and it's NOT activated, you will see the sidebar nav (regardless of your policy permissions).

# user bob with the following policy
path "sys/sync/*" {
  capabilities = [ "deny" ]
}

path "sys/activation-flags/secrets-sync/*" {
  capabilities = [ "deny" ]
}

If you're an Enterprise user with Secrets Sync on your license and it IS activated, you will only see Secrets Sync if you have access to sys/sync endpoints.

Below is a user Bob with the ability to read on sys/sync/destinations only, and he can see it.

Below is a user Maria with the default policy (no access to sys/sync), she can not see it.

ui/lib/sync/addon/components/sync-header.hbs

ui/lib/sync/addon/routes/secrets.ts

github-actions · 2024-05-29T16:42:34Z

CI Results:
All Go tests succeeded! ✅

ui/tests/integration/components/clients/counts/nav-bar-test.js

…ures after actication

andaley · 2024-05-29T23:13:11Z

ui/lib/sync/addon/components/secrets/sync-activation-modal.ts

@@ -43,7 +43,7 @@ export default class SyncActivationModal extends Component<Args> {
        .adapterFor('application')
        .ajax('/v1/sys/activation-flags/secrets-sync/activate', 'POST', { namespace });
      // must refresh and not transition because transition does not refresh the model from within a namespace
-      yield this.router.refresh();
+      yield this.router.refresh('vault.cluster');


you can probably take this or leave it, but i tried explicitly passing in the parent route that we want refreshed here to see if it'd help us assert that the get activated features was successfully re-fetched. i tried this based on the docs i read here:

doing this works fine in the UI, and after adding a bunch of console.log statements i can see even in the test output that _/activation-flags is fetched after a POST, but i can't get the assertions working how i'd like. because we have these assertions that effectively test that the UI updates once the feature is activated i think we're safe skipping the extra assertions.

ui/tests/acceptance/sync/secrets/overview-test.js

Monkeychip · 2024-05-30T17:29:26Z

ui/app/services/permissions.js

The list here came from me scanning through the sync API. We should test lots of policies to make sure I've gotten all the options.

github-actions · 2024-05-30T17:36:44Z

Build Results:
All builds succeeded! ✅

andaley

code here looks good! coupla small comments and then i'll circle back to this to do a thorough QA. nice job!

ui/app/components/clients/page/counts.hbs

ui/app/services/permissions.js

andaley · 2024-05-30T18:45:54Z

ui/tests/acceptance/sync/secrets/overview-test.js

+      syncHandlers(this.server);
+      await authPage.login();


a note for other code reviewers: Angel and i moved authPage.login out of the top-level module and into these individual ones because fetchActivatedFeatures is called at the vault.cluster route, which happens before the user is authenticated. as a result auth.login was executing and calling fetchActivatedFeatures before our mirage overrides like those on lines 74-95 had the chance to intercept the request.

andaley · 2024-05-30T18:53:35Z

ui/tests/unit/services/flags-test.js

+          // the activate endpoint is located at a different path than sys/sync.
+          // the expected UX experience: if the feature is not activated, regardless of permissions
+          // the user should see the landing page and a banner that tells them to either have an admin activate the feature or activate it themselves


very helpful comment! 👏

hellobontempo · 2024-05-30T21:50:46Z

ui/app/services/flags.ts

    // Response could change between user sessions.
    // Fire off endpoint without checking if activated features are already set.
+    if (this.version.isCommunity) return;


Edge case question - if a customer downgrades from enterprise, are their activated flags preserved? (Not necessary for this PR, mostly just curious)

I think I've asked Robert this, and more or less no. I think because there's a conditional wrapping that endpoint which is checking for enterprise, so it would return nothing. That being said I haven't tested this workflow.

hellobontempo · 2024-05-30T21:52:52Z

ui/app/services/flags.ts

+    if (!isEnterprise) return false;
+    if (isHvdManaged) return true;


Are HVD clusters enterprise? ~ til ~ I was going to suggest rearranging this order, but disregard if HVD clusters are enterprise

they sure are 🙂

ui/lib/sync/addon/components/sync-header.hbs

ui/app/components/clients/page/counts.hbs

hellobontempo · 2024-05-30T22:12:53Z

ui/app/routes/vault/cluster.js

@@ -63,6 +63,10 @@ export default Route.extend(ModelBoundaryRoute, ClusterRoute, {
      'Cannot use VAULT_CLOUD_ADMIN_NAMESPACE flag with non-enterprise Vault version',
      !(managedRoot && this.version.isCommunity)
    );
+
+    // activatedFlags are called this high in routing to return a response used to show/hide Secrets sync on sidebar nav.


Thank you for this comment!

hellobontempo · 2024-05-30T22:21:56Z

ui/app/services/flags.ts

@@ -86,4 +88,21 @@ export default class flagsService extends Service {
      this.secretsSyncActivatePath.get('canUpdate') !== false
    );
  }
+
+  get showSecretsSync() {


Could you explain your reasoning for putting this getter here? Since it's compiling logic from flags, permissions and versions services for just secret sync, it feels a little arbitrary living in the flag service, which I understood to be just for returning info about cluster flags.

Though looking at the other getter here canActivateSecretsSync it seems like this flags service is also sort of a secrets sync permissions service 😅

I realize we're limited on time for this PR, but want to suggest that a future improvement is maybe separating or clarifying concerns here. Perhaps that means having another service (auth service?) responsible for managing user state related logic

Noelle and I chatted about this. Because we're using it in more than one places, and the concept is to keep access to this route consistent in both clients and sidebar nav we opted to leave it in the service.

But yes agree, flags (similar to what has happened to version service) is growing past it's original scope.

hellobontempo · 2024-05-30T22:29:57Z

ui/tests/acceptance/sync/secrets/overview-test.js

+        await runCmd(`write sys/namespaces/admin -f`, false);
+        await authPage.loginNs('admin');
+        await runCmd(`write sys/namespaces/foo -f`, false);
+        await authPage.loginNs('admin/foo');


Is the full test suite passing locally? Typically with namespace tests we have to manually clear the auth form namespace query params otherwise they stick for subsequent (unrelated) test runs.

https://github.com/hashicorp/vault/blob/main/ui/tests/acceptance/secrets/backend/kv/kv-v2-workflow-edge-cases-test.js#L422

I'm really struggling with replication and oidc tests running enterprise locally, but chunking it out between these two suites everything passes.

Also these tests were not changed/added just moved because we deleted things

hellobontempo · 2024-05-30T22:32:56Z

ui/tests/integration/components/sidebar/nav/cluster-test.js

@@ -84,31 +84,6 @@ module('Integration | Component | sidebar-nav-cluster', function (hooks) {
    });
  });

-  test('it should render badge for promotional links on community version', async function (assert) {


Should we have a test that it renders the HVD plus badge?

It still does, just farther down https://github.com/hashicorp/vault/blob/main/ui/app/services/permissions.js#L254

hellobontempo · 2024-05-30T22:41:33Z

ui/tests/unit/services/flags-test.js

+      this.version.type = 'community';
+
+      this.server.get('sys/activation-flags', () => {
+        assert.true(false, 'activation-flags is not called');


If we don't want this called, could we have this endpoint throw an error? This can make it a little clearer that the assertion inside that mirage request should not actually be hit

vault/ui/tests/integration/components/dashboard/overview-test.js

Lines 105 to 108 in 1e8eefa

this.server.get(

'sys/internal/counters/activity',

() => new Error('uh oh! a request was made to sys/internal/counters/activity')

);

this.server.get( 'sys/activation-flags', () => new Error('uh oh! a request was made to sys/activation-flags, this should not happen for community versions') );

👆 love that idea!

@noelledaley You gave it to me on one of my recent PRs! 🎉 😄

hellobontempo · 2024-05-30T22:45:43Z

ui/app/services/permissions.js

@@ -49,6 +49,12 @@ const API_PATHS = {
  settings: {
    customMessages: 'sys/config/ui/custom-messages',
  },
+  sync: {


I'm still learning out this service works...but do you know why just having

sync: 'sys/sync/',

which ends in a / isn't sufficient here?

nope! 🙃 This is what I pinged Chelsea about in the dev channel. The logic of comparison is backwards from what you'd think they be comparing. I'll ping you on that thread which has links, etc. But it's also something that I can more easily clarify in standup.

* routes: add redirect if user does not have access to sync * tests: verify redirect on sync overview page happens * tests: organize tests modules to ensure enterprise is explicitly set up

hellobontempo · 2024-06-10T18:02:52Z

ui/lib/sync/addon/routes/secrets.ts

@@ -13,13 +13,8 @@ export default class SyncSecretsRoute extends Route {
  @service declare readonly router: RouterService;
  @service declare readonly flags: FlagService;

-  beforeModel() {


Commented in slack about refresh ramifications removing this fetch to only happen in the cluster.js route

hellobontempo

Nice work! Just the question about fetching the activation flags

intial changes, haven't tested client counts or done test coverage

64c2fd0

Monkeychip added ui backport/1.17.x labels May 29, 2024

Monkeychip added this to the 1.17.0 milestone May 29, 2024

github-actions bot added the hashicorp-contributed-pr If the PR is HashiCorp (i.e. not-community) contributed label May 29, 2024

Monkeychip commented May 29, 2024

View reviewed changes

ui/lib/sync/addon/components/sync-header.hbs Show resolved Hide resolved

Monkeychip commented May 29, 2024

View reviewed changes

ui/lib/sync/addon/routes/secrets.ts Outdated Show resolved Hide resolved

client count rename getter to clairfy

931a926

Monkeychip commented May 29, 2024

View reviewed changes

ui/tests/integration/components/clients/counts/nav-bar-test.js Outdated Show resolved Hide resolved

Monkeychip and others added 6 commits May 29, 2024 12:59

fix has-permission api-paths

ac6b262

wip

b1ad18f

wip

09162c4

fix: explicitly refresh vault.cluster model to re-fetch activatedFeat…

e67076e

…ures after actication

tests: fix # of assertions for verifying that activation was called

e69075b

tests: tidy overview-test

27df01a

andaley reviewed May 29, 2024

View reviewed changes

ui/tests/acceptance/sync/secrets/overview-test.js Outdated Show resolved Hide resolved

Monkeychip and others added 3 commits May 30, 2024 09:41

add additional api permission path and move fetch back to application

7fc2fd7

add test coverage for the service

36fbb78

Merge branch 'main' into ui/VAULT-27608/hide-secrets-sync-when-no-access

1f2a88b

Monkeychip commented May 30, 2024

View reviewed changes

Monkeychip marked this pull request as ready for review May 30, 2024 17:30

Monkeychip requested a review from a team as a code owner May 30, 2024 17:30

vercel bot deployed to Preview May 30, 2024 17:35 View deployment

Monkeychip added 3 commits May 30, 2024 11:44

cleanup

462d258

remove test that checked for upsell without license or on community

9f1ab09

small comment change

e4e1080

andaley reviewed May 30, 2024

View reviewed changes

hellobontempo reviewed May 30, 2024

View reviewed changes

ui/lib/sync/addon/components/sync-header.hbs Outdated Show resolved Hide resolved

hellobontempo reviewed May 30, 2024

View reviewed changes

ui/app/components/clients/page/counts.hbs Outdated Show resolved Hide resolved

hellobontempo reviewed May 30, 2024

View reviewed changes

Monkeychip removed this from the 1.17.0 milestone May 31, 2024

Monkeychip added 2 commits May 31, 2024 10:36

small nit changes from pr reviews

22ce68a

add defaults to sync mirage handler

6fd9b21

Monkeychip requested review from andaley, hashishaw and hellobontempo June 3, 2024 15:26

Monkeychip added this to the 1.17.1 milestone Jun 3, 2024

Gate sync overview route for users without access (#27320)

51bf477

* routes: add redirect if user does not have access to sync * tests: verify redirect on sync overview page happens * tests: organize tests modules to ensure enterprise is explicitly set up

Monkeychip added the do-not-merge label Jun 3, 2024

Merge branch 'main' into ui/VAULT-27608/hide-secrets-sync-when-no-access

e6b9811

vercel bot deployed to Preview June 4, 2024 15:03 View deployment

Monkeychip and others added 2 commits June 4, 2024 10:07

add type enterprise required now because we do a check for this first

8f0fed0

Merge branch 'main' into ui/VAULT-27608/hide-secrets-sync-when-no-access

0e598d8

vercel bot deployed to Preview June 10, 2024 15:07 View deployment

fix oss test

e8bf2c4

Monkeychip removed the do-not-merge label Jun 10, 2024

hellobontempo reviewed Jun 10, 2024

View reviewed changes

hellobontempo approved these changes Jun 10, 2024

View reviewed changes

Monkeychip merged commit 927da85 into main Jun 11, 2024
34 checks passed

Monkeychip deleted the ui/VAULT-27608/hide-secrets-sync-when-no-access branch June 11, 2024 14:20

hc-github-team-secure-vault-core mentioned this pull request Jun 11, 2024

Backport of UI Hide Secrets Sync from nav if not on license and/or no policy permissions into release/1.17.x #27436

Merged

1 task

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

UI Hide Secrets Sync from nav if not on license and/or no policy permissions #27262

UI Hide Secrets Sync from nav if not on license and/or no policy permissions #27262

Monkeychip commented May 29, 2024 •

edited

Loading

github-actions bot commented May 29, 2024 •

edited

Loading

andaley May 29, 2024 •

edited

Loading

Monkeychip May 30, 2024

github-actions bot commented May 30, 2024 •

edited

Loading

andaley left a comment

andaley May 30, 2024

andaley May 30, 2024

hellobontempo May 30, 2024

Monkeychip May 31, 2024 •

edited

Loading

hellobontempo May 30, 2024 •

edited

Loading

andaley May 30, 2024

hellobontempo May 30, 2024

hellobontempo May 30, 2024

Monkeychip May 31, 2024

hellobontempo May 30, 2024 •

edited

Loading

Monkeychip May 31, 2024 •

edited

Loading

hellobontempo May 30, 2024

Monkeychip May 31, 2024

hellobontempo May 30, 2024

andaley May 30, 2024

hellobontempo May 31, 2024

hellobontempo May 30, 2024

Monkeychip May 31, 2024

hellobontempo Jun 10, 2024

hellobontempo left a comment

		if (!isEnterprise) return false;
		if (isHvdManaged) return true;

	this.server.get(
	'sys/internal/counters/activity',
	() => new Error('uh oh! a request was made to sys/internal/counters/activity')
	);

UI Hide Secrets Sync from nav if not on license and/or no policy permissions #27262

UI Hide Secrets Sync from nav if not on license and/or no policy permissions #27262

Conversation

Monkeychip commented May 29, 2024 • edited Loading

github-actions bot commented May 29, 2024 • edited Loading

andaley May 29, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

github-actions bot commented May 30, 2024 • edited Loading

andaley left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Monkeychip May 31, 2024 • edited Loading

Choose a reason for hiding this comment

hellobontempo May 30, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hellobontempo May 30, 2024 • edited Loading

Choose a reason for hiding this comment

Monkeychip May 31, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hellobontempo left a comment

Choose a reason for hiding this comment

Monkeychip commented May 29, 2024 •

edited

Loading

github-actions bot commented May 29, 2024 •

edited

Loading

andaley May 29, 2024 •

edited

Loading

github-actions bot commented May 30, 2024 •

edited

Loading

Monkeychip May 31, 2024 •

edited

Loading

hellobontempo May 30, 2024 •

edited

Loading

hellobontempo May 30, 2024 •

edited

Loading

Monkeychip May 31, 2024 •

edited

Loading