-
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: Add missing auth config params #25646
UI: Add missing auth config params #25646
Conversation
|
||
// token_type should not be tuneable for the token auth method. | ||
if (this.model.methodType === 'token') { | ||
delete data.token_type; | ||
} | ||
|
||
this.model.userLockoutConfig.apiParams.forEach((attr) => { |
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 debated doing this in the serializer...but these models are already quite tangled up between tune actions, mount config relationships and auth mounts vs secret engine mounts.
Since there was already some data manipulation here (adding description
) above this seemed like a reasonable place for this so that moving the logic when we refactor would be easier.
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 it worth maybe adding test coverage to make sure that the values are sent in the tune request as expected by triggering a save in the component?
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.
yes! I was planning to add those but was waiting for folks to approve this logic happening here instead of the serializer.
@@ -68,19 +68,29 @@ export default class AuthMethodModel extends Model { | |||
return this.local ? 'local' : 'replicated'; | |||
} | |||
|
|||
userLockoutConfig = { |
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.
Originally I defined these in the component above, but old ember components are strict about what can be defined as a default and it didn't like objects or arrays as default variable values. Since I wanted to access these in the template, I defined them here.
CI Results: |
@@ -94,7 +104,7 @@ export default class AuthMethodModel extends Model { | |||
'accessor', | |||
'local', | |||
'sealWrap', | |||
'config.{listingVisibility,defaultLeaseTtl,maxLeaseTtl,tokenType,auditNonHmacRequestKeys,auditNonHmacResponseKeys,passthroughRequestHeaders}', | |||
'config.{listingVisibility,defaultLeaseTtl,maxLeaseTtl,tokenType,auditNonHmacRequestKeys,auditNonHmacResponseKeys,passthroughRequestHeaders,allowedResponseHeaders,pluginVersion}', |
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.
didn't add any user_lockout_config
here because they are only listed under tune
attrs in the docs
Build Results: |
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.
Very thoughtful approach for maximum backportability! I think the only things blocking me from approving are more test coverage around the expected new parameters. Otherwise looks great, thanks for this!
|
||
// token_type should not be tuneable for the token auth method. | ||
if (this.model.methodType === 'token') { | ||
delete data.token_type; | ||
} | ||
|
||
this.model.userLockoutConfig.apiParams.forEach((attr) => { |
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 it worth maybe adding test coverage to make sure that the values are sent in the tune request as expected by triggering a save in the component?
I wanted to make adding these parameters as nonintrusive as possible so this could be backported. There
MountConfig
model is shared between secret engine and auth mounts and the options component inherits from the auth config.... so overall there's some untangling and updating that could happen here. But again, held off so this would be a small change to add these parametersallowed_response_headers
plugin_version
user_lockout_config