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.

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 Database Secrets Engine (MongoDB) #10655

Merged
merged 147 commits into from
Feb 18, 2021
Merged

Conversation

Monkeychip
Copy link
Contributor

@Monkeychip Monkeychip commented Jan 5, 2021

Adding the Database Secret Engine to the UI. Specifically the MongoDb secret engine.

Screenshots below.

Create Connection

Rotate Root Creds after save on create connection

Static Role creation

View Connection

List Roles and actions

Overview page with full access

Generate Static Role Creds

Generate Dynamic Role Creds

@Monkeychip Monkeychip added the ui label Jan 5, 2021
Monkeychip and others added 25 commits January 5, 2021 13:13
…Overview tab, but have link to route. It's going be on the secret header list component
…rendered conditionally on secret list header
…orp/vault into ui/database-secrets-engine-enable
…orp/vault into ui/database-secrets-engine-enable
Copy link

@backspace backspace left a comment

Choose a reason for hiding this comment

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

I didn’t do an exhaustive review but I left some comments and observations, hopefully of some use! 💞

ui/app/adapters/database/connection.js Show resolved Hide resolved
adapter
.rotateRootCredentials(backend, id)
.then(() => {
this.flashMessages.success(`Success: ${id} connection was reset`);

Choose a reason for hiding this comment

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

I guess this is a copypaste error, it should probably be a different message?

The similarity of these two and the ones in components/database-connection makes me wonder whether a utility or somesuch might help DRY this up 🤔

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Making a ticket for this.

const STATEMENT_FIELDS = {
static: {
default: ['creation_statements', 'revocation_statements', 'rotation_statements'],
'mongodb-database-plugin': 'NONE', // will not show the section

Choose a reason for hiding this comment

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

Would [] work here vs a magic string?

Copy link
Contributor

Choose a reason for hiding this comment

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

falsey values return an empty state in the template, whereas truthy ones return the form (which iterates over this array). We needed one more state where the state is not empty but we don't want to show the section (Because it's not required for the type of role)... but I think with some refactoring we could probably make an empty array work 🤔

if (!Array.isArray(fields)) return fields;
const filtered = this.args.attrs.filter(a => {
const includes = fields.includes(a.name);
return includes;

Choose a reason for hiding this comment

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

maybe this could be inline vs using an immediately-returned intermediate variable?

const filtered = this.args.attrs.filter(a => fields.includes(a.name));

Even const filtered could maybe be removed in favour of returning the filter call; if the variable name were something more descriptive like argsIncludedInFields it might make sense to preserve as an explanation, but filtered being the result of a call to filter doesn’t tell us much 🤓

Copy link
Contributor Author

@Monkeychip Monkeychip Feb 17, 2021

Choose a reason for hiding this comment

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

Making a ticket for this.

this.roleType = 'static';
return;
} catch (error) {
errors.push(error.errors);

Choose a reason for hiding this comment

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

Have you been able to try this out with real errors? Given the name errors, would it make more sense to append them individually vs pushing an array into the errors array?

Choose a reason for hiding this comment

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

but also, maybe nothing is ever done with errors? 😳

Copy link
Contributor Author

@Monkeychip Monkeychip Feb 17, 2021

Choose a reason for hiding this comment

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

Making a ticket for this.

@buttonClasses="toolbar-link"
@onConfirmAction={{action 'delete'}}
@confirmTitle="Delete role?"
@confirmMessage="This role will be permanently deleted. You will need to re-create it to use it again."

Choose a reason for hiding this comment

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

I see some other messages use recreate so maybe it should be here too? 🤔

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Making a ticket for this.

@@ -72,6 +72,9 @@ export default Component.extend({
return '';
}),

// This is only used for `optional-text` editType
showInput: false,

Choose a reason for hiding this comment

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

Should this be added to the documented arguments at the top?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Making a ticket for this.

@@ -33,9 +34,11 @@ export default Component.extend({
isVisible: or('alwaysRender', 'value'),

alwaysRender: false,
renderBlank: false,

Choose a reason for hiding this comment

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

Should this also be documented? I am a non-knower of the conventions here though 😆

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Making a ticket for this.

ui/lib/core/addon/components/readonly-form-field.js Outdated Show resolved Hide resolved
@vercel vercel bot temporarily deployed to Preview – vault February 18, 2021 15:36 Inactive
@vercel vercel bot temporarily deployed to Preview – vault-storybook February 18, 2021 15:36 Inactive
@chelshaw chelshaw merged commit 4be76bf into master Feb 18, 2021
@chelshaw chelshaw deleted the ui/database-secrets-engine-enable branch February 18, 2021 16:36
catsby added a commit that referenced this pull request Feb 19, 2021
* master:
  Adds API docs for max_age role parameter of JWT/OIDC auth method (#10916)
  UI/Database Secrets Engine cleanup (#10949)
  helper/metricsutil: Prevent potential Ticker leak (#10913)
  core/expiration: Add backoff jitter to the expiration retries (#10937)
  Revert "Vault Dependency Upgrades [VAULT-871] (#10903)" (#10939)
  Vault Dependency Upgrades [VAULT-871] (#10903)
  Add docs for Agent's template_retry option added in #10644, based on those from consul-template configuration.  Also fix some existing config docs that weren't adhering to our conventions. (#10911)
  UI Database Secrets Engine (MongoDB) (#10655)
  OpenAPI - Don't panic if field isn't found (#10929)
  Vault-1403 Switch Expiration Manager to use Fairsharing Backpressure (#1709) (#10932)
  Update KV Secrets Engine index (#10933)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants