Record the GitHub account a contributor signs a CLA as - #5148
Record the GitHub account a contributor signs a CLA as#5148ahmedomosanya wants to merge 1 commit into
Conversation
|
Important Review skippedDraft detected. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
Comment |
There was a problem hiding this comment.
Pull request overview
Adds the v4 signing-identity endpoint supporting LFX Self Serve’s GitHub account-selection hand-off.
Changes:
- Adds account-to-user resolution, persistence, and refusal handling.
- Adds Swagger models, handler wiring, middleware identity propagation, and documentation.
- Adds service and handler tests.
Reviewed changes
Copilot reviewed 13 out of 13 changed files in this pull request and generated 7 comments.
Show a summary per file
| File | Description |
|---|---|
docs/MY_CLAS_API.md |
Documents the endpoint contract and resolution rules. |
cla-backend-go/v2/my_clas/signing_identity.go |
Implements identity binding. |
cla-backend-go/v2/my_clas/signing_identity_test.go |
Tests binding outcomes and refusals. |
cla-backend-go/v2/my_clas/service.go |
Extends the service interface and dependencies. |
cla-backend-go/v2/my_clas/service_test.go |
Extends repository test doubles. |
cla-backend-go/v2/my_clas/repository.go |
Adds consistent user-record reads. |
cla-backend-go/v2/my_clas/handlers.go |
Adds HTTP handling and error mapping. |
cla-backend-go/v2/my_clas/handlers_test.go |
Tests handler validation and responses. |
cla-backend-go/user/user.go |
Propagates verified callers through context. |
cla-backend-go/swagger/common/signing-identity.yaml |
Defines the response model. |
cla-backend-go/swagger/common/signing-identity-request.yaml |
Defines the request model. |
cla-backend-go/swagger/cla.v2.yaml |
Registers the v4 API operation. |
cla-backend-go/cmd/server.go |
Wires dependencies and caller context. |
Suppressed comments (1)
cla-backend-go/v2/my_clas/signing_identity_test.go:414
- “Unconsistent” should be “inconsistent.”
// The write succeeded; only the writer's own unconsistent read-back of it missed. Refusing
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| if selected.Username != "" && record.GithubUsername != selected.Username { | ||
| updated, err := s.usersWriter.UpdateUser(record.UserID, map[string]interface{}{ | ||
| "user_github_username": selected.Username, | ||
| }) |
There was a problem hiding this comment.
Fixed in 687479756. Confirmed against the base table before the handle write rather than after: confirmStillHolds does a strongly consistent read on the resolved record and refuses recorded_mismatch unless it still holds the submitted account, so the write no longer happens at all on a stale image.
Worth saying why this was more than a display defect, since the code comment called the handle display-only: the stored handle is an authorization input. UserIsApproved matches user.GithubUsername against a CCLA's GithubUsernameApprovalList (signatures/service.go:1626-1630), so a record left naming one account and numbered as another can be approved under the wrong company's list. Covered by TestBindSigningIdentity_StaleGithubIndexDoesNotOverwriteTheHandle.
| } | ||
| f["lfUsername"] = caller.LFUsername | ||
|
|
||
| selected := selectedAccount{ID: githubID, Username: githubUsername} |
There was a problem hiding this comment.
Confirmed and not fixed here — flagging it as the sharpest open question on this PR rather than closing it.
You are right that the handle is an authorization input, and the code comment calling it display-only understated it: UserIsApproved matches user.GithubUsername against a CCLA's GithubUsernameApprovalList with EqualFold (signatures/service.go:1626-1630). So binding an unrecorded account number together with someone else's approval-listed handle does grant approval-list membership. That is escalation, not just a wrong label, and it is a sharper statement of this endpoint's residual risk than the PR description currently makes.
Two things that bound it, neither of which makes it a non-issue:
- It is not a new door.
PUT /v3/usersalready persists a client-supplieduser_github_usernameanduser_github_idwith no ownership check, and its handler even locates the record by the submitted handle before saving onto it (users/handlers.go:85-107,users/repository.go:418-429). This endpoint is the better-guarded version of a write the service already accepts — it refuses a contested account, a duplicated one, a record naming nobody, and a record already bound. - Deriving the handle from the number does not close it. Resolving
githubId→ handle through GitHub would make the pair self-consistent, but squatting the victim's number then yields the victim's true handle and the same approval-list membership. The vector is the missing ownership proof, not the unvalidated pairing.
So the real fix is ownership proof on the numeric account, which this design deliberately does not have — ownership is established in LFX Self Serve from the accounts Auth0 reports linked to the session. That premise depends on this endpoint being reachable only by that service, which is what #5147's azp allow-list plus in-handler JWKS verification establishes. Landing this ahead of #5147 would ship the trust assumption before the mechanism that enforces it, which is why this PR is draft. Raising with the team before it leaves draft.
| updates["user_github_username"] = selected.Username | ||
| } | ||
|
|
||
| updated, err := s.usersWriter.UpdateUser(record.UserID, updates) |
There was a problem hiding this comment.
Fixed in 687479756. The adopt path now re-reads the record consistently before deciding, so lf_record_already_bound is evaluated against the base table rather than the LF-identity index. You are right about why this mattered more than the usual stale-read: the post-write confirmation would have found the account it submitted and reported success, so the refusal would never have fired for the one case it exists to catch. TestBindSigningIdentity_StaleLFIndexDoesNotOverwriteANewerBinding fails without the fix.
The ConditionExpression is deliberately still absent. A conditional write needs a condition-carrying method on the shared users repository, which many other callers use, and that is out of scope for this PR. The consistent read narrows the window from GSI replication lag to the microseconds between read and write; the residual concurrent-writer race is detected reactively by duplicate_github_id.
| return s.resolveExistingGithubRecord(f, caller, selected, byGithub[0]) | ||
| } | ||
|
|
||
| byLF, err := s.repo.GetUsersByLFUsername(ctx, caller.LFUsername) |
There was a problem hiding this comment.
Real, and deferred with a reason rather than fixed.
The case-sensitivity is not local to this lookup: lf-username-index is queried with an exact key condition everywhere, including the shared GetUserByLFUserName (users/repository.go:605) that createUserFromRequest uses — and that middleware creates an LF-only record when its lookup misses, on every request, before this handler runs. So a case-variant record already produces a duplicate today through a path this PR does not touch, and this endpoint's create branch inherits the behaviour rather than introducing it.
Fixing it properly means canonicalising lf_username on write plus a migration over the existing table, which is a change to shared user-record handling and much larger than this endpoint. Probing a second cased variant here would paper over it for one caller and leave the middleware still creating the duplicate a moment earlier in the same request.
Tracking it separately. Worth noting the mismatch is deliberate elsewhere in this file — identity_mismatch and the record-ownership comparison both use EqualFold precisely because casing does drift, so treating the lookup as exact while the comparisons are not is the inconsistency to resolve at the table level.
| // Refusal is a decision not to record an association, carrying the reason that decision | ||
| // was reached. Every refusal writes nothing: there is no partial-success path, because a | ||
| // refusal after a write would leave behind an association the evidence does not support. |
There was a problem hiding this comment.
Fixed in 687479756. The Refusal doc comment now names ReasonRecordedMismatch as the one post-write exception and says the write is not compensated, with the reason (a second unconfirmed write on a record the service has just lost confidence about is not an improvement) and the fact that retrying is safe.
| different account, or a record naming nobody are all refused rather than | ||
| resolved. Resolution keys on the GitHub account number before the LF identity, | ||
| because the authentication middleware may have created an LF-only record moments | ||
| earlier on this same request. Every refusal writes nothing. |
There was a problem hiding this comment.
Fixed in 687479756 — the swagger description now carries the same exception, so the generated API description agrees with docs/MY_CLAS_API.md and the contract.
Add POST /v4/my-clas/signing-identity, which records the GitHub account
a contributor is signing with on their EasyCLA user record and returns
the record identifier the Contributor Console hand-off consumes. Until
now that identifier was whichever record an identity search matched
first, which is neither the contributor's choice nor stable between
signings.
The endpoint does not verify that the submitted account belongs to the
caller. Ownership is established by LFX Self Serve, which offers the
contributor only the accounts the identity provider reports as linked to
their session. What is verified here is who is calling and whether
recording is safe against the records already held:
- the gateway-authenticated principal and the token's LF identity must
agree, because the write uses the token's identity and a
disagreement would record the account against a person the gateway
did not authenticate;
- an account held by another contributor's record, by a record naming
nobody, or by more than one record is refused, as is a caller whose
own record already holds a different account.
Every refusal carries its own reason code and writes nothing, so a
contested account, an unclaimable record and an already-bound caller can
be counted and acted on separately.
A record holding the account but carrying no LF identity is refused
rather than adopted. Writing the caller's identity onto it would reunite
the rightful contributor with their signature history, and would hand
any other caller somebody else's, since such records already carry
signatures. Distinguishing the two needs proof of ownership this
endpoint does not have.
Resolution reads the my_clas repository's slice-returning GSI queries
rather than the users package's single-result lookups: the latter warn
and return the first of several matches, which would make the duplicate
refusal unreachable and resolve a contested account to whichever record
the index returned first. The post-write confirmation is a strongly
consistent read on the base table, because the resolving lookup went
through an eventually consistent index and would otherwise refuse a
contributor whose write had in fact succeeded.
Signed-off-by: ahmedomosany <aopeyemi@contractor.linuxfoundation.org>
148d383 to
6874797
Compare
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 13 out of 13 changed files in this pull request and generated 1 comment.
Suppressed comments (6)
cla-backend-go/v2/my_clas/signing_identity.go:270
- This comment is incorrect: the stored handle is matched by
signatures/service.go:1623-1630against CCLA approval lists, so it is authorization data rather than display-only. Document that only signing-record resolution ignores the handle.
// The handle is display-only and drifts whenever the contributor renames themselves
// on GitHub. Refreshing it is not a re-association - the account number, which is what
// anything matches on, is unchanged and is not rewritten here.
cla-backend-go/v2/my_clas/signing_identity.go:216
- This exact GSI lookup does not honor the case-insensitive LF identity semantics used by the handler and
resolveExistingGithubRecord. If the caller's LF-only record was stored with different casing (a state the test atsigning_identity_test.go:242-260explicitly supports), this returns no match and the next branch creates a duplicate instead of adopting that record. Resolve LF usernames using a canonical stored form or another case-insensitive identity mapping before creating.
byLF, err := s.repo.GetUsersByLFUsername(ctx, caller.LFUsername)
cla-backend-go/v2/my_clas/signing_identity.go:127
githubUsernameis persisted without being bound togithubID, but it is an authorization input:signatures/service.go:1623-1630approves a user when the stored handle matches a CCLA approval list. The linked consumer derives the handle server-side, but this EasyCLA route is also directly callable with a contributor bearer token (as documented by the curl example), so a caller can bind their own ID with another employee's approved handle. Derive the handle from a trusted lookup ofgithubID, or require service-only/verifiable identity assertions before writing.
selected := selectedAccount{ID: githubID, Username: githubUsername}
cla-backend-go/swagger/common/signing-identity.yaml:27
- “Never matched on” is false:
signatures/service.go:1623-1630matches this stored handle against CCLA approval lists. Clarify that it is not used for record resolution but does affect approval authorization.
description: The recorded GitHub handle. Display only - never matched on.
docs/MY_CLAS_API.md:518
- “Never matched on” contradicts the next clause and the implementation:
signatures/service.go:1623-1630matches the stored handle against CCLA approval lists. Restrict the statement to record resolution so operators do not treat this as display-only data.
`githubUsername` is optional and is never matched on, but it is recorded, because approval
lists written against handles cannot match a record that has none. A blank value is
cla-backend-go/swagger/common/signing-identity-request.yaml:27
- The handle is not display-only: after it is stored,
signatures/service.go:1623-1630uses it for CCLA approval-list matching. The request contract should state that callers must provide the handle corresponding togithubId, since a mismatched value can change authorization outcomes.
The GitHub handle for the selected account. Display only and never matched on, but
recorded because approval lists written against handles cannot match a record
without one. Sent by the caller because the handle is not derivable here.
| if len(byLF) == 0 { | ||
| return s.createSigningRecord(f, caller, selected) |
|
Closing in favour of #5150 |
Adds
POST /v4/my-clas/signing-identity, which records the GitHub account a contributor is signing with on their EasyCLA user record and returns the record identifier the Contributor Console hand-off consumes. Until now that identifier was whichever record an identity search matched first, which is neither the contributor's choice nor stable between signings.Consumer side: lfx-self-serve#1252. Draft until that half is reviewed alongside it.
The one thing to know before reading the rest
This endpoint does not verify that the submitted GitHub account belongs to the caller. That is deliberate. Ownership is established by LFX Self Serve, which offers a contributor only the accounts the identity provider reports as linked to their session and refuses a submitted account that is not among them. This service records that account on trust.
Read that way, the refusals below are the whole safety story rather than belt-and-braces, which is why none of them should be relaxed without replacing them.
What this endpoint does verify:
X-USERNAME) must agree with the LF username in the caller's own verified Auth0 token. The write uses the token's identity, so a disagreement would record the account against a person the gateway did not authenticate —identity_mismatch, 403.record_conflict); held by a record naming nobody (record_unclaimed); held by more than one record (duplicate_github_id); or the caller's own record already holds a different account (lf_record_already_bound).Notes for the reviewer
A record holding the account but naming nobody is refused, not adopted. Writing the caller's identity onto it would reunite the rightful contributor with their signature history — and would hand any other caller somebody else's, since such records already carry signatures. Telling those two apart needs proof of ownership this endpoint does not have, so it refuses. The cost is real and worth watching: contributors with detached records still cannot see those CLAs, and the
record_unclaimedrate measures exactly how many.Resolution uses the
my_clasrepository's slice-returning lookups, not theuserspackage's.GetUserByGitHubID/GetUserByLFUserNamewarn and return the first of several matches, which would makeduplicate_github_idunreachable and silently resolve a contested account to whichever record the index happened to return. They also disagree about how a miss looks — one returnserrors.NotFound, the other(nil, nil).The post-write confirmation is a strongly consistent read on the base table. The resolving lookup goes through a GSI, which is always eventually consistent; confirming through the same path could not distinguish a genuine mismatch from its own stale view, and would refuse contributors whose write had in fact succeeded.
recorded_mismatchis the one refusal that leaves a write behind. It is only reachable after the write and does not undo it. A compensating delete would be a second unconfirmed write on a record the service has just lost confidence about, so it is documented rather than attempted. Retrying is safe: a record already holding the submitted account resolves as a match.Known and accepted
Squatting an unrecorded account number, and what that reaches. For an account no record holds yet, none of the refusals apply, so a caller can bind a number that is not theirs and the rightful owner later meets
record_conflict. This is worth stating more sharply than "a wrong label": the recorded handle is an authorization input, sinceUserIsApprovedmatchesuser.GithubUsernameagainst a CCLA'sGithubUsernameApprovalList(signatures/service.go:1626-1630). So a squatted binding can obtain approval-list membership under a company's CCLA. Deriving the handle from the number instead would not close it — squatting the victim's number yields the victim's handle and the same result. The vector is the absent ownership proof.Two things bound it. It is not a new door:
PUT /v3/usersalready persists a client-supplieduser_github_idanduser_github_usernamewith no ownership check, and its handler locates the record by the submitted handle before saving onto it (users/handlers.go:85-107,users/repository.go:418-429), which this endpoint refuses. And established bindings still cannot be reassigned. Watch therecord_conflictrate.This is why the PR is draft and why it should not land before #5147. Everything above rests on the endpoint being reachable only by Self Serve's confidential client, which is the premise Unicron 1224 #5147 enforces with
azpallow-listing plus in-handler JWKS verification. Merging the binding endpoint first would ship the trust assumption ahead of the mechanism.No conditional write between the resolve read and the write. Two concurrent binders of the same unrecorded number can both succeed, creating the duplicate state a later call refuses on. Closing it needs a condition-carrying method on the shared
usersrepository; detection is already in place. Both write paths now re-read the record on the base table first, so the window is the gap between that read and the write rather than GSI replication lag.lf_usernamelookups are case-sensitive while the comparisons around them are not.lf-username-indexis queried with an exact key condition everywhere, including the sharedGetUserByLFUserNamethatcreateUserFromRequestuses before this handler runs — and that middleware creates an LF-only record when its lookup misses. A case-variant record therefore already yields a duplicate through a path this PR does not touch. Resolving it means canonicalisinglf_usernameon write plus a migration, which is a change to shared user-record handling.Test plan
make swagger && make build-mac && make test && make lint— all green locally, including the license-header checkBindSigningIdentitytests cover each refusal, and assert on the stored record rather than only the error, so an implementation that writes and then errors cannot passX-ACL), and seeded fixtures for a two-account contributor, a one-account contributor, and an unclaimed record