Add /v2/user-from-token API and update a few v1/v2 APIs to require bearer token and check for auth user #4680
Conversation
…arer token and check for auth user Signed-off-by: Lukasz Gryglicki <[email protected]>
lukaszgryglicki
requested review from
mlehotskylf,
amolsontakke3576,
thakurveerendras and
jarias-lfx
|
""" WalkthroughAuthentication enforcement was added to signature request endpoints in the backend, requiring user identity verification. A new endpoint returns user info from a token. Several utility shell scripts were created or updated to require and handle authentication tokens and access control headers when interacting with the API, supporting local and production environments. Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant API
participant Auth
Client->>API: POST /v2/request-individual-signature (with JWT, X-ACL)
API->>Auth: Validate JWT
Auth-->>API: Authenticated user info
API->>API: Check user_id matches authenticated user
API-->>Client: Success or AuthError
Client->>API: GET /v2/user-from-token (with JWT, X-ACL)
API->>Auth: Validate JWT
Auth-->>API: Authenticated user info
API->>API: get_or_create_user(auth_user)
API-->>Client: User info JSON or error
Poem
✨ Finishing Touches
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (13)
cla-backend/cla/auth.py (2)
15-15: Line exceeds 100 characters
This commented line is 112 chars long, triggering Pylint's C0301. Wrap or shorten to comply with the 100-character limit.🧰 Tools
🪛 Pylint (3.3.7)
[convention] 15-15: Line too long (112/100)
(C0301)
101-102: Use logger.debug instead of prints
Replace the commentedcla.log.debug(...)calls, gated by the logging level, to integrate with the existing logging framework and avoid stdout pollution.utils/request_employee_signature_py_post.sh (4)
48-52: Validate and standardize secret file paths
The fallbackcat ./auth0.token.secretshould verify the file exists before reading and align the filename with the comment (auth0_token.secret). Consider:[[ -f ./auth0.token.secret ]] || { echo "auth0.token.secret not found"; exit 6; } TOKEN="$(< ./auth0.token.secret)"
60-64: Validate X-ACL secret file
Similarly, guardcat ./x-acl.secretwith a file existence check to avoid silent failures:[[ -f ./x-acl.secret ]] || { echo "x-acl.secret not found"; exit 7; } XACL="$(< ./x-acl.secret)"
1-3: Add strict mode for safer scripting
Introduceset -euo pipefailafter the shebang to catch errors and unset variables early:#!/bin/bash set -euo pipefail
76-80: Unify debug behaviour across scripts
In debug mode you only echo thecurlcommand but still pipe throughjqin the main execution. Consider moving the actualcurlexecution inside theif DEBUGblock (withoutjq) and using theelsebranch for the| jq -r '.'invocation, matching the pattern in other scripts.utils/request_individual_signature_py_post.sh (4)
45-49: Guard against missing token secret file
Beforecat ./auth0.token.secret, check that the file exists to prevent empty or missing tokens. Also synchronize the comment’s filename with the actual file. For example:[[ -f ./auth0.token.secret ]] || { echo "auth0.token.secret not found"; exit 5; } TOKEN="$(< ./auth0.token.secret)"
57-60: Guard against missing X-ACL secret file
Similarly, verify./x-acl.secretexists before reading, to avoid runtime errors:[[ -f ./x-acl.secret ]] || { echo "x-acl.secret not found"; exit 6; } XACL="$(< ./x-acl.secret)"
1-2: Enable strict shell settings
Addset -euo pipefailimmediately after the shebang to exit on errors and undefined variables:#!/bin/bash set -euo pipefail
68-72: Consistent debug command formatting
Align the debug echo and actualcurlinvocation with the employee script’s pattern: print and then run the command withoutjqunderif DEBUG, and use| jq -r '.'in theelsebranch. This standardizes debugging across utilities.cla-backend/cla/routes.py (3)
1214-1214: Good security enhancement: user identity verification added.The authentication enforcement properly ensures that only the authenticated user can request their own signature by:
- Requiring
auth_user: check_authparameter- Verifying the authenticated user's UUID matches the requested
user_id- Raising
AuthErrorwith 'permission denied' for mismatchesThis prevents unauthorized signature requests on behalf of other users.
Consider using f-string formatting consistently for the debug log message:
- cla.log.debug(f'request_individual_signature - auth user UUID {auth_user_id} is not the same as requested signature UUID {str(user_id)}') + cla.log.debug(f'request_individual_signature - auth user UUID {auth_user_id} is not the same as requested signature UUID {user_id}')Also applies to: 1237-1240
🧰 Tools
🪛 Pylint (3.3.7)
[convention] 1214-1214: Line too long (131/100)
(C0301)
1312-1312: Consistent authentication pattern applied to employee signatures.The same security verification is properly implemented here:
- Authentication requirement enforced
- User UUID validation against the authenticated user
- Consistent error handling and logging pattern
This maintains security consistency across all signature request endpoints.
Same f-string improvement suggestion as above:
- cla.log.debug(f'request_employee_signature - auth user UUID {auth_user_id} is not the same as requested signature UUID {str(user_id)}') + cla.log.debug(f'request_employee_signature - auth user UUID {auth_user_id} is not the same as requested signature UUID {user_id}')Also applies to: 1332-1335
1854-1883: New endpoint implementation is correct but has unused parameters.The
/v2/user-from-tokenendpoint correctly:
- Requires authentication via
auth_user: check_auth- Returns user data by calling
get_or_create_user(auth_user).to_dict()- Provides comprehensive documentation
However, the
requestandresponseparameters are unused in this simple endpoint.Remove unused parameters to clean up the function signature:
-def user_from_token(auth_user: check_auth, request, response): +def user_from_token(auth_user: check_auth):🧰 Tools
🪛 Pylint (3.3.7)
[warning] 1855-1855: Unused argument 'request'
(W0613)
[warning] 1855-1855: Unused argument 'response'
(W0613)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
cla-backend/cla/auth.py(2 hunks)cla-backend/cla/routes.py(5 hunks)utils/get_oauth_token.sh(1 hunks)utils/get_oauth_token_prod.sh(1 hunks)utils/get_user_from_token_py.sh(1 hunks)utils/request_corporate_signature_py_post.sh(2 hunks)utils/request_employee_signature_py_post.sh(2 hunks)utils/request_individual_signature_py_post.sh(2 hunks)
🧰 Additional context used
🪛 Pylint (3.3.7)
cla-backend/cla/auth.py
[convention] 15-15: Line too long (112/100)
(C0301)
cla-backend/cla/routes.py
[convention] 1214-1214: Line too long (131/100)
(C0301)
[convention] 1239-1239: Line too long (145/100)
(C0301)
[warning] 1239-1239: Use lazy % formatting in logging functions
(W1203)
[convention] 1334-1334: Line too long (143/100)
(C0301)
[warning] 1334-1334: Use lazy % formatting in logging functions
(W1203)
[warning] 1855-1855: Unused argument 'request'
(W0613)
[warning] 1855-1855: Unused argument 'response'
(W0613)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: build-test-lint
🔇 Additional comments (4)
utils/request_employee_signature_py_post.sh (1)
8-11: Approve usage comments update
The additions forTOKENandXACLin the usage section clearly document the new authentication requirements.utils/request_individual_signature_py_post.sh (1)
7-10: Approve usage documentation enhancements
The new comments forTOKENandXACLimprove clarity around required authentication variables.utils/get_user_from_token_py.sh (1)
1-48: LGTM! Well-structured utility script with proper error handling.The script follows good bash practices with:
- Clear documentation and usage examples
- Proper environment variable handling with file fallbacks
- Meaningful exit codes for different error conditions
- Secure variable quoting in the curl command
- Useful conditional debug output
The authentication pattern is consistent with the other utility scripts in this PR.
utils/request_corporate_signature_py_post.sh (1)
7-8: LGTM! Consistent authentication enforcement implementation.The authentication changes follow the established pattern:
- TOKEN and XACL environment variable handling with file fallbacks
- Sequential exit codes (5, 6) continuing the existing pattern
- Both debug and production curl commands properly updated with auth headers
- Clear documentation about the authentication requirements
This aligns well with the overall authentication enforcement across the utility scripts.
Also applies to: 13-13, 43-75
|
Hi @mlehotskylf - any update on this? is there any work on FE to support LFID based authentication so it can later call If so, can I see/review FE PR? |
…thenticated user is user on whose behalf we do action Signed-off-by: Lukasz Gryglicki <[email protected]>
There was a problem hiding this comment.
Actionable comments posted: 1
🔭 Outside diff range comments (1)
cla-backend/cla/routes.py (1)
167-191: Security check commented-out on whitelist request route
request_company_whitelist,invite_company_admin, andrequest_company_cclahave thecheck_user_id_is_currentcall commented out (L187, 219, 238).
This means a caller with a valid bearer token for any user can request whitelist/CCLA actions on another user’s behalf.Unless there is a business reason, re-enable the check or enforce an equivalent role/permission gate; otherwise the new auth hardening is inconsistent and introduces an escalation path.
🧹 Nitpick comments (3)
cla-backend/cla/routes.py (3)
40-47:check_user_id_is_current()– tighten wording & log accuracy
- The inline comment on L41 has typos (“tunr”, “chekc”) – minor but noisy in a public repo.
- The debug log (L45) hard-codes the string
request_individual_signature, so the same helper invoked from other routes will log an incorrect function name. Consider templating or removing the method reference.-# LG: comment this out to tunr off this chekc added after LFID is required everywhere in EasyCLA +# Set `CHECK_USER_ID` to False in env / settings if you temporarily need to disable this check. ... -cla.log.debug(f'request_individual_signature - auth user UUID {auth_user_id} is not the same as requested signature UUID {str(user_id)}') +cla.log.debug(f'permission check failed – auth user {auth_user_id} vs requested user {user_id}')
1246-1273:request_individual_signature– redundantrequestinjectionHug already allows you to access
hug.requestvia the global context if you need it. Passing the rawrequestobject through to the controller leaks framework details into lower layers and complicates unit testing.If only a few headers/body fields are needed, forward those primitives instead of the full object.
1886-1916:/v2/user-from-token– remove unused parameters
requestandresponseare accepted but never referenced. Excess params clutter the signature and can confuse Swagger/OpenAPI generation.-@hug.get("/user-from-token", versions=2) -def user_from_token(auth_user: check_auth, request, response): +@hug.get("/user-from-token", versions=2) +def user_from_token(auth_user: check_auth): ... return cla.controllers.user.get_or_create_user(auth_user).to_dict()
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
cla-backend/cla/routes.py(33 hunks)
🧰 Additional context used
🪛 Pylint (3.3.7)
cla-backend/cla/routes.py
[refactor] 809-809: Too many local variables (17/15)
(R0914)
[refactor] 809-809: Too many branches (14/12)
(R0912)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: build-test-lint
Assisted by [OpenAI](https://platform.openai.com/) Assisted by [GitHub Copilot](https://github.com/features/copilot)
Assisted by [OpenAI](https://platform.openai.com/) Assisted by [GitHub Copilot](https://github.com/features/copilot)
Assisted by [OpenAI](https://platform.openai.com/) Assisted by [GitHub Copilot](https://github.com/features/copilot)
Assisted by [OpenAI](https://platform.openai.com/) Assisted by [GitHub Copilot](https://github.com/features/copilot)
Assisted by [OpenAI](https://platform.openai.com/) Assisted by [GitHub Copilot](https://github.com/features/copilot)
Assisted by [OpenAI](https://platform.openai.com/) Assisted by [GitHub Copilot](https://github.com/features/copilot)
Assisted by [OpenAI](https://platform.openai.com/) Assisted by [GitHub Copilot](https://github.com/features/copilot)
Don’t merge this, even to
devyet - it requires FE support as explained above.Minimum changes needed from the
FE:bearer tokenwhen calling APIs:/v2/request-individual-signature,/v2/request-employee-signature.Bearer tokenuser must be the same user who is requesting employee/individual signature (ECLA/ICLA)./v2/user-from-tokenAPI that will return user data for authenticated user (bearer token).cc @mlehotskylf @thakurveerendras @amolsontakke3576 @jarias-lfx
Summary by CodeRabbit
Summary by CodeRabbit
New Features
Bug Fixes
Chores