Conversation
Signed-off-by: Lukasz Gryglicki <lgryglicki@cncf.io> Assisted by [OpenAI](https://platform.openai.com/) Assisted by [GitHub Copilot](https://github.com/features/copilot)
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughRefreshes and conditions for using LF-backed emails across Go and Python services: adds server-side sync of stored lf_email from auth tokens for existing users, propagates an allow-LF-email flag through signing/default-value flows, tightens user matching (raising on ambiguous matches), and updates call sites to validate resolved emails. Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant Server
participant Auth as AuthToken
participant UserSvc as User Controller
participant DB
Client->>Server: Request (authenticated)
Server->>Auth: Read claims (sub, email, lf_email)
Server->>UserSvc: get_or_create_user(auth_user)
UserSvc->>DB: query users by lf_username / email
alt single match
UserSvc->>Auth: compare stored lf_email with token.lf_email
alt different
UserSvc->>DB: update user.lf_email, set date_modified
end
UserSvc-->>Server: return resolved user
else multiple matches
UserSvc-->>Server: raise AmbiguousUserMatchError
end
Server-->>Client: respond (user or 409 on ambiguous)
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes 🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
📝 Coding Plan
Comment |
There was a problem hiding this comment.
Pull request overview
This PR introduces provider-specific handling for user email selection, primarily to prevent using lf_email outside of the Gerrit flow while still allowing it for Gerrit-based signing.
Changes:
- Add an
allow_lf_emailflag to user email resolution to control whetherlf_emailmay be used. - Update DocuSign signing flows (Python + Go) to allow
lf_emailonly when the return URL type is Gerrit. - Sync
lf_emailfrom the current auth token into existing user records on login/token parsing.
Reviewed changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
cla-backend/cla/models/dynamo_models.py |
Adds allow_lf_email option to User.get_user_email() to gate use of lf_email. |
cla-backend/cla/models/docusign_models.py |
Propagates provider-aware email selection into DocuSign default values and signing URL population. |
cla-backend/cla/controllers/user.py |
Updates an existing user’s lf_email from the auth token when it changes. |
cla-backend/cla/controllers/signing.py |
For GitLab, forces selection from user_emails (disallowing lf_email) before requesting signature. |
cla-backend-go/v2/sign/service.go |
Adds allowLFEmail plumbing and enforces Gerrit-only lf_email use in Go signing flows. |
cla-backend-go/cmd/server.go |
Syncs an existing user’s lf_email from the auth token when a matching user is found. |
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (2)
cla-backend-go/v2/sign/service.go (1)
2064-2066: Redundant call togetUserEmailon lines 2064-2065.The function
getUserEmailis called twice with the same arguments - once to check if the result is non-empty and again to assign the value. Consider storing the result in a variable to avoid duplicate computation.♻️ Suggested refactor to eliminate duplicate call
- if getUserEmail(userModel, preferredEmail, allowLFEmail) != "" { - userSignDetails.userSignatureEmail = getUserEmail(userModel, preferredEmail, allowLFEmail) + if email := getUserEmail(userModel, preferredEmail, allowLFEmail); email != "" { + userSignDetails.userSignatureEmail = email }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cla-backend-go/v2/sign/service.go` around lines 2064 - 2066, Store the result of getUserEmail(userModel, preferredEmail, allowLFEmail) in a local variable, check that the variable is non-empty, and then assign it to userSignDetails.userSignatureEmail instead of calling getUserEmail twice; update the code around the existing getUserEmail calls to use that temporary variable (retain the same semantics and variable names userModel, preferredEmail, allowLFEmail, and userSignDetails.userSignatureEmail).cla-backend/cla/models/docusign_models.py (1)
2414-2415: Use explicit optional typing forpreferred_email.Line 2414 uses implicit optional (
str = None). Prefer explicit optional typing to satisfy Ruff RUF013 and keep the signature precise.Proposed fix
-def create_default_individual_values(user: User, preferred_email: str = None, +def create_default_individual_values(user: User, preferred_email: Optional[str] = None, allow_lf_email: bool = True) -> Dict[str, Any]:🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cla-backend/cla/models/docusign_models.py` around lines 2414 - 2415, The function signature for create_default_individual_values uses an implicit optional type for preferred_email (str = None); update the signature to use explicit Optional typing (preferred_email: Optional[str] = None) and ensure Optional is imported from typing (or typing_extensions) in this module so the type checker and Ruff RUF013 are satisfied.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@cla-backend/cla/controllers/signing.py`:
- Around line 52-54: The check for primary_user_email currently only rejects
None but allows empty or whitespace-only strings; update the GitLab guard where
primary_user_email is assigned (from user.get_user_email) to treat
empty/whitespace as invalid by trimming and verifying a non-empty string (e.g.,
using str.strip()) before proceeding, and return the same error structure
{'errors': {'user_id': 'no gitlab user_emails found'}} when the email is blank
or None; ensure you update any conditional that currently compares to None to
perform this trimmed emptiness check instead.
In `@cla-backend/cla/models/docusign_models.py`:
- Around line 180-182: The flow may proceed with a missing provider-eligible
email causing a bad fallback when calling DocuSign; after computing
allow_lf_email and calling create_default_individual_values (i.e., after the
default_cla_values assignment), add a guard that checks
default_cla_values["email"] (or default_cla_values.get("email")) and if it's
falsy raise/return a clear error (e.g., raise a ValueError or return an
appropriate HTTP error) so the code fails fast instead of sending an invalid
"Unknown" email to DocuSign; ensure this check respects allow_lf_email semantics
(i.e., when allow_lf_email is False and email is None trigger the error).
In `@cla-backend/cla/models/dynamo_models.py`:
- Line 1774: The User interface in model_interfaces.py must be updated so its
get_user_email method signature matches the new implementation in
dynamo_models.py; change the abstract/interface declaration of
get_user_email(self) to get_user_email(self, preferred_email=None,
allow_lf_email=True) and update its docstring/type hints (return type and
parameter descriptions) to reflect the optional preferred_email and boolean
allow_lf_email semantics so alternate User implementations and mocks conform to
the new contract (also search for other concrete classes referencing
User.get_user_email to ensure they accept the new parameters).
---
Nitpick comments:
In `@cla-backend-go/v2/sign/service.go`:
- Around line 2064-2066: Store the result of getUserEmail(userModel,
preferredEmail, allowLFEmail) in a local variable, check that the variable is
non-empty, and then assign it to userSignDetails.userSignatureEmail instead of
calling getUserEmail twice; update the code around the existing getUserEmail
calls to use that temporary variable (retain the same semantics and variable
names userModel, preferredEmail, allowLFEmail, and
userSignDetails.userSignatureEmail).
In `@cla-backend/cla/models/docusign_models.py`:
- Around line 2414-2415: The function signature for
create_default_individual_values uses an implicit optional type for
preferred_email (str = None); update the signature to use explicit Optional
typing (preferred_email: Optional[str] = None) and ensure Optional is imported
from typing (or typing_extensions) in this module so the type checker and Ruff
RUF013 are satisfied.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: b0ab5125-cb96-4b72-ae6e-9847e4925c63
📒 Files selected for processing (6)
cla-backend-go/cmd/server.gocla-backend-go/v2/sign/service.gocla-backend/cla/controllers/signing.pycla-backend/cla/controllers/user.pycla-backend/cla/models/docusign_models.pycla-backend/cla/models/dynamo_models.py
…vider Assisted by [OpenAI](https://platform.openai.com/) Assisted by [GitHub Copilot](https://github.com/features/copilot)
Signed-off-by: Lukasz Gryglicki <lgryglicki@cncf.io> 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: 2
🧹 Nitpick comments (2)
cla-backend-go/v2/sign/service.go (2)
2076-2078: Redundant duplicate call togetUserEmail.The function
getUserEmailis called twice with identical arguments on lines 2076-2077. Consider calling it once and reusing the result.♻️ Proposed refactor to eliminate duplicate call
- if getUserEmail(userModel, preferredEmail, allowLFEmail) != "" { - userSignDetails.userSignatureEmail = getUserEmail(userModel, preferredEmail, allowLFEmail) + if email := getUserEmail(userModel, preferredEmail, allowLFEmail); email != "" { + userSignDetails.userSignatureEmail = email }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cla-backend-go/v2/sign/service.go` around lines 2076 - 2078, The code calls getUserEmail(userModel, preferredEmail, allowLFEmail) twice; call it once, store the result in a local variable (e.g., email := getUserEmail(...)) and then use that variable to set userSignDetails.userSignatureEmail and for any other checks to avoid the duplicate computation and potential side effects.
2238-2241: Minor typo in log field.The function name in the logrus field at line 2240 has a typo: "createDefaultIndiviualValues" should be "createDefaultIndividualValues".
✏️ Fix typo
func (s *service) createDefaultIndividualValues(user *v1Models.User, preferredEmail string, allowLFEmail bool) map[string]interface{} { f := logrus.Fields{ - "functionName": "sign.createDefaultIndiviualValues", + "functionName": "sign.createDefaultIndividualValues", }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cla-backend-go/v2/sign/service.go` around lines 2238 - 2241, The log field value in the logrus.Fields within the createDefaultIndividualValues function contains a typo ("createDefaultIndiviualValues"); update the "functionName" field string to the correct function name "createDefaultIndividualValues" so the logged functionName matches the actual function (look for the logrus.Fields declaration inside service.createDefaultIndividualValues).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@cla-backend/cla/controllers/user.py`:
- Around line 515-535: The current logic falls back to users[0] when multiple
matching users exist but no lf_sub/lf_email match is found, which can bind
sessions to the wrong account; change the behavior so that when len(users) > 1
and neither the lf_sub nor the email matched (i.e., can_sync_lf_email is False
after the loops), the function does not return users[0] but instead fails
closed—either raise a clear exception (e.g., ValueError or custom
AmbiguousUserError) or return None and ensure callers of this function handle
that case; update any callers to handle the new error/None return and keep
existing email-sync code (user.set_lf_email / user.save) for the accepted
single-match path.
In `@cla-backend/cla/models/docusign_models.py`:
- Around line 1362-1365: The debug log in cla.log.debug that currently includes
resolved_user_email is exposing PII; update the logging in the block that
constructs the message (the line referencing fn, sig_type, user.get_user_id(),
user.get_user_name(), resolved_user_email) to avoid raw emails—either replace
resolved_user_email with a masked value (e.g., mask_email(resolved_user_email)
using a small helper that keeps only domain or first char) or with a
boolean/presence flag like has_email=(resolved_user_email is not None) and log
that instead, ensuring cla.log.debug no longer outputs the full email.
---
Nitpick comments:
In `@cla-backend-go/v2/sign/service.go`:
- Around line 2076-2078: The code calls getUserEmail(userModel, preferredEmail,
allowLFEmail) twice; call it once, store the result in a local variable (e.g.,
email := getUserEmail(...)) and then use that variable to set
userSignDetails.userSignatureEmail and for any other checks to avoid the
duplicate computation and potential side effects.
- Around line 2238-2241: The log field value in the logrus.Fields within the
createDefaultIndividualValues function contains a typo
("createDefaultIndiviualValues"); update the "functionName" field string to the
correct function name "createDefaultIndividualValues" so the logged functionName
matches the actual function (look for the logrus.Fields declaration inside
service.createDefaultIndividualValues).
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: e876ab43-6d9c-435e-b616-b030725b6ee9
📒 Files selected for processing (6)
cla-backend-go/v2/sign/service.gocla-backend/cla/controllers/signing.pycla-backend/cla/controllers/user.pycla-backend/cla/models/docusign_models.pycla-backend/cla/models/model_interfaces.pycla-backend/cla/tests/unit/test_dynamo_models.py
Signed-off-by: Lukasz Gryglicki <lgryglicki@cncf.io> 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: 1
♻️ Duplicate comments (1)
cla-backend/cla/models/docusign_models.py (1)
1359-1384:⚠️ Potential issue | 🟠 MajorEnforce
has_resolved_user_emailinstead of only logging it.After Lines 1359-1362, a missing/blank
resolved_user_emailonly changes the log output. The code then keepsuser_signature_emailas"Unknown"or""and still reaches the DocuSign signer construction at Lines 1495-1501.request_individual_signature_gerrit()still reaches this path without the GitHub/GitLab guard, so this can fail at the provider boundary instead of returning a clear user error.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cla-backend/cla/models/docusign_models.py` around lines 1359 - 1384, The code currently only logs when resolved_user_email is blank but proceeds with user_signature_email possibly unset; update the block after computing has_resolved_user_email so that if has_resolved_user_email is False you log a clear warning (include fn, sig_type and signature.get_signature_reference_id()) and return early (same control flow as the DoesNotExist/Exception handlers) to prevent constructing a DocuSign signer with an unknown email; modify the logic around resolved_user_email / user_signature_email in the same scope where user.get_user_email(...) and has_resolved_user_email are computed (affecting request_individual_signature_gerrit() path) so the function exits on missing user email instead of continuing.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@cla-backend/cla/models/docusign_models.py`:
- Around line 180-191: Normalize return_url_type once before calling .lower()
and reuse it: compute a single normalized_return = (return_url_type or
'').lower(), then set allow_lf_email = normalized_return == 'gerrit' (instead of
calling return_url_type.lower() directly) and use provider_type =
normalized_return for the later provider checks and assignments (the variables
referenced are return_url_type, allow_lf_email, provider_type, and
default_cla_values in the function handling individual signatures).
---
Duplicate comments:
In `@cla-backend/cla/models/docusign_models.py`:
- Around line 1359-1384: The code currently only logs when resolved_user_email
is blank but proceeds with user_signature_email possibly unset; update the block
after computing has_resolved_user_email so that if has_resolved_user_email is
False you log a clear warning (include fn, sig_type and
signature.get_signature_reference_id()) and return early (same control flow as
the DoesNotExist/Exception handlers) to prevent constructing a DocuSign signer
with an unknown email; modify the logic around resolved_user_email /
user_signature_email in the same scope where user.get_user_email(...) and
has_resolved_user_email are computed (affecting
request_individual_signature_gerrit() path) so the function exits on missing
user email instead of continuing.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: dd6c79ea-560c-4b9e-9e72-e6f3e92733d8
📒 Files selected for processing (5)
cla-backend-go/v2/sign/service.gocla-backend/cla/controllers/company.pycla-backend/cla/controllers/user.pycla-backend/cla/models/docusign_models.pycla-backend/cla/routes.py
🚧 Files skipped from review as they are similar to previous changes (1)
- cla-backend-go/v2/sign/service.go
Signed-off-by: Lukasz Gryglicki <lgryglicki@cncf.io> Assisted by [OpenAI](https://platform.openai.com/) Assisted by [GitHub Copilot](https://github.com/features/copilot)
There was a problem hiding this comment.
♻️ Duplicate comments (1)
cla-backend/cla/models/docusign_models.py (1)
180-182:⚠️ Potential issue | 🔴 CriticalGuard provider type before callback/ACL branching.
Line 181 enables a Gerrit path (
allow_lf_email=True), but Lines 200-203 and 275-278 only initializecallback_url/aclfor GitHub/GitLab. Forprovider_type == "gerrit"(or empty), Line 279 readsaclbefore assignment and will raiseUnboundLocalError.Proposed fix
- normalized_return = (return_url_type or '').lower() + normalized_return = (return_url_type or 'github').strip().lower() allow_lf_email = normalized_return == "gerrit" default_cla_values = create_default_individual_values(user, preferred_email=preferred_email, allow_lf_email=allow_lf_email) provider_type = normalized_return + if provider_type not in ("github", "gitlab", "gerrit"): + return {'errors': {'return_url_type': f'unsupported provider "{return_url_type}"'}} + resolved_email = default_cla_values.get('email') if provider_type in ('github', 'gitlab'): resolved_email = resolved_email.strip() if isinstance(resolved_email, str) else None if not resolved_email: return {'errors': {'user_id': f'no {provider_type} user_emails found'}} default_cla_values['email'] = resolved_email # Generate signature callback url cla.log.debug('Individual Signature - get active signature metadata') signature_metadata = cla.utils.get_active_signature_metadata(user_id) cla.log.debug('Individual Signature - get active signature metadata: {}'.format(signature_metadata)) cla.log.debug('Individual Signature - get individual signature callback url') if provider_type == "github": callback_url = cla.utils.get_individual_signature_callback_url(user_id, signature_metadata) elif provider_type == "gitlab": callback_url = cla.utils.get_individual_signature_callback_url_gitlab(user_id, signature_metadata) + elif provider_type == "gerrit": + callback_url = self._generate_individual_signature_callback_url_gerrit(user_id) @@ - if provider_type == "github": - acl = user.get_user_github_id() - elif provider_type == "gitlab": - acl = user.get_user_gitlab_id() + if provider_type == "github": + acl = user.get_user_github_id() + signature.set_signature_acl(f'github:{acl}') + elif provider_type == "gitlab": + acl = user.get_user_gitlab_id() + signature.set_signature_acl(f'gitlab:{acl}') + else: # gerrit + acl = user.get_lf_username() + signature.set_signature_acl(acl) + if not acl: + return {'errors': {'user_id': f'missing ACL identity for provider "{provider_type}"'}} cla.log.debug('Individual Signature - setting ACL using user {} id: {}'.format(return_url_type, acl)) - signature.set_signature_acl('{}:{}'.format(provider_type, acl))Also applies to: 186-203, 275-280
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cla-backend/cla/models/docusign_models.py` around lines 180 - 182, The Gerrit path can leave acl and callback_url uninitialized causing UnboundLocalError; in the function that computes normalized_return/allow_lf_email (using normalized_return, allow_lf_email, create_default_individual_values) and later branches on provider_type to set callback_url and acl, add a guard for provider_type == "gerrit" (or a default else) so acl and callback_url are always assigned before use — either initialize acl and callback_url to sensible defaults near where normalized_return/allow_lf_email are computed or add explicit branches for "gerrit"/empty provider_type that set acl/callback_url before any reads (ensure the same fix at the other affected blocks around the existing provider_type branching at lines referenced).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@cla-backend/cla/models/docusign_models.py`:
- Around line 180-182: The Gerrit path can leave acl and callback_url
uninitialized causing UnboundLocalError; in the function that computes
normalized_return/allow_lf_email (using normalized_return, allow_lf_email,
create_default_individual_values) and later branches on provider_type to set
callback_url and acl, add a guard for provider_type == "gerrit" (or a default
else) so acl and callback_url are always assigned before use — either initialize
acl and callback_url to sensible defaults near where
normalized_return/allow_lf_email are computed or add explicit branches for
"gerrit"/empty provider_type that set acl/callback_url before any reads (ensure
the same fix at the other affected blocks around the existing provider_type
branching at lines referenced).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: a7639984-63c6-442c-b1c9-68e718a6d7e1
📒 Files selected for processing (1)
cla-backend/cla/models/docusign_models.py
Signed-off-by: Lukasz Gryglicki <lgryglicki@cncf.io> Assisted by [OpenAI](https://platform.openai.com/) Assisted by [GitHub Copilot](https://github.com/features/copilot)
There was a problem hiding this comment.
🧹 Nitpick comments (1)
utils/skip_cla_entry.sh (1)
131-135: Consider using jq helpers for JSON generation in existing modes.The new
add-key-item/delete-key-itemmodes usejq-based helpers (aws_key_json,aws_attr_names_json,aws_attr_value_string_json) which properly escape special characters. The existing modes (put-item,add-key) use string interpolation with only backslash escaping viased, which could produce malformed JSON if inputs contain double quotes or other JSON-special characters.For consistency and safety, consider refactoring the existing modes to use the same
jqhelpers.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@utils/skip_cla_entry.sh` around lines 131 - 135, The update-item command builds JSON via fragile string interpolation (the CMD variable using --key and --expression-attribute-values with embedded ${repo} and ${pat}), which can break on quotes or special chars; refactor the `update-item` / existing `add-key`/`put-item` code paths to use the existing jq helper functions (`aws_key_json`, `aws_attr_names_json`, `aws_attr_value_string_json`) to produce --key and --expression-attribute-values JSON safely, replacing the inline string construction in CMD while preserving use of STAGE, REGION, repo and pat variables.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@utils/skip_cla_entry.sh`:
- Around line 131-135: The update-item command builds JSON via fragile string
interpolation (the CMD variable using --key and --expression-attribute-values
with embedded ${repo} and ${pat}), which can break on quotes or special chars;
refactor the `update-item` / existing `add-key`/`put-item` code paths to use the
existing jq helper functions (`aws_key_json`, `aws_attr_names_json`,
`aws_attr_value_string_json`) to produce --key and --expression-attribute-values
JSON safely, replacing the inline string construction in CMD while preserving
use of STAGE, REGION, repo and pat variables.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 816bb9ac-7559-4b94-b8b0-1864510ee155
📒 Files selected for processing (1)
utils/skip_cla_entry.sh
…vider Assisted by [OpenAI](https://platform.openai.com/) Assisted by [GitHub Copilot](https://github.com/features/copilot)
…vider Assisted by [OpenAI](https://platform.openai.com/) Assisted by [GitHub Copilot](https://github.com/features/copilot)
initial work on this slack request.
Currently
wipas specs aren't yet clear.Signed-off-by: Lukasz Gryglicki lgryglicki@cncf.io
Assisted by OpenAI
Assisted by GitHub Copilot