[DAPS-1824] - Bug daps 1824 mapped collection consent flow #1831
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
nedvedba
added
Type: Bug
Contributor
Reviewer's GuideImplements a more robust mapped collection consent/auth flow by persisting collection and UI state through the Globus consent redirect, tightening token handling, and hardening endpoint-related APIs and logging. Sequence diagram for mapped collection consent and state restorationsequenceDiagram
actor User
participant Browser
participant EndpointBrowser
participant WebAPI as WebServer
participant ConsentHandler
participant GlobusAuth
participant AuthCallback as UiAuthnHandler
participant TokenHandler
participant Core as CoreSetAccessToken
participant DB as FoxxUserRouter
participant MainPage
User->>Browser: Open endpoint browser
Browser->>EndpointBrowser: List files on mapped collection
EndpointBrowser->>WebAPI: API call to mapped collection
WebAPI-->>EndpointBrowser: Error permission_denied / ConsentRequired
EndpointBrowser->>EndpointBrowser: Build stateObj (endpoint, path, mode)
EndpointBrowser->>EndpointBrowser: Optionally capture parent_dialog state
EndpointBrowser->>WebAPI: api.getGlobusConsentURL(endpoint_id, required_scopes, refresh_tokens=false, query_params, state)
WebAPI->>ConsentHandler: generateConsentURL(clientId, redirect_uri, requested_scopes, refresh_tokens, query_params, state)
ConsentHandler-->>WebAPI: consent_url
WebAPI-->>EndpointBrowser: consent_url
EndpointBrowser-->>User: Render link to login with required identity
User->>GlobusAuth: Click consent_url, login and grant consent
GlobusAuth-->>AuthCallback: Redirect to /ui/authn with code and state
AuthCallback->>AuthCallback: Validate user identity and derive username
AuthCallback->>AuthCallback: Parse state JSON
AuthCallback->>AuthCallback: Save restore_state to session
AuthCallback->>AuthCallback: Store collection_id and collection_type in session
AuthCallback->>TokenHandler: constructOptionalData(token_context)
TokenHandler->>TokenHandler: If collection_id present, set type=GLOBUS_TRANSFER and other=collection_id|scope
TokenHandler-->>AuthCallback: optional_data
AuthCallback->>Core: setAccessToken(uid, access_token, refresh_token, expires_in, optional_data, cb)
Core->>DB: Persist tokens for user and collection
DB-->>Core: Ack
Core-->>AuthCallback: cb(null)
AuthCallback->>Browser: Redirect to /ui/main
Browser->>WebAPI: GET /ui/main with session
WebAPI->>WebAPI: Extract restore_state from session
WebAPI-->>MainPage: Render main.ect with restore_state JSON
MainPage->>MainPage: On load, check tmpl_data.restore_state
MainPage->>MainPage: If parent_dialog.type is d_new_edit, import dlg_data_new_edit.js and reopen dialog
MainPage->>MainPage: If parent_dialog.type is transfer, import transfer/index.js and reopen transfer dialog
MainPage-->>User: Restored endpoint browser and dialogs
Entity relationship diagram for Globus collection and token after consenterDiagram
USER {
string _key
string eps
}
GLOBUS_COLLECTION {
string _key
string name
string description
string uuid
string required_scopes
string type
bool ha_enabled
}
GLOBUS_TOKEN {
string _key
string _from
string _to
string type
string dependent_scopes
number request_time
number last_used
string status
string access_token
string refresh_token
number expires_in
string other
}
USER ||--o{ GLOBUS_TOKEN : has
GLOBUS_COLLECTION ||--o{ GLOBUS_TOKEN : referenced_by
Updated class diagram for endpoint browsing and auth handlersclassDiagram
class EndpointBrowser {
+props endpoint
+props mode
+state path
+handleError(error)
}
class TransferUIManager {
-state frame
-#controller
+createDialog(labels)
}
class OAuthTokenHandler {
+constructOptionalData(token_context)
}
class ConsentHandler {
+generateConsentURL(clientId, redirectUri, requested_scopes, refresh_tokens, query_params, state)
}
class WebServer {
+storeCollectionId(req, res, next)
+getGlobusConsentURL(req, res)
+uiAuthnHandler(req, res)
+uiMain(req, res)
+setAccessToken(a_uid, a_acc_tok, a_ref_tok, a_expires_sec, token_optional_params, a_cb)
}
class MainPage {
+initFromTemplateData(tmpl_data)
+restoreDialogs(restore_state)
}
EndpointBrowser --> WebServer : api.getGlobusConsentURL
WebServer --> ConsentHandler : generateConsentURL
WebServer --> OAuthTokenHandler : constructOptionalData
WebServer --> WebServer : setAccessToken
WebServer --> MainPage : render main with restore_state
MainPage --> EndpointBrowser : restore endpoint_browser state
MainPage --> TransferUIManager : reopen transfer dialog
TransferUIManager --> TransferUIManager : createDialog attaches controller to frame
File-Level Changes
Possibly linked issues
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Hey - I've found 2 issues, and left some high level feedback:
- In
/ui/authnyou’re callinglogger.warning(...), but elsewhere onlylogger.info/logger.errorappear to be used; ifwarningisn’t defined on this logger it will throw during error handling—consider switching to the existinglogger.warn/logger.infostyle method or confirming the API. - The new
console.log('DEBUG: ...')calls in the Foxxuser_router(GLOBUS_TRANSFER branch) add a lot of noisy output and bypass your structured logging; consider either removing them or routing these through the existing logger with an appropriate log level.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- In `/ui/authn` you’re calling `logger.warning(...)`, but elsewhere only `logger.info`/`logger.error` appear to be used; if `warning` isn’t defined on this logger it will throw during error handling—consider switching to the existing `logger.warn`/`logger.info` style method or confirming the API.
- The new `console.log('DEBUG: ...')` calls in the Foxx `user_router` (GLOBUS_TRANSFER branch) add a lot of noisy output and bypass your structured logging; consider either removing them or routing these through the existing logger with an appropriate log level.
## Individual Comments
### Comment 1
<location> `web/datafed-ws.js:759-768` </location>
<code_context>
+ a_resp.redirect(redirect_path);
+ },
);
} catch (err) {
redirect_path = "/ui/error";
- logger.error("/ui/authn", getCurrentLineNumber(), err);
</code_context>
<issue_to_address>
**🚨 issue (security):** Session cleanup for registration tokens no longer runs on error, leaving sensitive data in the session.
Moving the cleanup from a `finally` block into the success callback means `name`, `email`, `uuids`, `acc_tok`, `acc_tok_ttl`, and `ref_tok` are only cleared on success. Any exception in the `try` path now leaves them in the session until expiry. Please reintroduce an unconditional cleanup path (e.g., a `finally` block or a dedicated `try/finally` around session field removal) to ensure PII and token data are always cleared, even on errors.
</issue_to_address>
### Comment 2
<location> `web/static/components/endpoint-browse/index.js:321-330` </location>
<code_context>
+ const stateObj = {
</code_context>
<issue_to_address>
**suggestion (bug_risk):** The serialized `state` object can become large and may risk exceeding URL length limits.
Because this includes `endpoint.rawData`, dialog metadata, and possibly many record IDs, the resulting OAuth `state` value may become too large for some browsers or intermediaries to handle. Consider trimming `rawData` to only what’s required, capping the number of record IDs, or storing the full state server-side and passing only a short lookup token in `state`.
Suggested implementation:
```javascript
// Serialize current state for restoration after consent flow.
// NOTE: Avoid putting the entire endpoint.rawData object into state,
// as it can become very large and cause URL length issues.
const rawEndpoint = this.props.endpoint?.rawData || {};
const endpointState = {
id: rawEndpoint.id,
name: rawEndpoint.name,
type: rawEndpoint.type,
};
const stateObj = {
endpoint_browser: {
endpoint: endpointState,
path: this.state.path,
mode: this.props.mode,
},
};
```
To fully implement the optimization described in your comment, you will likely also want to:
1. Identify where dialog metadata and record IDs are added to `stateObj` later in this function and:
- Trim any large payloads to the minimal data needed to restore UI state (e.g., avoid embedding entire records).
- Cap arrays of record IDs (for example, keep only the first N IDs that are required to resume the flow).
2. Optionally introduce a server-side state store:
- Generate a short token or ID on the server.
- Store the full state object server-side keyed by that token.
- Pass only the token in the OAuth `state` parameter instead of the full serialized state object.
3. Update the code that restores state after the consent flow to:
- Fetch the full state from the server using the token (if you adopt server-side state storage).
- Adjust any assumptions that the full `endpoint.rawData` object is available directly from `state`.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
Comment on lines
759
to
768
| } catch (err) { | ||
| redirect_path = "/ui/error"; | ||
| logger.error("/ui/authn", getCurrentLineNumber(), err); | ||
| logger.error( | ||
| "/ui/authn", | ||
| getCurrentLineNumber(), | ||
| "DEBUG: Exception in token handling: " + err, | ||
| ); | ||
| delete a_req.session.collection_id; | ||
| a_resp.redirect(redirect_path); | ||
| } |
Contributor
There was a problem hiding this comment.
🚨 issue (security): Session cleanup for registration tokens no longer runs on error, leaving sensitive data in the session.
Moving the cleanup from a finally block into the success callback means name, email, uuids, acc_tok, acc_tok_ttl, and ref_tok are only cleared on success. Any exception in the try path now leaves them in the session until expiry. Please reintroduce an unconditional cleanup path (e.g., a finally block or a dedicated try/finally around session field removal) to ensure PII and token data are always cleared, even on errors.
Collaborator
There was a problem hiding this comment.
Can you explain this concern more explicitly, I'm not seeing how the line numbers mentioned are relevant.
Contributor
There was a problem hiding this comment.
Thanks for asking for clarification — I can see how my original comment was unclear given the diff snippet in this thread doesn’t show the registration cleanup directly.
Here’s what I was referring to more concretely.
Where the sensitive data lives
During the /ui/authn flow, when a user is not yet registered, you stash a bunch of PII and tokens in the session:
// /ui/authn
if (!reply.user || !reply.user.length) {
// Not registered
a_req.session.uid = uid;
a_req.session.name = userinfo.name;
a_req.session.email = userinfo.email;
a_req.session.uuids = userinfo.identities_set;
a_req.session.acc_tok = xfr_token.access_token;
a_req.session.acc_tok_ttl = xfr_token.expires_in;
a_req.session.ref_tok = xfr_token.refresh_token;
a_resp.redirect("/ui/register");
}Those fields are then consumed in /api/usr/register:
// /api/usr/register
sendMessageDirect("UserCreateRequest", "", { ... }, function (reply) {
...
} else {
// Save access token
try {
setAccessToken(
a_req.session.uid,
a_req.session.acc_tok,
a_req.session.ref_tok,
a_req.session.acc_tok_ttl,
{},
(err) => {
if (err) {
logger.error("/api/usr/register", getCurrentLineNumber(), err);
a_resp.status(500).send("Registration failed during token set");
return;
}
// Remove data not needed for active session
delete a_req.session.name;
delete a_req.session.email;
delete a_req.session.uuids;
delete a_req.session.acc_tok;
delete a_req.session.acc_tok_ttl;
delete a_req.session.ref_tok;
delete a_req.session.uuids;
// Set session as registered user
a_req.session.reg = true;
a_resp.send(reply);
},
);
} catch (err) {
logger.error("/api/usr/register", getCurrentLineNumber(), err);
throw err;
}
}
});What changed in behavior
Previously (before this PR), the cleanup of name, email, uuids, acc_tok, acc_tok_ttl, and ref_tok happened in a finally block (or in a code path that ran regardless of whether setAccessToken succeeded). After your refactor, that cleanup is now inside the success callback for setAccessToken.
That means the cleanup only runs when:
- The
setAccessTokenfunction is called successfully, and - The async callback is invoked with
err == null.
But if any of these happen:
setAccessTokenthrows synchronously before invoking the callback (e.g., programming/validation error), or- The callback is invoked with an error and you
returnearly after sending a 500, or - There is some unexpected error path before reaching the callback,
then the session still contains:
session.name(PII)session.email(PII)session.uuids(identifiers)session.acc_tok(access token)session.ref_tok(refresh token)session.acc_tok_ttl
and those values are never cleared for that session unless the user retries and manages to complete a successful path later.
Why I called it “security” related
Normally, you want registration-time tokens and PII to be:
- Minimized: kept only as long as necessary
- Cleared on failure: especially if something goes wrong during registration, so you don’t retain stale tokens or extra PII in the session for longer than needed.
By tying cleanup only to the success branch of setAccessToken, the error paths effectively turn the session into long(er)-lived storage for that data. That’s what I was flagging.
Concrete suggestion
A safer pattern would be something like:
// Pseudocode for /api/usr/register
try {
setAccessToken(..., (err) => {
if (err) {
logger.error("/api/usr/register", getCurrentLineNumber(), err);
a_resp.status(500).send("Registration failed during token set");
return;
}
a_req.session.reg = true;
a_resp.send(reply);
});
} catch (err) {
logger.error("/api/usr/register", getCurrentLineNumber(), err);
a_resp.status(500).send("Registration failed during token set");
} finally {
// Always clear sensitive fields
delete a_req.session.name;
delete a_req.session.email;
delete a_req.session.uuids;
delete a_req.session.acc_tok;
delete a_req.session.acc_tok_ttl;
delete a_req.session.ref_tok;
delete a_req.session.uuids;
}Or, if you’re concerned about finally interacting with the async callback, you can centralize the cleanup into a helper and call it both in the error and success branches of the callback, plus in the catch block for synchronous errors.
I’m happy to adjust the comment to point at these specific lines and clarify that it’s about /api/usr/register rather than the authn snippet in this thread, if that helps tighten the review context.
Comment on lines
+321
to
+330
| const stateObj = { | ||
| endpoint_browser: { | ||
| endpoint: this.props.endpoint.rawData, | ||
| path: this.state.path, | ||
| mode: this.props.mode, | ||
| }, | ||
| }; | ||
|
|
||
| // Check if "New Data Record" dialog is open and save its state | ||
| const new_data_dlg = $("#d_new_edit"); |
Contributor
There was a problem hiding this comment.
suggestion (bug_risk): The serialized state object can become large and may risk exceeding URL length limits.
Because this includes endpoint.rawData, dialog metadata, and possibly many record IDs, the resulting OAuth state value may become too large for some browsers or intermediaries to handle. Consider trimming rawData to only what’s required, capping the number of record IDs, or storing the full state server-side and passing only a short lookup token in state.
Suggested implementation:
// Serialize current state for restoration after consent flow.
// NOTE: Avoid putting the entire endpoint.rawData object into state,
// as it can become very large and cause URL length issues.
const rawEndpoint = this.props.endpoint?.rawData || {};
const endpointState = {
id: rawEndpoint.id,
name: rawEndpoint.name,
type: rawEndpoint.type,
};
const stateObj = {
endpoint_browser: {
endpoint: endpointState,
path: this.state.path,
mode: this.props.mode,
},
};To fully implement the optimization described in your comment, you will likely also want to:
- Identify where dialog metadata and record IDs are added to
stateObjlater in this function and:- Trim any large payloads to the minimal data needed to restore UI state (e.g., avoid embedding entire records).
- Cap arrays of record IDs (for example, keep only the first N IDs that are required to resume the flow).
- Optionally introduce a server-side state store:
- Generate a short token or ID on the server.
- Store the full state object server-side keyed by that token.
- Pass only the token in the OAuth
stateparameter instead of the full serialized state object.
- Update the code that restores state after the consent flow to:
- Fetch the full state from the server using the token (if you adopt server-side state storage).
- Adjust any assumptions that the full
endpoint.rawDataobject is available directly fromstate.
Collaborator
There was a problem hiding this comment.
@nedvedba is raw endpoint data coming directly back from the Globus API? I don't think this is going to be a problem.
JoshuaSBrown
changed the title
Comment on lines
1871
to
1883
| @@ -1865,6 +1876,8 @@ router | |||
| status: "Started", | |||
| description: "Get recent end-points", | |||
| }); | |||
| client = g_lib.getUserFromClientID(req.queryParams.client); | |||
| first = client.eps && client.eps.length ? client.eps[0] : undefined; | |||
|
|
|||
| res.send(client.eps ? client.eps : []); | |||
| logger.logRequestSuccess({ | |||
Comment on lines
1890
to
1901
| @@ -1884,7 +1897,7 @@ router | |||
| routePath: basePath + "/ep/get", | |||
| status: "Failure", | |||
| description: "Get recent end-points", | |||
| extra: { most_recent: first, count: client?.eps?.length }, | |||
| extra: { most_recent: first, count: client && client.eps ? client.eps.length : 0 }, | |||
| error: e, | |||
| : ["openid", "profile", "email", "urn:globus:auth:scope:transfer.api.globus.org:all"]; | ||
|
|
||
| if (refresh_tokens) { | ||
| if (refresh_tokens && !scopes.includes("offline_access")) { |
Comment on lines
169
to
179
| case AccessTokenType.GLOBUS_DEFAULT: { | ||
| if (token_context.collection_id) { | ||
| const { collection_id, scope } = token_context; | ||
| if (!scope) { | ||
| throw new Error("Transfer token received without scope context"); | ||
| } | ||
| optional_data.type = AccessTokenType.GLOBUS_TRANSFER; | ||
| optional_data.other = collection_id + "|" + scope; | ||
| } | ||
| break; | ||
| } |
Collaborator
There was a problem hiding this comment.
I thought that GLOBUS_DEFAULT was always a TRANSFER token?
| const token_context = { | ||
| // passed values are mutable | ||
| resource_server: client_token.data.resource_sever, | ||
| resource_server: client_token.data.resource_server, |
Comment on lines
+831
to
+851
| {}, | ||
| (err) => { | ||
| if (err) { | ||
| logger.error("/api/usr/register", getCurrentLineNumber(), err); | ||
| a_resp.status(500).send("Registration failed during token set"); | ||
| return; | ||
| } | ||
| // Remove data not needed for active session | ||
| delete a_req.session.name; | ||
| delete a_req.session.email; | ||
| delete a_req.session.uuids; | ||
| delete a_req.session.acc_tok; | ||
| delete a_req.session.acc_tok_ttl; | ||
| delete a_req.session.ref_tok; | ||
| delete a_req.session.uuids; | ||
|
|
||
| // Set session as registered user | ||
| a_req.session.reg = true; | ||
|
|
||
| a_resp.send(reply); | ||
| }, |
Collaborator
There was a problem hiding this comment.
So what was the reason for moving this out of the finally block?
| scripts/admin_refresh_certs.sh | ||
| scripts/globus/__pycache__ | ||
| services/ | ||
| /services/ |
JoshuaSBrown
approved these changes
Jan 28, 2026
Collaborator
JoshuaSBrown
left a comment
JoshuaSBrown
left a comment
There was a problem hiding this comment.
I don't see any problems, this is a positive change. Nice work.
Collaborator
JoshuaSBrown
left a comment
JoshuaSBrown
left a comment
There was a problem hiding this comment.
How do we test this? So that we don't have a regression?
7 tasks
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Ticket
#1824
Description
Fixed the Mapped Collection Consent loop bug, and added single domain auth for endpoints that require it.
How Has This Been Tested?
Going through the consent flow, and then browsing the endpoint after being granted consent.
Summary by Sourcery
Improve Globus consent and single-domain authentication flow, ensuring mapped collection tokens and UI state are handled robustly through the consent/login process.
New Features:
Bug Fixes:
Enhancements: