feat(security): let a consumer supply the access token via setAccessTokenResolver - #329
feat(security): let a consumer supply the access token via setAccessTokenResolver#329gcutrini wants to merge 5 commits into
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 configurationConfiguration used: defaults Review profile: CHILL Plan: Team Run ID: 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. |
| if(typeof callback === 'function') | ||
| callback(e); | ||
| return Promise.reject(); | ||
| return; |
| */ | ||
| let _resolveAccessToken = null; | ||
|
|
||
| export const setAccessTokenResolver = (resolver) => { |
There was a problem hiding this comment.
@gcutrini The resolver is shared only across bundles that resolve to the same installed copy of the package, so the singleton guarantee (and the "every uicore lib entry shares one methods instance" statement in the description) holds only when the host's dependency tree hoists a single openstack-uicore-foundation.
Concrete case: in the current event-site tree, my-orders-tickets-widget, full-schedule-widget and schedule-filter-widget each carry a nested node_modules/openstack-uicore-foundation (they pin uicore as an exact-version dependency), and my-orders-tickets-widget/dist/index.js does require("openstack-uicore-foundation/lib/security/methods"), which resolves to its nested copy. In a cookie-only host, the host registers the resolver on the top-level copy while the widget's calls hit a copy whose resolver is still null, fall into the localStorage flow and throw AUTH_ERROR_MISSING_AUTH_INFO.
Suggested: no code change here. State the precondition in this JSDoc and in the PR description: the host must end up with a single hoisted copy of the package, so widgets should declare uicore as a peerDependency only, not as a pinned dependency. That way the host knows to dedupe before relying on the resolver.
There was a problem hiding this comment.
Fixed at the root instead of documenting it: since bd79348 the resolver no longer lives in module state. It lives on globalThis under Symbol.for('openstack-uicore-foundation.accessTokenResolver'), so every copy of security/methods reads the same slot, including a nested copy inside a widget's own node_modules. The single-hoisted-copy precondition is gone. The only case that still misses is a nested copy of an OLDER uicore version without this code, and that is equally true of any approach.
| // (e.g. query-actions) inlines its own copy with separate state. Other internal | ||
| // modules are stateless, so they stay inlined. | ||
| const METHODS_SRC = path.resolve(__dirname, 'src/components/security/methods.js'); | ||
| const METHODS_MODULE = `${PKG_NAME}/lib/security/methods`; |
There was a problem hiding this comment.
@gcutrini Every other lib bundle now requires this module by package name (43 bundles in the built output), and that require does not resolve in a symlinked install. Node and webpack (resolve.symlinks defaults to true) resolve a linked package to its real path, and from there no node_modules/openstack-uicore-foundation exists.
Reproduced with the built lib: a copied node_modules layout loads fine; a symlinked one fails with "Error: Cannot find module 'openstack-uicore-foundation/lib/security/methods'"; --preserve-symlinks loads fine again. summit-admin's verification docs name yarn link as a supported way to test a uicore branch (docs/plans/2026-08-10-uicore-sponsor-order-grid-line-id.md), so this changes that workflow.
Suggested: keep the rule as is, and add a note in the readme or the PR description that linked dev installs need resolve.symlinks: false in the consumer's webpack config, or a file: install. Published installs are unaffected.
There was a problem hiding this comment.
Resolved by the same change: the externals rule is deleted, so no bundle requires the package by its own name anymore. Your symlink repro should pass now with no resolve.symlinks changes, and summit-admin's yarn link workflow keeps working. This also makes the Copilot comment about the external type moot.
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.
| const shareSecurityMethods = ({ request, context, contextInfo }, cb) => { | ||
| if (!contextInfo || !contextInfo.issuer || !request.startsWith('.')) return cb(); | ||
| const resolved = path.resolve(context, request); | ||
| if (resolved === METHODS_SRC || `${resolved}.js` === METHODS_SRC) { | ||
| return cb(null, METHODS_MODULE); | ||
| } |
| 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
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.