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.

Sign up for GitHub

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: Move useOpenApi and getHelpUrl methods to util #27764

Merged
merged 4 commits into from
Jul 15, 2024
Merged

UI: Move useOpenApi and getHelpUrl methods to util #27764

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
bf8aec2
Add map between model types and helpUrls, update tests
hashishaw Jul 11, 2024
195c915
replace modelProto.getHelpUrl with new helper util
hashishaw Jul 11, 2024
f826de8
Remove all useOpenApi and getHelpUrl instances from models
hashishaw Jul 11, 2024
3571fac
Add missing auth config model type
hashishaw Jul 11, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 0 additions & 3 deletions ui/app/models/auth-config.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,4 @@ import Model, { belongsTo } from '@ember-data/model';

export default Model.extend({
backend: belongsTo('auth-method', { inverse: 'authConfigs', readOnly: true, async: false }),
getHelpUrl: function (backend) {
return `/v1/auth/${backend}/config?help=1`;
},
});
1 change: 0 additions & 1 deletion ui/app/models/auth-config/azure.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,6 @@ import { combineFieldGroups } from 'vault/utils/openapi-to-attrs';
import fieldToAttrs from 'vault/utils/field-to-attrs';

export default AuthConfig.extend({
useOpenAPI: true,
tenantId: attr('string', {
label: 'Tenant ID',
helpText: 'The tenant ID for the Azure Active Directory organization',
Expand Down
1 change: 0 additions & 1 deletion ui/app/models/auth-config/gcp.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,6 @@ import { combineFieldGroups } from 'vault/utils/openapi-to-attrs';
import fieldToAttrs from 'vault/utils/field-to-attrs';

export default AuthConfig.extend({
useOpenAPI: true,
// We have to leave this here because the backend doesn't support the file type yet.
credentials: attr('string', {
editType: 'file',
Expand Down
1 change: 0 additions & 1 deletion ui/app/models/auth-config/github.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,6 @@ import fieldToAttrs from 'vault/utils/field-to-attrs';
import { combineFieldGroups } from 'vault/utils/openapi-to-attrs';

export default AuthConfig.extend({
useOpenAPI: true,
organization: attr('string'),
baseUrl: attr('string', {
label: 'Base URL',
Expand Down
1 change: 0 additions & 1 deletion ui/app/models/auth-config/jwt.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,6 @@ import fieldToAttrs from 'vault/utils/field-to-attrs';
import { combineFieldGroups } from 'vault/utils/openapi-to-attrs';

export default AuthConfig.extend({
useOpenAPI: true,
oidcDiscoveryUrl: attr('string', {
label: 'OIDC discovery URL',
helpText:
Expand Down
1 change: 0 additions & 1 deletion ui/app/models/auth-config/kubernetes.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,7 +11,6 @@ import { combineFieldGroups } from 'vault/utils/openapi-to-attrs';
import fieldToAttrs from 'vault/utils/field-to-attrs';

export default AuthConfig.extend({
useOpenAPI: true,
kubernetesHost: attr('string', {
helpText:
'Host must be a host string, a host:port pair, or a URL to the base of the Kubernetes API server.',
Expand Down
1 change: 0 additions & 1 deletion ui/app/models/auth-config/ldap.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,7 +11,6 @@ import fieldToAttrs from 'vault/utils/field-to-attrs';
import { combineFieldGroups } from 'vault/utils/openapi-to-attrs';

export default AuthConfig.extend({
useOpenAPI: true,
certificate: attr({
label: 'Certificate',
editType: 'file',
Expand Down
1 change: 0 additions & 1 deletion ui/app/models/auth-config/okta.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,6 @@ import fieldToAttrs from 'vault/utils/field-to-attrs';
import { combineFieldGroups } from 'vault/utils/openapi-to-attrs';

export default AuthConfig.extend({
useOpenAPI: true,
orgName: attr('string', {
helpText: 'Name of the organization to be used in the Okta API',
}),
Expand Down
1 change: 0 additions & 1 deletion ui/app/models/auth-config/radius.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,6 @@ import { combineFieldGroups } from 'vault/utils/openapi-to-attrs';
import fieldToAttrs from 'vault/utils/field-to-attrs';

export default AuthConfig.extend({
useOpenAPI: true,
host: attr('string'),
secret: attr('string'),

Expand Down
4 changes: 0 additions & 4 deletions ui/app/models/kmip/config.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,11 +9,7 @@ import { combineFieldGroups } from 'vault/utils/openapi-to-attrs';
import fieldToAttrs from 'vault/utils/field-to-attrs';

export default Model.extend({
useOpenAPI: true,
ca: belongsTo('kmip/ca', { async: false, inverse: 'config' }),
getHelpUrl(path) {
return `/v1/${path}/config?help=1`;
},

fieldGroups: computed('newFields', function () {
let groups = [{ default: ['listenAddrs', 'connectionTimeout'] }];
Expand Down
5 changes: 1 addition & 4 deletions ui/app/models/kmip/role.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -36,13 +36,10 @@ export const COMPUTEDS = {
};

export default Model.extend(COMPUTEDS, {
useOpenAPI: true,
backend: attr({ readOnly: true }),
scope: attr({ readOnly: true }),
name: attr({ readOnly: true }),
getHelpUrl(path) {
return `/v1/${path}/scope/example/role/example?help=1`;
},

fieldGroups: computed('fields', 'defaultFields.length', 'tlsFields', function () {
const groups = [{ TLS: this.tlsFields }];
if (this.defaultFields.length) {
Expand Down
7 changes: 0 additions & 7 deletions ui/app/models/pki/certificate/base.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,6 @@
*/

import Model, { attr } from '@ember-data/model';
import { assert } from '@ember/debug';
import { service } from '@ember/service';
import { withFormFields } from 'vault/decorators/model-form-fields';
import lazyCapabilities, { apiPath } from 'vault/macros/lazy-capabilities';
Expand All @@ -25,15 +24,9 @@ const certDisplayFields = ['certificate', 'commonName', 'revocationTime', 'seria
export default class PkiCertificateBaseModel extends Model {
@service secretMountPath;

get useOpenAPI() {
return true;
}
get backend() {
return this.secretMountPath.currentPath;
}
getHelpUrl() {
assert('You must provide a helpUrl for OpenAPI', true);
}

// The attributes parsed from parse-pki-cert util live here
@attr parsedCertificate;
Expand Down
3 changes: 0 additions & 3 deletions ui/app/models/pki/certificate/generate.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -34,8 +34,5 @@ const certDisplayFields = [
];
@withFormFields(certDisplayFields, generateFromRole)
export default class PkiCertificateGenerateModel extends PkiCertificateBaseModel {
getHelpUrl(backend) {
return `/v1/${backend}/issue/example?help=1`;
}
@attr('string') role; // role name to issue certificate against for request URL
}
3 changes: 0 additions & 3 deletions ui/app/models/pki/certificate/sign.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,9 +23,6 @@ const generateFromRole = [
];
@withFormFields(null, generateFromRole)
export default class PkiCertificateSignModel extends PkiCertificateBaseModel {
getHelpUrl(backend) {
return `/v1/${backend}/sign/example?help=1`;
}
@attr('string') role; // role name to create certificate against for request URL

@attr('string', {
Expand Down
8 changes: 0 additions & 8 deletions ui/app/models/pki/config/acme.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,16 +10,8 @@ import lazyCapabilities, { apiPath } from 'vault/macros/lazy-capabilities';
@withFormFields()
export default class PkiConfigAcmeModel extends Model {
// This model uses the backend value as the model ID
get useOpenAPI() {
return true;
}

getHelpUrl(backendPath) {
return `/v1/${backendPath}/config/acme?help=1`;
}

// attrs order in the form is determined by order here

@attr('boolean', {
label: 'ACME enabled',
subText: 'When ACME is disabled, all requests to ACME directory URLs will return 404.',
Expand Down
7 changes: 0 additions & 7 deletions ui/app/models/pki/config/cluster.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,13 +10,6 @@ import lazyCapabilities, { apiPath } from 'vault/macros/lazy-capabilities';
@withFormFields()
export default class PkiConfigClusterModel extends Model {
// This model uses the backend value as the model ID
get useOpenAPI() {
return true;
}

getHelpUrl(backendPath) {
return `/v1/${backendPath}/config/cluster?help=1`;
}

@attr('string', {
label: "Mount's API path",
Expand Down
6 changes: 0 additions & 6 deletions ui/app/models/pki/config/urls.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,12 +10,6 @@ import lazyCapabilities, { apiPath } from 'vault/macros/lazy-capabilities';
@withFormFields()
export default class PkiConfigUrlsModel extends Model {
// This model uses the backend value as the model ID
get useOpenAPI() {
return true;
}
getHelpUrl(backendPath) {
return `/v1/${backendPath}/config/urls?help=1`;
}

@attr({
label: 'Issuing certificates',
Expand Down
5 changes: 1 addition & 4 deletions ui/app/models/pki/issuer.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -27,10 +27,7 @@ const displayFields = [
@withFormFields(inputFields, displayFields)
export default class PkiIssuerModel extends Model {
@service secretMountPath;
// TODO use openAPI after removing route extension (see pki/roles route for example)
get useOpenAPI() {
return false;
}

get backend() {
return this.secretMountPath.currentPath;
}
Expand Down
8 changes: 0 additions & 8 deletions ui/app/models/pki/role.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,14 +18,6 @@ const validations = {
export default class PkiRoleModel extends Model {
@service version; // noStoreMetadata is enterprise-only, so we need this available

get useOpenAPI() {
// must be a getter so it can be accessed in path-help.js
return true;
}
getHelpUrl(backend) {
return `/v1/${backend}/roles/example?help=1`;
}

@attr('string', { readOnly: true }) backend;

get formFieldGroups() {
Expand Down
4 changes: 0 additions & 4 deletions ui/app/models/pki/sign-intermediate.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -25,10 +25,6 @@ const validations = {
'maxPathLength',
])
export default class PkiSignIntermediateModel extends PkiCertificateBaseModel {
getHelpUrl(backend) {
return `/v1/${backend}/issuer/example/sign-intermediate?help=1`;
}

@attr issuerRef;

@attr('string', {
Expand Down
8 changes: 0 additions & 8 deletions ui/app/models/pki/tidy.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -125,14 +125,6 @@ export default class PkiTidyModel extends Model {
})
tidyRevokedCerts;

get useOpenAPI() {
return true;
}

getHelpUrl(backend) {
return `/v1/${backend}/config/auto-tidy?help=1`;
}

get allGroups() {
const groups = [{ autoTidy: ['enabled', 'intervalDuration'] }, ...this.sharedFields];
return this._expandGroups(groups);
Expand Down
4 changes: 0 additions & 4 deletions ui/app/models/role-ssh.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -47,10 +47,6 @@ const CA_FIELDS = [
];

export default Model.extend({
useOpenAPI: true,
getHelpUrl: function (backend) {
return `/v1/${backend}/roles/example?help=1`;
},
zeroAddress: attr('boolean', {
readOnly: true,
}),
Expand Down
12 changes: 6 additions & 6 deletions ui/app/services/path-help.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -26,6 +26,7 @@ import {
filterPathsByItemType,
pathToHelpUrlSegment,
reducePathsByPathName,
getHelpUrlForModel,
} from 'vault/utils/openapi-helpers';
import { isPresent } from '@ember/utils';

Expand Down Expand Up @@ -53,17 +54,17 @@ export default Service.extend({
const modelName = `model:${modelType}`;

const modelFactory = owner.factoryFor(modelName);
let newModel, helpUrl;
let helpUrl = getHelpUrlForModel(modelType, backend);

let newModel;
// if we have a factory, we need to take the existing model into account
if (modelFactory) {
debug(`Model factory found for ${modelType}`);
newModel = modelFactory.class;
const modelProto = newModel.proto();
if (newModel.merged || modelProto.useOpenAPI !== true) {
if (newModel.merged || !helpUrl) {
return resolve();
}

helpUrl = modelProto.getHelpUrl(backend);
return this.registerNewModelWithProps(helpUrl, backend, newModel, modelName);
} else {
debug(`Creating new Model for ${modelType}`);
Expand All @@ -73,7 +74,6 @@ export default Service.extend({
// we don't have an apiPath for dynamic secrets
// and we don't need paths for them yet
if (!apiPath) {
helpUrl = newModel.proto().getHelpUrl(backend);
return this.registerNewModelWithProps(helpUrl, backend, newModel, modelName);
}

Expand All @@ -98,7 +98,7 @@ export default Service.extend({
return reject();
}

helpUrl = `/v1/${apiPath}${path.slice(1)}?help=true` || newModel.proto().getHelpUrl(backend);
helpUrl = `/v1/${apiPath}${path.slice(1)}?help=true`;
pathInfo.paths = paths;
newModel = newModel.extend({ paths: pathInfo });
return this.registerNewModelWithProps(helpUrl, backend, newModel, modelName);
Expand Down
34 changes: 34 additions & 0 deletions ui/app/utils/openapi-helpers.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -132,3 +132,37 @@ export function filterPathsByItemType(pathInfo: PathsInfo, itemType: string): Pa
return itemType === path.itemType;
});
}

/**
* This object maps model names to the openAPI path that hydrates the model, given the backend path.
*/
const OPENAPI_POWERED_MODELS = {
'role-ssh': (backend: string) => `/v1/${backend}/roles/example?help=1`,
'auth-config/azure': (backend: string) => `/v1/auth/${backend}/config?help=1`,
'auth-config/cert': (backend: string) => `/v1/auth/${backend}/config?help=1`,
'auth-config/gcp': (backend: string) => `/v1/auth/${backend}/config?help=1`,
'auth-config/github': (backend: string) => `/v1/auth/${backend}/config?help=1`,
'auth-config/jwt': (backend: string) => `/v1/auth/${backend}/config?help=1`,
'auth-config/kubernetes': (backend: string) => `/v1/auth/${backend}/config?help=1`,
'auth-config/ldap': (backend: string) => `/v1/auth/${backend}/config?help=1`,
'auth-config/okta': (backend: string) => `/v1/auth/${backend}/config?help=1`,
'auth-config/radius': (backend: string) => `/v1/auth/${backend}/config?help=1`,
'kmip/config': (backend: string) => `/v1/${backend}/config?help=1`,
'kmip/role': (backend: string) => `/v1/${backend}/scope/example/role/example?help=1`,
'pki/role': (backend: string) => `/v1/${backend}/roles/example?help=1`,
'pki/tidy': (backend: string) => `/v1/${backend}/config/auto-tidy?help=1`,
'pki/sign-intermediate': (backend: string) => `/v1/${backend}/issuer/example/sign-intermediate?help=1`,
'pki/certificate/generate': (backend: string) => `/v1/${backend}/issue/example?help=1`,
'pki/certificate/sign': (backend: string) => `/v1/${backend}/sign/example?help=1`,
'pki/config/acme': (backend: string) => `/v1/${backend}/config/acme?help=1`,
'pki/config/cluster': (backend: string) => `/v1/${backend}/config/cluster?help=1`,
'pki/config/urls': (backend: string) => `/v1/${backend}/config/urls?help=1`,
};

export function getHelpUrlForModel(modelType: string, backend: string) {
const urlFn = OPENAPI_POWERED_MODELS[modelType as keyof typeof OPENAPI_POWERED_MODELS] as (
Copy link
Contributor

Choose a reason for hiding this comment

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

do we need both keyof and typeof here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yup, because OPENAPI_POWERED_MODELS is a const not an interface, so we need typeof so typescript is happy

backend: string
) => string;
if (!urlFn) return null;
return urlFn(backend);
}
12 changes: 5 additions & 7 deletions ui/tests/acceptance/open-api-path-help-test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,7 @@ import authPage from 'vault/tests/pages/auth';
import { deleteAuthCmd, deleteEngineCmd, mountAuthCmd, mountEngineCmd, runCmd } from '../helpers/commands';
import expectedSecretAttrs from 'vault/tests/helpers/openapi/expected-secret-attrs';
import expectedAuthAttrs from 'vault/tests/helpers/openapi/expected-auth-attrs';
import { getHelpUrlForModel } from 'vault/utils/openapi-helpers';

/**
* This set of tests is for ensuring that backend changes to the OpenAPI spec
Expand Down Expand Up @@ -72,8 +73,7 @@ function secretEngineHelper(test, secretEngine) {
// A given secret engine might have multiple models that are openApi driven
modelNames.forEach((modelName) => {
test(`${modelName} model getProps returns correct attributes`, async function (assert) {
const model = this.store.createRecord(modelName, {});
const helpUrl = model.getHelpUrl(this.backend);
const helpUrl = getHelpUrlForModel(modelName, this.backend);
const result = await this.pathHelp.getProps(helpUrl, this.backend);
const expected = engineData[modelName];
// Expected values should be updated to match "actual" (result)
Expand All @@ -98,8 +98,7 @@ function authEngineHelper(test, authBackend) {
if (itemName.startsWith('auth-config/')) {
// Config test doesn't need to instantiate a new model
test(`${itemName} model`, async function (assert) {
const model = this.store.createRecord(itemName, {});
const helpUrl = model.getHelpUrl(this.mount);
const helpUrl = getHelpUrlForModel(itemName, this.mount);
const result = await this.pathHelp.getProps(helpUrl, this.mount);
const expected = authData[itemName];
assert.deepEqual(
Expand All @@ -116,9 +115,8 @@ function authEngineHelper(test, authBackend) {
const modelName = `generated-${itemName}-${authBackend}`;
// Generated items need to instantiate the model first via getNewModel
await this.pathHelp.getNewModel(modelName, this.mount, `auth/${this.mount}/`, itemName);
const model = this.store.createRecord(modelName, {});
// Generated items don't have this method -- helpUrl is calculated in path-help.js line 101
const helpUrl = model.getHelpUrl(this.mount);
// Generated items don't have helpUrl method -- helpUrl is calculated in path-help.js line 101
Copy link
Contributor

Choose a reason for hiding this comment

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

thank you for this comment! do we have test coverage that assures removing the || newModel.proto().getHelpUrl(backend); of the conditional in path-help is alright?

From what I gather it seems like all non-generated items have paths defined in the new helper, getHelpUrlForModel and any generated items should otherwise follow the pattern of /v1/${apiPath}${path.slice(1)}?help=true - just wanted to double check!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good question! I removed the || because the first statement would never be falsey.

// original
helpUrl = `/v1/${apiPath}${path.slice(1)}?help=true` || newModel.proto().getHelpUrl(backend);

even if apiPath and path.slice(1) resolve to empty strings, the first part will resolve at minimum to /v1/?help=true.

Otherwise your comment is true -- if we have the base model for it, the helpUrl should be defined in getHelpUrlForModel. If it's generated (like for userpass users) then it follows the pattern in line 101 of path-help service

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Also, this is a skipped test -- I just didn't want any instances of getHelpUrl laying around so I could be sure I got them all 😂

Copy link
Contributor

Choose a reason for hiding this comment

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

great thinking! nice to keep up with tech debt while we have context. awesome! that all makes sense, thanks for explaining.

const helpUrl = `/v1/auth/${this.mount}?help=1`;
const result = await this.pathHelp.getProps(helpUrl, this.mount);
const expected = authData[modelName];
assert.deepEqual(result, expected, `getProps returns expected attributes for ${modelName}`);
Expand Down
Loading
Loading