Feature DAPS 1215 foxx UserGetAccessTokenRequest mapped collection support #1284
Conversation
…UserAccessTokenReply to support finding correct tokens
…& DatabaseAPI.hpp Use updated names and types. ClientWorker.cpp Update comments.
…oken match and missing match cases
…ir/list to test out changes.
…es on covered cases; add notes on code
…lientWorker.cpp Add refresh token try/catch to send through flow if refresh fails
…irect to error when needs_consent
…duced for handling logic for validating params when getting tokens and building GetAccessToken responses. user_router.js Replace logic with calls to new lib
…n. user_router.js Remove unnecessary comment.
…ken document required fields. user_token.test.js Add preliminary unit tests for static methods.
… naming to match convention.
… Add simple tests for get/token endpoint. user_token.js Fix naming for token document scopes field. user_token.test.js Changes for scope field bugfix.
…, add note about token type.
…API.cpp Remove TODOs. SDMS_Auth.proto Remove unused field.
…DatabaseAPI.cpp Add note on possibly missing field. user_token.test.js Update tests to check for new expected values.
t-ramz
added
Component: Database
Reviewer's Guide by SourceryThis pull request adds support for mapped collections when fetching user tokens using the Sequence diagram for user token retrieval with collection supportsequenceDiagram
participant Client
participant Router
participant UserToken
participant Database
participant GlobusAPI
Client->>Router: GET /token/get
Note over Router: Check collection params
Router->>UserToken: validateRequestParams()
UserToken-->>Router: collection_token status
alt has collection token
Router->>Database: Query token edges
Database-->>Router: token matches
alt token needs refresh
Router->>GlobusAPI: refreshAccessToken()
GlobusAPI-->>Router: new token
Router->>Database: userSetAccessToken()
end
end
Router->>UserToken: formatUserToken()
UserToken-->>Router: formatted response
Router-->>Client: token response
Entity relationship diagram for user tokens and collectionserDiagram
User ||--o{ GlobusToken : has
GlobusToken ||--o| Collection : associated_with
User {
string _id
string access
string refresh
number expiration
}
GlobusToken {
string access
string refresh
number expiration
number type
string dependent_scopes
}
Collection {
string _id
string collection_type
}
Class diagram for UserToken implementationclassDiagram
class UserToken {
+validateRequestParams(query_params: object) boolean
+formatUserToken(is_collection_token: boolean, token_document: object, needs_consent: boolean) userTokenResponse
}
class userTokenResponse {
+needs_consent: boolean
+access: string
+refresh: string
+expires_in: number
+token_type: AccessTokenType
+scopes: string
}
class AccessTokenType {
<<enumeration>>
GLOBUS_AUTH
GLOBUS_TRANSFER
GLOBUS_DEFAULT
ACCESS_SENTINEL
}
UserToken ..> userTokenResponse : creates
UserToken ..> AccessTokenType : uses
File-Level Changes
Assessment against 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}
There was a problem hiding this comment.
Hey @t-ramz - I've reviewed your changes - here's some feedback:
Overall Comments:
- Consider adding error logging in ClientWorker.cpp when token refresh fails, to help with debugging consent flow issues.
Here's what I looked at during the review
- 🟡 General issues: 1 issue found
- 🟢 Security: all looks good
- 🟡 Testing: 1 issue found
- 🟡 Complexity: 1 issue found
- 🟢 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.
| const std::string collection_id, | ||
| const std::string collection_type, | ||
| bool &needs_consent, | ||
| int &token_type, // TODO: use underlying type? |
There was a problem hiding this comment.
suggestion: Use a proper enum type for token_type instead of int for better type safety
Suggested implementation:
bool &needs_consent,
enum class TokenType {
Bearer,
Basic,
Digest,
OAuth
} &token_type,
std::string &scopes, LogContext log_context);
This change will require:
- Updating all calls to userGetAccessToken() to use the new TokenType enum
- Updating the implementation of userGetAccessToken() to work with the enum
- If the token_type values need to match specific integer values (e.g., for protocol compatibility), you can explicitly set them in the enum like:
enum class TokenType { Bearer = 1, Basic = 2, // etc. };
There was a problem hiding this comment.
I had trouble converting from the numeric type I get as a return from JSON deserialization to the AccessTokenType that is represented here as an int. I chose to go the path of simply representing this as an int for now, which is the direct type that AccessTokenType (which is unfortunately not an enum class) compiles to in order to make the single comparison I absolutely need.
| @@ -697,6 +698,10 @@ router | |||
| router | |||
| .get("/token/get", function (req, res) { | |||
There was a problem hiding this comment.
issue (complexity): Consider using a direct AQL query to simplify edge filtering logic and improve performance.
The edge filtering logic can be simplified by replacing the manual filtering with a direct AQL query. This would reduce nesting and improve readability while maintaining the same functionality:
let token_document = user;
let needs_consent = false;
if (collection_token) {
// Direct query instead of filtering edges
const query = aql`
FOR token IN globus_token
FILTER token._from == ${user._id}
AND token._to == ${globus_collection._id}
LIMIT 1
RETURN token
`;
const token_matches = g_db._query(query).toArray();
if (token_matches.length === 1) {
token_document = token_matches[0];
needs_consent = false;
} else {
token_document = {};
needs_consent = true;
}
}This approach:
- Eliminates edge filtering and multiple nested conditions
- Performs filtering at the database level for better performance
- Maintains the same functionality with clearer intent
- Reduces cognitive complexity by flattening the logic
…5-foxx-UserGetAccessRequest-mapped-collection
| let needs_consent = false; | ||
| if (collection_token) { | ||
| const globus_collection = g_db.globus_coll.exists({ _key: collection_id }); | ||
| // TODO: should this be a query? |
There was a problem hiding this comment.
Another alternative is to use
const token_matches = g_db.globus_token.byExample({
_from: user._id
_to: globus_collection?._id,
}).toArray();The change would require a pre-check of globus_collection's existence, otherwise Arango will throw an error.
But this may be both more readable and more performant.
| let needs_consent = false; | ||
| if (collection_token) { | ||
| const globus_collection = g_db.globus_coll.exists({ _key: collection_id }); | ||
| // TODO: should this be a query? |
There was a problem hiding this comment.
Another possibility is the query:
const token_matches = g_db._query(
"FOR token_match " +
"IN globus_token " +
"FILTER token_match._from == @user_id " +
" AND token_match._to == @collection_id " +
"RETURN token_match",
{
user_id: user._id,
collection_id: globus_collection._id,
}
).toArray();This would require no other changes to logic. I'm not sure how much more performant or readable it is, however.
There was a problem hiding this comment.
I’m a fan of using built-in methods to accomplish tasks. Raw queries nowadays are only necessary if we can’t accomplish what we need natively with our dependencies or it’s blocking other queries (some kind of table lock etc)
There was a problem hiding this comment.
Agreed. Do you think that the byExample method or outEdges method and then filter on the resulting array is more readable?
There was a problem hiding this comment.
I like byExample. Seems straightforward for what we need
const user = g_db.u.byExample({ _id: req.queryParams.subject }).toArray();
``There was a problem hiding this comment.
I also like that, and I found other uses of byExample in the Foxx API, so I think it fits in better as well. Will make that change 🫡
There was a problem hiding this comment.
This has been implemented, along with an additional test to look for the missing collection case!
There was a problem hiding this comment.
On further thought, should this return a needs_consent message when the collection does not exist?
There was a problem hiding this comment.
Yeah, that was the old behavior. Brought that back.
| } else { | ||
| try { | ||
| m_globus_api.refreshAccessToken(ref_tok, acc_tok, expires_in); | ||
| m_db_client.userSetAccessToken( |
There was a problem hiding this comment.
This attempts to cover condition 2, "If token is in database for mapped collection, check expiration, if invalid use refresh token to create new access token, save in database, return needs_consent bool false"
| if (expires_in < 300) { | ||
| if (needs_consent) { | ||
| // short circuit to reply | ||
| } else if (expires_in < 300) { |
There was a problem hiding this comment.
This attempts to cover condition 1, "If token is in database for mapped collection, check expiration if valid return token, needs_consent bool is false"
| } catch (TraceException &e) { // NOTE: assumes refresh failed (invalid or | ||
| // failure). new token fetched will upsert | ||
| // and overwrite old values on database | ||
| needs_consent = true; |
There was a problem hiding this comment.
This attempts to cover condition 3, "If token is in database for mapped collection, check expiration, if invalid refresh token fails, return needs_consent true"
| } else { | ||
| // Cases covered: no matches, no tokens, no collection - all require same consent flow | ||
| token_document = {}; | ||
| needs_consent = true; |
There was a problem hiding this comment.
This attempts to cover condition 4, "If token is not in database, return needs_consent: true"
| } | ||
| // TODO: account for AccessTokenType; currently only GLOBUS_DEFAULT and GLOBUS_TRANSFER are supported | ||
| token_document = token_matches[0]; | ||
| needs_consent = false; |
There was a problem hiding this comment.
This attempts to cover condition 1, "If token is in database for mapped collection, check expiration if valid return token, needs_consent bool is false"
| optional string collection_id = 1; | ||
| optional string collection_type = 2; // TODO: use enum |
There was a problem hiding this comment.
Yes, do you have that in an issue?
GUEST
MAPPED
PERSONAL
We might need to explore what the options should be for this.
There was a problem hiding this comment.
Not in this pr though.
| result_token.access = token_document.access; | ||
| result_token.refresh = token_document.refresh; | ||
| if (token_document.expiration) { | ||
| const expires_in = token_document.expiration - Math.floor(Date.now() / 1000); |
There was a problem hiding this comment.
What does Math.floor(Date.now() / 1000) corrospond to
There was a problem hiding this comment.
This was a copy/paste from https://github.com/ORNL/DataFed/blob/devel/core/database/foxx/api/user_router.js#L720
This is normalizing the data stored on the database from the expiry date/time to a time until expiry.
| let needs_consent = false; | ||
| if (collection_token) { | ||
| const globus_collection = g_db.globus_coll.exists({ _key: collection_id }); | ||
| // TODO: should this be a query? |
There was a problem hiding this comment.
I like byExample. Seems straightforward for what we need
const user = g_db.u.byExample({ _id: req.queryParams.subject }).toArray();
``
common/proto/common/SDMS_Auth.proto
Outdated
| required uint32 expires_in = 2; // Access token expiration in seconds | ||
| required string access = 1; // Globus access token | ||
| required uint32 expires_in = 2; // Access token expiration in seconds | ||
| required bool needs_consent = 3; // Indicate requirement of consent flow |
There was a problem hiding this comment.
If you make this required you will break the API, can we get away with keeping this optional? And assume if it was not specified then it does not need consent.
When I say break the API, I mean that it won't be backwards compatible with how it is implemented currently.
There was a problem hiding this comment.
In proto3 they have actually moved to using optional by default and avoided required for this reason.
There was a problem hiding this comment.
Good point. With that in mind, all code that can point to a mapped collection will need to validate both the field's existence and the field's value
There was a problem hiding this comment.
I made it optional, do you have an idea on how we can enforce some diligence on checks?
There was a problem hiding this comment.
When the message comes into the core service, whether from the web server or the python client, it will go through the DatabaseAPI class. That would be a good place.
| @@ -22,6 +22,7 @@ if( ENABLE_FOXX_TESTS ) | |||
| add_test(NAME foxx_support COMMAND "${CMAKE_CURRENT_SOURCE_DIR}/tests/test_foxx.sh" -t "unit_support") | |||
| add_test(NAME foxx_user_router COMMAND "${CMAKE_CURRENT_SOURCE_DIR}/tests/test_foxx.sh" -t "unit_user_router") | |||
| add_test(NAME foxx_authz_router COMMAND "${CMAKE_CURRENT_SOURCE_DIR}/tests/test_foxx.sh" -t "unit_authz_router") | |||
| add_test(NAME foxx_unit_user_token COMMAND "${CMAKE_CURRENT_SOURCE_DIR}/tests/test_foxx.sh" -t "unit_user_token") | |||
…ng Globus collection. Add globus_coll fixture.
… collection tokens are present. user_router.test.js Update test from error to needs_consent.
Closes #1215
PR Description
Add support for mapped collections when using
UserGetAccessTokenRequestmessage to fetch user tokens.Additionally, stub support for HA collections via comments and query parameters.
Tasks
Summary by Sourcery
Add support for mapped collections to the UserGetAccessTokenRequest message.
New Features:
Tests: