Multiple Active Client Secret Support for OAuth Applications#1238
Multiple Active Client Secret Support for OAuth Applications#1238hisanhunais wants to merge 54 commits intowso2:mainfrom
Conversation
hisanhunais
requested review from
Krishanx92,
lasanthaS,
piyumaldk and
tharikaGitHub
as code owners
hisanhunais
requested review from
Copilot
and removed request for
Krishanx92,
lasanthaS,
piyumaldk and
tharikaGitHub
WalkthroughAdds multi-client-secret support and UI for secret lifecycle: create (with expiry), one‑time display, copy/mask, delete and list. New dialogs, SecretsTable, utilities, Application secret APIs, and propagation of consumerSecret through token/key generation and try‑out/wizard flows; many localization keys added. Changes
Sequence Diagram(s)sequenceDiagram
actor User
participant TokenManager as TokenManager
participant NewSecretDialog as NewSecretDialog
participant AppAPI as Application API
participant SecretValueDialog as SecretValueDialog
User->>TokenManager: Click "Generate Keys" (multi-secret enabled)
TokenManager->>NewSecretDialog: Open create-secret dialog
User->>NewSecretDialog: Submit description & expiry
NewSecretDialog->>AppAPI: generateSecret(keyMappingId, additionalProps)
AppAPI-->>NewSecretDialog: return secret value
NewSecretDialog-->>TokenManager: onCreate(payload, secret)
TokenManager->>SecretValueDialog: Open with secret (one-time)
User->>SecretValueDialog: Close dialog
TokenManager->>AppAPI: generateKeys(keyRequest with secret metadata)
AppAPI-->>TokenManager: key generation success
sequenceDiagram
actor User
participant SecretsTable as SecretsTable
participant DeleteDialog as DeleteSecretDialog
participant AppAPI as Application API
User->>SecretsTable: Click delete on a secret row
SecretsTable->>DeleteDialog: Open confirmation dialog
User->>DeleteDialog: Confirm delete
DeleteDialog->>AppAPI: deleteSecret(keyMappingId, secretId)
AppAPI-->>DeleteDialog: success
DeleteDialog-->>SecretsTable: notify deletion
SecretsTable->>AppAPI: getSecrets(keyMappingId)
AppAPI-->>SecretsTable: updated secrets list
SecretsTable-->>User: refreshed table
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
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 |
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 17
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
portals/devportal/src/main/webapp/source/src/app/components/Shared/AppsAndKeys/ViewKeys.jsx (1)
165-171: Critical: DuplicatecomponentDidUpdatemethod definitions.The class defines
componentDidUpdatetwice (lines 165-171 and 206-211). In JavaScript classes, only the last definition is used, making the first one dead code. Both implementations appear identical, so remove one.🐛 Proposed fix - remove the duplicate
Remove lines 206-211 (the second definition) as it's redundant:
- /** - * Adding this here becasue it is not possible to add in the render method becasue isKeyJWT in state is used - * to close the dialog box and render method will casue this to be always true and cannot close the box. - * Rule is ignored becasue according to react docs its ok to setstate as long as we are checking a condition - * This is an ani pattern to be fixed later. - * wso2/product-apim#5293 - * https://reactjs.org/docs/react-component.html#componentdidupdate - * `@param` {*} prevProps previous props - * `@memberof` ViewKeys - */ - componentDidUpdate(prevProps) { - const { isKeyJWT } = this.props; - if (isKeyJWT && !prevProps.isKeyJWT) { - this.setState({ isKeyJWT: true }); - } - }Also applies to: 206-211
🤖 Fix all issues with AI agents
In
`@portals/devportal/src/main/webapp/source/src/app/components/Apis/Details/GoToTryOut.jsx`:
- Around line 123-124: Remove all debug console.log statements from the
GoToTryOut.jsx component before merging: delete the console.log that prints
'isMultipleSecretsAllowed2' concatenated with isMultipleSecretsAllowed (the
value produced by
isMultipleClientSecretsEnabled(selectedKeyManager.additionalProperties)) and the
other debug logs noted around lines 160, 216, and 220; if runtime logging is
genuinely required replace concatenation with proper logging using template
literals or separate console arguments (e.g.,
console.log('isMultipleSecretsAllowed', isMultipleSecretsAllowed)) and ensure
any logs are purposeful and gated behind a debug flag rather than left in
production code.
- Around line 219-221: generatedKeys returned from generateKeys (called via
taskManager) can be null when keys already exist, so avoid dereferencing it
directly; modify the code around the taskManager(generateKeys(...), 'generate')
call to check whether generatedKeys is non-null before accessing
generatedKeys.consumerSecret and calling setConsumerSecretValue, and handle the
null branch by either using the existing keys (e.g., read from the response that
indicates keys exist) or skipping the setConsumerSecretValue call and
logging/returning early; reference the generateKeys invocation, the
generatedKeys variable, and setConsumerSecretValue in your change.
In
`@portals/devportal/src/main/webapp/source/src/app/components/Shared/ApiTryOut/SelectAppPanel.jsx`:
- Around line 214-240: The new consumer secret field uses missing imports and
undefined props/state: import InputAdornment, IconButton, Visibility, and
VisibilityOff from `@mui/material/`@mui/icons-material respectively; add a local
state showSecret with useState in SelectAppPanel and implement toggleVisibility
to flip it; ensure consumerSecret and onConsumerSecretChange (and
isMultipleSecretsAllowed) are passed into SelectAppPanel props (or derive them
from existing state/context) and update the component signature to accept these
props; finally, wire the TextField value to consumerSecret and onChange to
onConsumerSecretChange so the field compiles and behaves correctly.
In
`@portals/devportal/src/main/webapp/source/src/app/components/Shared/ApiTryOut/TryOutController.jsx`:
- Around line 364-366: The Consumer Secret input currently uses hardcoded
booleans and an undefined handler; define an onConsumerSecretChange handler that
calls setConsumerSecret(e.target.value) and attach it to the input, change the
input type to type={showSecret ? "text" : "password"}, change the toggle button
onClick to onClick={() => setShowSecret(!showSecret)}, and update the visibility
icon condition from true to use showSecret so the UI reflects the showSecret
state consistently with the existing showSecret and consumerSecret state and the
generateToken call.
In
`@portals/devportal/src/main/webapp/source/src/app/components/Shared/AppsAndKeys/KeyConfiguration.jsx`:
- Line 352: The onClick handlers are incorrectly invoking onCopy immediately by
using .then(onCopy()); change them to pass the function reference or a lambda so
onCopy runs after the promise resolves (e.g.,
navigator.clipboard.writeText(tokenEndpoint).then(onCopy) or .then(() =>
onCopy())), and apply the same fix for the other instances (the onClick using
navigator.clipboard.writeText at the other two locations).
- Around line 528-536: The mapped AppConfiguration elements in the
applicationConfiguration.map call lack a unique React key; update the map to
pass a stable key prop (e.g., key={config.id} or key={config.applicationId},
falling back to key={config.name} or index only if no stable id exists) to each
<AppConfiguration /> so React can track list items; ensure you choose a stable,
unique field from the config object referenced in getPreviousValue/config to
avoid using array index unless unavoidable.
In
`@portals/devportal/src/main/webapp/source/src/app/components/Shared/AppsAndKeys/Secrets/NewSecretDialog.jsx`:
- Around line 149-150: The Dialog currently uses the raw onClose prop instead of
the component's handleClose, so backdrop or Escape closes bypass form reset;
change the Dialog's onClose binding to call handleClose (the component's close
handler) and ensure handleClose both resets the form state (e.g., via resetForm
or clearing relevant useState values) and then invokes the external onClose
prop, so all close paths (backdrop, Escape, explicit cancel) trigger the same
reset logic.
In
`@portals/devportal/src/main/webapp/source/src/app/components/Shared/AppsAndKeys/Secrets/SecretsTable.jsx`:
- Around line 347-357: The sx prop on the Paper component contains duplicate
padding keys (p: 2 and p: 3); remove the redundant p: 2 so only the intended
padding remains. Locate the Paper JSX with the sx object in SecretsTable.jsx and
delete the first p: 2 entry, leaving p: 3 as the effective padding value.
- Line 66: The Application.get(props.appId) call is happening on every render
because applicationPromise is created at component scope; wrap that call in a
useMemo so the promise (and any underlying API client) is only recreated when
props.appId changes—replace the top-level applicationPromise with a memoized
version using useMemo and props.appId as the dependency; keep the original
variable name applicationPromise so downstream usage (e.g., in effects or JSX)
doesn't change.
- Around line 73-77: The catch block in SecretsTable.jsx is using this.setState
in a functional component; add a React state hook like const [notFound,
setNotFound] = useState(false) at the top of the SecretsTable component and
replace this.setState({ notFound: true }) with setNotFound(true); update any
references that expected this.state.notFound to use the notFound variable and
consider rendering a ResourceNotFound component when notFound is true.
In
`@portals/devportal/src/main/webapp/source/src/app/components/Shared/AppsAndKeys/Secrets/SecretValueDialog.jsx`:
- Around line 46-49: The localization key used on the Close button is
misleading: change the FormattedMessage id from
'Shared.AppsAndKeys.Secrets.SecretValueDialog.delete.button' to something
accurate like 'Shared.AppsAndKeys.Secrets.SecretValueDialog.close.button' in
SecretValueDialog.jsx (the FormattedMessage instance rendering the Close
button), and update the corresponding entry in your locale/messages resource
files to match the new key (preserving the defaultMessage 'Close'); also search
for any usages or tests referencing the old delete.button key and update them
accordingly.
In
`@portals/devportal/src/main/webapp/source/src/app/components/Shared/AppsAndKeys/Secrets/util.js`:
- Around line 11-26: The function getClientSecretCount currently returns -1 for
"unlimited" and doesn't handle parseInt producing NaN; update it to match the
JSDoc by returning Infinity when multiple secrets are enabled but no
client_secret_count is provided or when the parsed count is not a valid number,
and keep returning 1 when additionalProps is falsy or multiple secrets are
disabled; reference getClientSecretCount and isMultipleClientSecretsEnabled to
locate the logic and validate the parseInt(result, 10) value, using a fallback
to Infinity on invalid input and also update the JSDoc to reflect returning
Infinity rather than -1.
In
`@portals/devportal/src/main/webapp/source/src/app/components/Shared/AppsAndKeys/TokenManager.jsx`:
- Around line 429-434: When loading an application or switching key-manager tabs
the isMultipleSecretsAllowed state must be updated to reflect the currently
selected key manager: in loadApplication replace the conditional-only setState
with an unconditional setState that sets isMultipleSecretsAllowed to the boolean
result of isMultipleClientSecretsEnabled(selectdKM.additionalProperties) (fixing
the selectedKM variable name if needed), and in handleTabChange recompute
isMultipleClientSecretsEnabled for the newly selected key manager and setState({
isMultipleSecretsAllowed: <result> }) so the UI toggles off when the new KM does
not support multiple client secrets.
- Around line 833-852: The JSX in keyConfigurationContent incorrectly uses the
React-reserved prop name key (key={key}) which is swallowed by React; change
that prop to a non-reserved name (e.g., keyValue={key}) when rendering
KeyConfiguration (matching the usage at line ~1166), and remove or replace the
top-level React key attribute only if this component is part of a list—if it is
inside a list keep a proper list key (key={someUniqueId}) but do not rely on it
to pass the actual key value to KeyConfiguration; update usages of
KeyConfiguration.props accordingly.
- Around line 690-695: The orphaned promise from
this.application.then(application =>
application.getSecrets(applicationKey.keyMappingId)) is neither awaited nor
handled so errors are swallowed and the result is discarded; either remove that
call if it was a leftover debug artifact, or properly consume it by awaiting the
promise (or returning it) and handling success/error (e.g., update state with
the secrets or call the existing secrets refresh handler and add a .catch to log
or surface errors) in the TokenManager.jsx code path where this.application and
applicationKey.keyMappingId are used.
In
`@portals/devportal/src/main/webapp/source/src/app/components/Shared/AppsAndKeys/ViewCurl.jsx`:
- Around line 158-162: The clipboard write promise is calling onCopy immediately
by using .then(onCopy()), so change each occurrence where
navigator.clipboard.writeText(...).then(onCopy()) to pass the callback instead
of invoking it (use .then(onCopy) or .then(() => onCopy())), e.g. in the onClick
handler that builds the curl string (references: navigator.clipboard.writeText,
onCopy, tokenEndpoint, getAuthSnippetString) and the two other similar clipboard
calls in this file; update all three instances to use .then(onCopy) or .then(()
=> onCopy()).
🧹 Nitpick comments (17)
portals/devportal/src/main/webapp/source/src/app/components/Shared/AppsAndKeys/Secrets/SecretValueDialog.jsx (2)
34-34: Simplify boolean JSX attribute.
isGenerated={true}can be written as justisGeneratedfor brevity.Proposed fix
- <ViewSecret secret={{ consumerSecret: secret }} isGenerated={true} /> + <ViewSecret secret={{ consumerSecret: secret }} isGenerated />
6-6: Unused import.
DialogContentTextis imported but used only as a wrapper withcomponent="div". Consider using a plaindivorBoxinstead, or remove the import if not needed.portals/devportal/src/main/webapp/source/src/app/components/Apis/Details/GoToTryOut.jsx (1)
450-450: Consider user feedback on clipboard copy.The copy button uses
navigator.clipboard.writeText()without any visual feedback. The existing codebase usesAlertfor notifications. Consider adding a success toast to confirm the copy action.Proposed enhancement
<Button size='small' variant='outlined' - onClick={() => navigator.clipboard.writeText(consumerSecretValue)} + onClick={() => { + navigator.clipboard.writeText(consumerSecretValue); + Alert.info(intl.formatMessage({ + id: 'Apis.Details.GoToTryOut.consumerSecret.copied', + defaultMessage: 'Consumer secret copied to clipboard', + })); + }} >portals/devportal/src/main/webapp/source/src/app/components/Shared/ApiTryOut/SelectAppPanel.jsx (1)
1-1: Unused import:useStateis imported but not used.The
useStatehook is imported but there's no state defined in this component. IfshowSecretstate is added to fix the visibility toggle, this import will become necessary.portals/devportal/src/main/webapp/source/src/app/components/Shared/AppsAndKeys/ViewSecret.jsx (1)
222-228: Consider adding defaultProps or PropTypes default.While the destructuring default (
isGenerated = false) works, for consistency with React patterns, you might also setisGenerated: falseindefaultPropsor mark it in PropTypes. The current approach is valid but mixing patterns can be confusing.Optional: Add defaultProps for consistency
ViewSecret.propTypes = { classes: PropTypes.shape({}).isRequired, secret: PropTypes.shape({ consumerSecret: PropTypes.string.isRequired, }).isRequired, isGenerated: PropTypes.bool, }; +ViewSecret.defaultProps = { + isGenerated: false, +}; + export default injectIntl((ViewSecret));portals/devportal/src/main/webapp/source/src/app/components/Shared/AppsAndKeys/Secrets/DeleteSecret.jsx (1)
25-28: Consider async error handling foronDelete.If
onDeleteis async or can throw, the dialog will close even on failure, potentially confusing users. Consider wrapping in try-catch or awaiting the result before closing.Suggested improvement
- const handleConfirmDelete = () => { - onDelete(); - setOpen(false); + const handleConfirmDelete = async () => { + try { + await onDelete(); + setOpen(false); + } catch (error) { + // Dialog stays open on error; caller should handle error display + console.error('Delete failed:', error); + } };portals/devportal/src/main/webapp/source/src/app/components/Shared/AppsAndKeys/KeyConfiguration.jsx (1)
574-600: AddkeyValueto PropTypes.The new
keyValueprop is used (line 155) but not declared in PropTypes.Proposed fix
KeyConfiguration.propTypes = { classes: PropTypes.instanceOf(Object).isRequired, keyRequest: PropTypes.shape({ callbackUrl: PropTypes.string, selectedGrantTypes: PropTypes.array, }).isRequired, isUserOwner: PropTypes.bool.isRequired, isKeysAvailable: PropTypes.bool.isRequired, keyManagerConfig: PropTypes.any.isRequired, notFound: PropTypes.bool, setGenerateEnabled: PropTypes.func.isRequired, updateKeyRequest: PropTypes.func.isRequired, validating: PropTypes.bool, mode: PropTypes.string, selectedApp: PropTypes.shape({ tokenType: PropTypes.string.isRequired, appId: PropTypes.string, value: PropTypes.string, owner: PropTypes.string, hashEnabled: PropTypes.bool, }), + keyValue: PropTypes.any, };portals/devportal/src/main/webapp/source/src/app/components/Shared/AppsAndKeys/Tokens.jsx (1)
107-122: Add PropTypes for new props.The component receives
multipleSecretsAllowedandonConsumerSecretChangeas new props but they are not declared in the PropTypes. This is inconsistent with the existing validation pattern.♻️ Add PropTypes for new props
tokens.contextTypes = { intl: PropTypes.shape({}).isRequired, }; +tokens.propTypes = { + multipleSecretsAllowed: PropTypes.bool, + onConsumerSecretChange: PropTypes.func, + accessTokenRequest: PropTypes.shape({}).isRequired, + updateAccessTokenRequest: PropTypes.func.isRequired, + subscriptionScopes: PropTypes.arrayOf(PropTypes.string).isRequired, +}; export default (tokens);portals/devportal/src/main/webapp/source/src/app/components/Shared/AppsAndKeys/Secrets/NewSecretDialog.jsx (1)
24-27: Consider consistent initial validity state.
isValidstarts asfalse, buthandleCloseresets it totrue. This inconsistency could cause the Create button to be incorrectly enabled if the dialog is opened after being closed once with a non-custom expiry selected.♻️ Proposed fix for consistent state
- const [isValid, setIsValid] = useState(false); + const [isValid, setIsValid] = useState(true);Since the default
expiryOptionis "30" (a predefined valid value),isValidshould start astrueto match the initial form state.portals/devportal/src/main/webapp/source/src/app/components/Shared/AppsAndKeys/Secrets/SecretsTable.jsx (2)
599-602: Add missing PropTypes forsecretCountandhashEnabled.These props are used in the component but not declared in PropTypes.
♻️ Proposed fix
SecretsTable.propTypes = { appId: PropTypes.string, keyMappingId: PropTypes.string, + secretCount: PropTypes.number, + hashEnabled: PropTypes.bool, };
100-110: Potential stale state when adjusting pagination.Calling
setPageinside thesetSecretscallback may reference a stalepagevalue due to closure. Consider using the functional update form forsetPageas well, or restructure to avoid nested state updates.♻️ Alternative approach using functional update
setSecrets((prev) => { const updated = prev.filter((s) => s.secretId !== secretId); - - // Adjust pagination if current page is now invalid - const maxPage = Math.ceil(updated.length / rowsPerPage) - 1; - if (page > maxPage) { - setPage(maxPage); - } - return updated; }); + // Adjust pagination after state update + setPage((currentPage) => { + const newLength = secrets.length - 1; + const maxPage = Math.max(0, Math.ceil(newLength / rowsPerPage) - 1); + return currentPage > maxPage ? maxPage : currentPage; + });portals/devportal/src/main/webapp/source/src/app/data/Application.jsx (2)
358-378: Remove commented-out code.The commented-out lines (370-377) appear to be leftover from copy-paste. Dead code should be removed to maintain readability.
♻️ Proposed fix
generateSecret(keyMappingId, additionalProperties) { const promisedSecret = this.client.then((client) => { const requestContent = { additionalProperties, }; const payload = { applicationId: this.id, keyMappingId }; const body = { requestBody: requestContent }; return client.apis['Application Secrets'].generateConsumerSecret(payload, body); }); return promisedSecret.then((secretResponse) => { const secret = secretResponse.obj; return secret; - // if (keyType === 'PRODUCTION') { - // this.productionKeys.set(keyManager, keysResponse.obj); - // return this.productionKeys.get(keyManager); - // } else { - // this.sandboxKeys.set(keyManager, keysResponse.obj); - // return this.sandboxKeys.get(keyManager); - // } }); }
380-394: Remove commented-out code.Similar to
generateSecret, this method contains leftover commented code (lines 386-392) that should be removed.♻️ Proposed fix
getSecrets(keyMappingId) { return this.client.then((client) => client.apis['Application Secrets'] .getConsumerSecrets({ applicationId: this.id, keyMappingId })) .then((secretsResponse) => { const secrets = secretsResponse.obj.list; return secrets; - // this._setKeys(keys); - // this._setTokens(keys); - // if (keyType === 'PRODUCTION') { - // return this.productionKeys; - // } else { - // return this.sandboxKeys; - // } }); }portals/devportal/src/main/webapp/source/src/app/components/Shared/AppsAndKeys/TokenManager.jsx (4)
299-304: Missing semicolon at line 303.Minor syntax inconsistency - missing semicolon after the
setStatecall.Proposed fix
handleCloseSecretValueDialog = () => { this.setState({ openSecretValueDialog: false, generatedSecret: null, - }) + }); };
606-631:secretMetastate is set but never consumed.The
secretMetastate variable (line 625) is populated but doesn't appear to be used anywhere in the component. If it's intended for future use, consider adding a comment; otherwise, it can be removed to avoid confusion.
833-938:keyConfigurationContentfunction is defined but never called.This function is defined but never invoked in the render method. The actual key configuration UI is rendered directly in the JSX below (starting around line 1163). This appears to be dead code that should either be removed or used to reduce duplication.
1187-1229: Inconsistent use ofgenerateKeysvshandleGenerateKeysClick.Line 1195 uses
this.generateKeysdirectly while line 1229 usesthis.handleGenerateKeysClick. The latter correctly routes through the multi-secret dialog flow when enabled.For the non-owner case (line 1195), the button is disabled anyway, so this may be intentional. However, for consistency and future-proofing, consider using
handleGenerateKeysClickin both places.Proposed fix for consistency
onClick={ - key ? this.updateKeys : this.generateKeys + key ? this.updateKeys : this.handleGenerateKeysClick }
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (17)
portals/devportal/src/main/webapp/site/public/locales/en.jsonportals/devportal/src/main/webapp/source/src/app/components/Apis/Details/GoToTryOut.jsxportals/devportal/src/main/webapp/source/src/app/components/Shared/ApiTryOut/SelectAppPanel.jsxportals/devportal/src/main/webapp/source/src/app/components/Shared/ApiTryOut/TryOutController.jsxportals/devportal/src/main/webapp/source/src/app/components/Shared/AppsAndKeys/KeyConfiguration.jsxportals/devportal/src/main/webapp/source/src/app/components/Shared/AppsAndKeys/Secrets/DeleteSecret.jsxportals/devportal/src/main/webapp/source/src/app/components/Shared/AppsAndKeys/Secrets/NewSecretDialog.jsxportals/devportal/src/main/webapp/source/src/app/components/Shared/AppsAndKeys/Secrets/SecretValueDialog.jsxportals/devportal/src/main/webapp/source/src/app/components/Shared/AppsAndKeys/Secrets/SecretsTable.jsxportals/devportal/src/main/webapp/source/src/app/components/Shared/AppsAndKeys/Secrets/util.jsportals/devportal/src/main/webapp/source/src/app/components/Shared/AppsAndKeys/TokenManager.jsxportals/devportal/src/main/webapp/source/src/app/components/Shared/AppsAndKeys/Tokens.jsxportals/devportal/src/main/webapp/source/src/app/components/Shared/AppsAndKeys/ViewCurl.jsxportals/devportal/src/main/webapp/source/src/app/components/Shared/AppsAndKeys/ViewKeys.jsxportals/devportal/src/main/webapp/source/src/app/components/Shared/AppsAndKeys/ViewSecret.jsxportals/devportal/src/main/webapp/source/src/app/data/Application.jsxportals/devportal/src/main/webapp/source/src/app/data/Constants.jsx
🧰 Additional context used
🧬 Code graph analysis (10)
portals/devportal/src/main/webapp/source/src/app/components/Apis/Details/GoToTryOut.jsx (2)
portals/devportal/src/main/webapp/source/src/app/components/Shared/AppsAndKeys/KeyConfiguration.jsx (2)
isMultipleSecretsAllowed(193-193)keyRequest(157-159)portals/devportal/src/main/webapp/source/src/app/components/Shared/AppsAndKeys/Secrets/util.js (1)
isMultipleClientSecretsEnabled(34-36)
portals/devportal/src/main/webapp/source/src/app/components/Shared/AppsAndKeys/Secrets/SecretValueDialog.jsx (1)
portals/devportal/src/main/webapp/source/src/app/components/Shared/AppsAndKeys/Secrets/DeleteSecret.jsx (1)
open(16-16)
portals/devportal/src/main/webapp/source/src/app/components/Shared/AppsAndKeys/Secrets/NewSecretDialog.jsx (2)
portals/devportal/src/main/webapp/source/src/app/components/Shared/AppsAndKeys/Secrets/SecretsTable.jsx (1)
intl(64-64)portals/devportal/src/main/webapp/source/src/app/data/Constants.jsx (1)
CONSTS(17-38)
portals/devportal/src/main/webapp/source/src/app/components/Shared/AppsAndKeys/ViewCurl.jsx (1)
portals/devportal/src/main/webapp/source/src/app/components/Shared/AppsAndKeys/Secrets/util.js (1)
isMultipleClientSecretsEnabled(34-36)
portals/devportal/src/main/webapp/source/src/app/components/Shared/AppsAndKeys/Secrets/SecretsTable.jsx (6)
portals/devportal/src/main/webapp/source/src/app/components/Shared/AppsAndKeys/Secrets/NewSecretDialog.jsx (3)
intl(28-28)error(26-26)NewSecretDialog(17-289)portals/devportal/src/main/webapp/source/src/app/components/Shared/AppsAndKeys/Secrets/DeleteSecret.jsx (2)
intl(17-17)DeleteSecretDialog(15-86)portals/devportal/src/main/webapp/source/src/app/data/Application.jsx (1)
Application(28-472)portals/devportal/src/main/webapp/source/src/app/components/Shared/AppsAndKeys/Secrets/util.js (1)
maskSecret(46-59)portals/devportal/src/main/webapp/source/src/app/data/Constants.jsx (1)
CONSTS(17-38)portals/devportal/src/main/webapp/source/src/app/components/Shared/AppsAndKeys/Secrets/SecretValueDialog.jsx (1)
SecretValueDialog(14-54)
portals/devportal/src/main/webapp/source/src/app/components/Shared/ApiTryOut/TryOutController.jsx (3)
portals/devportal/src/main/webapp/source/src/app/components/Shared/AppsAndKeys/Tokens.jsx (2)
consumerSecret(111-111)showSecret(112-112)portals/devportal/src/main/webapp/source/src/app/components/Shared/AppsAndKeys/Secrets/util.js (1)
isMultipleClientSecretsEnabled(34-36)portals/devportal/src/main/webapp/source/src/app/components/Apis/Details/AsyncApiConsole/AsyncApiConsole.jsx (1)
securitySchemeType(77-77)
portals/devportal/src/main/webapp/source/src/app/data/Application.jsx (3)
portals/devportal/src/main/webapp/source/src/app/components/Shared/ApiTryOut/TryOutController.jsx (1)
consumerSecret(190-190)portals/devportal/src/main/webapp/source/src/app/components/Shared/AppsAndKeys/Tokens.jsx (1)
consumerSecret(111-111)portals/devportal/src/main/webapp/source/src/app/components/Shared/AppsAndKeys/Secrets/SecretsTable.jsx (1)
secrets(55-55)
portals/devportal/src/main/webapp/source/src/app/components/Shared/AppsAndKeys/KeyConfiguration.jsx (1)
portals/devportal/src/main/webapp/source/src/app/components/Shared/AppsAndKeys/Secrets/util.js (1)
isMultipleClientSecretsEnabled(34-36)
portals/devportal/src/main/webapp/source/src/app/components/Shared/AppsAndKeys/TokenManager.jsx (3)
portals/devportal/src/main/webapp/source/src/app/components/Shared/AppsAndKeys/Secrets/util.js (1)
isMultipleClientSecretsEnabled(34-36)portals/devportal/src/main/webapp/source/src/app/components/Shared/AppsAndKeys/Secrets/NewSecretDialog.jsx (1)
NewSecretDialog(17-289)portals/devportal/src/main/webapp/source/src/app/data/Constants.jsx (1)
CONSTS(17-38)
portals/devportal/src/main/webapp/source/src/app/components/Shared/AppsAndKeys/Tokens.jsx (2)
portals/devportal/src/main/webapp/source/src/app/components/Shared/ApiTryOut/TryOutController.jsx (2)
consumerSecret(190-190)showSecret(191-191)portals/devportal/src/main/webapp/source/src/app/components/Shared/AppsAndKeys/ViewCurl.jsx (1)
multipleSecretsAllowed(96-96)
🪛 Biome (2.1.2)
portals/devportal/src/main/webapp/source/src/app/components/Shared/AppsAndKeys/Secrets/SecretsTable.jsx
[error] 351-351: This property is later overwritten by an object member with the same name.
Overwritten with this property.
If an object property with the same name is defined multiple times (except when combining a getter with a setter), only the last definition makes it into the object and previous definitions are ignored.
Unsafe fix: Remove this property.
(lint/suspicious/noDuplicateObjectKeys)
portals/devportal/src/main/webapp/source/src/app/components/Shared/ApiTryOut/TryOutController.jsx
[error] 847-847: Unexpected constant condition.
(lint/correctness/noConstantCondition)
[error] 861-861: Unexpected constant condition.
(lint/correctness/noConstantCondition)
portals/devportal/src/main/webapp/source/src/app/components/Shared/AppsAndKeys/KeyConfiguration.jsx
[error] 529-535: Missing key property for this element in iterable.
The order of the items may change, and having a key can help React identify which item was moved.
Check the React documentation.
(lint/correctness/useJsxKeyInIterable)
🔇 Additional comments (27)
portals/devportal/src/main/webapp/source/src/app/data/Constants.jsx (1)
34-37: LGTM!The new
SECRET_DIALOG_MODESconstant is well-structured and follows the existing pattern in the file. Good use of an enum-like object for mode disambiguation.portals/devportal/src/main/webapp/source/src/app/components/Apis/Details/GoToTryOut.jsx (1)
402-471: Consumer secret UI block is well-structured.The conditional rendering logic correctly checks
multipleSecretsEnabled && consumerSecretValue && !anyErrors. The UI provides appropriate visual hierarchy with title, value, copy action, and security notice.portals/devportal/src/main/webapp/source/src/app/components/Shared/AppsAndKeys/ViewSecret.jsx (1)
139-163: LGTM!Clean implementation of the
isGeneratedprop with appropriate default value and conditional help text rendering. The different messages for "generated" vs "regenerated" secrets provide clear user guidance.portals/devportal/src/main/webapp/source/src/app/components/Shared/AppsAndKeys/Secrets/util.js (2)
34-36: LGTM!Clean implementation using optional chaining for safe property access.
46-58: LGTM!The masking logic is straightforward and handles edge cases correctly.
portals/devportal/src/main/webapp/source/src/app/components/Shared/ApiTryOut/TryOutController.jsx (2)
386-389: LGTM!Good addition of user-friendly error messaging using Alert when token generation fails.
604-606: Consider guarding against undefinedselectedKMObject.
enableGetTestKeyButtondepends onisMultipleClientSecretsAllowed, which accessesselectedKMObject?.additionalProperties. While optional chaining is used, ifselectedKMObjectis null/undefined initially,isMultipleClientSecretsAllowedwill befalse, which may be the intended fallback behavior but could mask configuration issues.portals/devportal/src/main/webapp/source/src/app/components/Shared/AppsAndKeys/Secrets/DeleteSecret.jsx (1)
30-86: LGTM!Well-structured confirmation dialog with proper accessibility attributes (
aria-labelledby,aria-describedby) and appropriate i18n support.portals/devportal/src/main/webapp/source/src/app/components/Shared/AppsAndKeys/KeyConfiguration.jsx (2)
193-202: LGTM!Good pattern: accordion auto-expands when no key exists (
!keyValue) and collapses when a key is present. The effect correctly resets state whenkeyValuechanges.
541-566: LGTM!Clean conditional rendering pattern using an IIFE to extract the advanced configurations block and wrap it in an Accordion when multiple secrets are enabled.
portals/devportal/src/main/webapp/source/src/app/components/Shared/AppsAndKeys/ViewCurl.jsx (3)
96-108: LGTM!Clean helper function and reusable snippet for the multiple secrets case.
231-314: Token exchange flow doesn't support multiple secrets.The
alias !== nullbranch (token exchange) still uses hardcodedAuthorization: Basicheader and doesn't usemultipleSecretsAllowedorgetAuthSnippetString(). If multiple secrets should be supported for token exchange, this branch needs updating for consistency.Is this intentional, or should the token exchange flow also support the multiple secrets pattern?
128-137: LGTM!Good conditional rendering that shows the appropriate authorization snippet based on whether multiple secrets are enabled.
portals/devportal/src/main/webapp/source/src/app/components/Shared/AppsAndKeys/Tokens.jsx (1)
132-158: LGTM!The Consumer Secret input field implementation is well done with proper visibility toggle, password masking, and conditional rendering based on
multipleSecretsAllowed.portals/devportal/src/main/webapp/source/src/app/components/Shared/AppsAndKeys/Secrets/NewSecretDialog.jsx (1)
30-97: LGTM!The input validation logic is thorough - handling keyboard input, paste events, and onChange with proper error messaging and temporary error display.
portals/devportal/src/main/webapp/source/src/app/data/Application.jsx (1)
169-185: LGTM!The conditional logic correctly handles the case when
consumerSecretis provided externally, omittingrevokeTokenwhich aligns with the multi-secret flow where a specific secret is used rather than the stored one.portals/devportal/src/main/webapp/source/src/app/components/Shared/AppsAndKeys/ViewKeys.jsx (3)
336-383: LGTM!The
generateAccessTokenmethod correctly passesconsumerSecretonly whenmultipleSecretsAllowedis true, preserving backward compatibility for the standard flow.
453-541: LGTM!The Consumer Secret section is correctly hidden when
multipleSecretsAllowedis true, delegating secret management to theSecretsTablecomponent instead.
711-719: LGTM!The
SecretsTableintegration correctly passes required props includingappId,keyMappingId,secretCount, andhashEnabledfor proper secret management.portals/devportal/src/main/webapp/source/src/app/components/Shared/AppsAndKeys/TokenManager.jsx (6)
35-35: LGTM! New imports for multi-secret support.The imports for
CONSTS,isMultipleClientSecretsEnabled,NewSecretDialog, andSecretValueDialogare correctly added to support the new multiple client secrets feature.Also applies to: 62-64
245-252: LGTM! New state variables for secret management.The new state variables properly support the multi-secret workflow:
openSecretValueDialog/generatedSecretfor displaying generated secretsisMultipleSecretsAllowedto track feature enablementopenSecretCreateDialog/pendingGenerate/secretMetafor the creation flow
549-553: LGTM! Secret dialog integration in generateKeys.Correctly opens the
SecretValueDialogwith the generated secret when multiple secrets are allowed, ensuring the user can copy the secret before it's no longer visible.
585-596: LGTM! Conditional key generation flow.The method correctly introduces an intermediate step (secret creation dialog) when multiple secrets are enabled, while preserving the existing flow for standard key generation.
1267-1277: LGTM! Dialog integration in render.The
NewSecretDialogandSecretValueDialogare correctly integrated with appropriate props for open state, close handlers, and data.
1463-1475: Multi-secret flow not implemented fortokenType === 'BOTH'section.The 'BOTH' token type section (lines 1319-1530) uses
this.generateKeysdirectly and doesn't include theNewSecretDialogorSecretValueDialog. If multiple client secrets should be supported for this token type as well, this section needs similar updates.Please verify whether the multi-secret feature is intentionally limited to the 'DIRECT' token type, or if this section should also be updated.
portals/devportal/src/main/webapp/site/public/locales/en.json (2)
681-728: LGTM! Comprehensive localization keys for secrets management.The new translation keys are well-organized and follow the existing naming conventions. They cover:
- Delete confirmation dialogs
- Secret creation dialog with expiry options
- Table headings and status messages
- Error and success messages
- Tooltips and accessibility text
The pluralization syntax at line 706 (
{count} day{count, plural, one {} other {s}}) is correctly formatted for ICU MessageFormat.
244-246: LGTM! GoToTryOut consumer secret localization.New keys for displaying and copying consumer secrets in the try-out flow are correctly added.
✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.
| isMultipleSecretsAllowed = isMultipleClientSecretsEnabled(selectedKeyManager.additionalProperties); | ||
| console.log('isMultipleSecretsAllowed2' + isMultipleSecretsAllowed); |
There was a problem hiding this comment.
Remove debug console.log statements before merging.
Multiple console.log statements remain in the code (lines 124, 160, 216, 220). These appear to be debugging artifacts and should be removed for production.
Additionally, the string concatenation with objects (e.g., 'isMultipleSecretsAllowed2' + isMultipleSecretsAllowed) won't produce useful output. If logging is needed, use template literals or separate arguments.
Also applies to: 160-160, 216-216, 220-220
🤖 Prompt for AI Agents
In
`@portals/devportal/src/main/webapp/source/src/app/components/Apis/Details/GoToTryOut.jsx`
around lines 123 - 124, Remove all debug console.log statements from the
GoToTryOut.jsx component before merging: delete the console.log that prints
'isMultipleSecretsAllowed2' concatenated with isMultipleSecretsAllowed (the
value produced by
isMultipleClientSecretsEnabled(selectedKeyManager.additionalProperties)) and the
other debug logs noted around lines 160, 216, and 220; if runtime logging is
genuinely required replace concatenation with proper logging using template
literals or separate console arguments (e.g.,
console.log('isMultipleSecretsAllowed', isMultipleSecretsAllowed)) and ensure
any logs are purposeful and gated behind a debug flag rather than left in
production code.
portals/devportal/src/main/webapp/source/src/app/components/Apis/Details/GoToTryOut.jsx
Outdated
Show resolved
Hide resolved
portals/devportal/src/main/webapp/source/src/app/components/Shared/ApiTryOut/SelectAppPanel.jsx
Outdated
Show resolved
Hide resolved
| undefined, | ||
| undefined, | ||
| consumerSecret |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
find . -name "Application.jsx" -o -name "Application.js" | head -20Repository: wso2/apim-apps
Length of output: 129
🏁 Script executed:
cat -n ./portals/devportal/src/main/webapp/source/src/app/data/Application.jsx | grep -A 20 "generateToken"Repository: wso2/apim-apps
Length of output: 1238
🏁 Script executed:
cat -n ./portals/devportal/src/main/webapp/source/src/app/components/Shared/ApiTryOut/TryOutController.jsx | sed -n '355,375p'Repository: wso2/apim-apps
Length of output: 1101
🏁 Script executed:
cat -n ./portals/devportal/src/main/webapp/source/src/app/components/Shared/ApiTryOut/TryOutController.jsx | sed -n '840,870p'Repository: wso2/apim-apps
Length of output: 2269
Fix critical bugs in Consumer Secret UI implementation.
The generateToken signature matches the call correctly. However, the Consumer Secret field (lines 847-862) has multiple critical issues:
- Line 856:
onConsumerSecretChangehandler is undefined. Define this handler to update theconsumerSecretstate. - Line 847:
type={true ? "text" : "password"}uses a constant condition. Replace withtype={showSecret ? "text" : "password"}to toggle visibility. - Line 860:
onClick={true}is invalid. Replace withonClick={() => setShowSecret(!showSecret)}. - Line 861: The visibility icon condition
{true ? ...}should use theshowSecretstate variable instead.
The showSecret state is already defined but unused. Use it consistently for the visibility toggle.
🤖 Prompt for AI Agents
In
`@portals/devportal/src/main/webapp/source/src/app/components/Shared/ApiTryOut/TryOutController.jsx`
around lines 364 - 366, The Consumer Secret input currently uses hardcoded
booleans and an undefined handler; define an onConsumerSecretChange handler that
calls setConsumerSecret(e.target.value) and attach it to the input, change the
input type to type={showSecret ? "text" : "password"}, change the toggle button
onClick to onClick={() => setShowSecret(!showSecret)}, and update the visibility
icon condition from true to use showSecret so the UI reflects the showSecret
state consistently with the existing showSecret and consumerSecret state and the
generateToken call.
...ls/devportal/src/main/webapp/source/src/app/components/Shared/ApiTryOut/TryOutController.jsx
Outdated
Show resolved
Hide resolved
| * @returns {number} - Number of allowed client secrets, Infinity if not specified but enabled, 0 if disabled | ||
| */ | ||
| export function getClientSecretCount(additionalProps) { | ||
| if (!additionalProps) return 1; | ||
|
|
||
| const multipleSecretsEnabled = isMultipleClientSecretsEnabled(additionalProps); | ||
| if (multipleSecretsEnabled) { | ||
| const countStr = additionalProps["client_secret_count"]; | ||
| if (!countStr) { | ||
| // Default to unlimited | ||
| return -1; | ||
| } | ||
| return parseInt(countStr, 10); | ||
| } | ||
| return 1; | ||
| } |
There was a problem hiding this comment.
JSDoc mismatch and missing NaN handling in getClientSecretCount.
Two issues:
- The JSDoc states it returns
Infinitywhen not specified but enabled, but the code returns-1(line 21). parseInton an invalid string returnsNaN, which could cause issues downstream.
Proposed fix
/**
* Reads the client secret count from additional properties
* only if multiple client secrets are enabled.
*
* `@param` {Object} additionalProps - The additionalProperties object from Key Manager config
- * `@returns` {number} - Number of allowed client secrets, Infinity if not specified but enabled, 0 if disabled
+ * `@returns` {number} - Number of allowed client secrets, -1 if unlimited, 1 if disabled
*/
export function getClientSecretCount(additionalProps) {
if (!additionalProps) return 1;
const multipleSecretsEnabled = isMultipleClientSecretsEnabled(additionalProps);
if (multipleSecretsEnabled) {
const countStr = additionalProps["client_secret_count"];
if (!countStr) {
// Default to unlimited
return -1;
}
- return parseInt(countStr, 10);
+ const count = parseInt(countStr, 10);
+ return Number.isNaN(count) ? -1 : count;
}
return 1;
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| * @returns {number} - Number of allowed client secrets, Infinity if not specified but enabled, 0 if disabled | |
| */ | |
| export function getClientSecretCount(additionalProps) { | |
| if (!additionalProps) return 1; | |
| const multipleSecretsEnabled = isMultipleClientSecretsEnabled(additionalProps); | |
| if (multipleSecretsEnabled) { | |
| const countStr = additionalProps["client_secret_count"]; | |
| if (!countStr) { | |
| // Default to unlimited | |
| return -1; | |
| } | |
| return parseInt(countStr, 10); | |
| } | |
| return 1; | |
| } | |
| * `@returns` {number} - Number of allowed client secrets, -1 if unlimited, 1 if disabled | |
| */ | |
| export function getClientSecretCount(additionalProps) { | |
| if (!additionalProps) return 1; | |
| const multipleSecretsEnabled = isMultipleClientSecretsEnabled(additionalProps); | |
| if (multipleSecretsEnabled) { | |
| const countStr = additionalProps["client_secret_count"]; | |
| if (!countStr) { | |
| // Default to unlimited | |
| return -1; | |
| } | |
| const count = parseInt(countStr, 10); | |
| return Number.isNaN(count) ? -1 : count; | |
| } | |
| return 1; | |
| } |
🤖 Prompt for AI Agents
In
`@portals/devportal/src/main/webapp/source/src/app/components/Shared/AppsAndKeys/Secrets/util.js`
around lines 11 - 26, The function getClientSecretCount currently returns -1 for
"unlimited" and doesn't handle parseInt producing NaN; update it to match the
JSDoc by returning Infinity when multiple secrets are enabled but no
client_secret_count is provided or when the parsed count is not a valid number,
and keep returning 1 when additionalProps is falsy or multiple secrets are
disabled; reference getClientSecretCount and isMultipleClientSecretsEnabled to
locate the logic and validate the parseInt(result, 10) value, using a fallback
to Infinity on invalid input and also update the JSDoc to reflect returning
Infinity rather than -1.
portals/devportal/src/main/webapp/source/src/app/components/Shared/AppsAndKeys/TokenManager.jsx
Outdated
Show resolved
Hide resolved
portals/devportal/src/main/webapp/source/src/app/components/Shared/AppsAndKeys/TokenManager.jsx
Outdated
Show resolved
Hide resolved
portals/devportal/src/main/webapp/source/src/app/components/Shared/AppsAndKeys/TokenManager.jsx
Show resolved
Hide resolved
portals/devportal/src/main/webapp/source/src/app/components/Shared/AppsAndKeys/ViewCurl.jsx
Show resolved
Hide resolved
| fullWidth | ||
| variant="outlined" | ||
| margin="normal" | ||
| type={true ? "text" : "password"} |
There was a problem hiding this comment.
Hardcoded boolean true makes the ternary operator always evaluate to 'text'. This should be replaced with a state variable like showSecret to allow toggling visibility.
| <IconButton onClick={true} edge="end"> | ||
| {true ? <VisibilityOff /> : <Visibility />} |
There was a problem hiding this comment.
Hardcoded boolean values in lines 860 and 861. The onClick handler should be a toggle function (e.g., toggleVisibility), and the icon conditional should use a state variable like showSecret.
| )} | ||
| name="consumerSecret" | ||
| value={consumerSecret} | ||
| onChange={(e) => onConsumerSecretChange(e.target.value)} |
There was a problem hiding this comment.
The function onConsumerSecretChange is referenced but never defined in this component. It should be defined similar to other handler functions or passed via props.
| onChange={(e) => onConsumerSecretChange(e.target.value)} | |
| onChange={handleChanges} |
| /> | ||
| </RadioGroup> | ||
| </FormControl> | ||
| {isMultipleSecretsAllowed && ( |
There was a problem hiding this comment.
Multiple undefined variables are referenced: isMultipleSecretsAllowed, consumerSecret, showSecret, onConsumerSecretChange, and toggleVisibility. These need to be defined in the component or passed via props. Additionally, missing imports for InputAdornment, IconButton, Visibility, and VisibilityOff.
| "Shared.AppsAndKeys.Secrets.DeleteSecret.delete.secret.tooltip.title": "Delete secret", | ||
| "Shared.AppsAndKeys.Secrets.NewSecretDialog.cancel.button": "Cancel", | ||
| "Shared.AppsAndKeys.Secrets.NewSecretDialog.create.button": "Create", | ||
| "Shared.AppsAndKeys.Secrets.NewSecretDialog.custom.expiry.time.label": "Custom Expiry Time(days)", |
There was a problem hiding this comment.
Missing space before opening parenthesis. Should be 'Custom Expiry Time (days)' with a space.
| "Shared.AppsAndKeys.Secrets.NewSecretDialog.custom.expiry.time.label": "Custom Expiry Time(days)", | |
| "Shared.AppsAndKeys.Secrets.NewSecretDialog.custom.expiry.time.label": "Custom Expiry Time (days)", |
| @@ -1,4 +1,4 @@ | |||
| import React from 'react'; | |||
| import React, { useState } from 'react'; | |||
There was a problem hiding this comment.
Unused import useState.
| import React, { useState } from 'react'; | |
| import React from 'react'; |
| const [ksGenerated, setKSGenerated] = useState(false); | ||
| const [showMoreGWUrls, setShowMoreGWUrls] = useState(false); | ||
| const [tokenValue, setTokenValue] = useState(''); | ||
| const [consumerSecret, setConsumerSecret] = useState(''); |
There was a problem hiding this comment.
Unused variable setConsumerSecret.
| const [consumerSecret, setConsumerSecret] = useState(''); | |
| const [consumerSecret] = useState(''); |
| const [showMoreGWUrls, setShowMoreGWUrls] = useState(false); | ||
| const [tokenValue, setTokenValue] = useState(''); | ||
| const [consumerSecret, setConsumerSecret] = useState(''); | ||
| const [showSecret, setShowSecret] = useState(false); |
There was a problem hiding this comment.
Unused variable showSecret.
| const [showSecret, setShowSecret] = useState(false); |
| const [showMoreGWUrls, setShowMoreGWUrls] = useState(false); | ||
| const [tokenValue, setTokenValue] = useState(''); | ||
| const [consumerSecret, setConsumerSecret] = useState(''); | ||
| const [showSecret, setShowSecret] = useState(false); |
There was a problem hiding this comment.
Unused variable setShowSecret.
| const [showSecret, setShowSecret] = useState(false); | |
| const [showSecret] = useState(false); |
| generateAccessToken = () => { | ||
| const { accessTokenRequest, isUpdating } = this.state; | ||
| generateAccessToken = (multipleSecretsAllowed) => { | ||
| const { accessTokenRequest, isUpdating, consumerSecret } = this.state; |
There was a problem hiding this comment.
Unused variable isUpdating.
| const { accessTokenRequest, isUpdating, consumerSecret } = this.state; | |
| const { accessTokenRequest, consumerSecret } = this.state; |
hisanhunais
force-pushed
the
client-secret-rotation
branch
from
2490842 to
46947a4
Compare
There was a problem hiding this comment.
Actionable comments posted: 15
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
portals/devportal/src/main/webapp/source/src/app/components/Shared/ApiTryOut/SelectAppPanel.jsx (1)
147-241:⚠️ Potential issue | 🟠 MajorConsumer secret field is absent from the
allApplications !== nullbranch — rendering will silently skip it in the common case.The
{isMultipleSecretsAllowed && <TextField ... />}block (lines 214-239) sits exclusively inside theallApplications === nullelse-branch. WhenallApplicationsis populated (the typical path for a user who has created at least one application), the component renders the first branch (lines 148-177), which has no consumer secret field. The feature is therefore invisible in the normal flow.The consumer secret field (or its equivalent extraction to a shared block) must also be present in the
allApplications !== nullFormControl(after line 177), or alternatively moved outside the ternary entirely.🔧 Sketch of the fix (outside the ternary)
</FormControl> )} </Grid> + {isMultipleSecretsAllowed && ( + <Grid x={12} md={6} className={classes.centerItems}> + <TextField + fullWidth + variant='outlined' + margin='normal' + type={showSecret ? 'text' : 'password'} + label={( + <FormattedMessage + id='Shared.AppsAndKeys.Tokens.consumer.secret' + defaultMessage='Consumer Secret' + /> + )} + name='consumerSecret' + value={consumerSecret} + onChange={(e) => onConsumerSecretChange(e.target.value)} + InputProps={{ + endAdornment: ( + <InputAdornment position='end'> + <IconButton onClick={toggleVisibility} edge='end'> + {showSecret ? <VisibilityOff /> : <Visibility />} + </IconButton> + </InputAdornment> + ), + }} + /> + </Grid> + )} </Root>🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@portals/devportal/src/main/webapp/source/src/app/components/Shared/ApiTryOut/SelectAppPanel.jsx` around lines 147 - 241, The consumer secret TextField is only rendered in the else-branch when allApplications === null, so when allApplications !== null the secret input is missing; move or duplicate the isMultipleSecretsAllowed block (the TextField using consumerSecret, onConsumerSecretChange, showSecret, toggleVisibility and InputAdornment/Visibility icons) into the FormControl used when allApplications !== null (or pull the block out of the ternary entirely and render it after the RadioGroup/FormControl) so that the consumer secret appears regardless of the allApplications branch; ensure you reuse the existing props/state names (isMultipleSecretsAllowed, consumerSecret, onConsumerSecretChange, showSecret, toggleVisibility) and keep the TextField’s InputProps and adornment intact.portals/devportal/src/main/webapp/source/src/app/components/Shared/ApiTryOut/TryOutController.jsx (1)
521-530:⚠️ Potential issue | 🟡 Minor
consumerSecretnot cleared when application or key manager changes.When
selectedApplicationchanges (line 521), all access tokens are explicitly reset butconsumerSecretis not. Since consumer secrets are per-application-credential, a stale secret from application A will persist in state when the user switches to application B, potentially silently using wrong credentials when generating a test key.The same gap exists for the
selectedKeyManagercase (line 529).🐛 Proposed fix
case 'selectedApplication': setProductionAccessToken(''); setSandboxAccessToken(''); setProductionApiKey(''); setSandboxApiKey(''); setSelectedApplication(value); + setConsumerSecret(''); break; case 'selectedKeyManager': setSelectedKeyManager(value, true, selectedApplication); + setConsumerSecret(''); break;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@portals/devportal/src/main/webapp/source/src/app/components/Shared/ApiTryOut/TryOutController.jsx` around lines 521 - 530, The consumerSecret state is not being cleared when application or key manager changes; update the handlers for the 'selectedApplication' and 'selectedKeyManager' cases to also reset consumerSecret (call the same state setter used for consumerSecret) alongside setProductionAccessToken, setSandboxAccessToken, setProductionApiKey, setSandboxApiKey before calling setSelectedApplication or setSelectedKeyManager so stale secrets cannot be reused.portals/devportal/src/main/webapp/source/src/app/components/Apis/Details/Credentials/Wizard/GenerateAccessTokenStep.jsx (1)
111-111:⚠️ Potential issue | 🟡 MinorDebug
console.logwith object concatenation should be removed.
responseis an object — string concatenation will produce[object Object]. This looks like a leftover debug statement.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@portals/devportal/src/main/webapp/source/src/app/components/Apis/Details/Credentials/Wizard/GenerateAccessTokenStep.jsx` at line 111, Remove the leftover debug concatenation console.log call in GenerateAccessTokenStep.jsx (the line "console.log('token generated successfully ' + response);") because response is an object; either delete this debug line or replace it with a proper logger/structured log (e.g., use console.log('token generated successfully', response) or the app's logging utility) inside the token generation handler so the object is logged correctly.
🧹 Nitpick comments (5)
portals/devportal/src/main/webapp/source/src/app/components/Shared/ApiTryOut/TryOutController.jsx (2)
816-823: RedundantisConsumerSecretRequired &&guard insidehelperText.The enclosing
{isConsumerSecretRequired && ( <TextField ... /> )}at line 800 already ensures this block only renders whenisConsumerSecretRequiredistrue, so the leading guard in thehelperTextexpression is unnecessary.♻️ Proposed simplification
helperText={ - isConsumerSecretRequired && !consumerSecret?.trim() + !consumerSecret?.trim() ? <FormattedMessage id='Apis.Details.TryOutConsole.consumerSecret.required.helper' defaultMessage='Consumer Secret is required to generate a new Test Key.' /> : null }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@portals/devportal/src/main/webapp/source/src/app/components/Shared/ApiTryOut/TryOutController.jsx` around lines 816 - 823, The helperText expression redundantly checks isConsumerSecretRequired even though the whole TextField is already wrapped by {isConsumerSecretRequired && (...)}, so inside the TextField's helperText (in TryOutController.jsx) remove the leading isConsumerSecretRequired && guard and simply use consumerSecret?.trim() ? null : <FormattedMessage ... />; update the helperText to reference only consumerSecret (and FormattedMessage id 'Apis.Details.TryOutConsole.consumerSecret.required.helper') to keep behavior identical but eliminate the redundant condition.
364-366: Refactor to use named parameters for clarity and robustness. The twoundefinedarguments represent positional placeholders forisTokenExchangeandexternalTokenbeforeconsumerSecret. While the current call is correct, explicit positional arguments are fragile: future changes to the function signature (parameter reordering or insertion) could silently introduce bugs. Consider refactoring to pass these as an object destructured by the function, or document the parameter order prominently to mitigate risk.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@portals/devportal/src/main/webapp/source/src/app/components/Shared/ApiTryOut/TryOutController.jsx` around lines 364 - 366, The call that currently passes two positional undefined values followed by consumerSecret (the callsite in TryOutController.jsx where consumerSecret is passed with two leading undefineds) should be changed to use named parameters: update the target function to accept a single options object (e.g., { isTokenExchange, externalToken, consumerSecret }) or overload it to accept an options object, then update this call to pass { isTokenExchange: undefined, externalToken: undefined, consumerSecret } (or appropriate explicit booleans/values) so the intent is explicit and resilient to signature changes; modify the function definition and all callsites accordingly (search for the function by name where these args are used) and update any JSDoc/type annotations to reflect the new object parameter.portals/devportal/src/main/webapp/source/src/app/data/Application.jsx (1)
397-408:deleteSecretreturns raw HTTP status instead of parsed response.The method returns
response.status(a number like 200/204), whilegenerateSecretreturnssecretResponse.obj. This inconsistency in return types may lead to confusion for callers. Consider returning a boolean (response.ok) for consistency with other delete-like methods in this class (e.g.,cleanUpKeysreturnsresponse.ok).Suggested fix
- return promisedDelete.then((response) => response.status); + return promisedDelete.then((response) => response.ok);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@portals/devportal/src/main/webapp/source/src/app/data/Application.jsx` around lines 397 - 408, The deleteSecret method currently returns numeric HTTP status (response.status) which is inconsistent with other delete-like methods (e.g., cleanUpKeys) and with generateSecret's parsed object; change deleteSecret (and its promise chain around client.apis['Application Secrets'].revokeConsumerSecret) to return a boolean indicating success (use response.ok) instead of response.status so callers get a consistent boolean result like cleanUpKeys; update the returned promise to resolve to response.ok.portals/devportal/src/main/webapp/source/src/app/components/Shared/AppsAndKeys/Secrets/SecretsTable.jsx (1)
89-91:useEffectwon't re-fetch whenkeyMappingIdorappIdchanges.
fetchSecretscloses overapplicationPromise(keyed onprops.appId) andprops.keyMappingId. With an empty dependency array the table will show stale data if the parent passes a new key mapping or app ID without remounting the component.♻️ Suggested fix
- useEffect(() => { - fetchSecrets(); - }, []); + useEffect(() => { + fetchSecrets(); + }, [props.keyMappingId, applicationPromise]);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@portals/devportal/src/main/webapp/source/src/app/components/Shared/AppsAndKeys/Secrets/SecretsTable.jsx` around lines 89 - 91, The useEffect hooking fetchSecrets is using an empty dependency array so it doesn't re-run when props.appId or props.keyMappingId change; update the effect to depend on those values (and any state/closures it uses) so fetchSecrets runs when applicationPromise (derived from props.appId) or props.keyMappingId changes. Either add props.appId and props.keyMappingId (and applicationPromise if present in scope) to the useEffect dependency array, or memoize fetchSecrets with useCallback including those dependencies and then reference that memoized function in useEffect. Ensure the effect references the same identifiers shown (useEffect, fetchSecrets, props.appId, props.keyMappingId, applicationPromise) so stale data is avoided.portals/devportal/src/main/webapp/source/src/app/components/Shared/AppsAndKeys/TokenManager.jsx (1)
1272-1282:NewSecretDialog/SecretValueDialogrendered once per DIRECT key-manager, all bound to shared state.If the environment has multiple key managers with
tokenType === 'DIRECT', both dialogs are instantiated for each one, all controlled by the sameopenSecretCreateDialog/openSecretValueDialogstate. AlthoughTabPanelonly renders children for the active tab (preventing simultaneous visible duplication in the common case), this is fragile — consider hoisting the dialogs outside thekeyManagers.map()loop.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@portals/devportal/src/main/webapp/source/src/app/components/Shared/AppsAndKeys/TokenManager.jsx` around lines 1272 - 1282, The NewSecretDialog and SecretValueDialog are being instantiated inside the keyManagers.map() loop and thus created once per DIRECT key manager while all share the same open state (openSecretCreateDialog, openSecretValueDialog) and generatedSecret; hoist both dialog components out of the keyManagers.map() so only one instance of NewSecretDialog and one SecretValueDialog exist for the component, keep their props bound to the existing state and handlers (openSecretCreateDialog, openSecretValueDialog, generatedSecret, handleCloseSecretCreateDialog, handleCloseSecretValueDialog, handleCreateSecret) and ensure any per-key-manager context needed (e.g., which manager triggered open) is passed into the dialog via explicit prop (like activeKeyManagerId) when opening from inside the map.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@portals/devportal/src/main/webapp/site/public/locales/en.json`:
- Line 702: The localization key in en.json is incorrect: replace the entry
"Shared.AppsAndKeys.Secrets.SecretValueDialog.delete.button" with
"Shared.AppsAndKeys.Secrets.SecretValueDialog.close.button" so it matches the
lookup used in SecretValueDialog.jsx (which calls
Shared.AppsAndKeys.Secrets.SecretValueDialog.close.button); ensure the
translation text remains "Close" and that no duplicate delete.button key
remains.
In
`@portals/devportal/src/main/webapp/source/src/app/components/Apis/Details/Credentials/Wizard/GenerateKeysStep.jsx`:
- Line 261: Remove the debug console.log in GenerateKeysStep.jsx that prints
"Keys generated successfully with ID : " + response; locate the success path
where the keys are generated (the callback handling the response) and delete
that console.log; if you need to record the response for debugging in
non-production, replace it with a proper logger or persist/handle the response
object (e.g., use an existing logger or JSON.stringify(response) behind a debug
flag) instead of unconditional console.log.
In
`@portals/devportal/src/main/webapp/source/src/app/components/Apis/Details/GoToTryOut.jsx`:
- Around line 214-218: The destructuring of the taskManager(getKeyRequest(),
'prepare') result can throw if getKeyRequest() returns undefined; update the
code around the call to taskManager/getKeyRequest so you first await the result
into a variable (e.g., const prepareResult = await taskManager(getKeyRequest(),
'prepare')), check if prepareResult is defined before destructuring, and either
throw/return early (propagate the error) or handle the undefined case (e.g.,
show error state) to avoid destructuring from undefined; then use
prepareResult.keyRequest when calling generateKeys(keyRequest,
defaultApplication.value) and continue to call setMultipleSecretsEnabled and
setConsumerSecretValue only after ensuring generatedKeys exists.
In
`@portals/devportal/src/main/webapp/source/src/app/components/Shared/ApiTryOut/TryOutController.jsx`:
- Around line 828-834: The Consumer Secret visibility IconButton is missing an
accessibility label; update the IconButton (the element that calls onClick={()
=> setShowSecret(!showSecret)} and toggles between VisibilityOff and Visibility)
to include aria-label='toggle password visibility' (matching the existing
Password field) so screen readers can describe the control.
- Around line 607-617: The enableGetTestKeyButton logic incorrectly ignores
enableTokenHashing and can disable the button when the Consumer Secret field is
hidden; compute isConsumerSecretRequired first (using
isMultipleClientSecretsAllowed, securitySchemeType === 'OAUTH', selectedKMObject
and !selectedKMObject.enableTokenHashing) and then set enableGetTestKeyButton
based on isConsumerSecretRequired (if consumer secret is required, require
!!consumerSecret?.trim(), otherwise true); also update the tooltip condition
(currently using !enableGetTestKeyButton && securitySchemeType === 'OAUTH' &&
isMultipleClientSecretsAllowed) to use isConsumerSecretRequired instead so the
"Consumer Secret is required" hint only appears when the field is shown.
- Around line 50-51: The enableGetTestKeyButton logic must account for
enableTokenHashing: compute isConsumerSecretRequired using
selectedKMObject.enableTokenHashing (use selectedKMObject &&
!selectedKMObject.enableTokenHashing with isMultipleClientSecretsAllowed and
securitySchemeType === 'OAUTH'), then set enableGetTestKeyButton so that when
securitySchemeType === 'OAUTH' and isMultipleClientSecretsAllowed it requires
consumerSecret only if isConsumerSecretRequired (otherwise true). Also clear the
consumerSecret state on app/KM change by calling setConsumerSecret('') in the
reducer branches handling 'selectedApplication' and 'selectedKeyManager'. Add
aria-label='toggle password visibility' to the IconButton that toggles consumer
secret visibility. Remove the redundant isConsumerSecretRequired && from the
TextField helperText (since the TextField is only rendered when
isConsumerSecretRequired). Finally, replace all Grid prop usages of x= with xs=
for MUI v5 compatibility (e.g., Grid usages near the consumer secret and layout
code).
In
`@portals/devportal/src/main/webapp/source/src/app/components/Shared/AppsAndKeys/Secrets/NewSecretDialog.jsx`:
- Around line 117-130: showTemporaryError currently starts a 2s setTimeout that
updates state but never stores/clears the timer, risking stale setState calls on
unmount; modify showTemporaryError to save the timer id (e.g., in a ref) and
clear it in a cleanup useEffect (and clear any existing timer before creating a
new one) so setError/setHelperText aren’t called after unmount, and in
handleClose change setIsValid(true) to setIsValid(false) to match the initial
useState(false) and the intended "reset validity" behavior (refer to
showTemporaryError and handleClose to locate the changes).
- Around line 48-82: handleKeyDown currently allows "0" which later causes
handleCustomDaysChange (using /^[1-9][0-9]*$/) to reject a leading zero; update
handleKeyDown to reject a "0" when it's being entered as the first character
(e.g., when the current input is empty or the cursor/selection is at the start
with no preceding digits) so the keyboard guard matches the validator's rule —
detect the case (input value is empty or selectionStart === 0 with no selected
text) and call e.preventDefault() and showTemporaryError for a leading zero,
otherwise allow digits 0–9 as before; reference handleKeyDown and
handleCustomDaysChange to keep behavior consistent.
In
`@portals/devportal/src/main/webapp/source/src/app/components/Shared/AppsAndKeys/Secrets/SecretsTable.jsx`:
- Around line 614-617: SecretsTable.propTypes is missing declarations for props
that the component reads at runtime; add prop type entries for secretCount and
hashEnabled to SecretsTable.propTypes (e.g., declare secretCount as
PropTypes.number and hashEnabled as PropTypes.bool, optionally mark as
isRequired if they must be present) so the component's used props
(props.secretCount referenced in the render and props.hashEnabled referenced
earlier) are properly validated.
- Around line 109-116: When adjusting pagination after deleting a secret,
maxPage can become -1 (when updated.length === 0) which causes setPage(-1) and
breaks MUI TablePagination; update the logic in the component that computes
maxPage (where updated, rowsPerPage, page and setPage are used) to clamp maxPage
to a minimum of 0 (e.g., use Math.max(0, Math.ceil(updated.length / rowsPerPage)
- 1)) and only call setPage(maxPage) when page > maxPage so the page will never
be set to a negative value.
- Around line 134-141: Wrap the await applicationPromise call inside the
existing try/catch in handleCreateSecret so rejections from
Application.get(props.appId) are caught and handled (e.g., show error toast and
return); also build the payload conditionally so expiresIn is only included when
newSecret.expiresInSeconds is defined (e.g., if newSecret.expiresInSeconds !==
undefined then set expiresIn: newSecret.expiresInSeconds) to avoid sending
undefined for the "never expires" option. Ensure you reference
handleCreateSecret, applicationPromise, and newSecret.expiresInSeconds when
making the changes.
In
`@portals/devportal/src/main/webapp/source/src/app/components/Shared/AppsAndKeys/TokenManager.jsx`:
- Around line 62-65: Remove the duplicate named import of
isMultipleClientSecretsEnabled from the import list in TokenManager.jsx: keep a
single import statement that imports isMultipleClientSecretsEnabled from
'./Secrets/util' and delete the redundant import line so there is only one
reference to isMultipleClientSecretsEnabled in the module imports.
- Around line 838-943: The function keyConfigurationContent is dead and must be
removed or properly integrated: either delete the entire keyConfigurationContent
definition (since the inline DIRECT tab rendering already provides the UI) or
wire it into the render path used for the DIRECT tab and replace direct calls to
this.generateKeys with the handleGenerateKeysClick hook so the multi-secret
dialog flow is preserved; also stop passing React's reserved prop name key={key}
inside KeyConfiguration and Button (rename to something like keyObj or keyValue
and update references in KeyConfiguration and related handlers), and ensure any
references to keymanager.enableOAuthAppCreation and isKeyManagerAllowed logic
remain unchanged when moving/integrating the component.
In
`@portals/devportal/src/main/webapp/source/src/app/components/Shared/AppsAndKeys/ViewKeys.jsx`:
- Around line 593-594: In render(), the call to
isMultipleClientSecretsEnabled(keyManagerConfig.additionalProperties) can throw
if keyManagerConfig is null/undefined; update the usage in ViewKeys.jsx to guard
keyManagerConfig before accessing .additionalProperties (e.g., use optional
chaining or a default:
isMultipleClientSecretsEnabled(keyManagerConfig?.additionalProperties || {}) or
check if keyManagerConfig exists and handle the falsey case) so
multipleSecretsAllowed is computed safely; ensure any downstream logic that uses
multipleSecretsAllowed/secretCount still behaves correctly when keyManagerConfig
is missing.
In `@portals/devportal/src/main/webapp/source/src/app/data/Application.jsx`:
- Around line 359-395: Remove the leftover commented-out blocks inside the
generateSecret and getSecrets methods: delete the commented code that references
keyType/productionKeys/sandboxKeys and the unused keyManager/keysResponse lines
so each method returns only the actual secret or secrets as implemented; update
generateSecret (function generateSecret) and getSecrets (function getSecrets) to
contain no dead commented code and ensure return statements remain unchanged.
---
Outside diff comments:
In
`@portals/devportal/src/main/webapp/source/src/app/components/Apis/Details/Credentials/Wizard/GenerateAccessTokenStep.jsx`:
- Line 111: Remove the leftover debug concatenation console.log call in
GenerateAccessTokenStep.jsx (the line "console.log('token generated successfully
' + response);") because response is an object; either delete this debug line or
replace it with a proper logger/structured log (e.g., use console.log('token
generated successfully', response) or the app's logging utility) inside the
token generation handler so the object is logged correctly.
In
`@portals/devportal/src/main/webapp/source/src/app/components/Shared/ApiTryOut/SelectAppPanel.jsx`:
- Around line 147-241: The consumer secret TextField is only rendered in the
else-branch when allApplications === null, so when allApplications !== null the
secret input is missing; move or duplicate the isMultipleSecretsAllowed block
(the TextField using consumerSecret, onConsumerSecretChange, showSecret,
toggleVisibility and InputAdornment/Visibility icons) into the FormControl used
when allApplications !== null (or pull the block out of the ternary entirely and
render it after the RadioGroup/FormControl) so that the consumer secret appears
regardless of the allApplications branch; ensure you reuse the existing
props/state names (isMultipleSecretsAllowed, consumerSecret,
onConsumerSecretChange, showSecret, toggleVisibility) and keep the TextField’s
InputProps and adornment intact.
In
`@portals/devportal/src/main/webapp/source/src/app/components/Shared/ApiTryOut/TryOutController.jsx`:
- Around line 521-530: The consumerSecret state is not being cleared when
application or key manager changes; update the handlers for the
'selectedApplication' and 'selectedKeyManager' cases to also reset
consumerSecret (call the same state setter used for consumerSecret) alongside
setProductionAccessToken, setSandboxAccessToken, setProductionApiKey,
setSandboxApiKey before calling setSelectedApplication or setSelectedKeyManager
so stale secrets cannot be reused.
---
Duplicate comments:
In
`@portals/devportal/src/main/webapp/source/src/app/components/Apis/Details/GoToTryOut.jsx`:
- Line 159: Remove the debug console.log in GoToTryOut.jsx that concatenates a
string with the response object (the line "console.log('Keys generated
successfully with ID : ' + response)"); either delete it entirely or replace it
with a proper log that doesn't coerce the object to "[object Object]" (e.g.,
console.log('Keys generated successfully with ID:', response) or
console.log(JSON.stringify(response)) ) in the function/component where keys are
generated.
In
`@portals/devportal/src/main/webapp/source/src/app/components/Shared/ApiTryOut/SelectAppPanel.jsx`:
- Around line 214-240: This component references missing imports and props: add
imports for InputAdornment and IconButton from "@mui/material" and Visibility
and VisibilityOff from "@mui/icons-material"; then include the missing props in
the component's props destructuring (isMultipleSecretsAllowed, consumerSecret,
showSecret, onConsumerSecretChange, toggleVisibility) so the TextField block
uses the forwarded state/callbacks from TryOutController.jsx; ensure the parent
(TryOutController) passes those props down when rendering SelectAppPanel.
In
`@portals/devportal/src/main/webapp/source/src/app/components/Shared/ApiTryOut/TryOutController.jsx`:
- Around line 190-191: The state variables consumerSecret and showSecret in
TryOutController.jsx are correctly initialized and no changes are required;
leave the declarations const [consumerSecret, setConsumerSecret] = useState('')
and const [showSecret, setShowSecret] = useState(false) in place and proceed
with the approved changes.
In
`@portals/devportal/src/main/webapp/source/src/app/components/Shared/AppsAndKeys/KeyConfiguration.jsx`:
- Line 352: The onCopy handler is being called immediately because the code uses
.then(onCopy()) instead of passing the function to .then; update the clipboard
promise usage (the onClick arrow that calls
navigator.clipboard.writeText(tokenEndpoint).then(onCopy())) to pass the
callback without invoking it (e.g., .then(onCopy) or .then(() => onCopy())) so
onCopy runs after the Promise resolves; apply the same change to the other two
occurrences that use .then(onCopy()) on lines referenced in the review.
- Around line 528-536: The AppConfiguration elements rendered inside
applicationConfiguration.map are missing the required React key prop; update the
mapping in KeyConfiguration.jsx to pass a stable unique key to each
<AppConfiguration> (e.g., key={config.id} or key={config.applicationId}) and
fall back to key={configIndex} only if no stable id exists—leave other props
(config, previousValue via getPreviousValue(config), isUserOwner, handleChange,
subscriptionScopes) unchanged.
In
`@portals/devportal/src/main/webapp/source/src/app/components/Shared/AppsAndKeys/TokenManager.jsx`:
- Around line 695-700: The orphaned promise from
this.application.then(...).getSecrets(applicationKey.keyMappingId) silently
discards both result and errors; update the TokenManager.jsx code around the
this.application chain to either await the promise or return it so callers can
handle the result, and attach a .catch(...) to handle/log errors (or propagate
by rethrowing) — specifically ensure the call to
application.getSecrets(applicationKey.keyMappingId) inside the
this.application.then callback returns the secrets value (or is awaited) and
that any rejection is handled via a catch or propagated.
---
Nitpick comments:
In
`@portals/devportal/src/main/webapp/source/src/app/components/Shared/ApiTryOut/TryOutController.jsx`:
- Around line 816-823: The helperText expression redundantly checks
isConsumerSecretRequired even though the whole TextField is already wrapped by
{isConsumerSecretRequired && (...)}, so inside the TextField's helperText (in
TryOutController.jsx) remove the leading isConsumerSecretRequired && guard and
simply use consumerSecret?.trim() ? null : <FormattedMessage ... />; update the
helperText to reference only consumerSecret (and FormattedMessage id
'Apis.Details.TryOutConsole.consumerSecret.required.helper') to keep behavior
identical but eliminate the redundant condition.
- Around line 364-366: The call that currently passes two positional undefined
values followed by consumerSecret (the callsite in TryOutController.jsx where
consumerSecret is passed with two leading undefineds) should be changed to use
named parameters: update the target function to accept a single options object
(e.g., { isTokenExchange, externalToken, consumerSecret }) or overload it to
accept an options object, then update this call to pass { isTokenExchange:
undefined, externalToken: undefined, consumerSecret } (or appropriate explicit
booleans/values) so the intent is explicit and resilient to signature changes;
modify the function definition and all callsites accordingly (search for the
function by name where these args are used) and update any JSDoc/type
annotations to reflect the new object parameter.
In
`@portals/devportal/src/main/webapp/source/src/app/components/Shared/AppsAndKeys/Secrets/SecretsTable.jsx`:
- Around line 89-91: The useEffect hooking fetchSecrets is using an empty
dependency array so it doesn't re-run when props.appId or props.keyMappingId
change; update the effect to depend on those values (and any state/closures it
uses) so fetchSecrets runs when applicationPromise (derived from props.appId) or
props.keyMappingId changes. Either add props.appId and props.keyMappingId (and
applicationPromise if present in scope) to the useEffect dependency array, or
memoize fetchSecrets with useCallback including those dependencies and then
reference that memoized function in useEffect. Ensure the effect references the
same identifiers shown (useEffect, fetchSecrets, props.appId,
props.keyMappingId, applicationPromise) so stale data is avoided.
In
`@portals/devportal/src/main/webapp/source/src/app/components/Shared/AppsAndKeys/TokenManager.jsx`:
- Around line 1272-1282: The NewSecretDialog and SecretValueDialog are being
instantiated inside the keyManagers.map() loop and thus created once per DIRECT
key manager while all share the same open state (openSecretCreateDialog,
openSecretValueDialog) and generatedSecret; hoist both dialog components out of
the keyManagers.map() so only one instance of NewSecretDialog and one
SecretValueDialog exist for the component, keep their props bound to the
existing state and handlers (openSecretCreateDialog, openSecretValueDialog,
generatedSecret, handleCloseSecretCreateDialog, handleCloseSecretValueDialog,
handleCreateSecret) and ensure any per-key-manager context needed (e.g., which
manager triggered open) is passed into the dialog via explicit prop (like
activeKeyManagerId) when opening from inside the map.
In `@portals/devportal/src/main/webapp/source/src/app/data/Application.jsx`:
- Around line 397-408: The deleteSecret method currently returns numeric HTTP
status (response.status) which is inconsistent with other delete-like methods
(e.g., cleanUpKeys) and with generateSecret's parsed object; change deleteSecret
(and its promise chain around client.apis['Application
Secrets'].revokeConsumerSecret) to return a boolean indicating success (use
response.ok) instead of response.status so callers get a consistent boolean
result like cleanUpKeys; update the returned promise to resolve to response.ok.
| } else { | ||
| proceedToNextStep(); | ||
| } | ||
| console.log('Keys generated successfully with ID : ' + response); |
There was a problem hiding this comment.
Remove debug console.log from success path.
'Keys generated successfully with ID : ' + response coerces the response object to [object Object] and logs it unconditionally in production.
🐛 Proposed fix
- console.log('Keys generated successfully with ID : ' + response);📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| console.log('Keys generated successfully with ID : ' + response); |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@portals/devportal/src/main/webapp/source/src/app/components/Apis/Details/Credentials/Wizard/GenerateKeysStep.jsx`
at line 261, Remove the debug console.log in GenerateKeysStep.jsx that prints
"Keys generated successfully with ID : " + response; locate the success path
where the keys are generated (the callback handling the response) and delete
that console.log; if you need to record the response for debugging in
non-production, replace it with a proper logger or persist/handle the response
object (e.g., use an existing logger or JSON.stringify(response) behind a debug
flag) instead of unconditional console.log.
| const { keyRequest, isMultipleSecretsAllowed } = await taskManager(getKeyRequest(), 'prepare'); | ||
| setMultipleSecretsEnabled(isMultipleSecretsAllowed); | ||
| // Generate consumer key and secret | ||
| await taskManager(generateKeys(keyRequest, defaultApplication.value), 'generate'); | ||
| const generatedKeys = await taskManager(generateKeys(keyRequest, defaultApplication.value), 'generate'); | ||
| setConsumerSecretValue(generatedKeys?.consumerSecret); |
There was a problem hiding this comment.
Destructuring from taskManager result will throw if getKeyRequest fails.
getKeyRequest() has a .catch() (line 132) that swallows errors and implicitly returns undefined. When taskManager resolves with undefined, destructuring { keyRequest, isMultipleSecretsAllowed } on line 214 throws a TypeError. The subsequent generateKeys(keyRequest, ...) call on line 217 would then use undefined as keyRequest.
Guard against the undefined result or let the error propagate so taskManager handles it:
Proposed fix (option A: guard)
- const { keyRequest, isMultipleSecretsAllowed } = await taskManager(getKeyRequest(), 'prepare');
- setMultipleSecretsEnabled(isMultipleSecretsAllowed);
+ const prepareResult = await taskManager(getKeyRequest(), 'prepare');
+ if (!prepareResult) {
+ return;
+ }
+ const { keyRequest, isMultipleSecretsAllowed } = prepareResult;
+ setMultipleSecretsEnabled(isMultipleSecretsAllowed);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@portals/devportal/src/main/webapp/source/src/app/components/Apis/Details/GoToTryOut.jsx`
around lines 214 - 218, The destructuring of the taskManager(getKeyRequest(),
'prepare') result can throw if getKeyRequest() returns undefined; update the
code around the call to taskManager/getKeyRequest so you first await the result
into a variable (e.g., const prepareResult = await taskManager(getKeyRequest(),
'prepare')), check if prepareResult is defined before destructuring, and either
throw/return early (propagate the error) or handle the undefined case (e.g.,
show error state) to avoid destructuring from undefined; then use
prepareResult.keyRequest when calling generateKeys(keyRequest,
defaultApplication.value) and continue to call setMultipleSecretsEnabled and
setConsumerSecretValue only after ensuring generatedKeys exists.
| import { isMultipleClientSecretsEnabled } from 'AppComponents/Shared/AppsAndKeys/Secrets/util'; | ||
| import Alert from 'AppComponents/Shared/Alert'; |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# First, verify the import paths exist
echo "=== Checking for Secrets util file ==="
find . -type f -name "util.js" -o -name "util.jsx" -o -name "util.ts" | grep -i secret | head -20
echo -e "\n=== Checking for Alert component ==="
find . -type f \( -name "Alert.js" -o -name "Alert.jsx" -o -name "Alert.tsx" \) | grep -i "Shared" | head -20
echo -e "\n=== Checking the main TryOutController file ==="
ls -la "portals/devportal/src/main/webapp/source/src/app/components/Shared/ApiTryOut/TryOutController.jsx"Repository: wso2/apim-apps
Length of output: 661
🏁 Script executed:
#!/bin/bash
# Let's examine the TryOutController.jsx file to verify the issues mentioned
echo "=== Examining TryOutController.jsx imports (lines 45-55) ==="
sed -n '45,55p' "portals/devportal/src/main/webapp/source/src/app/components/Shared/ApiTryOut/TryOutController.jsx"
echo -e "\n=== Checking generateToken call (around line 364) ==="
sed -n '360,370p' "portals/devportal/src/main/webapp/source/src/app/components/Shared/ApiTryOut/TryOutController.jsx"
echo -e "\n=== Checking enableGetTestKeyButton logic (around lines 607-617) ==="
sed -n '605,620p' "portals/devportal/src/main/webapp/source/src/app/components/Shared/ApiTryOut/TryOutController.jsx"
echo -e "\n=== Checking Grid x prop usage (around line 799) ==="
sed -n '797,805p' "portals/devportal/src/main/webapp/source/src/app/components/Shared/ApiTryOut/TryOutController.jsx"
echo -e "\n=== Checking IconButton aria-label (around lines 828-834) ==="
sed -n '825,840p' "portals/devportal/src/main/webapp/source/src/app/components/Shared/ApiTryOut/TryOutController.jsx"Repository: wso2/apim-apps
Length of output: 3376
🏁 Script executed:
#!/bin/bash
# Verify the Application.generateToken method signature
echo "=== Looking for Application class/object ==="
find . -type f \( -name "*.js" -o -name "*.jsx" -o -name "*.ts" -o -name "*.tsx" \) | xargs grep -l "class Application\|const Application\|function Application" | head -10
echo -e "\n=== Searching for generateToken method definition ==="
find . -type f \( -name "*.js" -o -name "*.jsx" -o -name "*.ts" -o -name "*.tsx" \) | xargs grep -l "generateToken" | grep -i application | head -10Repository: wso2/apim-apps
Length of output: 973
🏁 Script executed:
#!/bin/bash
# Get the generateToken method signature
echo "=== Application.generateToken method signature ==="
grep -A 20 "generateToken" "./portals/devportal/src/main/webapp/source/src/app/data/Application.jsx" | head -30
echo -e "\n=== isMultipleClientSecretsEnabled function ==="
cat -n "./portals/devportal/src/main/webapp/source/src/app/components/Shared/AppsAndKeys/Secrets/util.js"
echo -e "\n=== Checking case statements around line 521-530 ==="
sed -n '515,535p' "portals/devportal/src/main/webapp/source/src/app/components/Shared/ApiTryOut/TryOutController.jsx"
echo -e "\n=== Checking helperText logic around lines 816-823 ==="
sed -n '810,830p' "portals/devportal/src/main/webapp/source/src/app/components/Shared/ApiTryOut/TryOutController.jsx"Repository: wso2/apim-apps
Length of output: 6665
🏁 Script executed:
#!/bin/bash
# Check for more context on Grid usage pattern in the file
echo "=== Checking all Grid x prop usages in the file ==="
grep -n "Grid.*x=" "portals/devportal/src/main/webapp/source/src/app/components/Shared/ApiTryOut/TryOutController.jsx"
echo -e "\n=== Checking if xs is used anywhere in the file ==="
grep -n "Grid.*xs=" "portals/devportal/src/main/webapp/source/src/app/components/Shared/ApiTryOut/TryOutController.jsx" | head -5
echo -e "\n=== Check Grid imports and MUI version ==="
head -40 "portals/devportal/src/main/webapp/source/src/app/components/Shared/ApiTryOut/TryOutController.jsx" | grep -i "grid\|material"
echo -e "\n=== Check for other visibility toggle buttons to compare aria-label usage ==="
grep -B 5 -A 5 "aria-label.*toggle.*password" "portals/devportal/src/main/webapp/source/src/app/components/Shared/ApiTryOut/TryOutController.jsx"Repository: wso2/apim-apps
Length of output: 2463
🏁 Script executed:
#!/bin/bash
# Check the tooltip/hint logic around lines 975-987 to verify the downstream issue
echo "=== Checking tooltip hint logic (lines 975-987) ==="
sed -n '970,990p' "portals/devportal/src/main/webapp/source/src/app/components/Shared/ApiTryOut/TryOutController.jsx"Repository: wso2/apim-apps
Length of output: 1716
Fix enableGetTestKeyButton logic to account for enableTokenHashing.
The button disable state (lines 607-617) doesn't consider the enableTokenHashing constraint that controls field visibility. When enableTokenHashing=true, the consumer secret field is hidden (lines 816-823: isConsumerSecretRequired = isMultipleClientSecretsAllowed && securitySchemeType === 'OAUTH' && !selectedKMObject.enableTokenHashing), but the button remains disabled because consumerSecret is empty. This creates a mismatch: the user cannot enter a secret yet the button requiring that secret is disabled.
Fix: Compute isConsumerSecretRequired first, then use it in the enableGetTestKeyButton logic:
const isConsumerSecretRequired = isMultipleClientSecretsAllowed && securitySchemeType === 'OAUTH' &&
selectedKMObject && !selectedKMObject.enableTokenHashing;
let enableGetTestKeyButton = true;
if (securitySchemeType === 'OAUTH' && isMultipleClientSecretsAllowed) {
enableGetTestKeyButton = isConsumerSecretRequired ? !!consumerSecret?.trim() : true;
}Also fix these secondary issues:
-
Consumer secret not cleared on app/KM change (lines 521-530): Add
setConsumerSecret('')in bothcase 'selectedApplication'andcase 'selectedKeyManager'to prevent old secrets from persisting. -
Missing aria-label on visibility toggle (line 831): Add
aria-label='toggle password visibility'to the IconButton, matching the pattern at line 885. -
Redundant condition in helperText (lines 820-821): Remove
isConsumerSecretRequired &&check since the TextField is only rendered whenisConsumerSecretRequiredis true. -
Grid breakpoint prop (lines 629, 760, 778, 799, 845): Change
x=toxs=for MUI v5 Grid compatibility. Thexprop is not recognized and is silently ignored.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@portals/devportal/src/main/webapp/source/src/app/components/Shared/ApiTryOut/TryOutController.jsx`
around lines 50 - 51, The enableGetTestKeyButton logic must account for
enableTokenHashing: compute isConsumerSecretRequired using
selectedKMObject.enableTokenHashing (use selectedKMObject &&
!selectedKMObject.enableTokenHashing with isMultipleClientSecretsAllowed and
securitySchemeType === 'OAUTH'), then set enableGetTestKeyButton so that when
securitySchemeType === 'OAUTH' and isMultipleClientSecretsAllowed it requires
consumerSecret only if isConsumerSecretRequired (otherwise true). Also clear the
consumerSecret state on app/KM change by calling setConsumerSecret('') in the
reducer branches handling 'selectedApplication' and 'selectedKeyManager'. Add
aria-label='toggle password visibility' to the IconButton that toggles consumer
secret visibility. Remove the redundant isConsumerSecretRequired && from the
TextField helperText (since the TextField is only rendered when
isConsumerSecretRequired). Finally, replace all Grid prop usages of x= with xs=
for MUI v5 compatibility (e.g., Grid usages near the consumer secret and layout
code).
| const isMultipleClientSecretsAllowed = isMultipleClientSecretsEnabled(selectedKMObject?.additionalProperties); | ||
|
|
||
| // When multiple client secrets are allowed, for OAuth security scheme, the consumer secret should | ||
| // be available for the Get Test Key button to be enabled. | ||
| let enableGetTestKeyButton = true; // default | ||
| if (securitySchemeType === 'OAUTH' && isMultipleClientSecretsAllowed) { | ||
| enableGetTestKeyButton = !!consumerSecret?.trim(); // must provide consumer secret | ||
| } | ||
|
|
||
| const isConsumerSecretRequired = isMultipleClientSecretsAllowed && securitySchemeType === 'OAUTH' && | ||
| selectedKMObject && !selectedKMObject.enableTokenHashing; |
There was a problem hiding this comment.
enableGetTestKeyButton incorrectly disables the button when enableTokenHashing is true.
enableGetTestKeyButton gates on securitySchemeType === 'OAUTH' && isMultipleClientSecretsAllowed, but the Consumer Secret field is only shown when !selectedKMObject.enableTokenHashing is also true (see isConsumerSecretRequired on line 616). When enableTokenHashing is true:
isConsumerSecretRequired→false→ field is hiddenenableGetTestKeyButton→!!''→false→ button is disabled
The user sees no input field to provide a consumer secret but the button is still blocked. The same condition mismatch propagates to the tooltip hint at line 980 (!enableGetTestKeyButton && securitySchemeType === 'OAUTH' && isMultipleClientSecretsAllowed), which would display a misleading "Consumer Secret is required" message even when the field is not shown.
Fix: compute isConsumerSecretRequired first and derive enableGetTestKeyButton from it.
🐛 Proposed fix
const isMultipleClientSecretsAllowed = isMultipleClientSecretsEnabled(selectedKMObject?.additionalProperties);
-// When multiple client secrets are allowed, for OAuth security scheme, the consumer secret should
-// be available for the Get Test Key button to be enabled.
-let enableGetTestKeyButton = true; // default
-if (securitySchemeType === 'OAUTH' && isMultipleClientSecretsAllowed) {
- enableGetTestKeyButton = !!consumerSecret?.trim(); // must provide consumer secret
-}
-
const isConsumerSecretRequired = isMultipleClientSecretsAllowed && securitySchemeType === 'OAUTH' &&
selectedKMObject && !selectedKMObject.enableTokenHashing;
+
+// GET TEST KEY is gated on consumer secret only when it is actually required (shown in UI).
+const enableGetTestKeyButton = !isConsumerSecretRequired || !!consumerSecret?.trim();And for the tooltip (line 980), tighten the condition to avoid the misleading hint:
-{!enableGetTestKeyButton && securitySchemeType === 'OAUTH' && isMultipleClientSecretsAllowed && (
+{isConsumerSecretRequired && !consumerSecret?.trim() && (🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@portals/devportal/src/main/webapp/source/src/app/components/Shared/ApiTryOut/TryOutController.jsx`
around lines 607 - 617, The enableGetTestKeyButton logic incorrectly ignores
enableTokenHashing and can disable the button when the Consumer Secret field is
hidden; compute isConsumerSecretRequired first (using
isMultipleClientSecretsAllowed, securitySchemeType === 'OAUTH', selectedKMObject
and !selectedKMObject.enableTokenHashing) and then set enableGetTestKeyButton
based on isConsumerSecretRequired (if consumer secret is required, require
!!consumerSecret?.trim(), otherwise true); also update the tooltip condition
(currently using !enableGetTestKeyButton && securitySchemeType === 'OAUTH' &&
isMultipleClientSecretsAllowed) to use isConsumerSecretRequired instead so the
"Consumer Secret is required" hint only appears when the field is shown.
| SecretsTable.propTypes = { | ||
| appId: PropTypes.string, | ||
| keyMappingId: PropTypes.string, | ||
| }; |
There was a problem hiding this comment.
secretCount and hashEnabled props are used but undeclared in PropTypes.
Both props.secretCount (line 330) and props.hashEnabled (line 160) are read by the component without being listed.
🛠️ Proposed fix
SecretsTable.propTypes = {
appId: PropTypes.string,
keyMappingId: PropTypes.string,
+ secretCount: PropTypes.number,
+ hashEnabled: PropTypes.bool,
};📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| SecretsTable.propTypes = { | |
| appId: PropTypes.string, | |
| keyMappingId: PropTypes.string, | |
| }; | |
| SecretsTable.propTypes = { | |
| appId: PropTypes.string, | |
| keyMappingId: PropTypes.string, | |
| secretCount: PropTypes.number, | |
| hashEnabled: PropTypes.bool, | |
| }; |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@portals/devportal/src/main/webapp/source/src/app/components/Shared/AppsAndKeys/Secrets/SecretsTable.jsx`
around lines 614 - 617, SecretsTable.propTypes is missing declarations for props
that the component reads at runtime; add prop type entries for secretCount and
hashEnabled to SecretsTable.propTypes (e.g., declare secretCount as
PropTypes.number and hashEnabled as PropTypes.bool, optionally mark as
isRequired if they must be present) so the component's used props
(props.secretCount referenced in the render and props.hashEnabled referenced
earlier) are properly validated.
portals/devportal/src/main/webapp/source/src/app/components/Shared/AppsAndKeys/TokenManager.jsx
Outdated
Show resolved
Hide resolved
| const keyConfigurationContent = (keymanager) => ( | ||
| <Box m={2}> | ||
| <KeyConfiguration | ||
| keys={keys} | ||
| key={key} | ||
| selectedApp={selectedApp} | ||
| selectedTab={selectedTab} | ||
| keyType={keyType} | ||
| updateKeyRequest={this.updateKeyRequest} | ||
| keyRequest={keyRequest} | ||
| isUserOwner={isUserOwner} | ||
| isKeysAvailable={!!key} | ||
| keyManagerConfig={keymanager} | ||
| validating={validating} | ||
| updateHasError={this.updateHasError} | ||
| callbackError={hasError} | ||
| setValidating={this.setValidating} | ||
| defaultTokenEndpoint={defaultTokenEndpoint} | ||
| mode={mode} | ||
| /> | ||
| <div className={classes.generateWrapper}> | ||
| <ScopeValidation | ||
| resourcePath={resourcePaths.APPLICATION_GENERATE_KEYS} | ||
| resourceMethod={resourceMethods.POST} | ||
| > | ||
| {!isOrgWideAppUpdateEnabled && !isUserOwner ? ( | ||
| <> | ||
| <Button | ||
| id='generate-keys' | ||
| variant='contained' | ||
| color='primary' | ||
| className={classes.button} | ||
| onClick={ | ||
| key ? this.updateKeys : this.generateKeys | ||
| } | ||
| disabled={(!isOrgWideAppUpdateEnabled && !isUserOwner) || isLoading || !keymanager.enableOAuthAppCreation | ||
| || (isKeyManagerAllowed | ||
| && !isKeyManagerAllowed(keymanager.name) | ||
| && ((keymanager.name !== 'Resident Key Manager') | ||
| || (!this.isTokenExchangeEnabled() | ||
| && keymanager.name === 'Resident Key Manager')))} | ||
| > | ||
| {key | ||
| ? this.props.intl.formatMessage({ | ||
| id: 'Shared.AppsAndKeys.TokenManager.update.keys', | ||
| defaultMessage: 'Update Keys', | ||
| }) | ||
| : this.props.intl.formatMessage({ | ||
| id: 'Shared.AppsAndKeys.TokenManager.generate.keys', | ||
| defaultMessage: 'Generate Keys', | ||
| })} | ||
| {isLoading && <CircularProgress size={20} />} | ||
| </Button> | ||
| <Typography variant='caption'> | ||
| <FormattedMessage | ||
| defaultMessage='Only owner can generate or update keys' | ||
| id='Shared.AppsAndKeys.TokenManager.key.and.user.owner' | ||
| /> | ||
| </Typography> | ||
| </> | ||
| ) : ( | ||
| <Box display='flex'> | ||
| <Button | ||
| id='generate-keys' | ||
| variant='contained' | ||
| color='primary' | ||
| className={classes.button} | ||
| onClick={key ? this.updateKeys : this.generateKeys} | ||
| disabled={hasError || (isLoading || !keymanager.enableOAuthAppCreation) | ||
| || (mode && mode === 'MAPPED') | ||
| || (isKeyManagerAllowed | ||
| && !isKeyManagerAllowed(keymanager.name) | ||
| && ((keymanager.name !== 'Resident Key Manager') | ||
| || (!this.isTokenExchangeEnabled() | ||
| && keymanager.name === 'Resident Key Manager')))} | ||
| > | ||
| {key | ||
| ? this.props.intl.formatMessage({ | ||
| id: 'Shared.AppsAndKeys.TokenManager.update', | ||
| defaultMessage: 'Update', | ||
| }) | ||
| : this.props.intl.formatMessage({ | ||
| id: 'Shared.AppsAndKeys.TokenManager.generate.keys', | ||
| defaultMessage: 'Generate Keys', | ||
| })} | ||
| {isLoading && <CircularProgress size={20} />} | ||
| </Button> | ||
| {!keymanager.enableOAuthAppCreation && ( | ||
| <Box m={2} display='flex'> | ||
| <WarningIcon className={classes.warningIcon} /> | ||
| <Typography variant='body1'> | ||
| <FormattedMessage | ||
| defaultMessage='Oauth app creation | ||
| disabled for {kmName} key manager' | ||
| id='Shared.AppsAndKeys.TokenManager.app.creation.disable.warn' | ||
| values={{ kmName: keymanager.displayName || keymanager.name }} | ||
| /> | ||
| </Typography> | ||
| </Box> | ||
| )} | ||
| </Box> | ||
| )} | ||
| </ScopeValidation> | ||
| </div> | ||
| </Box> | ||
| ); |
There was a problem hiding this comment.
keyConfigurationContent is dead code — never invoked anywhere in the JSX.
This function is defined at line 838 but is never called in the render output. The actual key-configuration + generate-button UI is rendered inline in the DIRECT tab path at lines 1168–1271. As a result:
- The new
handleGenerateKeysClickhook is absent fromkeyConfigurationContent(line 871/905 still callthis.generateKeysdirectly), so the multi-secret dialog flow would be skipped if this function were ever wired in. - Line 842 still passes
key={key}(React's reserved reconciliation prop), the same issue flagged in prior review for this file.
Either remove the function entirely (the inline code is the live path) or integrate it properly.
🗑️ Remove dead code
- const keyConfigurationContent = (keymanager) => (
- <Box m={2}>
- <KeyConfiguration
- ...
- </KeyConfiguration>
- ...
- </Box>
- );
-📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const keyConfigurationContent = (keymanager) => ( | |
| <Box m={2}> | |
| <KeyConfiguration | |
| keys={keys} | |
| key={key} | |
| selectedApp={selectedApp} | |
| selectedTab={selectedTab} | |
| keyType={keyType} | |
| updateKeyRequest={this.updateKeyRequest} | |
| keyRequest={keyRequest} | |
| isUserOwner={isUserOwner} | |
| isKeysAvailable={!!key} | |
| keyManagerConfig={keymanager} | |
| validating={validating} | |
| updateHasError={this.updateHasError} | |
| callbackError={hasError} | |
| setValidating={this.setValidating} | |
| defaultTokenEndpoint={defaultTokenEndpoint} | |
| mode={mode} | |
| /> | |
| <div className={classes.generateWrapper}> | |
| <ScopeValidation | |
| resourcePath={resourcePaths.APPLICATION_GENERATE_KEYS} | |
| resourceMethod={resourceMethods.POST} | |
| > | |
| {!isOrgWideAppUpdateEnabled && !isUserOwner ? ( | |
| <> | |
| <Button | |
| id='generate-keys' | |
| variant='contained' | |
| color='primary' | |
| className={classes.button} | |
| onClick={ | |
| key ? this.updateKeys : this.generateKeys | |
| } | |
| disabled={(!isOrgWideAppUpdateEnabled && !isUserOwner) || isLoading || !keymanager.enableOAuthAppCreation | |
| || (isKeyManagerAllowed | |
| && !isKeyManagerAllowed(keymanager.name) | |
| && ((keymanager.name !== 'Resident Key Manager') | |
| || (!this.isTokenExchangeEnabled() | |
| && keymanager.name === 'Resident Key Manager')))} | |
| > | |
| {key | |
| ? this.props.intl.formatMessage({ | |
| id: 'Shared.AppsAndKeys.TokenManager.update.keys', | |
| defaultMessage: 'Update Keys', | |
| }) | |
| : this.props.intl.formatMessage({ | |
| id: 'Shared.AppsAndKeys.TokenManager.generate.keys', | |
| defaultMessage: 'Generate Keys', | |
| })} | |
| {isLoading && <CircularProgress size={20} />} | |
| </Button> | |
| <Typography variant='caption'> | |
| <FormattedMessage | |
| defaultMessage='Only owner can generate or update keys' | |
| id='Shared.AppsAndKeys.TokenManager.key.and.user.owner' | |
| /> | |
| </Typography> | |
| </> | |
| ) : ( | |
| <Box display='flex'> | |
| <Button | |
| id='generate-keys' | |
| variant='contained' | |
| color='primary' | |
| className={classes.button} | |
| onClick={key ? this.updateKeys : this.generateKeys} | |
| disabled={hasError || (isLoading || !keymanager.enableOAuthAppCreation) | |
| || (mode && mode === 'MAPPED') | |
| || (isKeyManagerAllowed | |
| && !isKeyManagerAllowed(keymanager.name) | |
| && ((keymanager.name !== 'Resident Key Manager') | |
| || (!this.isTokenExchangeEnabled() | |
| && keymanager.name === 'Resident Key Manager')))} | |
| > | |
| {key | |
| ? this.props.intl.formatMessage({ | |
| id: 'Shared.AppsAndKeys.TokenManager.update', | |
| defaultMessage: 'Update', | |
| }) | |
| : this.props.intl.formatMessage({ | |
| id: 'Shared.AppsAndKeys.TokenManager.generate.keys', | |
| defaultMessage: 'Generate Keys', | |
| })} | |
| {isLoading && <CircularProgress size={20} />} | |
| </Button> | |
| {!keymanager.enableOAuthAppCreation && ( | |
| <Box m={2} display='flex'> | |
| <WarningIcon className={classes.warningIcon} /> | |
| <Typography variant='body1'> | |
| <FormattedMessage | |
| defaultMessage='Oauth app creation | |
| disabled for {kmName} key manager' | |
| id='Shared.AppsAndKeys.TokenManager.app.creation.disable.warn' | |
| values={{ kmName: keymanager.displayName || keymanager.name }} | |
| /> | |
| </Typography> | |
| </Box> | |
| )} | |
| </Box> | |
| )} | |
| </ScopeValidation> | |
| </div> | |
| </Box> | |
| ); |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@portals/devportal/src/main/webapp/source/src/app/components/Shared/AppsAndKeys/TokenManager.jsx`
around lines 838 - 943, The function keyConfigurationContent is dead and must be
removed or properly integrated: either delete the entire keyConfigurationContent
definition (since the inline DIRECT tab rendering already provides the UI) or
wire it into the render path used for the DIRECT tab and replace direct calls to
this.generateKeys with the handleGenerateKeysClick hook so the multi-secret
dialog flow is preserved; also stop passing React's reserved prop name key={key}
inside KeyConfiguration and Button (rename to something like keyObj or keyValue
and update references in KeyConfiguration and related handlers), and ensure any
references to keymanager.enableOAuthAppCreation and isKeyManagerAllowed logic
remain unchanged when moving/integrating the component.
| const multipleSecretsAllowed = isMultipleClientSecretsEnabled(keyManagerConfig.additionalProperties); | ||
| let secretCount; |
There was a problem hiding this comment.
Guard against keyManagerConfig being null/undefined before accessing .additionalProperties.
keyManagerConfig is a required-by-usage prop but has no null-guard here. An unexpected null value would throw at this point in render().
🛠️ Proposed fix
- const multipleSecretsAllowed = isMultipleClientSecretsEnabled(keyManagerConfig.additionalProperties);
+ const multipleSecretsAllowed = isMultipleClientSecretsEnabled(keyManagerConfig?.additionalProperties);
let secretCount;
if (multipleSecretsAllowed) {
- secretCount = getClientSecretCount(keyManagerConfig.additionalProperties);
+ secretCount = getClientSecretCount(keyManagerConfig?.additionalProperties);
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const multipleSecretsAllowed = isMultipleClientSecretsEnabled(keyManagerConfig.additionalProperties); | |
| let secretCount; | |
| const multipleSecretsAllowed = isMultipleClientSecretsEnabled(keyManagerConfig?.additionalProperties); | |
| let secretCount; | |
| if (multipleSecretsAllowed) { | |
| secretCount = getClientSecretCount(keyManagerConfig?.additionalProperties); | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@portals/devportal/src/main/webapp/source/src/app/components/Shared/AppsAndKeys/ViewKeys.jsx`
around lines 593 - 594, In render(), the call to
isMultipleClientSecretsEnabled(keyManagerConfig.additionalProperties) can throw
if keyManagerConfig is null/undefined; update the usage in ViewKeys.jsx to guard
keyManagerConfig before accessing .additionalProperties (e.g., use optional
chaining or a default:
isMultipleClientSecretsEnabled(keyManagerConfig?.additionalProperties || {}) or
check if keyManagerConfig exists and handle the falsey case) so
multipleSecretsAllowed is computed safely; ensure any downstream logic that uses
multipleSecretsAllowed/secretCount still behaves correctly when keyManagerConfig
is missing.
|
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
portals/devportal/src/main/webapp/source/src/app/components/Shared/ApiTryOut/TryOutController.jsx (1)
619-625:⚠️ Potential issue | 🔴 CriticalCritical:
useEffectcalled after early returns — Rules of Hooks violation.The
useEffectat line 619 is placed after the early-return guards at lines 575–580. WhenapiisnullornotFoundistrue, the component returns before this hook is reached, violating React's Rules of Hooks (hooks must be called unconditionally and in the same order every render). This will cause a React runtime error.Move this
useEffectabove the early returns (e.g., next to the otheruseEffectcalls around lines 486–500).Proposed fix
+ useEffect(() => { + if (securitySchemeType === 'API-KEY') { + setTokenValue(selectedKeyType === 'PRODUCTION' ? productionApiKey : sandboxApiKey); + } else { + setTokenValue(selectedKeyType === 'PRODUCTION' ? productionAccessToken : sandboxAccessToken); + } + }, [securitySchemeType, selectedKeyType, productionAccessToken, sandboxAccessToken, productionApiKey, sandboxApiKey]); + if (api == null) { return <Progress />; } if (notFound) { return 'API Not found !'; } ... - useEffect(() => { - if (securitySchemeType === 'API-KEY') { - setTokenValue(selectedKeyType === 'PRODUCTION' ? productionApiKey : sandboxApiKey); - } else { - setTokenValue(selectedKeyType === 'PRODUCTION' ? productionAccessToken : sandboxAccessToken); - } - }, [securitySchemeType, selectedKeyType, productionAccessToken, sandboxAccessToken, productionApiKey, sandboxApiKey]);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@portals/devportal/src/main/webapp/source/src/app/components/Shared/ApiTryOut/TryOutController.jsx` around lines 619 - 625, The useEffect that sets tokenValue (references: useEffect, setTokenValue, securitySchemeType, selectedKeyType, productionAccessToken, sandboxAccessToken, productionApiKey, sandboxApiKey) is placed after the component's early returns when api is null or notFound, which violates the Rules of Hooks; move this useEffect above the early-return guards (e.g., next to the other useEffect calls around the earlier block) so that the hook is invoked unconditionally and in the same order on every render.portals/devportal/src/main/webapp/source/src/app/components/Shared/AppsAndKeys/TokenExchangeKeyConfiguration.jsx (1)
369-379:⚠️ Potential issue | 🟠 Major
residentKMConsumerSecretpassed togenerateTokenis ignored in the token-exchange branch.When
generateAccessTokencallsapplication.generateToken()withisTokenExchange=trueandresidentKMConsumerSecretas the 7th argument, the parameter is completely ignored. InApplication.jsxlines 112–152, the token-exchange branch always useskeys.consumerSecret(line 132) and never reads theconsumerSecretparameter.By contrast, the non-token-exchange branch (lines 171–173) respects the parameter when provided, only falling back to
keys.consumerSecretif it's null.This means the consumer secret entered by the user in this dialog has no effect when token exchange is used. The token-exchange branch should be updated to conditionally use the
consumerSecretparameter, similar to the else branch.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@portals/devportal/src/main/webapp/source/src/app/components/Shared/AppsAndKeys/TokenExchangeKeyConfiguration.jsx` around lines 369 - 379, The token-exchange branch of the Application.generateToken function ignores the consumerSecret parameter so the residentKMConsumerSecret passed from generateAccessToken has no effect; update Application.jsx (the generateToken function) so that inside the token-exchange (isTokenExchange) branch you conditionally use the supplied consumerSecret parameter (the function's consumerSecret arg) when non-null/defined instead of always using keys.consumerSecret (same logic as the non-token-exchange/else branch), ensuring the token-exchange path falls back to keys.consumerSecret only if the consumerSecret parameter is not provided.
🧹 Nitpick comments (3)
portals/devportal/src/main/webapp/source/src/app/components/Shared/AppsAndKeys/Secrets/NewSecretDialog.jsx (1)
132-152:handleSubmitcallsonClose()synchronously afteronCreate— consider letting the parent control close.
handleSubmitcallsonCreate(payload)(line 148) and then immediately resets form and callsonClose()(lines 150–151). If the parent'sonCreateis asynchronous and may fail, the dialog will have already closed and reset. Consider lettingonCreatereturn a promise and only closing on success, or delegate the close entirely to the parent.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@portals/devportal/src/main/webapp/source/src/app/components/Shared/AppsAndKeys/Secrets/NewSecretDialog.jsx` around lines 132 - 152, handleSubmit currently calls onCreate(payload) synchronously then resets the form and calls onClose(), which closes the dialog even if onCreate fails; update handleSubmit to either await the promise returned by onCreate and only call setForm(initialFormState) and onClose() on successful resolution (and handle/display errors on rejection), or remove the onClose()/setForm calls and let the parent component decide when to close/reset after onCreate completes. Target functions/variables: handleSubmit, onCreate, onClose, setForm, initialFormState.portals/devportal/src/main/webapp/source/src/app/components/Shared/AppsAndKeys/TokenManager.jsx (1)
1267-1277: Minor: use destructured state variables for dialog props for consistency.
this.state.openSecretCreateDialog,this.state.openSecretValueDialog, andthis.state.generatedSecretare accessed directly here, inconsistent with the destructuring pattern at lines 825–829 used throughout the rest ofrender().♻️ Suggested refactor
Add to the destructuring at the top of
render():const { keys, keyRequest, isLoading, isKeyJWT, providedConsumerKey, providedConsumerSecret, selectedTab, keyManagers, validating, hasError, initialToken, - initialValidityTime, initialScopes, importDisabled, mode, tokenType, isOrgWideAppUpdateEnabled, isAccordionExpanded, + initialValidityTime, initialScopes, importDisabled, mode, tokenType, isOrgWideAppUpdateEnabled, isAccordionExpanded, + openSecretCreateDialog, openSecretValueDialog, generatedSecret, } = this.state;Then update the dialog props:
- <NewSecretDialog - open={this.state.openSecretCreateDialog} + <NewSecretDialog + open={openSecretCreateDialog} ... - <SecretValueDialog - open={this.state.openSecretValueDialog} + <SecretValueDialog + open={openSecretValueDialog} - secret={this.state.generatedSecret} + secret={generatedSecret}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@portals/devportal/src/main/webapp/source/src/app/components/Shared/AppsAndKeys/TokenManager.jsx` around lines 1267 - 1277, Extract openSecretCreateDialog, openSecretValueDialog, and generatedSecret from state in the render() destructuring (alongside other destructured state vars) and replace direct uses of this.state.openSecretCreateDialog, this.state.openSecretValueDialog, and this.state.generatedSecret in the NewSecretDialog and SecretValueDialog props with the newly destructured variables to match the existing pattern used earlier in render() and keep JSX consistent.portals/devportal/src/main/webapp/site/public/locales/en.json (1)
697-699: Three validation error keys are used in different contexts — consolidation would require refactoring.The three error keys in lines 697-699 are all actively used in NewSecretDialog.jsx, but for different validation triggers:
error.digitsOnly: real-time keystroke validation (handleKeyDown)error.only.positive.numbers: paste event validation (handlePaste)error.positive.whole.numbers: field change validation (handleCustomDaysChange)While
error.only.positive.numbersanderror.positive.whole.numbersmessages are nearly identical, consolidating them would require refactoring the validation logic across multiple handlers. If consolidation is desired, consider unifying the messages and using a single key across all three validation points.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@portals/devportal/src/main/webapp/site/public/locales/en.json` around lines 697 - 699, Consolidate the three nearly identical i18n keys into a single message to avoid duplication: create one key (e.g., "Shared.AppsAndKeys.Secrets.NewSecretDialog.error.positiveWholeNumbers") in en.json and remove the other two, then update NewSecretDialog.jsx to reference that single key from all validation points (handleKeyDown, handlePaste, handleCustomDaysChange); ensure each handler still triggers the same unified message for their respective validation failures and run tests to verify no missing-key regressions.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@portals/devportal/src/main/webapp/source/src/app/components/Apis/Details/GoToTryOut.jsx`:
- Around line 448-452: The onClick handler calls
navigator.clipboard.writeText(consumerSecretValue) without handling rejections;
wrap the clipboard call in a try/catch or add a .catch() to handle promise
rejections and avoid uncaught errors. Update the onClick callback that currently
calls navigator.clipboard.writeText(consumerSecretValue) to catch failures and
log or surface an error (and only call setCopied(true) on success), e.g., use
navigator.clipboard.writeText(...).then(() => setCopied(true)).catch(err => {/*
handle/log error and keep copied false */}) or the equivalent async/try-catch
around the same symbols.
In
`@portals/devportal/src/main/webapp/source/src/app/components/Shared/AppsAndKeys/TokenManager.jsx`:
- Around line 553-557: The SecretValueDialog is being opened unconditionally
when isMultipleSecretsAllowed is true, causing it to show null secrets for
intermediate key states; modify the branch that sets generatedSecret and
openSecretValueDialog to also check the keyState (e.g., only when keyState
indicates a completed/approved state and not CREATED or REJECTED) and that
response.consumerSecret is non-null/defined before calling this.setState({
generatedSecret: response.consumerSecret, openSecretValueDialog: true }); update
the logic where isMultipleSecretsAllowed is handled (the block referencing
generatedSecret, openSecretValueDialog, keyState, CREATED, REJECTED, and
consumerSecret) so the dialog only opens for completed keys with a real
consumerSecret.
---
Outside diff comments:
In
`@portals/devportal/src/main/webapp/source/src/app/components/Shared/ApiTryOut/TryOutController.jsx`:
- Around line 619-625: The useEffect that sets tokenValue (references:
useEffect, setTokenValue, securitySchemeType, selectedKeyType,
productionAccessToken, sandboxAccessToken, productionApiKey, sandboxApiKey) is
placed after the component's early returns when api is null or notFound, which
violates the Rules of Hooks; move this useEffect above the early-return guards
(e.g., next to the other useEffect calls around the earlier block) so that the
hook is invoked unconditionally and in the same order on every render.
In
`@portals/devportal/src/main/webapp/source/src/app/components/Shared/AppsAndKeys/TokenExchangeKeyConfiguration.jsx`:
- Around line 369-379: The token-exchange branch of the
Application.generateToken function ignores the consumerSecret parameter so the
residentKMConsumerSecret passed from generateAccessToken has no effect; update
Application.jsx (the generateToken function) so that inside the token-exchange
(isTokenExchange) branch you conditionally use the supplied consumerSecret
parameter (the function's consumerSecret arg) when non-null/defined instead of
always using keys.consumerSecret (same logic as the non-token-exchange/else
branch), ensuring the token-exchange path falls back to keys.consumerSecret only
if the consumerSecret parameter is not provided.
---
Duplicate comments:
In
`@portals/devportal/src/main/webapp/source/src/app/components/Apis/Details/GoToTryOut.jsx`:
- Line 160: Remove the debug console.log in GoToTryOut.jsx that does
"console.log('Keys generated successfully with ID : ' + response)"; either
delete it or replace it with a guarded debug statement that safely serializes
the response (e.g., use JSON.stringify(response)) and only runs in
non-production (check NODE_ENV or a debug flag). Locate the call inside the
key-generation handler in the GoToTryOut component and ensure any retained
logging is conditional and does not concatenate an object to a string.
- Around line 214-219: The destructuring of taskManager(getKeyRequest(),
'prepare') can throw if getKeyRequest() fails and taskManager resolves to
undefined; update the code around the taskManager(getKeyRequest(), 'prepare')
call (the block that sets keyRequest/isMultipleSecretsAllowed and calls
generateKeys) to validate the returned value before destructuring and before
calling generateKeys: call taskManager(getKeyRequest(), 'prepare'), check if the
result is truthy (or has the expected properties), handle the error path by
early returning or setting safe defaults and logging, only then destructure {
keyRequest, isMultipleSecretsAllowed } and proceed to call
taskManager(generateKeys(keyRequest,...), 'generate'), and ensure
setMultipleSecretsEnabled and setConsumerSecretValue are only called with valid
values.
In
`@portals/devportal/src/main/webapp/source/src/app/components/Shared/ApiTryOut/TryOutController.jsx`:
- Line 629: In TryOutController.jsx the MUI Grid elements (e.g., the <Grid>
usages that include className={classes.centerItems}) incorrectly use the
non-existent prop "x" instead of the MUI breakpoint prop "xs", so sizing is
ignored; update each offending Grid element (instances around the Grid with
className={classes.centerItems} and the other Grid usages noted) to use xs={...}
in place of x={...} so the intended responsive column sizing is applied.
- Around line 828-834: The consumer secret visibility IconButton (the one using
setShowSecret and showSecret with Visibility/VisibilityOff) is missing an
aria-label; add an accessibility label such as aria-label="toggle consumer
secret visibility" to that IconButton so it matches the password toggle pattern
and aids screen readers (update the IconButton element that calls onClick={() =>
setShowSecret(!showSecret)}).
- Around line 607-617: The Get Test Key button enablement currently checks
securitySchemeType and isMultipleClientSecretsAllowed but ignores token hashing;
change the logic so enableGetTestKeyButton is derived from
isConsumerSecretRequired (which already considers
isMultipleClientSecretsAllowed, securitySchemeType === 'OAUTH', and
!selectedKMObject.enableTokenHashing) and only falls back to checking
consumerSecret when isConsumerSecretRequired is true; also update the tooltip
text shown near the Get Test Key button (the tooltip referenced in the file) to
reflect that when enableTokenHashing is true the consumer secret field is hidden
and not required for the button to be enabled.
In
`@portals/devportal/src/main/webapp/source/src/app/components/Shared/AppsAndKeys/Secrets/NewSecretDialog.jsx`:
- Around line 117-130: In showTemporaryError, avoid state updates after unmount
by storing the setTimeout id in a ref (e.g., errorTimeoutRef), clear any
existing timeout before creating a new one, and clear the ref in a useEffect
cleanup so the timeout is cancelled on unmount; in handleClose, change
setIsValid(true) to setIsValid(false) to restore the initial invalid state
(matching the useState(false) initial value) and also clear any pending error
timeout via the same ref before resetting the form and calling onClose.
In
`@portals/devportal/src/main/webapp/source/src/app/components/Shared/AppsAndKeys/TokenManager.jsx`:
- Around line 831-936: Remove the dead function keyConfigurationContent entirely
(including its inner KeyConfiguration JSX and generate/update Button blocks)
since it's unused and contains stale logic; ensure there are no remaining
references to keyConfigurationContent, and if any future extraction is needed
prefer the existing inline KeyConfiguration block that uses the corrected prop
name (do not pass prop named key) and the centralized handlers (use
this.handleGenerateKeysClick instead of this.generateKeys, and keep
this.updateKeys for updates). Also verify there are no lingering uses of the
reserved prop name key inside that removed code and that any feature relying on
multi-secret dialog is triggered only from the live inline implementation.
In
`@portals/devportal/src/main/webapp/source/src/app/components/Shared/AppsAndKeys/ViewKeys.jsx`:
- Around line 593-597: The render code in ViewKeys.jsx reads
keyManagerConfig.additionalProperties without guarding for null/undefined;
update the logic around multipleSecretsAllowed, getClientSecretCount and calls
to isMultipleClientSecretsEnabled so they first verify keyManagerConfig exists
(e.g., use keyManagerConfig && keyManagerConfig.additionalProperties or optional
chaining keyManagerConfig?.additionalProperties) and provide a safe default
(false/0) for secretCount when keyManagerConfig is missing; modify the variables
multipleSecretsAllowed and secretCount initialization to use these guarded
checks so render() cannot throw.
---
Nitpick comments:
In `@portals/devportal/src/main/webapp/site/public/locales/en.json`:
- Around line 697-699: Consolidate the three nearly identical i18n keys into a
single message to avoid duplication: create one key (e.g.,
"Shared.AppsAndKeys.Secrets.NewSecretDialog.error.positiveWholeNumbers") in
en.json and remove the other two, then update NewSecretDialog.jsx to reference
that single key from all validation points (handleKeyDown, handlePaste,
handleCustomDaysChange); ensure each handler still triggers the same unified
message for their respective validation failures and run tests to verify no
missing-key regressions.
In
`@portals/devportal/src/main/webapp/source/src/app/components/Shared/AppsAndKeys/Secrets/NewSecretDialog.jsx`:
- Around line 132-152: handleSubmit currently calls onCreate(payload)
synchronously then resets the form and calls onClose(), which closes the dialog
even if onCreate fails; update handleSubmit to either await the promise returned
by onCreate and only call setForm(initialFormState) and onClose() on successful
resolution (and handle/display errors on rejection), or remove the
onClose()/setForm calls and let the parent component decide when to close/reset
after onCreate completes. Target functions/variables: handleSubmit, onCreate,
onClose, setForm, initialFormState.
In
`@portals/devportal/src/main/webapp/source/src/app/components/Shared/AppsAndKeys/TokenManager.jsx`:
- Around line 1267-1277: Extract openSecretCreateDialog, openSecretValueDialog,
and generatedSecret from state in the render() destructuring (alongside other
destructured state vars) and replace direct uses of
this.state.openSecretCreateDialog, this.state.openSecretValueDialog, and
this.state.generatedSecret in the NewSecretDialog and SecretValueDialog props
with the newly destructured variables to match the existing pattern used earlier
in render() and keep JSX consistent.
| onClick={() => { | ||
| navigator.clipboard.writeText(consumerSecretValue); | ||
| setCopied(true); | ||
| setTimeout(() => setCopied(false), 2000); | ||
| }} |
There was a problem hiding this comment.
navigator.clipboard.writeText called without error handling.
The Clipboard API can reject (insecure context, permission denied, user gesture required). The unhandled rejection will surface as a console error. Add a .catch() or wrap in try/catch.
Proposed fix
onClick={() => {
- navigator.clipboard.writeText(consumerSecretValue);
- setCopied(true);
- setTimeout(() => setCopied(false), 2000);
+ navigator.clipboard.writeText(consumerSecretValue)
+ .then(() => {
+ setCopied(true);
+ setTimeout(() => setCopied(false), 2000);
+ })
+ .catch(() => {
+ Alert.error('Failed to copy to clipboard');
+ });
}}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@portals/devportal/src/main/webapp/source/src/app/components/Apis/Details/GoToTryOut.jsx`
around lines 448 - 452, The onClick handler calls
navigator.clipboard.writeText(consumerSecretValue) without handling rejections;
wrap the clipboard call in a try/catch or add a .catch() to handle promise
rejections and avoid uncaught errors. Update the onClick callback that currently
calls navigator.clipboard.writeText(consumerSecretValue) to catch failures and
log or surface an error (and only call setCopied(true) on success), e.g., use
navigator.clipboard.writeText(...).then(() => setCopied(true)).catch(err => {/*
handle/log error and keep copied false */}) or the equivalent async/try-catch
around the same symbols.
| if (isMultipleSecretsAllowed) { | ||
| this.setState({ | ||
| generatedSecret: response.consumerSecret, openSecretValueDialog: true, | ||
| }); | ||
| } |
There was a problem hiding this comment.
SecretValueDialog opens even for CREATED/REJECTED key states where consumerSecret is likely null.
The secret dialog is triggered before the keyState branch check. When the key creation is pending approval (keyState === CREATED), the API response typically doesn't include a usable consumerSecret, so SecretValueDialog would open with secret={null}.
🐛 Proposed fix — guard on completed state
+ const keyStateCompleted = response.keyState !== this.keyStates.CREATED && response.keyState !== this.keyStates.REJECTED;
if (isMultipleSecretsAllowed) {
+ if (keyStateCompleted) {
this.setState({
generatedSecret: response.consumerSecret, openSecretValueDialog: true,
});
+ }
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@portals/devportal/src/main/webapp/source/src/app/components/Shared/AppsAndKeys/TokenManager.jsx`
around lines 553 - 557, The SecretValueDialog is being opened unconditionally
when isMultipleSecretsAllowed is true, causing it to show null secrets for
intermediate key states; modify the branch that sets generatedSecret and
openSecretValueDialog to also check the keyState (e.g., only when keyState
indicates a completed/approved state and not CREATED or REJECTED) and that
response.consumerSecret is non-null/defined before calling this.setState({
generatedSecret: response.consumerSecret, openSecretValueDialog: true }); update
the logic where isMultipleSecretsAllowed is handled (the block referencing
generatedSecret, openSecretValueDialog, keyState, CREATED, REJECTED, and
consumerSecret) so the dialog only opens for completed keys with a real
consumerSecret.
1 participant
Purpose
Summary by CodeRabbit