-
Notifications
You must be signed in to change notification settings - Fork 5
Add a setting for the fields visibility #14
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
Add a setting for the fields visibility #14
Conversation
… from page config
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 added some comments to simplify the review.
// If the secret fields are hidden from constructor, this setting comes from | ||
// PageConfig, we need to lock the fields visibility. | ||
if (options.showSecretFields === false) { | ||
Private.lockFieldsVisibility(); | ||
} |
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.
Lock the visibility setting if it is explicitly false at startup (coming from page config)
export function setSecretFieldsVisibility(value: boolean): boolean { | ||
if (!fieldsVisibilityLocked && value !== secretFieldsVisibility) { | ||
secretFieldsVisibility = value; | ||
return true; | ||
} | ||
return false; | ||
} |
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.
Do not update the visibility if locked.
if (PageConfig.getOption('secretsManager-showFields') === 'false') { | ||
showSecretFieldsConfig = false; | ||
} | ||
|
||
const manager = new SecretsManager({ | ||
showSecretFields: showSecretFieldsConfig | ||
}); |
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.
Read the page config and setup the fields visibility at instantiation if required, to lock the setting.
if (!showSecretFieldsConfig) { | ||
delete settings.schema.properties?.['ShowSecretFields']; | ||
return; | ||
} |
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.
Remove the visibility setting from the schema if it locked.
cc. @afshin, I don't know why I can't add you to the reviewers from UI... |
|
||
// Otherwise listen to it to update the field visibility. | ||
const updateFieldVisibility = () => { | ||
const showSecretField = |
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.
const showSecretField = | |
if(!showSecretFieldsConfig ){ | |
return; | |
} | |
const showSecretField = |
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.
Do you think this is necessary ?
This function shouldn't be called because of the return statement just above(on the same condition), and even if it is, that should not change anything because the settings is locked in the manager.
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.
indeed!
Thanks for the review |
This PR adds a setting for the field visibility, and a signal when this setting changes.
Updating this setting doesn't change anything in UI from the secrets manager itself.
Only the extensions that uses the secrets manager (and listen for this setting) can display or not some fields.
If the settings are hidden from page config, this setting is removed from the settings panel and cannot be updated (the value is locked). This should prevent third party extensions to modify the value to display the fields (and be able to read the values from the DOM).