Bug daps 1824 mapped collection consent flow #1831
Open
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.
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.
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: