[DLT-1110] Implement Consent Required Action (3/4 & 4/4) #1242
Conversation
AronPerez
changed the base branch from
devel
to
feat-DLT-1110-mapped-collection-endpoint-browse
AronPerez
force-pushed
the
feat-DLT-1110-mapped-collection-endpoint-browse-feat
branch
from
3adc821 to
72694a2
Compare
AronPerez
changed the base branch from
feat-DLT-1110-mapped-collection-endpoint-browse
to
devel
AronPerez
force-pushed
the
feat-DLT-1110-mapped-collection-endpoint-browse-feat
branch
5 times, most recently
from
84fdb01 to
eb861b6
Compare
AronPerez
force-pushed
the
feat-DLT-1110-mapped-collection-endpoint-browse-feat
branch
5 times, most recently
from
8979a66 to
01d680a
Compare
AronPerez
changed the title
AronPerez
force-pushed
the
feat-DLT-1110-mapped-collection-endpoint-browse-feat
branch
from
8bf61bc to
98ed92b
Compare
| function storeCollectionId(req, res, next) { | ||
| if (req.query.collection_id) { | ||
| req.session.collection_id = req.query.collection_id; | ||
| } | ||
| next(); | ||
| } | ||
|
|
There was a problem hiding this comment.
We're storing the collection_id in session since there's no real nice way to save it as a cookie in the FE
| @@ -621,6 +634,7 @@ app.get("/ui/authn", (a_req, a_resp) => { | |||
| } catch (err) { | |||
| redirect_path = "/ui/error"; | |||
| logger.error("/ui/authn", getCurrentLineNumber(), err); | |||
| delete a_req.session.collection_id; | |||
There was a problem hiding this comment.
If our auth fails delete the collection_id from session
| app.get("/api/globus/consent_url", storeCollectionId, (a_req, a_resp) => { | ||
| const { requested_scopes, state, refresh_tokens, query_params } = a_req.query; | ||
|
|
||
| const consent_url = generateConsentURL( | ||
| g_oauth_credentials.clientId, | ||
| g_oauth_credentials.redirectUri, | ||
| refresh_tokens, | ||
| requested_scopes, | ||
| query_params, | ||
| state, | ||
| ); | ||
|
|
||
| a_resp.json({ consent_url }); | ||
| }); | ||
|
|
There was a problem hiding this comment.
Pulled the business logic out, this will just handle the request
| @@ -39,7 +39,8 @@ | |||
| "chai": "^4", | |||
| "esm": "^3.2.25", | |||
| "mocha": "^10.8.2", | |||
| "pug": "^3.0.3" | |||
| "pug": "^3.0.3", | |||
| "sinon": "17.0.0" | |||
There was a problem hiding this comment.
Used for sandboxing tests with mocha/chai
| state, | ||
| ) => { | ||
| const scopes = requested_scopes | ||
| ? requested_scopes |
There was a problem hiding this comment.
Assuming it's always an array here.
There was a problem hiding this comment.
The return object from Globus has, in my experience, always held an array of scope strings. Here's a doc reference https://docs.globus.org/api/transfer/overview/#data_access_consent
| @@ -110,7 +112,7 @@ export default class OAuthTokenHandler { | |||
| switch ( | |||
| resource_server // TODO: exhaustive coverage of types | |||
| ) { | |||
| case "auth.globus.org": { | |||
| case AUTH_PREFIX: { | |||
There was a problem hiding this comment.
Figured we'd pull out this since it's commonly used
web/services/auth/constants.js
Outdated
There was a problem hiding this comment.
Not sure what the feelings are on a constants file
There was a problem hiding this comment.
I like it, almost went this route myself
| pathNavigator() { | ||
| return ` | ||
| <div class='endpoint-browser col-flex'> | ||
| <div class="path-navigator row-flex"> | ||
| <label class="path-label">Path:</label> | ||
| <div class="path-input-container"> | ||
| <input type="text" id="path" value="${this.state.path}"/> | ||
| </div> | ||
| <div> | ||
| <button id="up" class="btn small">Up</button> | ||
| </div> | ||
| </div> | ||
| </div> | ||
| `; | ||
| } | ||
|
|
||
| fileTree() { | ||
| return ` | ||
| <div class="endpoint-browser file-tree-view ui-widget content"> | ||
| <table id="file_tree"> | ||
| <colgroup> | ||
| <col/> | ||
| <col/> | ||
| <col/> | ||
| </colgroup> | ||
| <tbody> | ||
| <tr> | ||
| <td></td> | ||
| <td></td> | ||
| <td></td> | ||
| </tr> | ||
| </tbody> | ||
| </table> | ||
| </div> | ||
| `; | ||
| } |
There was a problem hiding this comment.
Styling is in CSS file
| { | ||
| title: CONFIG.PATH.UP, | ||
| icon: CONFIG.UI.ICONS.FOLDER, | ||
| key: CONFIG.PATH.UP, | ||
| is_dir: true, | ||
| }, |
There was a problem hiding this comment.
Gives is the .. in the endpoint view
| key: entry.name, | ||
| is_dir: false, | ||
| size: util.sizeToString(entry.size), | ||
| date: new Date(entry.last_modified.replace(" ", "T")).toLocaleString(), |
| handleApiError(data) { | ||
| if (data.code === "ConsentRequired") { | ||
| api.getGlobusConsentURL( | ||
| (ok, data) => { | ||
| const source = [ | ||
| { | ||
| title: `<span class='ui-state-error'>Consent Required: Please provide <a href="${data.consent_url}">consent</a>.</span>`, | ||
| icon: false, | ||
| is_dir: true, | ||
| }, | ||
| ]; | ||
|
|
||
| $.ui.fancytree.getTree("#file_tree").reload(source); | ||
| this.state.loading = false; | ||
| $("#file_tree").fancytree("enable"); | ||
| }, | ||
| this.props.endpoint.id, | ||
| data.required_scopes, | ||
| ); | ||
| } else { | ||
| return [ | ||
| { | ||
| title: `<span class='ui-state-error'>Error: ${data.message}</span>`, | ||
| icon: false, | ||
| is_dir: true, | ||
| }, | ||
| ]; | ||
| } | ||
| } |
There was a problem hiding this comment.
This can be used to handle whatever errors we get on ls command
| @@ -90,9 +90,8 @@ export class TransferEndpointManager { | |||
|
|
|||
| if (ok && !data.code) { | |||
| console.info("Direct endpoint match found:", data); | |||
| this.#controller.uiManager.updateEndpoint(data); | |||
| this.#controller.uiManager.state.endpointOk = true; | |||
There was a problem hiding this comment.
endpointOk is being removed since this was associated with activating an endpoint
| this.#controller.uiManager.updateEndpoint(data); | ||
| this.#controller.uiManager.state.endpointOk = true; | ||
| this.#controller.uiManager.updateButtonStates(); | ||
| this.#controller.uiManager.enableBrowseButton(true); |
There was a problem hiding this comment.
If we have a direct match, enable the ability to press the browse button and ls
| @@ -61,7 +59,6 @@ export class TransferUIManager { | |||
| this.initializeEndpointInput(); | |||
| this.initializeTransferOptions(); | |||
| this.initializeBrowseButton(); | |||
| this.updateButtonStates(); | |||
There was a problem hiding this comment.
Pulling this out since redundant due to initializeButtons
AronPerez
added
Type: Bug
| @@ -1543,6 +1557,21 @@ app.post("/api/cat/search", (a_req, a_resp) => { | |||
| }); | |||
| }); | |||
|
|
|||
| app.get("/api/globus/consent_url", storeCollectionId, (a_req, a_resp) => { | |||
| const { requested_scopes, state, refresh_tokens, query_params } = a_req.query; | |||
There was a problem hiding this comment.
Not for this PR - As a note, it would be nice to have some validators for the datafed-web API as well.
There was a problem hiding this comment.
@sourcery-ai Write up an issue that is a task to address the need for validation request middleware.
Suggest zod or valibot
There was a problem hiding this comment.
@sourcery-ai guide
There was a problem hiding this comment.
@sourcery-ai summary
|
@sourcery-ai !review |
|
Sure! I'm generating a new review now. |
Reviewer's Guide by SourceryThis PR implements the new consent flow for mapped collections and refactors various parts of the application. The changes include moving consent URL generation to a dedicated service, updating server endpoints to support the new flow, refactoring UI logic in the transfer modal and endpoint management, removing obsolete test code, and adding a new endpoint browser component with its own UI and styling. Sequence diagram for Consent Flow API InteractionsequenceDiagram
participant U as User
participant WC as Web Client
participant DF as DataFed Server
participant CH as ConsentHandler Service
participant GA as Globus Auth (External)
U->>WC: Initiate consent-required action
WC->>DF: GET /api/globus/consent_url (with parameters)
DF->>CH: Call generateConsentURL(clientId, redirectUri, refresh_tokens, scopes, query_params, state)
CH->>GA: Construct consent URL using AUTH_URL and credentials
GA-->>CH: Consent URL
CH-->>DF: Return consent URL
DF-->>WC: JSON { consent_url }
WC->>U: Display consent prompt with consent URL link
File-Level Changes
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}
There was a problem hiding this comment.
Hey @AronPerez - I've reviewed your changes - here's some feedback:
Overall Comments:
- Remove commented-out and dead code in TransferUIManager to reduce clutter.
- Pass query_params as an object (not as a JSON string) in getGlobusConsentURL to match generateConsentURL’s expected input.
Here's what I looked at during the review
- 🟡 General issues: 1 issue found
- 🟢 Security: all looks good
- 🟢 Testing: all looks good
- 🟢 Complexity: all looks good
- 🟢 Documentation: all looks good
Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
| // Initialize FancyTree with configuration | ||
| $("#file_tree", this.element).fancytree({ | ||
| checkbox: false, | ||
| extensions: ["themeroller", "table"], | ||
| nodata: false, | ||
| themeroller: { activeClass: "my-fancytree-active", hoverClass: "" }, | ||
| table: { nodeColumnIdx: 0 }, | ||
| source: [{ title: "loading...", icon: false, is_dir: true }], | ||
| selectMode: 1, | ||
| // Render additional columns (size and date) |
There was a problem hiding this comment.
suggestion: Scope jQuery selectors to the component instance.
Currently, selectors such as
Suggested implementation:
this.element.find("#up").on("click", () => this.navigate(CONFIG.PATH.UP)); this.element.find("#path").on("input", () => {
clearTimeout(this.state.timer);
this.state.timer = setTimeout(() => {
this.state.path = this.element.find("#path").val();
this.loadTree();
}, CONFIG.UI.DELAY);
}); // Initialize FancyTree with configuration
this.element.find("#file_tree").fancytree({If there are other selectors (e.g., for '#sel_btn') elsewhere in this file or in related components, change them similarly to use this.element.find(...).
There was a problem hiding this comment.
It makes sense, but using $(..., ...) notation involves looking at the first parameter, the selector, the 2nd being the element/frame. We've scoped out request down to the element, as far as I understand
There was a problem hiding this comment.
You're right that the second argument to the jQuery function $(selector, context) limits the scope of the search. However, the context you're providing is likely the document or some other high-level element. While this is better than a completely global search, it's still broader than necessary and can lead to unexpected behavior if the same IDs are used within other parts of the application, even if those parts aren't currently visible.
My suggestion to use this.element.find(...) is more specific. It ensures that the selector only looks within the current component's DOM element. This creates a stronger isolation and prevents conflicts even if other parts of the application use the same IDs. It's a best practice for encapsulating components and improving maintainability.
Think of it this way: You've built a house (your component) and you're looking for a specific light switch ("#file_tree"). Using just $("#file_tree") is like searching the entire neighborhood for that light switch. Using $(..., document) is like searching your street. But this.element.find("#file_tree") is like searching only inside your house. It's much more efficient and guarantees you'll find the correct switch, even if your neighbor happens to have a switch with the same name.
Could you try replacing the selectors with this.element.find(...) and see if it works as expected? If you encounter any issues, please let me know and we can troubleshoot together.
| const scopes = requested_scopes | ||
| ? requested_scopes | ||
| : ["openid", "profile", "email", "urn:globus:auth:scope:transfer.api.globus.org:all"]; |
There was a problem hiding this comment.
suggestion (code-quality): Avoid unneeded ternary statements (simplify-ternary)
| const scopes = requested_scopes | |
| ? requested_scopes | |
| : ["openid", "profile", "email", "urn:globus:auth:scope:transfer.api.globus.org:all"]; | |
| const scopes = requested_scopes || ["openid", "profile", "email", "urn:globus:auth:scope:transfer.api.globus.org:all"]; | |
Explanation
It is possible to simplify certain ternary statements into either use of an|| or !.This makes the code easier to read, since there is no conditional logic.
There was a problem hiding this comment.
I like this, I’ll implement it later
| let updatedPath; | ||
| if (newPath === CONFIG.PATH.UP) { | ||
| // Handle "up" navigation | ||
| if (current.length === 1) return; // Already at root |
There was a problem hiding this comment.
suggestion (code-quality): Use block braces for ifs, whiles, etc. (use-braces)
| if (current.length === 1) return; // Already at root | |
| if (current.length === 1) { |
Explanation
It is recommended to always use braces and create explicit statement blocks.Using the allowed syntax to just write a single statement can lead to very confusing
situations, especially where subsequently a developer might add another statement
while forgetting to add the braces (meaning that this wouldn't be included in the condition).
| * Load tree data from API | ||
| */ | ||
| loadTree() { | ||
| if (this.state.loading) return; |
There was a problem hiding this comment.
suggestion (code-quality): Use block braces for ifs, whiles, etc. (use-braces)
| if (this.state.loading) return; | |
| if (this.state.loading) { |
Explanation
It is recommended to always use braces and create explicit statement blocks.Using the allowed syntax to just write a single statement can lead to very confusing
situations, especially where subsequently a developer might add another statement
while forgetting to add the braces (meaning that this wouldn't be included in the condition).
| */ | ||
| handleSelect() { | ||
| const node = $.ui.fancytree.getTree("#file_tree").activeNode; | ||
| if (!node || !this.props.onSelect) return; |
There was a problem hiding this comment.
suggestion (code-quality): Use block braces for ifs, whiles, etc. (use-braces)
| if (!node || !this.props.onSelect) return; | |
| if (!node || !this.props.onSelect) { |
Explanation
It is recommended to always use braces and create explicit statement blocks.Using the allowed syntax to just write a single statement can lead to very confusing
situations, especially where subsequently a developer might add another statement
while forgetting to add the braces (meaning that this wouldn't be included in the condition).
|
Hey @AronPerez, I've posted a new review for you! |
| @@ -0,0 +1,60 @@ | |||
| import { AUTH_URL } from "./constants.js"; | |||
There was a problem hiding this comment.
I like this usage a lot. We should extract more constants later.
|
@sourcery-ai summary of the usage of middleware validation in web/DataFed-ws.js |
|
@sourcery-ai guide |
AronPerez
force-pushed
the
feat-DLT-1110-mapped-collection-endpoint-browse-feat
branch
from
98ed92b to
d129616
Compare
PR Description
This PR:
utils.jstoservices/auth//api/globus/consent_urlAddresses
How Was This Tested
Manually
compose/all./generate_env.sh./generate_globus_files.sh.env./build_images_for_compose.shdocker compose upip/hoststep 4<name> Setup Clienthttpsredirect uri toRedirectsUSE_CASE - Transfer Data - Download Data Record
USE_CASE - Transfer Data - New Data Record
USE_CASE - Transfer Data - Update Data Record
Mocha/Chai
cd webcp package.json.in package.jsonpackage.jsonnpm installnpm run testArtifacts (if appropripate)
Create Data Record from Guest Collection
Screen.Recording.2025-02-07.at.2.16.32.PM.mov
Upload to Data Record with existing data
Screen.Recording.2025-02-07.at.1.56.43.PM.mov
Create Data Record on Mapped Collection Requiring Consent
Screen.Recording.2025-02-07.at.2.21.59.PM.mov
Download (single data record) to Mapped Collection
Screen.Recording.2025-02-07.at.1.57.48.PM.mov
Download (multiple files) to Mapped Collection
Screen.Recording.2025-02-07.at.2.25.01.PM.mov
web/TestingTasks
Summary by Sourcery
Implement consent flow for mapped collections.
New Features:
Tests: