Unicron v3 apis test coverage dev 2 #4846
Conversation
…arer token and check for auth user Signed-off-by: Lukasz Gryglicki <[email protected]>
…thenticated user is user on whose behalf we do action Signed-off-by: Lukasz Gryglicki <[email protected]>
Signed-off-by: Łukasz Gryglicki <[email protected]>
Signed-off-by: Łukasz Gryglicki <[email protected]>
Signed-off-by: Lukasz Gryglicki <[email protected]>
Signed-off-by: Lukasz Gryglicki <[email protected]> Assisted by [OpenAI](https://platform.openai.com/) Assisted by [GitHub Copilot](https://github.com/features/copilot)
|
Note Other AI code review bot(s) detectedCodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review. WalkthroughLarge-scale authentication enhancement and V3 API testing infrastructure. Adds auth_user parameter to route handlers with identity verification checks in routes.py. Introduces V3 E2E test suites for multiple API endpoints. Updates utility scripts for multi-region AWS parameter management and adds authentication headers to request-signing scripts. Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant Route as Route Handler
participant Auth as auth_user
participant Check as check_user_id_is_current()
participant Controller
Client->>Route: Request (user_id)
Route->>Auth: Extract auth_user
Route->>Check: check_user_id_is_current(auth_user, user_id)
alt User ID matches
Check-->>Route: OK
Route->>Controller: Proceed with user-specific operation
Controller-->>Route: Result
Route-->>Client: 200 Response
else User ID mismatch
Check-->>Route: Permission Denied Error
Route-->>Client: 403/401 Response
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes
Possibly related PRs
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro Disabled knowledge base sources:
📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
Comment |
Signed-off-by: Lukasz Gryglicki <[email protected]> Assisted by [OpenAI](https://platform.openai.com/) Assisted by [GitHub Copilot](https://github.com/features/copilot)
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 23
🧹 Nitpick comments (19)
utils/request_corporate_signature_py_post.sh (1)
7-8: Document authentication header requirements clearly.The script now requires TOKEN (Auth0 JWT) and XACL headers for authentication. The comments on lines 7-8 are helpful. Consider adding a brief note about the secret files (
auth0.token.secretandx-acl.secret) that will be loaded if environment variables are not set, to make it clearer for users running this locally.Also applies to: 43-65
cla-backend/cla/routes.py (1)
43-44: Consider caching user lookup to avoid repeated database calls.The helper calls
get_or_create_user(auth_user)on every invocation, which may result in repeated database queries for the same authenticated user across multiple authorization checks in a single request path.Consider one of these approaches:
- Cache the result in the request context
- Pass the resolved user_id directly if
auth_useralready contains it- Ensure
get_or_create_userhas efficient caching internallyExample with request context:
def check_user_id_is_current(auth_user, user_id, request=None): # Try to get cached user_id from request context first if request and hasattr(request.context, 'auth_user_id'): auth_user_id = request.context['auth_user_id'] else: auth_user_id = cla.controllers.user.get_or_create_user(auth_user).get_user_id() if request: request.context['auth_user_id'] = auth_user_id if str(user_id) != auth_user_id: cla.log.debug(f'check_user_id_is_current - auth user UUID {auth_user_id} is not the same as requested user UUID {str(user_id)}') raise cla.auth.AuthError('permission denied')tests/functional/utils/run-single-test.sh (1)
39-48: Avoid eval; build the --spec list robustly.
Current loop + eval is fragile for paths and quoting. Join via paste and drop eval.Apply this diff:
- CMD="xvfb-run -a npx cypress run --spec " - for file in cypress/e2e/v${V}/*.cy.ts - do - if [ "${CMD: -1}" = " " ] - then - CMD="${CMD}${file}" - continue - fi - CMD="${CMD},${file}" - done + # Build comma-separated spec list safely + SPEC_LIST="$(printf '%s\n' cypress/e2e/v${V}/*.cy.ts | paste -sd, -)" + CMD="xvfb-run -a npx cypress run --spec ${SPEC_LIST}" @@ -eval "${CMD}" +"${CMD}"Optional hardening:
- Add
set -euo pipefailafter the shebang.- If you prefer arrays, use:
CMD=(xvfb-run -a npx cypress run --spec "${SPEC_LIST}")and run"${CMD[@]}".Also applies to: 88-88
tests/functional/cypress/e2e/v3/version.cy.ts (1)
15-23: Type safety for bearerToken initialization.
Avoid assigning null to a string in TS; also no need to pass the null to getTokenKey.- let bearerToken: string = null; + let bearerToken: string | null = null; before(() => { if (bearerToken == null) { - getTokenKey(bearerToken); + getTokenKey(bearerToken); cy.window().then((win) => { bearerToken = win.localStorage.getItem('bearerToken'); }); } });Optional: coerce
localto boolean for downstream helpers:
const local = String(Cypress.env('LOCAL')) === '1';tests/functional/cypress/e2e/v3/health.cy.ts (1)
15-23: Token variable type.
Align with strict TS: use nullable type on init.- let bearerToken: string = null; + let bearerToken: string | null = null;tests/functional/cypress/e2e/v3/docs.cy.ts (1)
15-23: Nullable bearerToken.- let bearerToken: string = null; + let bearerToken: string | null = null;tests/functional/cypress/e2e/v3/github-organizations.cy.ts (2)
15-16: Clarify ALLOW_FAIL semantics and usageName suggests “allow failing” but you assign it to failOnStatusCode. Invert or rename to avoid confusion.
Example:
- let allowFail: boolean = !(Cypress.env('ALLOW_FAIL') === 1); + const failOnStatusCode = + !['1', 1, true, 'true'].includes(Cypress.env('ALLOW_FAIL'));Then use failOnStatusCode where needed.
58-87: Optional: simplify flakiness loopUse Cypress._.times or extract a small helper to reduce repetition across files.
Example:
- cy.wrap([1, 2, 3]).each((iteration) => { + Cypress._.times(3, (i) => { + const iteration = i + 1; cy.task('log', `GitHub Organizations test iteration ${iteration}/3`); return cy .request({ ... }) .then((response) => { ... }); - }); + });tests/functional/cypress/e2e/v3/users.cy.ts (3)
14-16: Clarify ALLOW_FAIL semantics and usageInvert/rename to match Cypress’ failOnStatusCode.
- let allowFail: boolean = !(Cypress.env('ALLOW_FAIL') === 1); + const failOnStatusCode = + !['1', 1, true, 'true'].includes(Cypress.env('ALLOW_FAIL'));
28-63: Make remote expectation tolerant (avoid brittleness when config is fixed)Remote branch hard-codes 500 “username not found”. Accept 200 or 500 so tests pass once AUTH0_USERNAME_CLAIM is corrected.
- } else { - // Remote server - expect 500 due to misconfigured AUTH0_USERNAME_CLAIM - cy.task('log', 'V3 remote server: username claim issue - this is a known deployment configuration issue'); - expect(response.status).to.equal(500); - expect(response.body).to.have.property('code', 500); - expect(response.body.message).to.include('username not found'); - } + } else { + // Remote: tolerate known misconfig (500) but allow success if fixed + if (response.status === 500 && response.body?.message?.includes('username not found')) { + cy.task('log', 'V3 remote: AUTH0_USERNAME_CLAIM misconfigured'); + expect(response.status).to.equal(500); + expect(response.body).to.have.property('code', 500); + expect(response.body.message).to.include('username not found'); + } else { + validate_200_Status(response); + expect(response.body).to.be.an('object'); + } + }
70-78: Use the corrected failOnStatusCode flagThe current allowFail name/logic is inverted. Use the renamed flag.
- failOnStatusCode: allowFail, // Use allowFail like V4 - expect success + failOnStatusCode: failOnStatusCode, // expect successtests/functional/cypress/e2e/v3/template.cy.ts (2)
47-59: Also drop Bearer from the flakiness run for the public endpointSame rationale: exercise unauth path.
- headers: getXACLHeader(), - auth: { bearer: bearerToken }, + headers: getXACLHeader(),
14-16: Clarify ALLOW_FAIL semanticsRename/invert to reflect failOnStatusCode.
- let allowFail: boolean = !(Cypress.env('ALLOW_FAIL') === 1); + const failOnStatusCode = + !['1', 1, true, 'true'].includes(Cypress.env('ALLOW_FAIL'));tests/functional/cypress/e2e/v3/company.cy.ts (3)
81-89: Public endpoint includes Bearer token — remove to validate unauth accessKeep the request unauthenticated to verify “no auth required”.
- headers: getXACLHeader(), - auth: { bearer: bearerToken }, + headers: getXACLHeader(),
154-164: Flakiness run against public search shouldn’t send BearerSame rationale; exercise unauth path.
- headers: getXACLHeader(), - auth: { bearer: bearerToken }, + headers: getXACLHeader(),
15-16: Clarify ALLOW_FAIL semanticsRename/invert to reflect failOnStatusCode.
- let allowFail: boolean = !(Cypress.env('ALLOW_FAIL') === 1); + const failOnStatusCode = + !['1', 1, true, 'true'].includes(Cypress.env('ALLOW_FAIL'));tests/functional/cypress/e2e/v3/signatures.cy.ts (3)
34-42: Public endpoint includes Bearer token — removeEnsure the unauth path is actually exercised.
- headers: getXACLHeader(), - auth: { bearer: bearerToken }, + headers: getXACLHeader(),
128-138: Flakiness run against public signatures endpoint shouldn’t send BearerSame rationale.
- headers: getXACLHeader(), - auth: { bearer: bearerToken }, + headers: getXACLHeader(),
15-16: Clarify ALLOW_FAIL semanticsRename/invert to reflect failOnStatusCode.
- let allowFail: boolean = !(Cypress.env('ALLOW_FAIL') === 1); + const failOnStatusCode = + !['1', 1, true, 'true'].includes(Cypress.env('ALLOW_FAIL'));
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Jira integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (25)
cla-backend-go/users/handlers.go(1 hunks)cla-backend/cla/routes.py(32 hunks)tests/functional/cypress/e2e/v3/cla-manager.cy.ts(1 hunks)tests/functional/cypress/e2e/v3/company.cy.ts(1 hunks)tests/functional/cypress/e2e/v3/docs.cy.ts(1 hunks)tests/functional/cypress/e2e/v3/events.cy.ts(1 hunks)tests/functional/cypress/e2e/v3/gerrits.cy.ts(1 hunks)tests/functional/cypress/e2e/v3/github-organizations.cy.ts(1 hunks)tests/functional/cypress/e2e/v3/github-repositories.cy.ts(1 hunks)tests/functional/cypress/e2e/v3/github.cy.ts(1 hunks)tests/functional/cypress/e2e/v3/health.cy.ts(1 hunks)tests/functional/cypress/e2e/v3/organization.cy.ts(1 hunks)tests/functional/cypress/e2e/v3/project.cy.ts(1 hunks)tests/functional/cypress/e2e/v3/signatures.cy.ts(1 hunks)tests/functional/cypress/e2e/v3/template.cy.ts(1 hunks)tests/functional/cypress/e2e/v3/users.cy.ts(1 hunks)tests/functional/cypress/e2e/v3/version.cy.ts(1 hunks)tests/functional/utils/run-single-test.sh(2 hunks)utils/get_dev_claims.sh(1 hunks)utils/get_prod_claims.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)utils/restore_dev_claims.sh(1 hunks)utils/set_dev_claims.sh(1 hunks)
🔇 Additional comments (15)
utils/set_dev_claims.sh (1)
6-8: Regional expansion to us-east-2 looks correct.The new us-east-2 commands properly mirror the us-east-1 pattern with consistent parameter names and values.
utils/restore_dev_claims.sh (1)
7-10: Regional expansion to us-east-2 looks correct.The new us-east-2 operations properly mirror the us-east-1 pattern with consistent parameter names and operation types (put and delete).
utils/get_dev_claims.sh (1)
6-9: Regional expansion to us-east-2 looks correct.The new us-east-2 commands properly mirror the us-east-1 pattern with consistent parameter names, query filters, and output formatting.
utils/request_employee_signature_py_post.sh (2)
76-80: Consistent authentication header pattern approved.The authentication handling and curl invocation follow the same secure pattern as the corporate signature script, with proper error handling and distinct exit codes (though they should be standardized across all three scripts).
48-70: Standardize exit codes with other signature scripts.This script uses exit codes 6 and 7 for missing TOKEN and XACL respectively, while
request_corporate_signature_py_post.shuses 5 and 6. Align these across all three signature request scripts for consistency.utils/request_individual_signature_py_post.sh (2)
68-74: Standardize curl execution pattern across signature scripts.This script has curl execution inside the DEBUG/else conditional blocks (lines 71 and 73), while
request_corporate_signature_py_post.shandrequest_employee_signature_py_post.shhave curl execute unconditionally outside the DEBUG block. Although the functional behavior is similar (curl always runs), the code structure differs inconsistently across the three scripts.Consider restructuring to match the pattern in the other two scripts: place curl outside the DEBUG block and only echo the command in DEBUG mode.
45-67: Exit code inconsistency persists across scripts.This script uses exit codes 5 and 6 for missing TOKEN and XACL (matching corporate signature), while employee signature uses 6 and 7. Standardize exit codes across all three scripts.
utils/request_corporate_signature_py_post.sh (1)
43-65: Exit code inconsistency across signature scripts.This script uses exit code 5 for missing TOKEN and exit code 6 for missing XACL. However,
request_employee_signature_py_post.shuses exit code 6 for missing TOKEN. Consider standardizing exit codes across all three signature request scripts for consistency and easier error handling.cla-backend/cla/routes.py (2)
190-190: Inconsistent authorization enforcement creates security gaps.The
check_user_id_is_currentverification is commented out in several user-specific endpoints (request_company_allowlist,invite_company_admin,request_company_ccla), while it's enforced in similar routes elsewhere. This creates an inconsistent security posture where some user-specific operations are protected and others are not.Please clarify the authorization requirements for these endpoints. If these routes should allow users to perform actions on behalf of other users, document why. Otherwise, uncomment the checks:
- # check_user_id_is_current(auth_user, user_id) + check_user_id_is_current(auth_user, user_id)Run this script to identify all user-specific routes and their authorization status:
Also applies to: 222-222, 241-241
1432-1432: Verify authentication requirements for DocuSign callback endpoints.The
check_user_id_is_currentcalls are commented out inpost_individual_signed_gitlab(line 1432) andpost_individual_signed_gerrit(line 1446). These appear to be DocuSign callback endpoints that may not have access toauth_user, but leaving them unprotected could allow unauthorized signature submissions.Please verify:
- Do these callback endpoints receive authentication tokens?
- If not, what other security measures protect them (e.g., DocuSign webhook signatures, IP allowlists)?
The PR description mentions "potential issues with security handling (authentication) for some cases" — these might be the concerning endpoints.
Also applies to: 1446-1446
tests/functional/utils/run-single-test.sh (1)
24-25: Usage examples update looks good.
Covers v3/v4 clearly. No issues.tests/functional/cypress/e2e/v3/events.cy.ts (2)
1-10: Helper name consistency: getXACLHeader vs getXACLHeaders.
Ensure a single exported helper name and update imports across files.Run to verify the exported name(s):
35-40: Remove duplicateauthkeys and standardize Bearer auth via headers.
Duplicate keys are confusing; prefer explicit Authorization header (you already use this pattern below).- headers: getXACLHeader(), - auth: { bearer: bearerToken }, - auth: { - bearer: bearerToken, - }, + headers: { ...getXACLHeader(), Authorization: `Bearer ${bearerToken}` }, @@ - headers: getXACLHeader(), - auth: { bearer: bearerToken }, - auth: { - bearer: bearerToken, - }, + headers: { ...getXACLHeader(), Authorization: `Bearer ${bearerToken}` }, @@ - headers: getXACLHeader(), - auth: { bearer: bearerToken }, - auth: { - bearer: bearerToken, - }, + headers: { ...getXACLHeader(), Authorization: `Bearer ${bearerToken}` },Optional: If you intend to use
auth: { bearer }, confirm Cypress version supports it.Also applies to: 57-62, 81-86
tests/functional/cypress/e2e/v3/users.cy.ts (1)
101-136: Nice: strict 401 checks for unauthenticated user endpointsAssertions are crisp (status, code, message). Keep as-is.
tests/functional/cypress/e2e/v3/cla-manager.cy.ts (1)
19-27: Token retrieval pattern appears ineffective.The call to
getTokenKey(bearerToken)on line 22 passes anullvalue but doesn't use the return value. IfgetTokenKeyis intended to fetch and return the token, the pattern should bebearerToken = getTokenKey()or the function should handle localStorage directly without requiring the parameter.Verify the implementation of
getTokenKeyto confirm the correct usage pattern:
| # Check if authenticated user (via bearer token) is the same as user_id - if not raise exception permission denied | ||
| # LG: comment this out to tunr off this chekc added after LFID is required everywhere in EasyCLA | ||
| def check_user_id_is_current(auth_user, user_id): | ||
| auth_user_id = cla.controllers.user.get_or_create_user(auth_user).get_user_id() | ||
| if str(user_id) != auth_user_id: | ||
| cla.log.debug(f'request_individual_signature - auth user UUID {auth_user_id} is not the same as requested signature UUID {str(user_id)}') | ||
| raise cla.auth.AuthError('permission denied') |
There was a problem hiding this comment.
Fix typos and misleading context in authentication helper.
The helper has two issues:
- Typo in comment: "tunr off this chekc" → "turn off this check"
- Log message references
request_individual_signaturebut this is a generic helper used across many endpoints
Apply this diff:
-# Check if authenticated user (via bearer token) is the same as user_id - if not raise exception permission denied
-# LG: comment this out to tunr off this chekc added after LFID is required everywhere in EasyCLA
+# Check if authenticated user (via bearer token) is the same as user_id - if not raise permission denied exception
+# LG: comment this out to turn off this check added after LFID is required everywhere in EasyCLA
def check_user_id_is_current(auth_user, user_id):
auth_user_id = cla.controllers.user.get_or_create_user(auth_user).get_user_id()
if str(user_id) != auth_user_id:
- 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'check_user_id_is_current - auth user UUID {auth_user_id} is not the same as requested user UUID {str(user_id)}')
raise cla.auth.AuthError('permission denied')📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| # Check if authenticated user (via bearer token) is the same as user_id - if not raise exception permission denied | |
| # LG: comment this out to tunr off this chekc added after LFID is required everywhere in EasyCLA | |
| def check_user_id_is_current(auth_user, user_id): | |
| auth_user_id = cla.controllers.user.get_or_create_user(auth_user).get_user_id() | |
| if str(user_id) != auth_user_id: | |
| cla.log.debug(f'request_individual_signature - auth user UUID {auth_user_id} is not the same as requested signature UUID {str(user_id)}') | |
| raise cla.auth.AuthError('permission denied') | |
| # Check if authenticated user (via bearer token) is the same as user_id - if not raise permission denied exception | |
| # LG: comment this out to turn off this check added after LFID is required everywhere in EasyCLA | |
| def check_user_id_is_current(auth_user, user_id): | |
| auth_user_id = cla.controllers.user.get_or_create_user(auth_user).get_user_id() | |
| if str(user_id) != auth_user_id: | |
| cla.log.debug(f'check_user_id_is_current - auth user UUID {auth_user_id} is not the same as requested user UUID {str(user_id)}') | |
| raise cla.auth.AuthError('permission denied') |
🤖 Prompt for AI Agents
In cla-backend/cla/routes.py around lines 41 to 47, fix the comment typo and
misleading log context: update the comment to "turn off this check" (instead of
"tunr off this chekc") and change the debug log to a generic message that does
not reference request_individual_signature (e.g., mention "auth user UUID does
not match requested user UUID" or similar) so the helper's log accurately
reflects its generic use; keep the permission denied raise behavior unchanged.
| let bearerToken: string = null; | ||
| before(() => { | ||
| if (bearerToken == null) { | ||
| getTokenKey(bearerToken); | ||
| cy.window().then((win) => { | ||
| bearerToken = win.localStorage.getItem('bearerToken'); | ||
| }); | ||
| } | ||
| }); |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Fix token bootstrap and type
Same as other files; remove arg to getTokenKey, chain, assert.
- let bearerToken: string = null;
+ let bearerToken: string | null = null;
before(() => {
- if (bearerToken == null) {
- getTokenKey(bearerToken);
- cy.window().then((win) => {
- bearerToken = win.localStorage.getItem('bearerToken');
- });
- }
+ if (bearerToken == null) {
+ getTokenKey();
+ return cy.window().then((win) => {
+ bearerToken = win.localStorage.getItem('bearerToken');
+ expect(bearerToken, 'bearerToken').to.be.a('string').and.not.be.empty;
+ });
+ }
});📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| let bearerToken: string = null; | |
| before(() => { | |
| if (bearerToken == null) { | |
| getTokenKey(bearerToken); | |
| cy.window().then((win) => { | |
| bearerToken = win.localStorage.getItem('bearerToken'); | |
| }); | |
| } | |
| }); | |
| let bearerToken: string | null = null; | |
| before(() => { | |
| if (bearerToken == null) { | |
| getTokenKey(); | |
| return cy.window().then((win) => { | |
| bearerToken = win.localStorage.getItem('bearerToken'); | |
| expect(bearerToken, 'bearerToken').to.be.a('string').and.not.be.empty; | |
| }); | |
| } | |
| }); |
🤖 Prompt for AI Agents
In tests/functional/cypress/e2e/v3/company.cy.ts around lines 19 to 27, the
token bootstrap passes bearerToken into getTokenKey and uses cy.window()
separately, which is incorrect; call getTokenKey() with no arguments, chain its
returned Cypress command (e.g., .then(...)) to read the token from localStorage
and assign it to bearerToken, and add an assertion that bearerToken exists and
is a string. Ensure the flow is synchronous via Cypress chaining (no external
variable access before assignment) so the token is available for subsequent
tests.
|
|
||
| describe('To Validate & test Documentation APIs via API call (V3)', function () { | ||
| const claEndpoint = getAPIBaseURL('v3'); | ||
| let allowFail: boolean = !(Cypress.env('ALLOW_FAIL') === 1); |
There was a problem hiding this comment.
Fix ALLOW_FAIL toggle here as well.
- let allowFail: boolean = !(Cypress.env('ALLOW_FAIL') === 1);
+ const allowFail: boolean = String(Cypress.env('ALLOW_FAIL')) !== '1';📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| let allowFail: boolean = !(Cypress.env('ALLOW_FAIL') === 1); | |
| const allowFail: boolean = String(Cypress.env('ALLOW_FAIL')) !== '1'; |
🤖 Prompt for AI Agents
In tests/functional/cypress/e2e/v3/docs.cy.ts around line 11, the ALLOW_FAIL
toggle is using numeric comparison and a negation which misinterprets the
environment value; change the logic to directly compare the env string to '1' so
allowFail becomes true when ALLOW_FAIL is set to 1 (e.g., set allowFail to
Cypress.env('ALLOW_FAIL') === '1').
| describe('To Validate & test Events APIs via API call (V3)', function () { | ||
| //Reference api doc: V3 API events endpoints | ||
| const claEndpoint = getAPIBaseURL('v3'); | ||
| let allowFail: boolean = !(Cypress.env('ALLOW_FAIL') === 1); |
There was a problem hiding this comment.
Fix ALLOW_FAIL toggle (string vs number).
- let allowFail: boolean = !(Cypress.env('ALLOW_FAIL') === 1);
+ const allowFail: boolean = String(Cypress.env('ALLOW_FAIL')) !== '1';📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| let allowFail: boolean = !(Cypress.env('ALLOW_FAIL') === 1); | |
| const allowFail: boolean = String(Cypress.env('ALLOW_FAIL')) === '1'; |
🤖 Prompt for AI Agents
In tests/functional/cypress/e2e/v3/events.cy.ts around line 14, the ALLOW_FAIL
toggle currently compares Cypress.env('ALLOW_FAIL') to the number 1 but
Cypress.env returns strings; update the check to compare against the string '1'
or coerce the env value to a number (e.g., Number(Cypress.env('ALLOW_FAIL')) ===
1) so allowFail is set correctly; replace the current expression with one of
those options and ensure falsy/undefined values default as intended.
| // Test public endpoints (no auth required) | ||
| it('Get Templates without auth - Record should return 200 Response', function () { | ||
| cy.request({ | ||
| method: 'GET', | ||
| url: `${claEndpoint}template`, | ||
| timeout: timeout, | ||
| failOnStatusCode: allowFail, | ||
| headers: getXACLHeader(), | ||
| auth: { bearer: bearerToken }, | ||
| // No auth required for this endpoint | ||
| }).then((response) => { |
There was a problem hiding this comment.
Public endpoint sends Bearer token — remove to truly test “no auth”
Including auth undermines the “no auth required” assertion and can mask regressions.
- headers: getXACLHeader(),
- auth: { bearer: bearerToken },
- // No auth required for this endpoint
+ headers: getXACLHeader(),
+ // No auth required for this endpoint (intentionally no Authorization)🤖 Prompt for AI Agents
In tests/functional/cypress/e2e/v3/template.cy.ts around lines 28–38, the "no
auth required" test is sending a Bearer token which defeats the purpose; remove
the auth option and any Authorization/Bearer token from the request (and remove
or adjust getXACLHeader usage if it injects an Authorization header) so the
cy.request is issued without credentials or bearer tokens, leaving other
headers/timeouts unchanged.
| let bearerToken: string = null; | ||
| before(() => { | ||
| if (bearerToken == null) { | ||
| getTokenKey(bearerToken); | ||
| cy.window().then((win) => { | ||
| bearerToken = win.localStorage.getItem('bearerToken'); | ||
| }); | ||
| } | ||
| }); |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Fix token bootstrap: wrong arg, missing chaining, and null type
Same issue as other files: don’t pass bearerToken into getTokenKey; chain and assert token presence; allow null type.
- let bearerToken: string = null;
+ let bearerToken: string | null = null;
before(() => {
- if (bearerToken == null) {
- getTokenKey(bearerToken);
- cy.window().then((win) => {
- bearerToken = win.localStorage.getItem('bearerToken');
- });
- }
+ if (bearerToken == null) {
+ getTokenKey();
+ return cy.window().then((win) => {
+ bearerToken = win.localStorage.getItem('bearerToken');
+ expect(bearerToken, 'bearerToken').to.be.a('string').and.not.be.empty;
+ });
+ }
});📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| let bearerToken: string = null; | |
| before(() => { | |
| if (bearerToken == null) { | |
| getTokenKey(bearerToken); | |
| cy.window().then((win) => { | |
| bearerToken = win.localStorage.getItem('bearerToken'); | |
| }); | |
| } | |
| }); | |
| let bearerToken: string | null = null; | |
| before(() => { | |
| if (bearerToken == null) { | |
| getTokenKey(); | |
| return cy.window().then((win) => { | |
| bearerToken = win.localStorage.getItem('bearerToken'); | |
| expect(bearerToken, 'bearerToken').to.be.a('string').and.not.be.empty; | |
| }); | |
| } | |
| }); |
🤖 Prompt for AI Agents
In tests/functional/cypress/e2e/v3/users.cy.ts around lines 18-26, change the
bearerToken declaration to allow null (let bearerToken: string | null = null),
stop passing bearerToken into getTokenKey (call getTokenKey() with no
arguments), and properly chain/assert the token: call getTokenKey(); then use
cy.window().then(win => { bearerToken = win.localStorage.getItem('bearerToken');
expect(bearerToken).to.not.be.null; }) so the token is set and asserted
synchronously within the Cypress chain.
|
|
||
| describe('To Validate & test Version APIs via API call (V3)', function () { | ||
| const claEndpoint = getAPIBaseURL('v3'); | ||
| let allowFail: boolean = !(Cypress.env('ALLOW_FAIL') === 1); |
There was a problem hiding this comment.
Fix ALLOW_FAIL toggle (string vs number).
Cypress envs are strings; current check flips semantics.
Apply this diff:
- let allowFail: boolean = !(Cypress.env('ALLOW_FAIL') === 1);
+ // ALLOW_FAIL=1 => do not fail early on 4xx/5xx
+ const allowFail: boolean = String(Cypress.env('ALLOW_FAIL')) !== '1';Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In tests/functional/cypress/e2e/v3/version.cy.ts around line 11 the ALLOW_FAIL
check compares Cypress.env('ALLOW_FAIL') to the number 1, but Cypress env values
are strings which causes the boolean to be inverted; change the check to compare
to the string '1' and remove the unnecessary negation so allowFail is true only
when ALLOW_FAIL === '1' (e.g., set allowFail based on Cypress.env('ALLOW_FAIL')
=== '1').
| aws --profile lfproduct-prod --region us-east-2 ssm get-parameter --name "/cla-auth0-username-claim-prod" --query "Parameter.Value" --output text | ||
| aws --profile lfproduct-prod --region us-east-2 ssm get-parameter --name "/cla-auth0-username-claim-cli-prod" --query "Parameter.Value" --output text | ||
| aws --profile lfproduct-prod --region us-east-2 ssm get-parameter --name "/cla-auth0-email-claim-cli-prod" --query "Parameter.Value" --output text | ||
| ews --profile lfproduct-prod --region us-east-2 ssm get-parameter --name "/cla-auth0-name-claim-cli-prod" --query "Parameter.Value" --output text |
There was a problem hiding this comment.
Critical syntax error: ews should be aws.
Line 9 contains a typo that will cause the script to fail during execution. The command starts with "ews" instead of "aws", making it an invalid shell command.
Apply this diff to fix the syntax error:
-ews --profile lfproduct-prod --region us-east-2 ssm get-parameter --name "/cla-auth0-name-claim-cli-prod" --query "Parameter.Value" --output text
+aws --profile lfproduct-prod --region us-east-2 ssm get-parameter --name "/cla-auth0-name-claim-cli-prod" --query "Parameter.Value" --output text📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| ews --profile lfproduct-prod --region us-east-2 ssm get-parameter --name "/cla-auth0-name-claim-cli-prod" --query "Parameter.Value" --output text | |
| aws --profile lfproduct-prod --region us-east-2 ssm get-parameter --name "/cla-auth0-name-claim-cli-prod" --query "Parameter.Value" --output text |
🤖 Prompt for AI Agents
In utils/get_prod_claims.sh around line 9, the AWS CLI command has a typo: it
starts with "ews" instead of "aws"; update the command to begin with "aws" so
the script invokes the AWS CLI correctly (replace the leading "ews" with "aws"
on line 9).
| if [ ! -z "$DEBUG" ] | ||
| then | ||
| echo "curl -s -XPOST -H 'Content-Type: application/json' '${API_URL}/v1/request-corporate-signature' -d '{\"project_id\":\"${project_id}\",\"company_id\":\"${company_id}\",\"return_url_type\":\"${return_url_type}\",\"return_url\":\"${return_url}\"}' | jq -r '.'" | ||
| echo "curl -s -XPOST -H 'X-ACL: ${XACL}' -H 'Authorization: Bearer ${TOKEN}' -H 'Content-Type: application/json' '${API_URL}/v1/request-corporate-signature' -d '{\"project_id\":\"${project_id}\",\"company_id\":\"${company_id}\",\"return_url_type\":\"${return_url_type}\",\"return_url\":\"${return_url}\"}' | jq -r '.'" | ||
| fi | ||
| curl -s -XPOST -H "Content-Type: application/json" "${API_URL}/v1/request-corporate-signature" -d "{\"project_id\":\"${project_id}\",\"company_id\":\"${company_id}\",\"return_url_type\":\"${return_url_type}\",\"return_url\":\"${return_url}\"}" | jq -r '.' | ||
| curl -s -XPOST -H "X-ACL: ${XACL}" -H "Authorization: Bearer ${TOKEN}" -H "Content-Type: application/json" "${API_URL}/v1/request-corporate-signature" -d "{\"project_id\":\"${project_id}\",\"company_id\":\"${company_id}\",\"return_url_type\":\"${return_url_type}\",\"return_url\":\"${return_url}\"}" | jq -r '.' |
There was a problem hiding this comment.
Secrets exposed in debug output.
Lines 73 and 75 include plaintext TOKEN and XACL headers in the debug echo and curl commands. While appropriate for local development, ensure debug output is not logged to persistent storage in production-like environments. Consider adding a notice in the documentation about this.
🤖 Prompt for AI Agents
In utils/request_corporate_signature_py_post.sh around lines 71 to 75 the DEBUG
branch prints and executes a curl command that exposes sensitive headers (TOKEN
and XACL) in plaintext; update the debug echo to avoid printing secrets (mask
values or show placeholders like <TOKEN> and <XACL>), and change the actual curl
invocation when DEBUG is enabled to either omit echoing sensitive values or log
only the command template; additionally gate debug output so it never writes to
persistent logs in production (e.g., only enable DEBUG in local dev), and add a
short note in the script header or project docs warning that DEBUG may reveal
secrets and must not be enabled in production-like environments.
There was a problem hiding this comment.
Pull Request Overview
This PR implements authentication and authorization enhancements to the CLA backend system along with infrastructure improvements and comprehensive V3 API test coverage.
- Adds user ID validation checks to ensure authenticated users can only access their own resources
- Adds authentication requirements to previously public endpoints in both Python and Go backends
- Extends AWS SSM parameter management scripts to support multi-region (us-east-2) deployments
- Introduces comprehensive V3 API test suite covering all major endpoints (company, events, GitHub, project, signatures, users, etc.)
Reviewed Changes
Copilot reviewed 25 out of 26 changed files in this pull request and generated 46 comments.
Show a summary per file
| File | Description |
|---|---|
| utils/set_dev_claims.sh | Adds us-east-2 region support for SSM parameters |
| utils/restore_dev_claims.sh | Adds us-east-2 region support for parameter restoration |
| utils/get_prod_claims.sh | Adds us-east-2 region support for production parameter retrieval |
| utils/get_dev_claims.sh | Adds us-east-2 region support for dev parameter retrieval |
| utils/request_individual_signature_py_post.sh | Adds TOKEN and XACL authentication headers to API requests |
| utils/request_employee_signature_py_post.sh | Adds TOKEN and XACL authentication headers to API requests |
| utils/request_corporate_signature_py_post.sh | Adds TOKEN and XACL authentication headers to API requests |
| tests/functional/utils/run-single-test.sh | Improves test runner to support multiple test files and better usage examples |
| tests/functional/cypress/e2e/v3/*.cy.ts | Adds comprehensive V3 API test suite (13 new test files) |
| cla-backend/cla/routes.py | Adds check_user_id_is_current validation and auth_user parameter to endpoints |
| cla-backend-go/users/handlers.go | Improves nil checking for user models |
Comments suppressed due to low confidence (5)
tests/functional/cypress/e2e/v3/events.cy.ts:36
- This property is duplicated in a later property.
auth: { bearer: bearerToken },
tests/functional/cypress/e2e/v3/events.cy.ts:58
- This property is duplicated in a later property.
auth: { bearer: bearerToken },
tests/functional/cypress/e2e/v3/events.cy.ts:82
- This property is duplicated in a later property.
auth: { bearer: bearerToken },
tests/functional/cypress/e2e/v3/project.cy.ts:37
- This property is duplicated in a later property.
auth: { bearer: bearerToken },
tests/functional/cypress/e2e/v3/project.cy.ts:137
- This property is duplicated in a later property.
auth: { bearer: bearerToken },
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| aws --profile lfproduct-prod --region us-east-2 ssm get-parameter --name "/cla-auth0-username-claim-prod" --query "Parameter.Value" --output text | ||
| aws --profile lfproduct-prod --region us-east-2 ssm get-parameter --name "/cla-auth0-username-claim-cli-prod" --query "Parameter.Value" --output text | ||
| aws --profile lfproduct-prod --region us-east-2 ssm get-parameter --name "/cla-auth0-email-claim-cli-prod" --query "Parameter.Value" --output text | ||
| ews --profile lfproduct-prod --region us-east-2 ssm get-parameter --name "/cla-auth0-name-claim-cli-prod" --query "Parameter.Value" --output text |
There was a problem hiding this comment.
Corrected spelling of 'ews' to 'aws'.
| ews --profile lfproduct-prod --region us-east-2 ssm get-parameter --name "/cla-auth0-name-claim-cli-prod" --query "Parameter.Value" --output text | |
| aws --profile lfproduct-prod --region us-east-2 ssm get-parameter --name "/cla-auth0-name-claim-cli-prod" --query "Parameter.Value" --output text |
| ) | ||
|
|
||
| # Check if authenticated user (via bearer token) is the same as user_id - if not raise exception permission denied | ||
| # LG: comment this out to tunr off this chekc added after LFID is required everywhere in EasyCLA |
There was a problem hiding this comment.
Corrected spelling errors: 'tunr' should be 'turn' and 'chekc' should be 'check'.
| # LG: comment this out to tunr off this chekc added after LFID is required everywhere in EasyCLA | |
| # LG: comment this out to turn off this check added after LFID is required everywhere in EasyCLA |
| timeout: timeout, | ||
| failOnStatusCode: allowFail, | ||
| headers: getXACLHeader(), | ||
| auth: { bearer: bearerToken }, |
There was a problem hiding this comment.
Duplicate 'auth' property definition. The second definition on lines 38-40 will override the first one on line 37, making line 37 redundant. Remove the duplicate property.
| auth: { bearer: bearerToken }, |
| timeout: timeout, | ||
| failOnStatusCode: allowFail, | ||
| headers: getXACLHeader(), | ||
| auth: { bearer: bearerToken }, |
There was a problem hiding this comment.
Duplicate 'auth' property definition. The second definition on lines 138-140 will override the first one on line 137, making line 37 redundant. Remove the duplicate property.
| auth: { bearer: bearerToken }, |
| headers: getXACLHeader(), | ||
| auth: { bearer: bearerToken }, | ||
| auth: { | ||
| bearer: bearerToken, | ||
| }, |
There was a problem hiding this comment.
Duplicate 'auth' property definition. The second definition on lines 37-39 will override the first one on line 36, making line 36 redundant. Remove the duplicate property.
| let bearerToken: string = null; | ||
| before(() => { | ||
| if (bearerToken == null) { | ||
| getTokenKey(bearerToken); |
There was a problem hiding this comment.
Superfluous argument passed to function getTokenKey.
| getTokenKey(bearerToken); | |
| getTokenKey(); |
| let bearerToken: string = null; | ||
| before(() => { | ||
| if (bearerToken == null) { | ||
| getTokenKey(bearerToken); |
There was a problem hiding this comment.
Superfluous argument passed to function getTokenKey.
| getTokenKey(bearerToken); | |
| getTokenKey(); |
| let bearerToken: string = null; | ||
| before(() => { | ||
| if (bearerToken == null) { | ||
| getTokenKey(bearerToken); |
There was a problem hiding this comment.
Superfluous argument passed to function getTokenKey.
| getTokenKey(bearerToken); | |
| getTokenKey(); |
| let bearerToken: string = null; | ||
| before(() => { | ||
| if (bearerToken == null) { | ||
| getTokenKey(bearerToken); |
There was a problem hiding this comment.
Superfluous argument passed to function getTokenKey.
| getTokenKey(bearerToken); | |
| getTokenKey(); |
| let bearerToken: string = null; | ||
| before(() => { | ||
| if (bearerToken == null) { | ||
| getTokenKey(bearerToken); |
There was a problem hiding this comment.
Superfluous argument passed to function getTokenKey.
| getTokenKey(bearerToken); | |
| getTokenKey(); |
|
@copilot hold reviews until |
|
@lukaszgryglicki I've opened a new pull request, #4847, to work on those changes. Once the pull request is ready, I'll request review from you. |
|
@CodeRa hold reviews until |
Signed-off-by: Lukasz Gryglicki <[email protected]> Assisted by [OpenAI](https://platform.openai.com/) Assisted by [GitHub Copilot](https://github.com/features/copilot)
Signed-off-by: Lukasz Gryglicki <[email protected]> Assisted by [OpenAI](https://platform.openai.com/) Assisted by [GitHub Copilot](https://github.com/features/copilot)
There was a problem hiding this comment.
Actionable comments posted: 11
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
cla-backend/cla/routes.py (1)
190-194: Do not ship with user-id verification commented out.Security check is disabled in multiple endpoints. This permits cross-user actions if upstream routing doesn’t guard sufficiently. Re‑enable or gate behind a feature flag before merge.
Suggested pattern:
if getattr(cla.config, 'ENFORCE_USER_MATCH', True): check_user_id_is_current(auth_user, user_id)Please apply to the commented sites in:
- request_company_allowlist (Line 190)
- invite_company_admin (Line 222)
- request_company_ccla (Line 241)
- request_individual_signature (Line 1276)
- request_employee_signature (Line 1369)
- check_and_prepare_employee_signature (Line 1390)
Also applies to: 222-227, 241-243, 1276-1279, 1369-1372, 1390-1392
♻️ Duplicate comments (25)
utils/get_prod_claims.sh (1)
9-9: Critical syntax error persists:ewsshould beaws.Line 9 still contains the typo flagged in the previous review. The command starts with "ews" instead of "aws", making it an invalid shell command that will fail during execution.
Apply this diff to fix the syntax error:
-ews --profile lfproduct-prod --region us-east-2 ssm get-parameter --name "/cla-auth0-name-claim-cli-prod" --query "Parameter.Value" --output text +aws --profile lfproduct-prod --region us-east-2 ssm get-parameter --name "/cla-auth0-name-claim-cli-prod" --query "Parameter.Value" --output textutils/request_employee_signature_py_post.sh (1)
78-78: Secrets exposed in debug output.Lines 78 and 80 include plaintext TOKEN and XACL values in the debug echo and curl command. While appropriate for local development, ensure debug output is never logged to persistent storage in production-like environments. Mask sensitive values in debug output (e.g., show
<TOKEN>and<XACL>placeholders) or add a prominent warning in the script header that DEBUG must not be enabled in production.Also applies to: 80-80
utils/request_corporate_signature_py_post.sh (1)
73-73: Secrets exposed in debug output (flagged in prior review).Lines 73 and 75 include plaintext TOKEN and XACL values. A similar concern was raised in a previous review. Ensure debug output is never logged to persistent storage and consider masking sensitive values or adding a production warning to the script header.
Also applies to: 75-75
tests/functional/cypress/e2e/v3/github-organizations.cy.ts (1)
15-27: Fix token bootstrap: remove arg, chain storage read, and type nullable.Prevents races and TS type errors.
- let bearerToken: string = null; + let bearerToken: string | null = null; before(() => { if (bearerToken == null) { - getTokenKey(bearerToken); - cy.window().then((win) => { - bearerToken = win.localStorage.getItem('bearerToken'); - }); + // getTokenKey populates localStorage + getTokenKey(); + return cy.window().then((win) => { + bearerToken = win.localStorage.getItem('bearerToken'); + expect(bearerToken, 'bearerToken').to.be.a('string').and.not.be.empty; + }); } });cla-backend/cla/routes.py (1)
41-47: Fix helper comment/log and use explicit f-string conversions.Typos and misleading log context; also prefer f-string conversion flags over str().
-# Check if authenticated user (via bearer token) is the same as user_id - if not raise exception permission denied -# LG: comment this out to tunr off this chekc added after LFID is required everywhere in EasyCLA +# Check if authenticated user (via bearer token) is the same as user_id — if not, raise permission denied +# LG: comment this out to turn off this check added after LFID is required everywhere in EasyCLA def check_user_id_is_current(auth_user, user_id): auth_user_id = cla.controllers.user.get_or_create_user(auth_user).get_user_id() if str(user_id) != auth_user_id: - 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'check_user_id_is_current - auth user UUID {auth_user_id!s} ' + f'is not the same as requested user UUID {user_id!s}' + ) raise cla.auth.AuthError('permission denied')tests/functional/cypress/e2e/v3/version.cy.ts (2)
11-11: Fix ALLOW_FAIL toggle (string vs number).Cypress envs are strings; current check flips semantics.
- let allowFail: boolean = !(Cypress.env('ALLOW_FAIL') === 1); + // ALLOW_FAIL=1 => do not fail early on 4xx/5xx + const failOnStatusCode: boolean = String(Cypress.env('ALLOW_FAIL')) !== '1';Also update the request to use:
- failOnStatusCode: allowFail, + failOnStatusCode: failOnStatusCode,
15-23: Fix token bootstrap to avoid race and TS nullability issues.Use no-arg getTokenKey(), chain localStorage read, and allow null type.
- let bearerToken: string = null; + let bearerToken: string | null = null; before(() => { if (bearerToken == null) { - getTokenKey(bearerToken); - cy.window().then((win) => { - bearerToken = win.localStorage.getItem('bearerToken'); - }); + getTokenKey(); + return cy.window().then((win) => { + bearerToken = win.localStorage.getItem('bearerToken'); + expect(bearerToken, 'bearerToken').to.be.a('string').and.not.be.empty; + }); } });cla-backend-go/users/handlers.go (1)
260-266: Return 404 for missing user; keep 400 only for real errors.Current logic synthesizes an error and returns BadRequest when the user is not found. Prefer explicit 404 for nil userModel to avoid misleading clients.
- if err != nil || userModel == nil { - if err == nil { - err = fmt.Errorf("user not found for user_id: %s", params.UserID) - } - log.WithFields(f).Warnf("error retrieving user for user_id: %s, error: %+v", params.UserID, err) - return users.NewGetUserCompatBadRequest().WithPayload(errorResponse(err)) - } + if err != nil { + log.WithFields(f).Warnf("error retrieving user for user_id: %s, error: %+v", params.UserID, err) + return users.NewGetUserCompatBadRequest().WithPayload(errorResponse(err)) + } + if userModel == nil { + log.WithFields(f).Warnf("user not found for user_id: %s", params.UserID) + return users.NewGetUserCompatNotFound() + }tests/functional/cypress/e2e/v3/docs.cy.ts (1)
11-11: Normalize ALLOW_FAIL parsing (string env).Cypress.env returns strings; compare to '1' without negation confusion.
- let allowFail: boolean = !(Cypress.env('ALLOW_FAIL') === 1); + const allowFail: boolean = String(Cypress.env('ALLOW_FAIL')) !== '1';tests/functional/cypress/e2e/v3/users.cy.ts (1)
18-26: Fix token bootstrap: wrong arg, missing chaining, and null type.- let bearerToken: string = null; + let bearerToken: string | null = null; before(() => { if (bearerToken == null) { - getTokenKey(bearerToken); - cy.window().then((win) => { - bearerToken = win.localStorage.getItem('bearerToken'); - }); + getTokenKey(); + return cy.window().then((win) => { + bearerToken = win.localStorage.getItem('bearerToken'); + expect(bearerToken, 'bearerToken').to.be.a('string').and.not.be.empty; + }); } });tests/functional/cypress/e2e/v3/health.cy.ts (1)
11-11: Normalize ALLOW_FAIL parsing (string env).- let allowFail: boolean = !(Cypress.env('ALLOW_FAIL') === 1); + const allowFail: boolean = String(Cypress.env('ALLOW_FAIL')) !== '1';tests/functional/cypress/e2e/v3/events.cy.ts (2)
14-14: Normalize ALLOW_FAIL parsing (string env).- let allowFail: boolean = !(Cypress.env('ALLOW_FAIL') === 1); + const allowFail: boolean = String(Cypress.env('ALLOW_FAIL')) !== '1';
49-51: Avoid hard‑coded IDs; parameterize or skip when absent.- it('Get Company Events with authentication - Record should return 200 Response', function () { - const companyID = 'd9428888-122b-4b20-8c4a-0c9a1a6f9b8e'; // Example UUID + it('Get Company Events with authentication - Record should return 200 Response', function () { + const companyID = Cypress.env('COMPANY_ID'); + if (!companyID) { this.skip(); }And for unauthorized tests:
- const exampleCompanyID = 'd9428888-122b-4b20-8c4a-0c9a1a6f9b8e'; - const exampleUserID = 'd9428888-122b-4b20-8c4a-0c9a1a6f9b8f'; + const exampleCompanyID = Cypress.env('COMPANY_ID') || '00000000-0000-0000-0000-000000000000'; + const exampleUserID = Cypress.env('USER_ID') || '00000000-0000-0000-0000-000000000000';Optionally resolve IDs dynamically (list and pick one).
Also applies to: 97-99
tests/functional/cypress/e2e/v3/company.cy.ts (1)
19-27: Fix token bootstrap and TS type; ensure Cypress chaining.Use union type, call getTokenKey() without args, and assert token via returned chain.
- let bearerToken: string = null; + let bearerToken: string | null = null; before(() => { if (bearerToken == null) { - getTokenKey(bearerToken); - cy.window().then((win) => { - bearerToken = win.localStorage.getItem('bearerToken'); - }); + getTokenKey(); + return cy.window().then((win) => { + bearerToken = win.localStorage.getItem('bearerToken'); + expect(bearerToken, 'bearerToken').to.be.a('string').and.not.be.empty; + }); } });tests/functional/cypress/e2e/v3/template.cy.ts (2)
18-26: Fix token bootstrap and TS type; chain to ensure token availability.- let bearerToken: string = null; + let bearerToken: string | null = null; before(() => { if (bearerToken == null) { - getTokenKey(bearerToken); - cy.window().then((win) => { - bearerToken = win.localStorage.getItem('bearerToken'); - }); + getTokenKey(); + return cy.window().then((win) => { + bearerToken = win.localStorage.getItem('bearerToken'); + expect(bearerToken, 'bearerToken').to.be.a('string').and.not.be.empty; + }); } });
30-39: Public “no auth” test includes Bearer token — remove it.This was flagged earlier; please keep the request unauthenticated.
failOnStatusCode: allowFail, headers: getXACLHeader(), - auth: { bearer: bearerToken }, // No auth required for this endpointtests/functional/cypress/e2e/v3/gerrits.cy.ts (1)
19-27: Standardize token bootstrap and typing; chain for determinism.- let bearerToken: string = null; + let bearerToken: string | null = null; before(() => { if (bearerToken == null) { - getTokenKey(bearerToken); - cy.window().then((win) => { - bearerToken = win.localStorage.getItem('bearerToken'); - }); + getTokenKey(); + return cy.window().then((win) => { + bearerToken = win.localStorage.getItem('bearerToken'); + expect(bearerToken, 'bearerToken').to.be.a('string').and.not.be.empty; + }); } });tests/functional/cypress/e2e/v3/signatures.cy.ts (1)
19-27: Fix token bootstrap and typing; ensure ordered Cypress chain.- let bearerToken: string = null; + let bearerToken: string | null = null; before(() => { if (bearerToken == null) { - getTokenKey(bearerToken); - cy.window().then((win) => { - bearerToken = win.localStorage.getItem('bearerToken'); - }); + getTokenKey(); + return cy.window().then((win) => { + bearerToken = win.localStorage.getItem('bearerToken'); + expect(bearerToken, 'bearerToken').to.be.a('string').and.not.be.empty; + }); } });tests/functional/cypress/e2e/v3/project.cy.ts (3)
19-27: Fix token bootstrap and TS type; ensure chaining completes before use.Declare bearerToken as string | null, call getTokenKey() with no arg, and assert token via a returned chain to avoid race conditions.
- let bearerToken: string = null; + let bearerToken: string | null = null; before(() => { if (bearerToken == null) { - getTokenKey(bearerToken); - cy.window().then((win) => { - bearerToken = win.localStorage.getItem('bearerToken'); - }); + getTokenKey(); + return cy.window().then((win) => { + bearerToken = win.localStorage.getItem('bearerToken'); + expect(bearerToken, 'bearerToken').to.be.a('string').and.not.be.empty; + }); } });
37-40: Duplicate auth property — remove the redundant entry.Only keep a single auth field to avoid overriding the first one. Static analysis flags this.
headers: getXACLHeader(), - auth: { bearer: bearerToken }, auth: { bearer: bearerToken, },
137-140: Duplicate auth property (again) — consolidate.Same issue in the flakiness test block.
headers: getXACLHeader(), - auth: { bearer: bearerToken }, auth: { bearer: bearerToken, },tests/functional/cypress/e2e/v3/github-repositories.cy.ts (1)
19-27: Token retrieval pattern is ineffective.This issue has already been flagged in previous review comments. The
getTokenKeyfunction doesn't return a value and the null parameter passed is not used effectively.tests/functional/cypress/e2e/v3/organization.cy.ts (2)
19-27: Token retrieval pattern is ineffective.This issue has already been flagged in previous review comments.
32-51: Inconsistent authentication configuration.This inconsistency between the auth configuration (line 42) and the comment (line 43) has already been flagged in previous review comments.
tests/functional/cypress/e2e/v3/github.cy.ts (1)
19-27: Token retrieval pattern is ineffective.This issue has already been flagged in previous review comments.
🧹 Nitpick comments (29)
utils/request_employee_signature_py_post.sh (3)
48-69: Potential issues with file read error handling.Lines 51 and 62 use
catto read secret files without checking if they exist first. If the file is missing,catfails silently and the variable remains empty, resulting in a misleading error message ("unable to obtain") rather than indicating the file couldn't be found. Consider adding explicit file existence checks:if [ ! -f ./auth0.token.secret ]; then echo "$0: auth0.token.secret file not found" exit 8 fi TOKEN="$(cat ./auth0.token.secret)"
50-50: Remove commented-out legacy code.Line 50 contains a commented-out
sourcestatement that appears to be legacy. Remove it to reduce clutter.
48-69: DRY: Extract TOKEN/XACL loading logic into a shared utility.The TOKEN and XACL loading pattern (lines 48–69) is duplicated identically in
utils/request_corporate_signature_py_post.shandutils/request_individual_signature_py_post.sh. Extract this logic into a sourced shell script (e.g.,utils/auth_helpers.sh) to centralize secret loading, error handling, and exit codes.utils/request_corporate_signature_py_post.sh (3)
43-64: Potential issues with file read error handling.Lines 46 and 57 use
catto read secret files without checking if they exist first. If the file is missing,catfails silently and the variable remains empty, resulting in a misleading error message ("unable to obtain") rather than indicating the file couldn't be found. Consider adding explicit file existence checks (see similar suggestion inutils/request_employee_signature_py_post.sh).
45-45: Remove commented-out legacy code.Line 45 contains a commented-out
sourcestatement that appears to be legacy. Remove it to reduce clutter.
43-64: DRY: Extract TOKEN/XACL loading logic into a shared utility.The TOKEN and XACL loading pattern (lines 43–64) is duplicated identically in
utils/request_employee_signature_py_post.shandutils/request_individual_signature_py_post.sh. Extract this logic into a sourced shell script (e.g.,utils/auth_helpers.sh) to centralize secret loading, error handling, and exit codes.utils/request_individual_signature_py_post.sh (4)
45-66: Potential issues with file read error handling.Lines 48 and 59 use
catto read secret files without checking if they exist first. If the file is missing,catfails silently and the variable remains empty, resulting in a misleading error message ("unable to obtain") rather than indicating the file couldn't be found. Consider adding explicit file existence checks (see similar suggestion inutils/request_employee_signature_py_post.sh).
47-47: Remove commented-out legacy code.Line 47 contains a commented-out
sourcestatement that appears to be legacy. Remove it to reduce clutter.
70-70: Secrets exposed in debug output.Lines 70, 71, and 73 include plaintext TOKEN and XACL values in debug output and curl commands. While appropriate for local development, ensure debug output is never logged to persistent storage in production-like environments. Consider masking sensitive values in debug output (e.g., show
<TOKEN>and<XACL>placeholders) or add a production warning to the script header.Also applies to: 71-71, 73-73
45-66: DRY: Extract TOKEN/XACL loading logic into a shared utility.The TOKEN and XACL loading pattern (lines 45–66) is duplicated identically in the other two signature request scripts. Extract this logic into a sourced shell script (e.g.,
utils/auth_helpers.sh) to centralize secret loading, error handling, and exit codes.tests/functional/cypress/e2e/v3/github-organizations.cy.ts (1)
15-17: ALLOW_FAIL semantics and unused var.Cypress env values are strings; current check is wrong and the variable is unused. Either remove it or wire it properly.
Option A (remove):
- let allowFail: boolean = !(Cypress.env('ALLOW_FAIL') === 1);Option B (wire correctly and use):
- let allowFail: boolean = !(Cypress.env('ALLOW_FAIL') === 1); + // ALLOW_FAIL=1 => do not fail early on 4xx/5xx + const failOnStatusCode = String(Cypress.env('ALLOW_FAIL')) !== '1';Then use:
- failOnStatusCode: false, + failOnStatusCode,Apply similarly to other requests in this file.
Also applies to: 33-43, 65-75, 110-120, 202-211
cla-backend/cla/routes.py (1)
660-667: Silence ARG001 by marking unused auth_user parameters.Several handlers require auth for routing but do not reference auth_user. Rename to _auth_user to satisfy linters without behavior change.
Example:
-def get_all_companies(auth_user: check_auth): +def get_all_companies(_auth_user: check_auth):Apply similarly to other listed handlers.
Also applies to: 670-677, 680-687, 786-793, 815-833, 1050-1058, 1103-1110
tests/functional/utils/run-single-test.sh (1)
39-48: Simplify and harden multi-spec construction.Use printf to build a comma-separated list without trailing-comma logic.
- CMD="xvfb-run -a npx cypress run --spec " - for file in cypress/e2e/v${V}/*.cy.ts - do - if [ "${CMD: -1}" = " " ] - then - CMD="${CMD}${file}" - continue - fi - CMD="${CMD},${file}" - done + specs=$(printf '%s,' cypress/e2e/v${V}/*.cy.ts | sed 's/,$//') + CMD="xvfb-run -a npx cypress run --spec ${specs}"tests/functional/cypress/e2e/v3/docs.cy.ts (1)
15-23: Fix token bootstrap: wrong arg + missing chaining/assert.Don’t pass an argument to getTokenKey; chain and assert token presence. Also allow null type.
- let bearerToken: string = null; + let bearerToken: string | null = null; before(() => { if (bearerToken == null) { - getTokenKey(bearerToken); - cy.window().then((win) => { - bearerToken = win.localStorage.getItem('bearerToken'); - }); + getTokenKey(); + return cy.window().then((win) => { + bearerToken = win.localStorage.getItem('bearerToken'); + expect(bearerToken, 'bearerToken').to.be.a('string').and.not.be.empty; + }); } });Optional: since docs endpoints are public, you can remove the token bootstrap entirely to speed up the suite.
tests/functional/cypress/e2e/v3/users.cy.ts (2)
14-14: Normalize ALLOW_FAIL parsing (string env).Avoid numeric comparison and negation; compare to '1'.
- let allowFail: boolean = !(Cypress.env('ALLOW_FAIL') === 1); + const allowFail: boolean = String(Cypress.env('ALLOW_FAIL')) !== '1';
101-137: Reuse shared 401 validator to avoid env‑specific drift.Replace custom 401 assertions with validate_401_Status to handle local vs remote differences consistently.
- // V3 OAuth2 endpoints should return 401 when no token provided - expect(response.status).to.equal(401); - expect(response.body).to.have.property('code', 401); - expect(response.body).to.have.property('message'); - expect(response.body.message).to.include('unauthenticated'); + // V3 OAuth2 endpoints should return 401 when no token provided + validate_401_Status(response, local);tests/functional/cypress/e2e/v3/health.cy.ts (1)
15-23: Fix token bootstrap: wrong arg + missing chaining/assert.Health is public — consider removing token bootstrap entirely.
- let bearerToken: string = null; + let bearerToken: string | null = null; before(() => { if (bearerToken == null) { - getTokenKey(bearerToken); - cy.window().then((win) => { - bearerToken = win.localStorage.getItem('bearerToken'); - }); + getTokenKey(); + return cy.window().then((win) => { + bearerToken = win.localStorage.getItem('bearerToken'); + expect(bearerToken, 'bearerToken').to.be.a('string').and.not.be.empty; + }); } });tests/functional/cypress/e2e/v3/events.cy.ts (1)
18-26: Fix token bootstrap: wrong arg + missing chaining/assert.- let bearerToken: string = null; + let bearerToken: string | null = null; before(() => { if (bearerToken == null) { - getTokenKey(bearerToken); - cy.window().then((win) => { - bearerToken = win.localStorage.getItem('bearerToken'); - }); + getTokenKey(); + return cy.window().then((win) => { + bearerToken = win.localStorage.getItem('bearerToken'); + expect(bearerToken, 'bearerToken').to.be.a('string').and.not.be.empty; + }); } });tests/functional/cypress/e2e/v3/cla-manager.cy.ts (3)
15-15: Normalize ALLOW_FAIL parsing (string env).- let allowFail: boolean = !(Cypress.env('ALLOW_FAIL') === 1); + const allowFail: boolean = String(Cypress.env('ALLOW_FAIL')) !== '1';
19-27: Fix token bootstrap: wrong arg + missing chaining/assert.- let bearerToken: string = null; + let bearerToken: string | null = null; before(() => { if (bearerToken == null) { - getTokenKey(bearerToken); - cy.window().then((win) => { - bearerToken = win.localStorage.getItem('bearerToken'); - }); + getTokenKey(); + return cy.window().then((win) => { + bearerToken = win.localStorage.getItem('bearerToken'); + expect(bearerToken, 'bearerToken').to.be.a('string').and.not.be.empty; + }); } });
31-33: Avoid hard‑coded IDs; use env vars and skip when missing.Prevents flaky 404s across envs.
- const companyID = 'd9428888-122b-4b20-8c4a-0c9a1a6f9b8e'; // Example UUID - const projectID = 'a092M00001IV4SfQAL'; // Example SFID + const companyID = Cypress.env('COMPANY_ID'); + const projectID = Cypress.env('PROJECT_SFID'); + if (!companyID || !projectID) { this.skip(); }Apply similarly in the flakiness block.
Also applies to: 61-63
tests/functional/cypress/e2e/v3/project.cy.ts (1)
15-15: Coerce ALLOW_FAIL env to string/number explicitly.Cypress.env may return a string; strict equality to number 1 can misbehave.
- let allowFail: boolean = !(Cypress.env('ALLOW_FAIL') === 1); + let allowFail: boolean = String(Cypress.env('ALLOW_FAIL')) !== '1';tests/functional/cypress/e2e/v3/company.cy.ts (1)
15-15: Coerce ALLOW_FAIL env properly.Avoid number-vs-string pitfalls.
- let allowFail: boolean = !(Cypress.env('ALLOW_FAIL') === 1); + let allowFail: boolean = String(Cypress.env('ALLOW_FAIL')) !== '1';tests/functional/cypress/e2e/v3/template.cy.ts (1)
14-14: Coerce ALLOW_FAIL env to avoid type mismatch.- let allowFail: boolean = !(Cypress.env('ALLOW_FAIL') === 1); + let allowFail: boolean = String(Cypress.env('ALLOW_FAIL')) !== '1';tests/functional/cypress/e2e/v3/gerrits.cy.ts (1)
15-15: Coerce ALLOW_FAIL env explicitly.- let allowFail: boolean = !(Cypress.env('ALLOW_FAIL') === 1); + let allowFail: boolean = String(Cypress.env('ALLOW_FAIL')) !== '1';tests/functional/cypress/e2e/v3/signatures.cy.ts (2)
168-181: Auth-required tests should allow 404/405 like other suites.Mirror the conditional handling used elsewhere to reduce false failures.
- // V3 OAuth2 endpoints should return 401 when no token provided - validate_401_Status(response, local); + // Prefer 401; tolerate 404/405 for non-existent routes + if (response.status === 401) { + validate_401_Status(response, local); + } else if (response.status === 404) { + validate_404_Status(response); + } else if (response.status === 405) { + expect(response.status).to.equal(405); + } else { + expect(response.status, `Expected 401, 404, or 405 but got ${response.status}`).to.not.equal(200); + }
15-15: Coerce ALLOW_FAIL env to avoid string/number mismatch.- let allowFail: boolean = !(Cypress.env('ALLOW_FAIL') === 1); + let allowFail: boolean = String(Cypress.env('ALLOW_FAIL')) !== '1';tests/functional/cypress/e2e/v3/organization.cy.ts (1)
53-73: Consider removing authentication if endpoint is public.If the organization search endpoint is truly public (as indicated by comments in the previous test), the bearer token authentication on line 66 is unnecessary and creates inconsistency with the stated intent.
Apply this diff if the endpoint is confirmed to be public:
headers: getXACLHeader(), - auth: { bearer: bearerToken },tests/functional/cypress/e2e/v3/github.cy.ts (1)
55-72: Consider tightening test expectations.The test accepts any response as valid, which provides minimal validation. Once the expected behavior of the OAuth callback is confirmed, consider adding specific assertions for success/error scenarios.
Example refinement:
}).then((response) => { - // This endpoint might return various responses depending on the auth flow - // Accept any response as valid for testing purposes - cy.log(`GitHub redirect returned status: ${response.status}`); - expect(response.status).to.be.a('number'); + // Handle expected OAuth callback responses + if (response.status === 302 || response.status === 200) { + cy.log(`OAuth callback successful: ${response.status}`); + } else if (response.status === 400 || response.status === 401) { + cy.log(`OAuth callback error (expected): ${response.status}`); + } else { + cy.log(`Unexpected OAuth callback status: ${response.status}`); + } + expect(response.status).to.be.greaterThan(0); });
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Jira integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (25)
cla-backend-go/users/handlers.go(1 hunks)cla-backend/cla/routes.py(32 hunks)tests/functional/cypress/e2e/v3/cla-manager.cy.ts(1 hunks)tests/functional/cypress/e2e/v3/company.cy.ts(1 hunks)tests/functional/cypress/e2e/v3/docs.cy.ts(1 hunks)tests/functional/cypress/e2e/v3/events.cy.ts(1 hunks)tests/functional/cypress/e2e/v3/gerrits.cy.ts(1 hunks)tests/functional/cypress/e2e/v3/github-organizations.cy.ts(1 hunks)tests/functional/cypress/e2e/v3/github-repositories.cy.ts(1 hunks)tests/functional/cypress/e2e/v3/github.cy.ts(1 hunks)tests/functional/cypress/e2e/v3/health.cy.ts(1 hunks)tests/functional/cypress/e2e/v3/organization.cy.ts(1 hunks)tests/functional/cypress/e2e/v3/project.cy.ts(1 hunks)tests/functional/cypress/e2e/v3/signatures.cy.ts(1 hunks)tests/functional/cypress/e2e/v3/template.cy.ts(1 hunks)tests/functional/cypress/e2e/v3/users.cy.ts(1 hunks)tests/functional/cypress/e2e/v3/version.cy.ts(1 hunks)tests/functional/utils/run-single-test.sh(2 hunks)utils/get_dev_claims.sh(1 hunks)utils/get_prod_claims.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)utils/restore_dev_claims.sh(1 hunks)utils/set_dev_claims.sh(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (16)
tests/functional/cypress/e2e/v3/project.cy.ts (1)
tests/functional/cypress/support/commands.js (9)
getAPIBaseURL(236-253)local(237-237)bearerToken(288-288)getTokenKey(289-322)getXACLHeader(255-266)validate_200_Status(47-52)validate_404_Status(109-117)validate_401_Status(85-97)validate_expected_status(155-198)
tests/functional/cypress/e2e/v3/docs.cy.ts (1)
tests/functional/cypress/support/commands.js (6)
getAPIBaseURL(236-253)local(237-237)bearerToken(288-288)getTokenKey(289-322)validate_200_Status(47-52)validate_expected_status(155-198)
tests/functional/cypress/e2e/v3/cla-manager.cy.ts (1)
tests/functional/cypress/support/commands.js (9)
getAPIBaseURL(236-253)local(237-237)bearerToken(288-288)getTokenKey(289-322)getXACLHeader(255-266)validate_200_Status(47-52)validate_404_Status(109-117)validate_401_Status(85-97)validate_expected_status(155-198)
tests/functional/cypress/e2e/v3/version.cy.ts (1)
tests/functional/cypress/support/commands.js (6)
getAPIBaseURL(236-253)local(237-237)bearerToken(288-288)getTokenKey(289-322)validate_200_Status(47-52)validate_expected_status(155-198)
tests/functional/cypress/e2e/v3/github-organizations.cy.ts (1)
tests/functional/cypress/support/commands.js (9)
getAPIBaseURL(236-253)local(237-237)bearerToken(288-288)getTokenKey(289-322)getXACLHeader(255-266)validate_200_Status(47-52)validate_404_Status(109-117)validate_401_Status(85-97)validate_expected_status(155-198)
tests/functional/cypress/e2e/v3/gerrits.cy.ts (1)
tests/functional/cypress/support/commands.js (9)
getAPIBaseURL(236-253)local(237-237)bearerToken(288-288)getTokenKey(289-322)getXACLHeader(255-266)validate_200_Status(47-52)validate_404_Status(109-117)validate_401_Status(85-97)validate_expected_status(155-198)
tests/functional/cypress/e2e/v3/template.cy.ts (1)
tests/functional/cypress/support/commands.js (7)
getAPIBaseURL(236-253)local(237-237)bearerToken(288-288)getTokenKey(289-322)getXACLHeader(255-266)validate_200_Status(47-52)validate_expected_status(155-198)
tests/functional/cypress/e2e/v3/github-repositories.cy.ts (1)
tests/functional/cypress/support/commands.js (9)
getAPIBaseURL(236-253)local(237-237)bearerToken(288-288)getTokenKey(289-322)getXACLHeader(255-266)validate_200_Status(47-52)validate_404_Status(109-117)validate_401_Status(85-97)validate_expected_status(155-198)
tests/functional/cypress/e2e/v3/company.cy.ts (1)
tests/functional/cypress/support/commands.js (9)
getAPIBaseURL(236-253)local(237-237)bearerToken(288-288)getTokenKey(289-322)getXACLHeader(255-266)validate_200_Status(47-52)validate_404_Status(109-117)validate_401_Status(85-97)validate_expected_status(155-198)
tests/functional/cypress/e2e/v3/github.cy.ts (1)
tests/functional/cypress/support/commands.js (6)
getAPIBaseURL(236-253)local(237-237)bearerToken(288-288)getTokenKey(289-322)getXACLHeader(255-266)validate_expected_status(155-198)
tests/functional/cypress/e2e/v3/health.cy.ts (1)
tests/functional/cypress/support/commands.js (6)
getAPIBaseURL(236-253)local(237-237)bearerToken(288-288)getTokenKey(289-322)validate_200_Status(47-52)validate_expected_status(155-198)
tests/functional/cypress/e2e/v3/events.cy.ts (1)
tests/functional/cypress/support/commands.js (8)
getAPIBaseURL(236-253)local(237-237)bearerToken(288-288)getTokenKey(289-322)getXACLHeader(255-266)validate_200_Status(47-52)validate_401_Status(85-97)validate_expected_status(155-198)
tests/functional/cypress/e2e/v3/users.cy.ts (1)
tests/functional/cypress/support/commands.js (7)
getAPIBaseURL(236-253)local(237-237)bearerToken(288-288)getTokenKey(289-322)validate_200_Status(47-52)getXACLHeader(255-266)validate_expected_status(155-198)
tests/functional/cypress/e2e/v3/organization.cy.ts (1)
tests/functional/cypress/support/commands.js (7)
getAPIBaseURL(236-253)local(237-237)bearerToken(288-288)getTokenKey(289-322)getXACLHeader(255-266)validate_200_Status(47-52)validate_expected_status(155-198)
cla-backend/cla/routes.py (4)
cla-backend/cla/controllers/user.py (2)
get_or_create_user(489-515)get_user(28-72)cla-backend/cla/models/model_interfaces.py (1)
get_user_id(333-340)cla-backend/cla/models/dynamo_models.py (5)
get_user_id(1757-1758)get_user_id(4624-4625)get_user_id(4804-4805)get_user_id(5502-5503)get(3873-3882)cla-backend/cla/auth.py (1)
AuthError(26-32)
tests/functional/cypress/e2e/v3/signatures.cy.ts (1)
tests/functional/cypress/support/commands.js (9)
getAPIBaseURL(236-253)local(237-237)bearerToken(288-288)getTokenKey(289-322)getXACLHeader(255-266)validate_200_Status(47-52)validate_404_Status(109-117)validate_401_Status(85-97)validate_expected_status(155-198)
🪛 Biome (2.1.2)
tests/functional/cypress/e2e/v3/project.cy.ts
[error] 37-37: This property is later overwritten by an object member with the same name.
Overwritten with this property.
If an object property with the same name is defined multiple times (except when combining a getter with a setter), only the last definition makes it into the object and previous definitions are ignored.
Unsafe fix: Remove this property.
(lint/suspicious/noDuplicateObjectKeys)
[error] 137-137: This property is later overwritten by an object member with the same name.
Overwritten with this property.
If an object property with the same name is defined multiple times (except when combining a getter with a setter), only the last definition makes it into the object and previous definitions are ignored.
Unsafe fix: Remove this property.
(lint/suspicious/noDuplicateObjectKeys)
tests/functional/cypress/e2e/v3/events.cy.ts
[error] 36-36: This property is later overwritten by an object member with the same name.
Overwritten with this property.
If an object property with the same name is defined multiple times (except when combining a getter with a setter), only the last definition makes it into the object and previous definitions are ignored.
Unsafe fix: Remove this property.
(lint/suspicious/noDuplicateObjectKeys)
[error] 58-58: This property is later overwritten by an object member with the same name.
Overwritten with this property.
If an object property with the same name is defined multiple times (except when combining a getter with a setter), only the last definition makes it into the object and previous definitions are ignored.
Unsafe fix: Remove this property.
(lint/suspicious/noDuplicateObjectKeys)
[error] 82-82: This property is later overwritten by an object member with the same name.
Overwritten with this property.
If an object property with the same name is defined multiple times (except when combining a getter with a setter), only the last definition makes it into the object and previous definitions are ignored.
Unsafe fix: Remove this property.
(lint/suspicious/noDuplicateObjectKeys)
🪛 Ruff (0.14.2)
cla-backend/cla/routes.py
46-46: Use explicit conversion flag
Replace with conversion flag
(RUF010)
47-47: Avoid specifying long messages outside the exception class
(TRY003)
172-172: Unused function argument: auth_user
(ARG001)
199-199: Unused function argument: auth_user
(ARG001)
232-232: Unused function argument: auth_user
(ARG001)
497-497: Unused function argument: auth_user
(ARG001)
507-507: Unused function argument: auth_user
(ARG001)
660-660: Unused function argument: auth_user
(ARG001)
670-670: Unused function argument: auth_user
(ARG001)
680-680: Unused function argument: auth_user
(ARG001)
786-786: Unused function argument: auth_user
(ARG001)
815-815: Unused function argument: auth_user
(ARG001)
1050-1050: Unused function argument: auth_user
(ARG001)
1103-1103: Unused function argument: auth_user
(ARG001)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: cypress-functional
- GitHub Check: build-test-lint
🔇 Additional comments (13)
utils/set_dev_claims.sh (1)
6-8: Regional expansion to us-east-2 is well-structured and consistent.The new commands correctly replicate the us-east-1 pattern with consistent parameter names and values across both regions.
utils/get_dev_claims.sh (1)
6-9: Regional fetch operations correctly parallel us-east-1 pattern.The new us-east-2 commands consistently replicate the parameter fetch operations with identical names and query/output formatting.
utils/restore_dev_claims.sh (1)
7-10: Regional restore operations maintain consistent patterns with us-east-1.The new us-east-2 put/delete operations correctly replicate the us-east-1 pattern, ensuring symmetric claim restoration across both regions.
utils/get_prod_claims.sh (1)
6-8: Regional us-east-2 fetch commands well-formed (lines 6-8).The first three new us-east-2 commands correctly replicate the us-east-1 pattern with proper syntax and consistent parameter names.
tests/functional/cypress/e2e/v3/version.cy.ts (1)
1-7: Remove unused import; correct original claim about function name.The function
getXACLHeadersis correctly exported from support/commands.js and the import name is accurate. However, this import is unused in version.cy.ts and should be removed for cleanliness.-import { - validate_200_Status, - getTokenKey, - getAPIBaseURL, - getXACLHeaders, - validate_expected_status, -} from '../../support/commands'; +import { + validate_200_Status, + getTokenKey, + getAPIBaseURL, + validate_expected_status, +} from '../../support/commands';Likely an incorrect or invalid review comment.
tests/functional/cypress/e2e/v3/github-repositories.cy.ts (4)
29-56: LGTM: Appropriate handling of authenticated endpoint.The test correctly handles both successful (200) and not-found (404) responses for the authenticated GitHub repositories endpoint.
83-112: LGTM: Good flakiness detection pattern.The triple-iteration test is a solid approach to catch intermittent failures in the GitHub repositories endpoint.
114-172: LGTM: Comprehensive authentication enforcement testing.The tests appropriately verify that endpoints require authentication by attempting access without tokens and validating 401/404/405 responses.
174-271: LGTM: Comprehensive negative test coverage.The data-driven expected-failure tests provide thorough coverage of malformed requests with appropriate local/remote environment handling.
tests/functional/cypress/e2e/v3/organization.cy.ts (1)
75-190: LGTM: Thorough negative test coverage.The expected-failure tests comprehensively cover error scenarios including missing parameters, unsupported methods, and invalid paths with proper local/remote handling.
tests/functional/cypress/e2e/v3/github.cy.ts (3)
29-53: LGTM: Appropriate OAuth redirect handling.The test correctly handles OAuth flow expectations (302 redirect or error responses) and disables redirect following to capture the response.
74-95: LGTM: Flakiness detection for OAuth endpoint.The triple-iteration pattern is appropriate for detecting intermittent failures, though expectations could be tightened once API behavior is confirmed.
97-210: LGTM: Comprehensive negative test coverage.The expected-failure tests thoroughly cover error scenarios including missing required parameters, unsupported HTTP methods, and invalid endpoints with proper local/remote environment differentiation.
| cy.request({ | ||
| method: 'GET', | ||
| url: `${claEndpoint}organization/search?companyName=${encodeURIComponent(companyName)}`, | ||
| timeout: timeout, | ||
| failOnStatusCode: allowFail, | ||
| headers: getXACLHeader(), | ||
| auth: { bearer: bearerToken }, | ||
| // No auth required for this endpoint | ||
| }).then((response) => { | ||
| validate_200_Status(response); |
There was a problem hiding this comment.
“Without auth” test is sending a Bearer token — remove it.
This masks authorization regressions.
headers: getXACLHeader(),
- auth: { bearer: bearerToken },
// No auth required for this endpoint📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| cy.request({ | |
| method: 'GET', | |
| url: `${claEndpoint}organization/search?companyName=${encodeURIComponent(companyName)}`, | |
| timeout: timeout, | |
| failOnStatusCode: allowFail, | |
| headers: getXACLHeader(), | |
| auth: { bearer: bearerToken }, | |
| // No auth required for this endpoint | |
| }).then((response) => { | |
| validate_200_Status(response); | |
| cy.request({ | |
| method: 'GET', | |
| url: `${claEndpoint}organization/search?companyName=${encodeURIComponent(companyName)}`, | |
| timeout: timeout, | |
| failOnStatusCode: allowFail, | |
| headers: getXACLHeader(), | |
| // No auth required for this endpoint | |
| }).then((response) => { | |
| validate_200_Status(response); |
🤖 Prompt for AI Agents
In tests/functional/cypress/e2e/v3/company.cy.ts around lines 81 to 90 the
"Without auth" test is still sending an auth: { bearer: bearerToken } property
which defeats the test; remove the auth option (and any bearerToken usage) from
the cy.request call so the request is sent unauthenticated, keep headers and
other options intact, and run the test to ensure it fails or passes based on
actual unauthenticated behavior.
| .request({ | ||
| method: 'GET', | ||
| url: `${claEndpoint}organization/search?companyName=${encodeURIComponent(companyName)}`, | ||
| timeout: timeout, | ||
| failOnStatusCode: allowFail, | ||
| headers: getXACLHeader(), | ||
| auth: { bearer: bearerToken }, | ||
| }) |
There was a problem hiding this comment.
Flakiness check for public endpoint also sends auth — remove it.
Keep the request unauthenticated to truly exercise the public path.
headers: getXACLHeader(),
- auth: { bearer: bearerToken },📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| .request({ | |
| method: 'GET', | |
| url: `${claEndpoint}organization/search?companyName=${encodeURIComponent(companyName)}`, | |
| timeout: timeout, | |
| failOnStatusCode: allowFail, | |
| headers: getXACLHeader(), | |
| auth: { bearer: bearerToken }, | |
| }) | |
| .request({ | |
| method: 'GET', | |
| url: `${claEndpoint}organization/search?companyName=${encodeURIComponent(companyName)}`, | |
| timeout: timeout, | |
| failOnStatusCode: allowFail, | |
| headers: getXACLHeader(), | |
| }) |
🤖 Prompt for AI Agents
In tests/functional/cypress/e2e/v3/company.cy.ts around lines 157 to 164 the GET
request to the public search endpoint is being sent with an auth bearer token,
which defeats the purpose of exercising the unauthenticated public path; remove
the auth: { bearer: bearerToken } property (and any other authorization-specific
headers if present) from the request options so the request is truly
unauthenticated, leaving method, url, timeout, failOnStatusCode, and non-auth
headers intact.
| validate_200_Status, | ||
| getTokenKey, | ||
| getAPIBaseURL, | ||
| getXACLHeaders, |
There was a problem hiding this comment.
Fix import: module has no exported member 'getXACLHeaders'.
Support exports getXACLHeader (singular). This import will fail TypeScript compile. Remove it (unused here) or rename.
- getXACLHeaders,📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| getXACLHeaders, |
🤖 Prompt for AI Agents
In tests/functional/cypress/e2e/v3/docs.cy.ts around line 5, the import
references getXACLHeaders (plural) which does not exist; the correct export is
getXACLHeader (singular) or the import is unused. Fix by removing the invalid
getXACLHeaders import or rename it to getXACLHeader to match the module export,
and update any usages accordingly so TypeScript compiles.
| headers: getXACLHeader(), | ||
| auth: { bearer: bearerToken }, | ||
| auth: { | ||
| bearer: bearerToken, | ||
| }, |
There was a problem hiding this comment.
Remove duplicate object keys: later auth overwrites earlier.
Duplicate keys in request options are ignored except the last one; keep a single auth property.
headers: getXACLHeader(),
- auth: { bearer: bearerToken },
auth: {
bearer: bearerToken,
},Apply the same fix at the other two call sites.
Also applies to: 58-61, 82-85
🧰 Tools
🪛 Biome (2.1.2)
[error] 36-36: This property is later overwritten by an object member with the same name.
Overwritten with this property.
If an object property with the same name is defined multiple times (except when combining a getter with a setter), only the last definition makes it into the object and previous definitions are ignored.
Unsafe fix: Remove this property.
(lint/suspicious/noDuplicateObjectKeys)
🤖 Prompt for AI Agents
In tests/functional/cypress/e2e/v3/events.cy.ts around lines 35-39, 58-61 and
82-85 there are duplicate request option keys (two auth properties), causing the
earlier auth to be overwritten; remove the duplicate so each request has a
single auth object (auth: { bearer: bearerToken }) and ensure headers and other
options remain intact; apply the same single-auth fix at the two other call
sites mentioned.
| validate_200_Status, | ||
| getTokenKey, | ||
| getAPIBaseURL, | ||
| getXACLHeaders, |
There was a problem hiding this comment.
Fix import: module has no exported member 'getXACLHeaders'.
Support defines getXACLHeader (singular). This will break type-check. Remove (unused) or correct name.
- getXACLHeaders,🤖 Prompt for AI Agents
In tests/functional/cypress/e2e/v3/health.cy.ts around line 5 the import
references getXACLHeaders which does not exist; replace it with the correct
exported name getXACLHeader (singular) or remove the import entirely if it's
unused. Update the import list to reference getXACLHeader and adjust any usage
sites to the singular name so TypeScript type-checks correctly.
| .request({ | ||
| method: 'GET', | ||
| url: `${claEndpoint}signatures/project/${projectID}/company/${companyID}`, | ||
| timeout: timeout, | ||
| failOnStatusCode: allowFail, | ||
| headers: getXACLHeader(), | ||
| auth: { bearer: bearerToken }, | ||
| }) |
There was a problem hiding this comment.
Flakiness check for public endpoint includes auth — remove it.
failOnStatusCode: allowFail,
headers: getXACLHeader(),
- auth: { bearer: bearerToken },🤖 Prompt for AI Agents
In tests/functional/cypress/e2e/v3/signatures.cy.ts around lines 131 to 138 the
GET request to the public signatures endpoint wrongly includes authentication;
remove the auth property and any Authorization/X-ACL auth headers so the request
is truly public (keep timeout and failOnStatusCode as-is), i.e. stop sending
bearerToken and strip any headers that inject auth for this call.
| .request({ | ||
| method: 'GET', | ||
| url: `${claEndpoint}template`, | ||
| timeout: timeout, | ||
| failOnStatusCode: allowFail, | ||
| headers: getXACLHeader(), | ||
| auth: { bearer: bearerToken }, | ||
| }) | ||
| .then((response) => { |
There was a problem hiding this comment.
Flakiness check for public endpoint includes auth — remove it.
failOnStatusCode: allowFail,
headers: getXACLHeader(),
- auth: { bearer: bearerToken },📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| .request({ | |
| method: 'GET', | |
| url: `${claEndpoint}template`, | |
| timeout: timeout, | |
| failOnStatusCode: allowFail, | |
| headers: getXACLHeader(), | |
| auth: { bearer: bearerToken }, | |
| }) | |
| .then((response) => { | |
| .request({ | |
| method: 'GET', | |
| url: `${claEndpoint}template`, | |
| timeout: timeout, | |
| failOnStatusCode: allowFail, | |
| headers: getXACLHeader(), | |
| }) | |
| .then((response) => { |
🤖 Prompt for AI Agents
In tests/functional/cypress/e2e/v3/template.cy.ts around lines 52 to 60, the
flakiness check is calling the public `${claEndpoint}template` endpoint while
still supplying authentication (bearer token) and ACL headers; remove the auth:
{ bearer: bearerToken } and any getXACLHeader() usage so the request is truly
unauthenticated/public, keeping timeout and failOnStatusCode/allowFail as-is.
| if [ -z "$TOKEN" ] | ||
| then | ||
| # source ./auth0_token.secret | ||
| TOKEN="$(cat ./auth0.token.secret)" | ||
| fi | ||
|
|
||
| if [ -z "$TOKEN" ] | ||
| then | ||
| echo "$0: TOKEN not specified and unable to obtain one" | ||
| exit 5 | ||
| fi | ||
|
|
||
| if [ -z "$XACL" ] | ||
| then | ||
| XACL="$(cat ./x-acl.secret)" | ||
| fi | ||
|
|
||
| if [ -z "$XACL" ] | ||
| then | ||
| echo "$0: XACL not specified and unable to obtain one" | ||
| exit 6 | ||
| fi |
There was a problem hiding this comment.
Inconsistent exit codes across related scripts.
This script exits with codes 5 and 6 for missing TOKEN and XACL, but utils/request_employee_signature_py_post.sh uses codes 6 and 7 for the same errors, while utils/request_individual_signature_py_post.sh uses codes 5 and 6 (matching this script). Standardize exit codes across all three scripts to enable consistent error handling by callers.
🤖 Prompt for AI Agents
In utils/request_corporate_signature_py_post.sh around lines 43 to 64, the
script currently exits with codes 5 (missing TOKEN) and 6 (missing XACL) which
is inconsistent with utils/request_employee_signature_py_post.sh that uses 6 and
7; update this file so the TOKEN missing exit becomes 6 and the XACL missing
exit becomes 7 to match the employee script (ensure both the exit <n> values and
any associated messages reflect the new codes).
| if [ -z "$TOKEN" ] | ||
| then | ||
| # source ./auth0_token.secret | ||
| TOKEN="$(cat ./auth0.token.secret)" | ||
| fi | ||
|
|
||
| if [ -z "$TOKEN" ] | ||
| then | ||
| echo "$0: TOKEN not specified and unable to obtain one" | ||
| exit 6 | ||
| fi | ||
|
|
||
| if [ -z "$XACL" ] | ||
| then | ||
| XACL="$(cat ./x-acl.secret)" | ||
| fi | ||
|
|
||
| if [ -z "$XACL" ] | ||
| then | ||
| echo "$0: XACL not specified and unable to obtain one" | ||
| exit 7 | ||
| fi |
There was a problem hiding this comment.
Inconsistent exit codes across related scripts.
This script exits with codes 6 and 7 for missing TOKEN and XACL, but utils/request_corporate_signature_py_post.sh and utils/request_individual_signature_py_post.sh use codes 5 and 6 for the same errors. Standardize exit codes across all three scripts to enable consistent error handling by callers.
🤖 Prompt for AI Agents
In utils/request_employee_signature_py_post.sh around lines 48 to 69, the script
currently exits with codes 6 (missing TOKEN) and 7 (missing XACL) which is
inconsistent with utils/request_corporate_signature_py_post.sh and
utils/request_individual_signature_py_post.sh; update the exit codes to match
those scripts by changing the TOKEN missing exit from 6 to 5 and the XACL
missing exit from 7 to 6 so callers receive consistent error codes.
| if [ -z "$TOKEN" ] | ||
| then | ||
| # source ./auth0_token.secret | ||
| TOKEN="$(cat ./auth0.token.secret)" | ||
| fi | ||
|
|
||
| if [ -z "$TOKEN" ] | ||
| then | ||
| echo "$0: TOKEN not specified and unable to obtain one" | ||
| exit 5 | ||
| fi | ||
|
|
||
| if [ -z "$XACL" ] | ||
| then | ||
| XACL="$(cat ./x-acl.secret)" | ||
| fi | ||
|
|
||
| if [ -z "$XACL" ] | ||
| then | ||
| echo "$0: XACL not specified and unable to obtain one" | ||
| exit 6 | ||
| fi |
There was a problem hiding this comment.
Inconsistent exit codes across related scripts.
This script exits with codes 5 and 6 for missing TOKEN and XACL (matching utils/request_corporate_signature_py_post.sh), but utils/request_employee_signature_py_post.sh uses codes 6 and 7. Standardize exit codes across all three scripts to enable consistent error handling by callers.
🤖 Prompt for AI Agents
In utils/request_individual_signature_py_post.sh around lines 45 to 66, the
script uses exit codes 5 and 6 for missing TOKEN and XACL which is inconsistent
with utils/request_employee_signature_py_post.sh (which uses 6 and 7);
standardize exit codes across all three scripts by choosing a single convention
(pick 6 for missing TOKEN and 7 for missing XACL), update the two exit
statements in this file to exit 6 and 7 respectively, and then update the other
two scripts to use the same codes so callers can rely on consistent error
numbers.
|
It had a wrong base, closing. |
WIPcc @mlehotskylf - ETA tomorrow or next week, V3 tests are a bkit problematic and almost no v3 APIs are used anymre, even so that it looks like its security handling (auth) may be broken for some cases...Signed-off-by: Lukasz Gryglicki [email protected]
Assisted by OpenAI
Assisted by GitHub Copilot