-
Notifications
You must be signed in to change notification settings - Fork 4.3k
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.
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
UI: Implement KV patch+subkey [enterprise] #28212
UI: Implement KV patch+subkey [enterprise] #28212
Conversation
CI Results: |
ca1196d
to
f41e93e
Compare
* build json editor patch form * finish patch component and tests * add tab to each route * and path route * add overview tab to tests * update overview to use updated_time instead of created_time * redirect relevant secret.details to secret.index * compute secretState in component instead of pass as arg * add capabilities service * add error handling to fetchSubkeys adapter request * add overview tabs to test * add subtext to overview card * remaining redirects in secret edit * remove create new version from popup menu * fix breadcrumbs for overview * separate adding capabilities service * add service to kv engine * Revert "separate adding capabilities service" This reverts commit bb70b12. * Revert "add service to kv engine" This reverts commit bfa8805. * update navigation test * consistently navigate to secret.index route to be explicit * finish overview navigation tests * add copyright header * update delete tests * fix nav testrs * cleanup secret edit redirects * remove redundant async/awaits * fix create test * edge case tests * secret acceptance tests * final component tests
…(sidebranch) (#28192) * add tab to each route * and path route * add overview tab to tests * update overview to use updated_time instead of created_time * redirect relevant secret.details to secret.index * compute secretState in component instead of pass as arg * add capabilities service * add error handling to fetchSubkeys adapter request * add patch route and put in page component * add patch secret action to subkeys card * fix component name * add patch capability * alphabetize computed capabilities * update links, cleanup selectors * fix more merge conflict stuff * add capabilities test * add models to patch link * add test for patch route * rename external route * add error templates * make notes about enterprise tests, filter one * remove errors, transition (redirect) instead * redirect patch routes
* remove @secret from metadata details * use metadata model instead of secret in paths page * put delete back into kv/data adapter * grant access in control group test * update metadata route and permissions * remove secret from parent route, only fetch in details route * change more permissions to route perms, add tests * revert overview redirect from list view * wrap model in conditional for perms * remove redundant canReadCustomMetadata check * rename adapter method * handle overview 404 * remove comment * add customMetadata as an arg * update grantAccess in test * make version param easier to follow * VAULT-30494 handle 404 jira * refactor capabilities to return an object * update create tests * add test for default truthy capabilities
f41e93e
to
c432c74
Compare
* add enterprise check for subkey card * add max height and scroll to subkey card * only fetch subkeys if enterprise * remove check in overview * add test * Update ui/tests/integration/components/kv/page/kv-page-overview-test.js
Build Results: |
* add assertion * add optional chaining * create/delete versioned secret in each module * wait for transition * add another waitUntil
* add patch latest version action to toolbar * make isPatchAllowed arg all encompassing * no longer need model check * use hash so both promises fire at the same time * add subkeys to policy * Update ui/lib/kv/addon/routes/secret.js
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great work! Just some 🧹
secret | ||
); | ||
let route, params; | ||
switch (true) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Creative! Threw me off guard 😂
@@ -81,17 +81,27 @@ export default Route.extend({ | |||
const mode = this.routeName.split('.').pop(); | |||
// for kv v2, redirect users from the old url to the new engine url (1.15.0 +) | |||
if (secretEngine.type === 'kv' && secretEngine.version === 2) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since we're cleaning up the redirect logic, I believe we should also redirect for secretEngine.type === 'generic' && secretEngine.version === 2
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm hesitant to change or add anything to this logic flow, but happy to do this as a follow on! The only improvement/change I made by reorganizing the logic is to navigate to edit
or index
accordingly instead of arbitrarily go to details
. I think during the original implementation we weren't aware we had the route info available to discern between a user either viewing or editing secret so navigate to the details route
this method returns a capabilities model for each path in the array of paths | ||
*/ | ||
async fetchMultiplePaths(paths: string[]): Promise<Array<CapabilitiesModel>> | AdapterError { | ||
async fetchMultiplePaths(paths: string[]): MultipleCapabilities | AdapterError { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
async fetchMultiplePaths(paths: string[]): MultipleCapabilities | AdapterError { | |
async fetchMultiplePaths(paths: string[]): Promise<MultipleCapabilities | AdapterError> { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This actually doesn't return a promise! On line 45 we await the request and this just returns an object with the path as the key and value as the capabilities:
{
"my/api/path": {
canCreate: true,
canDelete: true,
canList: true,
canPatch: true,
canRead: true,
canSudo: true,
canUpdate: true,
};
}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Which reminds me that I need to remove the try...catch wrapping the whole thing!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I thought any async method technically returns a promise... 🤔
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
async is just necessary so we can await the promise on line 45, the method itself is not a promise, though!
this.router.transitionTo('vault.cluster.secrets.backend.kv.secret', { | ||
queryParams: { version: this.version }, | ||
}); | ||
// TODO test/update this when model moves to details route |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this TODO done?
ui/lib/kv/addon/routes/secret.js
Outdated
}); | ||
} | ||
|
||
@action | ||
willTransition(transition) { | ||
// TODO update this comment and refreshes |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🧹
@@ -45,7 +45,7 @@ export function breadcrumbsForSecret(backend, secretPath, lastItemCurrent = fals | |||
}; | |||
} | |||
if (!isDir) { | |||
return { label: segment.label, route: 'secret.details', models: [backend, segment.model] }; | |||
return { label: segment.label, route: 'secret', models: [backend, segment.model] }; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We might have missed secret.index
here?
return { label: segment.label, route: 'secret', models: [backend, segment.model] }; | |
return { label: segment.label, route: 'secret.index', models: [backend, segment.model] }; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh yeah - I didn't change this one intentionally because I was getting strange failures but let me look back into
* add conditional for enterprise checking tabs * cleanup fetchMultiplePaths method * add test * remove todo comment, ticket created and design wants to hold off * keep transition, update comments * cleanup tests, add index to breadcrumbs * add some test coverage * toggle so value is readable
I would like to see this feature in Vault CE, because due to this feature being implemented, #28261 will remove similar feature from Vault CE. @hellobontempo |
For now, we see this as an enterprise feature as many customers have requested this capability at the enterprise level. We consider feedback from the community as part of our roadmap planning process, so please upvote the feature request issue and we may consider it in the future. |
Description
This PR adds subkey/patch to the Vault UI for enterprise users.
subkey_mov.mov
TODO only if you're a HashiCorp employee
to N, N-1, and N-2, using the
backport/ent/x.x.x+ent
labels. If this PR is in the CE repo, you should only backport to N, using thebackport/x.x.x
label, not the enterprise labels.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.
in the PR description, commit message, or branch name.
description. Also, make sure the changelog is in this PR, not in your ENT PR.