Repository navigation
Conversation
…okenResolver Add setAccessTokenResolver: when a resolver is registered, getAccessToken delegates to it; otherwise the built-in flow is unchanged. Passing a non-function resets to the built-in. The resolver is module-level state, so externalize security/methods in the webpack build so every uicore lib entry shares one instance and sees the registered resolver.
Delegates to a registered resolver, a later resolver replaces the previous one, and a non-function argument clears it.
|
Important
This repository does not receive automatic reviews because it has fewer than 10 stars. ⚙️ Run configuration
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 |
_fetch rejected its promise after already calling back with the error. The query* functions never await that promise, so the rejection only surfaced as an unhandled rejection. Return instead. Also guard the 404 branch so a missing response or callback cannot throw.
With a resolver registered, query-actions, getUserInfo and AttendanceTracker send the resolver's token, and a resolver failure in query-actions reaches the caller's callback without a request.
|
Note on the externals rule added here. security/methods.js imported the SET_LOGGED_USER string from ./actions, and actions.js imports ./methods back. That import cycle already existed on v4.x and was harmless: the methods bundle just carried its own inlined copy of actions and methods. This PR externalizes security/methods so every bundle shares one instance (needed for the resolver state). With that rule, the inlined actions copy inside the methods bundle resolved to lib/security/methods.js itself, so the file required itself. It still loaded (nothing reads it at load time), but the inlined copies would see an empty module, and a consumer bundling uicore from source with esbuild can fail to resolve it. So it had to be fixed here, not later. Fix: the action-type strings moved to security/constants.js; actions.js re-exports them, so existing imports keep working. reducers.js and utils/actions.js also read from constants now, so they stop inlining the actions module. No API change. |
There was a problem hiding this comment.
🟡 Changes recommended
The new shareSecurityMethods webpack external currently returns an untyped external spec, which can generate incorrect runtime linkage (e.g., global-var externals instead of require) and break the intended singleton behavior.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Pull request overview
Adds an opt-in mechanism for consumers to supply access tokens to uicore via a registered resolver, enabling SSR/cookie-based hosts to keep tokens out of the browser JS context while keeping existing client-side token behavior as the default.
Changes:
- Introduces
setAccessTokenResolverand makesgetAccessTokendelegate to it when registered. - Refactors security action-type constants into
security/constantsand updates imports accordingly. - Adds focused Jest coverage to ensure resolver-provided tokens flow through key call sites (query-actions, getUserInfo, AttendanceTracker) and that resolver failures prevent downstream requests.
File summaries
| File | Description |
|---|---|
| webpack.common.js | Externalizes security/methods to ensure a singleton resolver state across multiple UMD entry bundles. |
| src/utils/query-actions.js | Prevents unhandled rejections when token retrieval fails; improves 404 callback safety. |
| src/utils/actions.js | Switches session-state constant import to security/constants. |
| src/utils/tests/query-actions.test.js | Verifies query-actions uses resolver token and stops on resolver failure. |
| src/components/security/reducers.js | Updates action-type imports to come from security/constants. |
| src/components/security/methods.js | Adds module-level access-token resolver and delegation in getAccessToken. |
| src/components/security/constants.js | Adds action-type constants previously declared in security/actions. |
| src/components/security/actions.js | Imports action-type constants from security/constants and re-exports them for compatibility. |
| src/components/security/tests/methods.test.js | Adds tests for resolver delegation/replacement/reset behavior. |
| src/components/security/tests/get-user-info.test.js | Verifies /members/me uses resolver-provided token via mocked request pipeline. |
| src/components/tests/attendance-tracker.test.js | Verifies AttendanceTracker metrics calls use resolver-provided token. |
Review details
- Files reviewed: 11/11 changed files
- Comments generated: 1
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| export const SESSION_STATE_STATUS_CHANGED = 'changed'; | ||
| export const SESSION_STATE_STATUS_ERROR = 'error'; | ||
| export const UPDATE_USER_INFO = 'UPDATE_USER_INFO'; | ||
| import { |
There was a problem hiding this comment.
whats the point of this change ? lets not refactor something if its out of scope. Also action name declarations tipically go in the actions file across all our products
There was a problem hiding this comment.
The implementation changed since this comment: the resolver no longer relies on the webpack externals rule, it lives on globalThis under Symbol.for (see the updated description), so state sharing no longer depends on how the bundles resolve their modules.
The constants move was tied to that old rule. methods and actions import each other (methods needs SET_LOGGED_USER from actions, actions imports methods), a circular import that has always existed upstream and is harmless while every bundle inlines its own copies. Externalizing methods turned the cycle into the methods bundle requiring its own file, and moving the action-type strings out was how I broke it.
With the new implementation the cycle is back to its long-standing harmless state, so the refactor is dropped: SET_LOGGED_USER stays in actions.js and webpack.common.js is untouched, PKG_NAME included. Untangling the circular import for real is worth its own PR.
| let http = request; | ||
| import URI from "urijs"; | ||
| import IdTokenVerifier from "idtoken-verifier"; | ||
| import {SET_LOGGED_USER} from "./actions"; |
There was a problem hiding this comment.
no need to do this according to the actions export
| const path = require('path'); | ||
| const nodeExternals = require('webpack-node-externals'); | ||
|
|
||
| const { name: PKG_NAME } = require('./package.json'); |
There was a problem hiding this comment.
by convention all capitals is for constants, use camelCase instead
…for global slot
The resolver moves from module state to
globalThis[Symbol.for('openstack-uicore-foundation.accessTokenResolver')],
so every copy of security/methods reads the same slot: bundles that inline
it, nested installs of the package, and symlinked dev installs. This drops
the webpack externals rule that made the module a cross-bundle singleton;
no bundle requires the package by its own name anymore, so yarn link
installs keep working and hosts no longer need a single hoisted copy.
bd79348 to
eb08c62
Compare
…lver contract
Read and write the resolver slot through a globalThis -> window -> {}
fallback (the showConfirmDialog bridge shape) so consumers on runtimes
without globalThis keep the built-in token flow instead of throwing.
Document the resolver contract on setAccessTokenResolver: the resolved
value is unvalidated downstream, a rejection is treated as an auth
failure by every caller, and a server-side registration is process-wide.
|
@smarcet the Sep 7 changes are addressed in |
There was a problem hiding this comment.
🟡 Changes recommended
The documentation inaccurately generalizes resolver rejection handling, while existing callers propagate, callback, log, or log out on errors differently, which could mislead consumers about transient authentication failures.
1 open issue
1 resolved since last review
Review effort: Lite
| * - a rejection is treated as an auth failure by every caller (the Dropzone | ||
| * input logs the user out via initLogOut); handle transient errors inside | ||
| * the resolver. |
There was a problem hiding this comment.
@gcutrini Copilot is right here. The sentence "a rejection is treated as an auth failure by every caller" overstates what the callers do; I wrote it in my earlier suggestion, so the fix is on my wording.
On this branch, a resolver rejection reaches each caller and each one handles it differently:
src/components/inputs/dropzone/index.js:170-180(accepthook) callsonError, theninitLogOut()unless the message starts withAUTH_ERROR_REFRESH_TOKEN_NETWORK_ERROR. This is the only path that logs the user out.src/components/inputs/dropzone/index.js:113(async status polling) callsstopPolling+onError, with no logout.src/components/security/actions.js:58(getUserInfo): the rejection escapes to whoever dispatched the thunk, beforeerrorHandleris set up.src/utils/query-actions.js:69(_fetch) passes the error to thequery*callback, andquery-actions.js:45(_fetchPromise) rejects to its caller.src/components/attendance-tracker.js:51/65/80only callconsole.log.
A host author reading the JSDoc should see what actually happens. Suggested replacement for that bullet:
* - a rejection propagates to each caller and is handled per caller: the
* Dropzone upload `accept` hook logs the user out via initLogOut,
* getUserInfo rejects to its dispatcher, the query* helpers pass the error
* to their callback, and AttendanceTracker only logs it. Handle transient
* errors inside the resolver.v4.x has the same block from #324, so the same wording should go there too.
ref: https://app.clickup.com/t/86bbnquuq
5.x line companion of #324 (same change on the v4.x line).
uicore's
getAccessTokenassumes the access token lives on the client. It reads and refreshes the token from local storage and serializes refreshes withnavigator.locks. That model does not fit a server-rendered host such as the Next.js event site, where the token is held in an encrypted httpOnly cookie and is never exposed to JavaScript. There is no client-side token for uicore to read, lock, or renew.Keeping the token out of JavaScript is also a deliberate security property. A token that never reaches the browser's JS context cannot be stolen by XSS or replayed from client code.
Several uicore modules call
getAccessToken, so the source must be injectable inside uicore rather than overridden by the consumer. Today a cookie-based host has to work around this by aliasingsecurity/methodsat build time and substituting its own implementation, which is invasive (build-system specific, replaces the whole module) and easy to drift out of sync.What
Add
setAccessTokenResolvertosecurity/methods: a consumer registers where the access token comes from, without replacing anything.getAccessToken. When a resolver is registered,getAccessTokendelegates to it; otherwise the built-in flow is unchanged. Passing a non-function resets to the built-in.setAccessTokenResolver.Renewal is the consumer's concern
When a resolver is registered, uicore delegates token retrieval to it and does not apply its own storage, locking, or renewal to the returned value. Freshness is left entirely to the consumer. A cookie-based host, for example, renews server-side (the server refreshes the session cookie via the refresh_token grant, and proxied API calls attach the real bearer from the cookie) and hands uicore only a "session present" placeholder, so there is nothing on the client to lock or renew.
Implementation note
The resolver lives on globalThis under a registered symbol:
globalThis[Symbol.for('openstack-uicore-foundation.accessTokenResolver')].setAccessTokenResolverwrites that slot;getAccessTokenreads it.Module state was not an option here. Every copy of
security/methodshas itsown module scope, and modern package managers routinely create several
copies of the package: pnpm materializes one per peer combination, widgets
can carry nested installs, and symlinked dev checkouts resolve to their own
tree. A resolver kept in module state is only visible to the copy that
registered it. The
Symbol.forregistry is shared by every copy on the page,so the singleton holds with no build-system help. React uses the same
pattern for its element symbols.
The webpack build is untouched: no bundle requires the package by its own
name, so
yarn linkinstalls keep working and hosts do not need a singlehoisted copy of the package.