Modifications in Management Console for Multiple Client Secret Support #7675
Conversation
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
Pull request overview
This PR modifies the Service Provider configuration section in the Management Console to support multiple client secrets. When the multiple client secrets feature is enabled, the UI masks the secret value with asterisks and hides the "Regenerate" button to prevent conflicts with the new multi-secret management approach.
Key Changes:
- Add configuration constant for checking if multiple client secrets are enabled
- Mask OAuth consumer secrets with asterisks when multiple secrets feature is active
- Conditionally hide the Show/Hide toggle and Regenerate button based on the feature flag
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 5 comments.
| File | Description |
|---|---|
ApplicationMgtUIConstants.java |
Adds MULTIPLE_CLIENT_SECRETS_ENABLED constant to store the configuration property name for the multiple client secrets feature |
configure-service-provider.jsp |
Implements UI logic to mask secrets and hide regenerate functionality when multiple client secrets are enabled, including conditional rendering of Show/Hide button and Regenerate action link |
| value="<%=Encode.forHtmlAttribute(oauthConsumerSecret)%>" | ||
| value="<%=Encode.forHtmlAttribute(displayValue)%>" | ||
| readonly="readonly"> | ||
| <%-- Show/Hide button only if multiple Secrets is NOT allowed --%> |
There was a problem hiding this comment.
The comment has two issues: 1) "Secrets" should not be capitalized mid-sentence as it's not a proper noun, and 2) grammatically it should be "are NOT allowed" not "is NOT allowed" (plural subject requires plural verb).
| <%-- Show/Hide button only if multiple Secrets is NOT allowed --%> | |
| <%-- Show/hide button only if multiple secrets are NOT allowed. --%> |
| // If multiple secrets is allowed, mask the secret | ||
| String displayValue = oauthConsumerSecret; | ||
| if (multipleSecretsAllowed) { | ||
| displayValue = "*".repeat(16); |
There was a problem hiding this comment.
The masking implementation uses a fixed length of 16 asterisks regardless of the actual secret length. This could leak information about the secret length. Consider either: 1) masking with asterisks that match the actual secret length to maintain consistency, or 2) consistently using a standard fixed length across the application for all masked secrets to avoid leaking any information.
| // If multiple secrets is allowed, mask the secret | |
| String displayValue = oauthConsumerSecret; | |
| if (multipleSecretsAllowed) { | |
| displayValue = "*".repeat(16); | |
| // If multiple secrets are allowed, mask the secret by preserving its length. | |
| String displayValue = oauthConsumerSecret; | |
| if (multipleSecretsAllowed && oauthConsumerSecret != null) { | |
| displayValue = "*".repeat(oauthConsumerSecret.length()); |
| <div> | ||
| <input style="border: none; background: white;" | ||
| type="password" autocomplete="off" | ||
| type="<%= multipleSecretsAllowed ? "text" : "password" %>" |
There was a problem hiding this comment.
When multiple secrets are allowed, the input type is changed from "password" to "text" while displaying masked asterisks. This approach might confuse users and could expose the field to browser password managers or autofill features that typically ignore text fields. Consider keeping the input type as "password" and using the value attribute with masked asterisks, or add appropriate autocomplete attributes to prevent unintended behavior.
| boolean multipleSecretsAllowed = | ||
| IdentityUtil.getProperty(ApplicationMgtUIConstants.MULTIPLE_CLIENT_SECRETS_ENABLED) != null && | ||
| Boolean.parseBoolean(IdentityUtil.getProperty(ApplicationMgtUIConstants.MULTIPLE_CLIENT_SECRETS_ENABLED)); |
There was a problem hiding this comment.
The IdentityUtil.getProperty() method is called twice in this expression - once for the null check and once for parsing. Consider storing the result in a local variable first to avoid the redundant call.
| %> | ||
| <% if (!(appBean.getOauthConsumerSecret() == null || "false".equals(isHashDisabled))) { %> | ||
| <% if (!(appBean.getOauthConsumerSecret() == null || "false".equals(isHashDisabled))) { | ||
| // If multiple secrets is allowed, mask the secret |
There was a problem hiding this comment.
The comment has two issues according to coding guidelines: 1) grammatically it should be "are allowed" not "is allowed" (plural subject requires plural verb), and 2) comments should end with a period.
|
1 participant
Purpose