add basic routes for secrets recovery by lane-wetmore · Pull Request #31412 · hashicorp/vault

lane-wetmore · 2025-08-01T21:22:35Z

Description

Adds routing for Secrets Recovery (currently hidden in production env)
Adds basic skeletons for route templates and page components

TODO only if you're a HashiCorp employee

Backport Labels: If this fix needs to be backported, use the appropriate backport/ label that matches the desired release branch. Note that in the CE repo, the latest release branch will look like backport/x.x.x, but older release branches will be backport/ent/x.x.x+ent.
- LTS: If this fixes a critical security vulnerability or severity 1 bug, it will also need to be backported to the current LTS versions of Vault. To ensure this, use all available enterprise labels.
ENT Breakage: If this PR either 1) removes a public function OR 2) changes the signature
of a public function, even if that change is in a CE file, double check that
applying the patch for this PR to the ENT repo and running tests doesn't
break any tests. Sometimes ENT only tests rely on public functions in CE
files.
Jira: If this change has an associated Jira, it's referenced either
in the PR description, commit message, or branch name.
RFC: If this change has an associated RFC, please link it in the description.
ENT PR: If this change has an associated ENT PR, please link it in the
description. Also, make sure the changelog is in this PR, not in your ENT PR.

PCI review checklist

I have documented a clear reason for, and description of, the change I am making.
If applicable, I've documented a plan to revert these changes if they require more than reverting the pull request.
If applicable, I've documented the impact of any changes to security controls.

Examples of changes to security controls include using new access control methods, adding or removing logging pipelines, etc.

vercel · 2025-08-01T21:22:46Z

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name	Status	Preview	Comments	Updated (UTC)
vault-ui	✅ Ready (Inspect)	Visit Preview	💬 Add feedback	Aug 4, 2025 10:24pm

github-actions · 2025-08-01T21:24:08Z

CI Results:
All Go tests succeeded! ✅

hellobontempo

Great work! I left some minor suggestions, and here's a draft PR of some small file changes for the routing, thought it'd be easier to explain file structure stuff paired with the relevant file 😄

hellobontempo · 2025-08-01T22:43:48Z

ui/app/components/recovery.hbs

@@ -0,0 +1,3 @@
+{{#if this.isNotProduction}}
+  {{yield}}


Since there's logic wrapping the sidenav link and the route itself, I don't think we also need a check in the component.

Is there a need for a component like this? The titles are mostly the same between views, except for the load form. I have a small re-filing suggestion but I'll open a PR to demonstrate since I think it might be easier to explain that way

I don't think so. I've gone ahead and removed it

hellobontempo · 2025-08-01T22:49:30Z

ui/app/components/sidebar/nav/cluster.hbs

      data-test-sidebar-nav-link="Secrets Sync"
    />
  {{/if}}
+  {{#if this.showSecretsRecovery}}


Could we rename this to isNotProduction since I think this name could easily be confused for production code since we may eventually have logic here that hides this route for various cluster states (like DR secondary)

hellobontempo · 2025-08-01T22:49:46Z

ui/app/components/sidebar/nav/cluster.js

  }
+
+  // TODO remove conditional once further feature work for single item recovery for release 1.21 is completed
+  get showSecretsRecovery() {


Thank you for adding the comment above! Same comment about naming here 😄

hellobontempo · 2025-08-01T22:53:09Z

ui/app/router.js

+      if (config.environment !== 'production') {
+        this.route('recovery', function () {
+          this.route('snapshots', function () {
+            this.route('load');
+            this.route('snapshot', function () {
+              this.route('manage');
+            });
+          });
+        });
+      }


I think it might be a little more intuitive if this was moved to below sync on line 19) so it sort of follows the sidebar nav order and makes it easier to find.

It's also hard to tell is the diff, but it looks like this is nested correctly so cluster is the immediate parent.

hellobontempo · 2025-08-01T22:55:33Z

ui/app/routes/vault/cluster/recovery.js

+
+import Route from '@ember/routing/route';
+
+export default class VaultClusterRecoveryRoute extends Route {}


Definitely a nit, but we typically drop VaultCluster from the route class name for brevity

hellobontempo · 2025-08-01T22:55:45Z

ui/app/routes/vault/cluster/recovery/snapshots.js

+
+import Route from '@ember/routing/route';
+
+export default class VaultClusterRecoverySnapshotsRoute extends Route {}


Suggested change

export default class VaultClusterRecoverySnapshotsRoute extends Route {}

export default class RecoverySnapshotsRoute extends Route {}

hellobontempo · 2025-08-01T23:14:05Z

ui/app/router.js

+            this.route('snapshot', function () {
+              this.route('manage');
+            });


In the RFC we had talked about this segment of the URL being a dynamic param called snapshot_id (As an aside, it's a good practice to have the url param - snapshot_id - match the param we'll use from the API.)

In practice, this means that buttons or actions that navigate to manage a snapshot would look something like:

const snapshot_id = 1234 this.router.transitionTo('recovery.snapshots.snapshot', snapshot_id)

If this is still the plan we'll need to define the path (docs) for the dynamic segment here. I'm also wondering if manage is even necessary 🤔 Part of me thinks we should just get rid of the child route since right now it's just a single view to either read or recover items. We could have the "read" view just live at snapshot. If doing more things with a snapshot becomes a thing we can always extend it in the future. What do you think?

Suggested change

this.route('snapshot', function () {

this.route('manage');

});

this.route('snapshot', { path: '/:snapshot_id } )

Yep, didn't mean to leave that out! I've added the dynamic param and removed the manage route for the time being. It'll be easy to add back in later if we decide we need it.

hellobontempo · 2025-08-01T23:16:16Z

ui/app/templates/vault/cluster/recovery/snapshots/snapshot/manage.hbs

+  SPDX-License-Identifier: BUSL-1.1
+}}
+
+<Recovery::Page::Snapshots::Snapshot::Manage />


If you agree with my routing comment above, we'll want to remember to rename this file to recovery/snapshots/snapshot.hbs (It could also just be snapshot.hbs but I kind of like when the "base" route is named index

And the page component renamed so it's: <Recovery::Page::Snapshots::Snapshot />

github-actions · 2025-08-04T21:43:06Z

Build Results:
All builds succeeded! ✅

hellobontempo · 2025-08-05T21:13:06Z

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

    });
  });

-  test('it should hide links and headings user does not have access too', async function (assert) {


hellobontempo · 2025-08-05T21:15:09Z

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

    assert
      .dom('[data-test-sidebar-nav-link]')
-      .exists({ count: 2 }, 'Nav links are hidden other than secrets and dashboard');
+      .exists({ count: 3 }, 'Nav links are hidden other than secrets, recovery and dashboard');


maybe add a todo since this will change, we'll want to gate these in the has-permission helper? Or we could update the helper api path map with the relevant routes and wrap the sidenav link now - up to you :)

emoncuso

Looks good to me! Just double check this works as expected when built for production, but otherwise good to go.

Thanks for adding a test to check visibility

emoncuso · 2025-08-05T22:08:03Z

ui/app/components/sidebar/nav/cluster.js


 import Component from '@glimmer/component';
 import { service } from '@ember/service';
+import config from 'vault/config/environment';


Oh cool, so this acts as a sort of development feature flag. Good idea!

(This can be in a separate PR) but this could be formalized a bit and drawn into a helper so you don't need the getter every time.

Yeah this is definitely a new pattern to use the environment state like this. But with the new release cycle, I foresee us developing more iteratively instead of relying on sidebranches so it would make sense to make an app-wide helper!

emoncuso · 2025-08-05T22:09:06Z

ui/app/router.js

      this.mount('config-ui');
      this.mount('sync');
+      // TODO remove conditional once further feature work for single item recovery for release 1.21 is completed
+      if (config.environment !== 'production') {


The router is a special place in ember land. You might want to double check that nothing weird happens once you build this for prod.

This is how we've been rendering the docfy docs, so far nothing weird has been reported 🙃

emoncuso · 2025-08-05T22:10:20Z

ui/app/router.js

+      // TODO remove conditional once further feature work for single item recovery for release 1.21 is completed
+      if (config.environment !== 'production') {
+        this.route('recovery', function () {
+          this.route('snapshots', function () {


[Optional] - It's 100% superflous, but I think it's nice to explicitly call out the index route in the router. I know it'd be diverent pattern in this file, but it's nice to see that there's something there at the default /

Suggested change

this.route('snapshots', function () {

this.route('snapshots', function () {

this.route('index', { path: '/' });

Lol yeah I definitely advised against this since ember docs callout it's not necessary so feels overly verbose. Plus if we explicitly defined an index route for every index that would make our router file huge and it's already a challenge to parse

lane-wetmore added 2 commits August 1, 2025 16:16

add basic routes for secrets recovery

a193a70

Merge branch 'main' into ui/VAULT-38371-secret-recovery-route-setup

06cd58f

github-actions bot added the hashicorp-contributed-pr If the PR is HashiCorp (i.e. not-community) contributed label Aug 1, 2025

vercel bot deployed to Preview – vault-ui August 1, 2025 21:23 View deployment

lint fix no index invocation

40e44eb

vercel bot deployed to Preview – vault-ui August 1, 2025 21:42 View deployment

lane-wetmore added this to the 1.21.0-rc1 milestone Aug 1, 2025

lane-wetmore added the pr/no-changelog label Aug 1, 2025

hide routes in production

304fa74

vercel bot deployed to Preview – vault-ui August 1, 2025 22:39 View deployment

hellobontempo reviewed Aug 1, 2025

View reviewed changes

update routes

c3afabb

vercel bot deployed to Preview – vault-ui August 4, 2025 21:37 View deployment

lane-wetmore marked this pull request as ready for review August 4, 2025 21:38

lane-wetmore requested a review from a team as a code owner August 4, 2025 21:38

Merge branch 'main' into ui/VAULT-38371-secret-recovery-route-setup

771a6cc

vercel bot deployed to Preview – vault-ui August 4, 2025 21:38 View deployment

add missing route js files

3284ff6

vercel bot deployed to Preview – vault-ui August 4, 2025 21:45 View deployment

add comments and clean up

080d7bf

vercel bot deployed to Preview – vault-ui August 4, 2025 21:49 View deployment

update tests

6c26cb7

vercel bot deployed to Preview – vault-ui August 4, 2025 22:24 View deployment

hellobontempo reviewed Aug 5, 2025

View reviewed changes

emoncuso reviewed Aug 5, 2025

View reviewed changes

hellobontempo approved these changes Aug 6, 2025

View reviewed changes

lane-wetmore merged commit 3d46115 into main Aug 6, 2025
34 checks passed

lane-wetmore deleted the ui/VAULT-38371-secret-recovery-route-setup branch August 6, 2025 17:02


		import Route from '@ember/routing/route';

		export default class VaultClusterRecoveryRoute extends Route {}

	export default class VaultClusterRecoverySnapshotsRoute extends Route {}
	export default class RecoverySnapshotsRoute extends Route {}

	this.route('snapshots', function () {
	this.route('snapshots', function () {
	this.route('index', { path: '/' });

Conversation

lane-wetmore commented Aug 1, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

TODO only if you're a HashiCorp employee

PCI review checklist

Uh oh!

vercel bot commented Aug 1, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

github-actions bot commented Aug 1, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

hellobontempo left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

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!

github-actions bot commented Aug 4, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

emoncuso 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!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

lane-wetmore commented Aug 1, 2025 •

edited

Loading

vercel bot commented Aug 1, 2025 •

edited

Loading

github-actions bot commented Aug 1, 2025 •

edited

Loading

hellobontempo left a comment •

edited

Loading