docs(easycla-ss): EasyCLA → Self Serve migration architecture docs - #5124
Open
mlehotskylf wants to merge 4 commits into
Open
docs(easycla-ss): EasyCLA → Self Serve migration architecture docs#5124mlehotskylf wants to merge 4 commits into
mlehotskylf wants to merge 4 commits into
Conversation
lukaszgryglicki
previously approved these changes
Jul 28, 2026
lukaszgryglicki
left a comment
lukaszgryglicki
left a comment
Member
There was a problem hiding this comment.
/lgtm (I already saw this in a feature branch and I'm basing my M1 milestone BE part implementation on this).
|
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:
WalkthroughAdds documentation for the EasyCLA-to-LFX Self Serve migration, covering the target architecture, milestone plan, authorization and role-mapping feasibility, and dev-environment validation spikes. ChangesEasyCLA Self Serve migration
Estimated code review effort: 2 (Simple) | ~10 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
Contributor
There was a problem hiding this comment.
Pull request overview
Defines Milestone 1 of the EasyCLA migration: a read-only “My CLAs” Profile view in LFX Self Serve.
Changes:
- Documents architecture, identity resolution, authorization, and data mapping.
- Defines API contracts, implementation tasks, and validation steps.
- Adds populated and empty-state UI mockups.
Reviewed changes
Copilot reviewed 15 out of 15 changed files in this pull request and generated 22 comments.
Show a summary per file
| File | Description |
|---|---|
tasks.md |
Implementation task breakdown. |
spec.md |
Requirements and acceptance criteria. |
research.md |
Technical decisions and findings. |
quickstart.md |
Local validation instructions. |
plan.md |
Implementation architecture and structure. |
mockups/README.md |
Mockup guide. |
mockups/my-clas-populated.html |
Populated-state design. |
mockups/my-clas-empty-state.html |
Empty-state design. |
data-model.md |
Agreement and identity models. |
contracts/upstream-easycla-api.md |
EasyCLA API contract. |
contracts/ss-me-clas-api.md |
Self Serve API contract. |
spike-runbook.md |
Authentication spike procedure. |
role-mapping-feasibility.md |
Authorization feasibility analysis. |
docs/easycla-ss-migration/README.md |
Architecture package index. |
architecture-proposal.md |
Migration architecture proposal. |
Comments suppressed due to low confidence (3)
specs/001-easycla-ss-integration-fable/m1-my-cla/quickstart.md:19
- The implemented UI is a Profile child route, not
/me/clas. This instruction currently sends validation to a route that does not exist.
2. Open **Me lens → My CLAs** (`/me/clas`).
specs/001-easycla-ss-integration-fable/m1-my-cla/spec.md:37
- The scope says the by-identity endpoint is the only EasyCLA change, but T010a also adds
projectNameto the signature schema/converter. Either include that response extension in scope or remove T010a; otherwise the normative spec and implementation plan disagree.
Out: any signing/writes, CCLA data (M4), approval lists/roles, changes to the PR remediation link, EasyCLA backend changes beyond the single expected read endpoint `GET /v4/users/by-identity` — see [contracts/upstream-easycla-api.md](contracts/upstream-easycla-api.md).
specs/001-easycla-ss-integration-fable/m1-my-cla/contracts/ss-me-clas-api.md:39
- FR-005a requires the CTA when no agreements are found as well as when GitHub is unlinked. The documented/implemented condition (
githubLinked=falseorunmatched) misses a matched user with a linked GitHub account and zero agreements. Include the empty-agreements condition, and point the contract at the implemented/profile/identitiesroute.
- **GitHub-link CTA**: when `identity.githubLinked=false` (no GitHub account linked to the LF identity), the UI shows "Don't see your CLAs? Link your GitHub account" pointing into SS's existing identity-linking flow (`/social/callback` social-connection pattern); on return, the page re-fetches and resolution now includes the linked GitHub ID (research R2).
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 18
🧹 Nitpick comments (2)
specs/001-easycla-ss-integration-fable/m1-my-cla/data-model.md (2)
30-30: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winUse
claTypeas the authoritative classifier.
research.mdLine [78] says the v2 payload exposesclaType(icla/ecla/ccla) and that the company-ID heuristic is fallback-only. Update this invariant so implementations do not misclassify records when the discriminator and company fields diverge.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@specs/001-easycla-ss-integration-fable/m1-my-cla/data-model.md` at line 30, Update the Classification rule to use the payload’s claType value as the authoritative classifier: map icla to an ICLA record, ecla to an ECLA record, and ccla to a corporate record excluded from M1. Retain the cclaCompanyID heuristic only as a fallback when claType is absent, and ensure implementations do not override an explicit claType based on company-field values.
42-45: 🗄️ Data Integrity & Integration | 🔵 Trivial | ⚡ Quick winStandardize the status vocabulary across the M1 docs.
data-model.mdusesvalid | superseded | inactive,spec.mdsaysvalid | superseded | expired, andresearch.mdmaps toValid / Superseded / Disabled / Invalidated / Incomplete. Pick one machine enum and map the display labels to it consistently everywhere.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@specs/001-easycla-ss-integration-fable/m1-my-cla/data-model.md` around lines 42 - 45, Standardize the M1 status vocabulary by selecting one machine-readable enum and using it consistently in data-model.md (anchor, lines 42-45), spec.md (lines 21-22), and research.md (lines 74-82). Update the Signed status type and all documented mappings so display labels consistently map to the chosen enum, without retaining conflicting values such as inactive, expired, Disabled, Invalidated, or Incomplete unless explicitly represented by the selected vocabulary.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@docs/easycla-ss-migration/spike-runbook.md`:
- Line 41: Update the “Getting a refresh token” guidance to remove any
recommendation to retrieve tokens from debug logs. Limit retrieval to approved
dev-only session-store extraction or a temporary authorization-code+PKCE flow,
explicitly require redaction from logs, and instruct engineers to revoke or
delete the token after the spike.
- Around line 27-39: The token checkpoint around TOKEN must fail closed: require
curl to return a successful HTTP status and validate that the response contains
a non-empty .access_token before proceeding. Replace the plain base64 decoding
in the checkpoint with JWT-aware decoding or URL-safe base64 handling that
restores required padding, and ensure decoding failures stop the runbook rather
than producing misleading output.
In
`@specs/001-easycla-ss-integration-fable/m1-my-cla/contracts/upstream-easycla-api.md`:
- Around line 1-3: Rename the contract heading to distinguish existing EasyCLA
APIs consumed by M1 from the planned new backend endpoint described later in the
document; preserve the endpoint and middleware details unchanged.
In
`@specs/001-easycla-ss-integration-fable/m1-my-cla/mockups/my-clas-empty-state.html`:
- Around line 131-134: Align the CTA with the M1-supported GitHub
identity-linking flow by removing unsupported Email and GitLab options from
my-clas-empty-state.html at lines 131-134, unless their resolution behavior is
added to the M1 contracts. Update
specs/001-easycla-ss-integration-fable/m1-my-cla/mockups/README.md lines 15-16
to describe the final CTA consistently.
In
`@specs/001-easycla-ss-integration-fable/m1-my-cla/mockups/my-clas-populated.html`:
- Around line 139-142: Remove the “or GitLab accounts” promise from the
identity-linking CTA in the infobox, since M1 currently supports only LF
username, verified email, and GitHub IDs. Keep the remaining CTA text and
styling unchanged.
In `@specs/001-easycla-ss-integration-fable/m1-my-cla/plan.md`:
- Around line 57-69: The plan tree at
specs/001-easycla-ss-integration-fable/m1-my-cla/plan.md:57-69 should use the
modules/profile/clas structure, remove the main-layout/sidebar edit, and
describe profile route/tab integration instead. Update the test target at
specs/001-easycla-ss-integration-fable/m1-my-cla/tasks.md:83 to
profile-clas.component, and revise
specs/001-easycla-ss-integration-fable/m1-my-cla/tasks.md:120 so its example
refers to profile route/tab work rather than routes plus sidebar.
In `@specs/001-easycla-ss-integration-fable/m1-my-cla/quickstart.md`:
- Around line 24-32: Update the SC-001 parity-check procedure in the quickstart
so it resolves every EasyCLA user ID, follows pagination via lastKeyScanned for
each identity, merges all returned records, and compares that complete set with
the SS list. Replace the single signatures/user/{userID} comparison while
preserving the existing sampling across N dev users.
In `@specs/001-easycla-ss-integration-fable/m1-my-cla/research.md`:
- Around line 67-68: Update the identity-source documentation to use the
complete set of verified user emails rather than the singular getEffectiveEmail
value. Ensure the described resolution flow unions every verified email for
EasyCLA matching, while preserving getEffectiveUsername and filtered GitHub
identities.
- Around line 62-68: Choose and consistently document the feature-flag contract
across both sites: in
specs/001-easycla-ss-integration-fable/m1-my-cla/research.md lines 62-68, update
the implementation note and related tasks to state the selected server-gated 404
or Angular-only behavior; in
specs/001-easycla-ss-integration-fable/m1-my-cla/contracts/ss-me-clas-api.md
lines 51-53, align the API contract by either requiring server-side gating or
explicitly documenting Angular-only gating and its flag-off behavior.
- Line 80: Align the projectName requirement and upstream-change policy across
all three documents: in
specs/001-easycla-ss-integration-fable/m1-my-cla/research.md:80, retain the
upstream response-field decision and define compatibility, scope, and bundled
tasks; in specs/001-easycla-ss-integration-fable/m1-my-cla/spec.md:34-37, expand
the out-of-scope statement to permit the required projectName response change;
and in
specs/001-easycla-ss-integration-fable/m1-my-cla/contracts/upstream-easycla-api.md:40-42,
update the one-endpoint/no-schema-change allowance to cover projectName, or
explicitly preserve projectID as the fallback.
- Line 3: Update the technical-context status statement in research.md to
reflect that T001/T002 remain unrun and the local api-gw audience is
inconclusive; keep M1 authorization conditional until the deployed-dev gateway
checks pass.
In `@specs/001-easycla-ss-integration-fable/m1-my-cla/tasks.md`:
- Line 83: Update task T024 to reference the final profile component name and
path, replacing my-clas.component with profile-clas.component while preserving
the existing test coverage requirements.
- Line 110: Update the cross-repo ownership note in tasks.md to include T010a
alongside T007–T010 and T028 as modifying cla-backend-go. Preserve the existing
ownership distinction and interim username-only resolution guidance.
- Line 120: Update the parallel example in tasks.md so T020 is described as
Profile-tab route work rather than “routes + sidebar,” while leaving the
adjacent T021 agreement-card entry unchanged.
- Line 51: Synchronize the finalized configuration contract: in
specs/001-easycla-ss-integration-fable/m1-my-cla/tasks.md:51, record removal of
CLA_SERVICE_BASE_URL, derive the base URL from API_GW_AUDIENCE, and use the
final flag name my-clas-enabled; in
specs/001-easycla-ss-integration-fable/m1-my-cla/quickstart.md:5-8, remove the
obsolete environment-variable prerequisite and replace the unresolved flag
placeholder with my-clas-enabled.
- Around line 68-74: The feature-flag contract is contradictory between T015 and
T018. Align the route-test requirement with the documented Angular-only gating
in T018 by removing the expectation that both API routes return 404 when the
flag is off, and state that authenticated direct API requests remain available
regardless of the UI flag.
- Around line 29-30: Sanitize committed personal data at both affected sites: in
specs/001-easycla-ss-integration-fable/m1-my-cla/tasks.md lines 29-30, require
T004 fixtures to use synthetic or redacted EasyCLA responses rather than raw
personal data; in
specs/001-easycla-ss-integration-fable/m1-my-cla/mockups/my-clas-populated.html
lines 93-124, replace the realistic employee name, email, and employer with
fictional sample values while preserving the mockup structure.
- Line 72: Mark T016 incomplete because full identity resolution remains
pending, or split the username-only implementation from the deferred three-key
union and verified-email/GitHub-ID matching work. Update the T016 completion
marker and status text so FR-005 is not represented as delivered while the
pending T010-dependent behavior remains outstanding.
---
Nitpick comments:
In `@specs/001-easycla-ss-integration-fable/m1-my-cla/data-model.md`:
- Line 30: Update the Classification rule to use the payload’s claType value as
the authoritative classifier: map icla to an ICLA record, ecla to an ECLA
record, and ccla to a corporate record excluded from M1. Retain the
cclaCompanyID heuristic only as a fallback when claType is absent, and ensure
implementations do not override an explicit claType based on company-field
values.
- Around line 42-45: Standardize the M1 status vocabulary by selecting one
machine-readable enum and using it consistently in data-model.md (anchor, lines
42-45), spec.md (lines 21-22), and research.md (lines 74-82). Update the Signed
status type and all documented mappings so display labels consistently map to
the chosen enum, without retaining conflicting values such as inactive, expired,
Disabled, Invalidated, or Incomplete unless explicitly represented by the
selected vocabulary.
🪄 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: f4a7c650-137f-48ea-8222-c1680839dcc8
📒 Files selected for processing (15)
docs/easycla-ss-migration/README.mddocs/easycla-ss-migration/architecture-proposal.mddocs/easycla-ss-migration/role-mapping-feasibility.mddocs/easycla-ss-migration/spike-runbook.mdspecs/001-easycla-ss-integration-fable/m1-my-cla/contracts/ss-me-clas-api.mdspecs/001-easycla-ss-integration-fable/m1-my-cla/contracts/upstream-easycla-api.mdspecs/001-easycla-ss-integration-fable/m1-my-cla/data-model.mdspecs/001-easycla-ss-integration-fable/m1-my-cla/mockups/README.mdspecs/001-easycla-ss-integration-fable/m1-my-cla/mockups/my-clas-empty-state.htmlspecs/001-easycla-ss-integration-fable/m1-my-cla/mockups/my-clas-populated.htmlspecs/001-easycla-ss-integration-fable/m1-my-cla/plan.mdspecs/001-easycla-ss-integration-fable/m1-my-cla/quickstart.mdspecs/001-easycla-ss-integration-fable/m1-my-cla/research.mdspecs/001-easycla-ss-integration-fable/m1-my-cla/spec.mdspecs/001-easycla-ss-integration-fable/m1-my-cla/tasks.md
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 15 out of 15 changed files in this pull request and generated 1 comment.
Comments suppressed due to low confidence (20)
specs/001-easycla-ss-integration-fable/m1-my-cla/contracts/upstream-easycla-api.md:14
- The contract omits the approved P10 trust boundary. The by-identity endpoint must only trust identities obtained server-side from
lfx.auth-service.user_identity.list, and its handler must allow-list the Self Serve confidential-clientazp, re-verify the bearer against JWKS, and deny missing/unparseable tokens. Merely placing this arbitrary-identity lookup on the secured router lets other authenticated callers query CLA matches for supplied identities. Update this contract and the implementation tasks to carry all P10 conditions fromarchitecture-proposal.md:94and lfx-self-serve#1216/#1224.
- Requires EasyCLA-team review; same no-ownership-check caveat as §2 — treat as internal/service API, SS is the authorization boundary.
specs/001-easycla-ss-integration-fable/m1-my-cla/contracts/ss-me-clas-api.md:53
- This feature-flag contract contradicts T018 and research R5: only the Angular Profile tab/route is gated, while the Express routes are always registered. Keeping the stated route-level 404 behavior will also drive the obsolete T015 tests.
Both routes and the Angular module are gated by LaunchDarkly flag `my-clas-enabled` (name TBD per repo convention). Flag off ⇒ routes 404, sidebar item hidden, route guard redirects to Me dashboard.
specs/001-easycla-ss-integration-fable/m1-my-cla/spec.md:36
- The scope still names a standalone
/me/clasmodule even though the accepted placement and implementation are the Profile tab at/profile/clas(T020 and PR #1203). This makes the feature specification disagree with its own implementation plan.
In: Me-lens module `/me/clas`, SS server routes, identity resolution + telemetry, PDF presigned-URL hand-off, empty state, feature flag.
specs/001-easycla-ss-integration-fable/m1-my-cla/plan.md:60
- This source tree still instructs implementers to create the abandoned standalone
modules/my-clasroute and, immediately below, edit the main-layout sidebar/app routes. T020 and PR #1203 instead place the feature undermodules/profile/clas, register aprofile/claschild route, and add a profile-layout tab. Update the tree so the plan is usable as implementation guidance.
├── app/modules/my-clas/ # NEW Angular lens module (Me lens)
│ ├── my-clas.routes.ts # exported MY_CLAS_ROUTES, lazy-loaded at /me/clas
│ ├── my-clas.component.ts|html # list view: ICLA + ECLA sections, empty state
specs/001-easycla-ss-integration-fable/m1-my-cla/tasks.md:68
- The
flag off ⇒ 404assertion is incompatible with the completed design in T018, where no server-side flag gate exists. Leaving it here requires tests for behavior the application intentionally does not implement; feature-off coverage belongs to the Angular guard/tab tests.
- [ ] T015 [P] [US1] Route/controller tests in `server/routes/__tests__/clas.route.spec.ts`: 401 without session; userID derived **only** from session (a request supplying user IDs is ignored); `GET /api/me/clas/:signatureId/pdf-url` returns **404** (never 403) for unknown, not-owned, and ECLA signature IDs; 502 `{ code: "UPSTREAM_ERROR" }` on upstream failure; flag off ⇒ 404 on both routes.
specs/001-easycla-ss-integration-fable/m1-my-cla/data-model.md:30
- This model still defines the company-field heuristic as the primary classifier, but R6 and the implementation make
claTypeauthoritative. If tests follow this rule, a record with an explicitclaTypecan be misclassified by fallback fields. Document the heuristic only for records whereclaTypeis absent.
**Classification rule**: `type=cla & referenceType=user & cclaCompanyID empty` ⇒ **ICLA** (has PDF); `type=cla & referenceType=user & cclaCompanyID set` ⇒ **ECLA** (no PDF, never offer download); `type=ccla` ⇒ corporate record — **excluded from M1** (Organization-lens data, M4).
specs/001-easycla-ss-integration-fable/m1-my-cla/mockups/my-clas-empty-state.html:140
- Acceptance scenario 3 requires the empty state to explain what CLAs are and link to documentation, but this design reference only reports that none were found. The linked implementation in PR #1203 follows this mockup and also lacks the required explanation/link, so the acceptance scenario is currently not represented in either design or implementation.
<div class="emptystate">
<div class="empty-icon">▤</div>
<h3>No CLAs on file yet</h3>
<p>We haven't found any signed ICLAs or ECLAs for your linked identities.</p>
</div>
specs/001-easycla-ss-integration-fable/m1-my-cla/tasks.md:44
- This task assumes secondary
user_emailscan be matched through the existing GSI-backed repository methods, butlf-email-indexindexes only primarylf_email; the companion backend PR #5126 explicitly documents that secondary emails are currently omitted to avoid a table scan. As written, FR-005 and SC-001 cannot be met for records reachable only by a verified secondary email. Define an indexed lookup strategy or explicitly revise the accepted scope and parity criteria.
- [ ] T008 Implement the service method in CLA `v2/users/` (or extend the existing users service per module pattern): union `GetUserByLFUserName` + per-email `GetUserByEmail`/`user_emails` match + per-githubId `GetUserByGitHubID`, reusing the existing GSI-backed repository methods in `users/repository.go` (`lf-username-index`, `lf-email-index`, `github-id-index` — githubId must stay **N-typed** or the GSI misses); dedupe by `user_id`. Unit tests for union/dedupe/empty-input in the same package.
specs/001-easycla-ss-integration-fable/m1-my-cla/tasks.md:47
- The cited
claGroupis loaded by the unrelated signature-invalidation flow, not by the user-signatures converter. Companion PR #5126 had to resolve each distinctprojectIDwith a per-request cache and leaveprojectNameempty on lookup failure. Record that actual design here instead of instructing implementers to read an unavailable “already-loaded” value.
- [ ] T010a **Add `projectName` to the signature response** (research R6 decision, 2026-07-28): add a `projectName` string to `swagger/common/signature.yaml`, `make swagger`, and populate it in the `/v4/signatures/user/{userID}` converter from the already-loaded CLA group (`claGroup.ProjectName`, cf. `v2/signatures/service.go:399,403,417`). Bundle with the T007–T010 PR. Closes the SS `projectName`-shows-ID gap; SS then maps `EasyClaSignature.projectName` straight through in `toMyClaAgreement` (drop the `projectID` fallback).
specs/001-easycla-ss-integration-fable/m1-my-cla/contracts/ss-me-clas-api.md:49
- As with the list route, the shared gateway helper can return 504 for an upstream timeout. Document it here so the PDF endpoint contract matches the actual transport behavior.
- **Errors**: 404 unknown/not-owned/ECLA id; 502 upstream failure.
specs/001-easycla-ss-integration-fable/m1-my-cla/quickstart.md:6
- These prerequisites still require the dropped
CLA_SERVICE_BASE_URLand present the token model as an open implementation choice. The current integration derives the URL fromAPI_GW_AUDIENCEand forwardsreq.apiGatewayToken; T002 remains an operational validation of that user-token path rather than a choice a quickstart user configures.
- Network access to EasyCLA **dev**: lfx-gateway dev URL for `/cla-service/v3|v4` (set `CLA_SERVICE_BASE_URL`), plus whichever token config the R3 spike selected (user bearer passthrough, or exchange/M2M client credentials).
specs/001-easycla-ss-integration-fable/m1-my-cla/research.md:3
- This states that all unknowns are resolved, but R3 still marks T001/T002 as unverified and R2/R6 still depend on T003/T004. Describe the document as tracking both verified facts and outstanding spikes so readers do not treat unresolved authorization and payload questions as settled.
All Technical Context unknowns resolved. Facts verified in code on 2026-07-11 unless noted.
specs/001-easycla-ss-integration-fable/m1-my-cla/quickstart.md:19
- The implemented UI is the Profile child route
/profile/clas, not/me/clas(T020 and lfx-self-serve#1203). This step sends anyone validating the feature to a route that does not exist.
2. Open **Me lens → My CLAs** (`/me/clas`).
specs/001-easycla-ss-integration-fable/m1-my-cla/tasks.md:60
- The phase goal still identifies
/me/clas, while T020 explicitly records that this route was not created. Use the implemented Profile route so the task checklist has one testable destination.
**Goal**: logged-in user sees all their signed ICLAs (any status, labeled) and currently valid ECLAs under Me lens `/me/clas`, downloads ICLA PDFs via short-lived links, sees the GitHub-link CTA when unlinked, and gets a clear empty state. Read-only; signing links out to Contributor Console.
specs/001-easycla-ss-integration-fable/m1-my-cla/mockups/my-clas-populated.html:150
- FR-001 and T021 require every ICLA to show a validity status, but the populated reference table has no Status column and never uses the defined
.statuspill. Add the status labels to this design reference; otherwise it depicts a UI that does not satisfy the core list requirement.
<th>Project</th>
<th>Type</th>
<th>Signed</th>
<th>Document</th>
</tr>
specs/001-easycla-ss-integration-fable/m1-my-cla/contracts/ss-me-clas-api.md:39
- The CTA target is documented as the social callback flow, but T020–T023 and PR #1203 link users to the existing
/profile/identitiespage. Consumers following this contract would navigate to the wrong entry point.
- **GitHub-link CTA**: when `identity.githubLinked=false` (no GitHub account linked to the LF identity), the UI shows "Don't see your CLAs? Link your GitHub account" pointing into SS's existing identity-linking flow (`/social/callback` social-connection pattern); on return, the page re-fetches and resolution now includes the linked GitHub ID (research R2).
docs/easycla-ss-migration/spike-runbook.md:68
- The role-less run uses
$TOKEN_B, but step 1 always assigns the exchanged token to$TOKEN. Following the runbook verbatim therefore sends an empty bearer token and produces a misleading 403 instead of testing ACS policy for user B.
-H "Authorization: Bearer $TOKEN_B"
docs/easycla-ss-migration/spike-runbook.md:36
- JWT segments use unpadded base64url, while
base64 -dexpects standard base64 and can fail on-/_characters or missing padding. Use a base64url-aware decoder so the spike's required claim check is reliable.
echo "$TOKEN" | cut -d. -f2 | base64 -d 2>/dev/null | jq . # inspect claims
docs/easycla-ss-migration/role-mapping-feasibility.md:30
- The memo still lists the SS Auth0 api-gw client grant as an unresolved staffing condition, but M1 research R3 records that
auth0_client_grant.lfxone_lfxgrants this audience in every environment. Keep only the deployed-token and role-less gateway calls as open conditions, and update the later “residual unknowns” statement as well.
Two conditions to convert into facts before committing M3–M5 staffing — both are one-day curl spikes (§7):
1. ACS's gateway-level path authorization must admit **ordinary users (no CLA role)** to the v4 *read* endpoints M1/M3 need.
2. The refresh-token exchange for the api-gw audience must be **granted to the SS Auth0 client** in each environment.
specs/001-easycla-ss-integration-fable/m1-my-cla/contracts/ss-me-clas-api.md:40
gatewayFetchsurfaces upstream gateway timeouts as 504 as well as 502 (research R5), but this API contract only documents 502. Include 504 as the same retryable upstream failure so clients and tests do not treat a real timeout as an undocumented response.
This issue also appears on line 49 of the same file.
- **Errors**: 502 with `{ code: "UPSTREAM_ERROR" }` when EasyCLA is unavailable (UI shows retryable error); identity-resolution failure is not an error (returns empty + `unmatched`).
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
docs/easycla-ss-migration/architecture-proposal.md (1)
94-94: 🩺 Stability & Availability | 🔵 TrivialDocument JWKS timeout and caching behavior.
The proposal confirms in-handler JWKS verification, while the supplied authentication path performs a network JWKS fetch during authentication. Specify bounded timeouts, cache/refresh behavior, and fail-closed handling so Auth0 or JWKS latency does not block the My CLAs request path.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@docs/easycla-ss-migration/architecture-proposal.md` at line 94, Update the “My CLAs” architecture proposal to specify bounded timeouts for JWKS retrieval, JWKS cache and refresh behavior, and fail-closed handling when Auth0/JWKS is unavailable, slow, or invalid. Clarify that these controls prevent JWKS latency from blocking the request path while preserving in-handler verification.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@docs/easycla-ss-migration/architecture-proposal.md`:
- Line 94: Update the P10 architecture proposal terminology at both references
to use “Approved List” instead of “allow-list,” including the azp validation
description, while preserving the existing meaning and avoiding whitelist
terminology.
---
Nitpick comments:
In `@docs/easycla-ss-migration/architecture-proposal.md`:
- Line 94: Update the “My CLAs” architecture proposal to specify bounded
timeouts for JWKS retrieval, JWKS cache and refresh behavior, and fail-closed
handling when Auth0/JWKS is unavailable, slow, or invalid. Clarify that these
controls prevent JWKS latency from blocking the request path while preserving
in-handler verification.
🪄 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: 3585e5c4-524f-43b5-8e31-c06ce136d7ea
📒 Files selected for processing (1)
docs/easycla-ss-migration/architecture-proposal.md
mlehotskylf
force-pushed
the
spec/easycla-ss-m1
branch
from
July 30, 2026 15:39
fb52ada to
cf219ea
Compare
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 12 out of 12 changed files in this pull request and generated 6 comments.
Comments suppressed due to low confidence (10)
specs/001-easycla-ss-integration-fable/m1-my-cla/tasks.md:45
- Secured-router placement alone does not satisfy the approved P10 trust boundary. Before EasyCLA trusts SS-supplied identities, the handler must re-verify the bearer against JWKS, require an allow-listed SS confidential-client
azp, and deny missing/unparseable tokens; SS must source identities via the auth-service RPC. Add these implementation and test requirements here, as tracked by #1224/#1225.
- [ ] T009 Wire the swagger-generated operation in the module's `handlers.go` `Configure(...)` (request/response translation only, per three-layer convention); register in `cmd/server.go` alongside the other v2 modules. Verify the path is on the **secured** router (gateway `cla-service.yaml` prefix rules) — it must NOT land on the public router; note in `contracts/upstream-easycla-api.md` that SS remains the authorization boundary.
specs/001-easycla-ss-integration-fable/m1-my-cla/contracts/upstream-easycla-api.md:13
- This contract specifies an endpoint that was abandoned: EasyCLA #5126 closed unmerged after the composed
/v4/my-clasAPIs landed in #5125/#5128. Replace this identity-lookup contract with the actual list/PDF contracts, including their ownership, validity, and identity-verification behavior.
Resolution unions three keys: LF username, verified emails, linked GitHub account(s) (research R2).
### NEW (expected required): GET /cla-service/v4/users/by-identity?lfUsername=…&email=…&githubId=…
- One small read endpoint to add in `cla-backend-go` (swagger-first in `cla.v2.yaml`, three-layer module pattern, read-only, no schema changes), wrapping the existing GSI-backed repository queries: `lf-username-index`, `lf-email-index` (+ `user_emails` match — verify `GetUsersByEmail` efficiency), `github-id-index` (`users/repository.go`; `GetUserByGitHubID` exists at service layer but is not exposed over HTTP today).
specs/001-easycla-ss-integration-fable/m1-my-cla/quickstart.md:6
- This prerequisite uses the dropped
CLA_SERVICE_BASE_URLand leaves the token model undecided, while R5/T011 finalize both choices. UseAPI_GW_AUDIENCEfor${API_GW_AUDIENCE}/cla-serviceand state that SS forwards the user-scopedapiGatewayToken; otherwise following the quickstart leaves the application misconfigured.
- Network access to EasyCLA **dev**: lfx-gateway dev URL for `/cla-service/v3|v4` (set `CLA_SERVICE_BASE_URL`), plus whichever token config the R3 spike selected (user bearer passthrough, or exchange/M2M client credentials).
specs/001-easycla-ss-integration-fable/m1-my-cla/contracts/ss-me-clas-api.md:53
- This contradicts R5/T018 and the implementation: the Express routes are registered unconditionally, so turning the flag off does not make
/api/me/clasreturn 404. Document the flag as Angular-only (tab visibility plusCanMatchon/profile/clas) rather than promising server-side gating that does not exist.
Both routes and the Angular module are gated by LaunchDarkly flag `my-clas-enabled` (name TBD per repo convention). Flag off ⇒ routes 404, sidebar item hidden, route guard redirects to Me dashboard.
specs/001-easycla-ss-integration-fable/m1-my-cla/tasks.md:68
- This test requirement cannot pass against the documented implementation because T018/R5 intentionally register both Express routes without a server-side flag check. Remove the “flag off ⇒ 404” assertion and test the Angular
CanMatchguard/tab visibility instead.
- [ ] T015 [P] [US1] Route/controller tests in `server/routes/__tests__/clas.route.spec.ts`: 401 without session; userID derived **only** from session (a request supplying user IDs is ignored); `GET /api/me/clas/:signatureId/pdf-url` returns **404** (never 403) for unknown, not-owned, and ECLA signature IDs; 502 `{ code: "UPSTREAM_ERROR" }` on upstream failure; flag off ⇒ 404 on both routes.
docs/easycla-ss-migration/architecture-proposal.md:9
- This link points to
specs/001-easycla-ss-integration-fable/spec.md, but that file is not present in the branch. The M1–M6 links later in this document are likewise absent, leaving the architecture package's implementation references unusable.
This document is self-contained for review purposes. Implementation-level specs (milestone scopes, acceptance criteria, M1 plan) live in [specs/001-easycla-ss-integration-fable/](../../specs/001-easycla-ss-integration-fable/spec.md) — linked below only for deep dives.
docs/easycla-ss-migration/role-mapping-feasibility.md:8
- All three cited program documents (
00-overview-fable.md,04-milestone-ccla-org-lens-fable.md, and the parentspec.md) are missing from this branch. Include them or replace these citations with available sources so readers can verify the memo's stated basis.
**Basis**: [00-overview-fable.md](../../specs/001-easycla-ss-integration-fable/00-overview-fable.md) §2.4/§3, [04-milestone-ccla-org-lens-fable.md](../../specs/001-easycla-ss-integration-fable/04-milestone-ccla-org-lens-fable.md) role options; code citations verified 2026-07-15, re-checked 2026-07-20
**Answers**: the open engineering action from the 2026-07-15 leadership review ([spec.md](../../specs/001-easycla-ss-integration-fable/spec.md) "Program review outcomes")
docs/easycla-ss-migration/README.md:12
- The linked program spec does not exist in this branch; the target directory contains only
m1-my-cla/. This makes the architecture-review reading path end in a 404, so add the omitted program docs or link directly to an included M1 document.
Implementation-level specifications (milestone scopes, acceptance criteria, per-milestone plans — used by the Spec Kit workflow) live separately in [specs/001-easycla-ss-integration-fable/](../../specs/001-easycla-ss-integration-fable/spec.md). This folder is for evaluating the architecture; that folder is for building it.
specs/001-easycla-ss-integration-fable/m1-my-cla/quickstart.md:19
- The implemented UI is a Profile child route, not
/me/clas(T020 and the linked implementation both use/profile/clas). This instruction currently sends testers to a nonexistent route.
2. Open **Me lens → My CLAs** (`/me/clas`).
specs/001-easycla-ss-integration-fable/m1-my-cla/tasks.md:60
- This goal still names
/me/clas, while T020 records that no such route exists and the feature is implemented at/profile/clas. Update the task-level goal so the checkpoint and E2E work target the real route.
**Goal**: logged-in user sees all their signed ICLAs (any status, labeled) and currently valid ECLAs under Me lens `/me/clas`, downloads ICLA PDFs via short-lived links, sees the GitHub-link CTA when unlinked, and gets a clear empty state. Read-only; signing links out to Contributor Console.
There was a problem hiding this comment.
Actionable comments posted: 6
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@docs/easycla-ss-migration/architecture-proposal.md`:
- Around line 6-7: Update the document header status near the architecture
review metadata to reflect the later P10 approval and confirmation dates, or
explicitly identify the July 20 entry as an earlier review distinct from the
July 28 approval and July 30 confirmation.
In `@docs/easycla-ss-migration/role-mapping-feasibility.md`:
- Around line 106-117: Update the prose terminology in the feasibility document
to use “Approved List” and “Approved Lists” instead of “Approval list” and
“approval lists,” including the consequence text and the referenced section.
Preserve literal API path segments such as /approval-list unchanged.
- Around line 18-30: Qualify the “without any gateway or EasyCLA changes”
statement in the migration recommendation. Clarify that no application code
changes are expected, while explicitly identifying ACS path-authorization
updates and per-environment Auth0 refresh-token grants as possible
policy/configuration changes pending the §7 spikes; avoid presenting the current
conclusion as guaranteed.
In `@specs/001-easycla-ss-integration-fable/m1-my-cla/quickstart.md`:
- Line 6: Update the quickstart configuration and verification steps to use
API_GW_AUDIENCE for deriving the EasyCLA base URL instead of
CLA_SERVICE_BASE_URL, and replace the outdated /me/clas navigation with the
implemented Profile child route. Apply these changes to the network-access
instructions and the related steps at lines 18–20, preserving the existing token
configuration guidance.
In `@specs/001-easycla-ss-integration-fable/m1-my-cla/tasks.md`:
- Around line 10-13: Update the repository references in the task documentation
to remove contributor-specific absolute filesystem paths, using repository names
with root-relative paths or neutral placeholders instead. Preserve the
instruction to verify current file paths and conventions before modifying either
repository.
- Line 125: Update the Phase 3 slicing strategy to reflect that T022 already
delivers the GitHub-link CTA. Describe the deferred T023 work as completing full
GitHub/email identity union and the remaining CTA component test coverage,
without implying the CTA implementation should be added later.
🪄 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: 2ed54599-909a-4868-99fe-bd35765ee4dd
📒 Files selected for processing (12)
docs/easycla-ss-migration/README.mddocs/easycla-ss-migration/architecture-proposal.mddocs/easycla-ss-migration/role-mapping-feasibility.mddocs/easycla-ss-migration/spike-runbook.mdspecs/001-easycla-ss-integration-fable/m1-my-cla/contracts/ss-me-clas-api.mdspecs/001-easycla-ss-integration-fable/m1-my-cla/contracts/upstream-easycla-api.mdspecs/001-easycla-ss-integration-fable/m1-my-cla/mockups/README.mdspecs/001-easycla-ss-integration-fable/m1-my-cla/mockups/my-clas-empty-state.htmlspecs/001-easycla-ss-integration-fable/m1-my-cla/mockups/my-clas-populated.htmlspecs/001-easycla-ss-integration-fable/m1-my-cla/quickstart.mdspecs/001-easycla-ss-integration-fable/m1-my-cla/research.mdspecs/001-easycla-ss-integration-fable/m1-my-cla/tasks.md
🚧 Files skipped from review as they are similar to previous changes (2)
- docs/easycla-ss-migration/spike-runbook.md
- specs/001-easycla-ss-integration-fable/m1-my-cla/contracts/upstream-easycla-api.md
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 12 out of 12 changed files in this pull request and generated no new comments.
Comments suppressed due to low confidence (23)
specs/001-easycla-ss-integration-fable/m1-my-cla/tasks.md:47
- These tasks still direct implementation of the now-closed, unmerged
users/by-identityapproach from EasyCLA PR #5126. EasyCLA PR #5125 is already merged and implements the consolidated/v4/my-clasAPIs, while the approved trust-boundary hardening remains tracked separately. Following this task chain would duplicate a superseded API and omit the remaining M1 work; rewrite the upstream tasks around the merged endpoints and outstanding hardening.
- [ ] T007 Add `GET /v4/users/by-identity` to CLA `swagger/cla.v2.yaml` (query params: `lfUsername`, repeated `email`, repeated `githubId`; response: array of existing v2 user models, deduped) and run `make swagger` to regenerate `gen/` (never hand-edit `gen/`). License header on any new file.
- [ ] T008 Implement the service method in CLA `v2/users/` (or extend the existing users service per module pattern): union `GetUserByLFUserName` + per-email `GetUserByEmail`/`user_emails` match + per-githubId `GetUserByGitHubID`, reusing the existing GSI-backed repository methods in `users/repository.go` (`lf-username-index`, `lf-email-index`, `github-id-index` — githubId must stay **N-typed** or the GSI misses); dedupe by `user_id`. Unit tests for union/dedupe/empty-input in the same package.
- [ ] T009 Wire the swagger-generated operation in the module's `handlers.go` `Configure(...)` (request/response translation only, per three-layer convention); register in `cmd/server.go` alongside the other v2 modules. Verify the path is on the **secured** router (gateway `cla-service.yaml` prefix rules) — it must NOT land on the public router; note in `contracts/upstream-easycla-api.md` that SS remains the authorization boundary.
- [ ] T010 Run CLA `make fmt && make build-mac && make test && make lint`; open the PR against `dev` with DCO signoff; after merge, verify the endpoint on dev via curl (quickstart-style call) and record the verified request/response in `contracts/upstream-easycla-api.md`.
- [ ] T010a **Add `projectName` to the signature response** (research R6 decision, 2026-07-28): add a `projectName` string to `swagger/common/signature.yaml`, `make swagger`, and populate it in the `/v4/signatures/user/{userID}` converter from the already-loaded CLA group (`claGroup.ProjectName`, cf. `v2/signatures/service.go:399,403,417`). Bundle with the T007–T010 PR. Closes the SS `projectName`-shows-ID gap; SS then maps `EasyClaSignature.projectName` straight through in `toMyClaAgreement` (drop the `projectID` fallback).
specs/001-easycla-ss-integration-fable/m1-my-cla/contracts/ss-me-clas-api.md:38
- A matched EasyCLA user can legitimately have zero agreements, so requiring
identity.unmatched=trueto drive the empty state misses the acceptance scenario for a CLA-less matched user. The empty state should depend onagreements.length === 0;unmatchedshould only control the identity/history warning.
- **Behavior**: ICLAs listed in any status with labels; ECLAs only when valid; sorted `signedOn` desc; empty `agreements` + `identity.unmatched=true` drives the "no CLA history found for your account" empty state.
specs/001-easycla-ss-integration-fable/m1-my-cla/contracts/ss-me-clas-api.md:53
- This contradicts the implemented flag behavior recorded in
research.mdand T018/T020: the Express routes are registered unconditionally, and only the Profile tab/Angular child route are gated. Keeping this contract would produce a route test that can never pass and incorrectly promises server-side dark-launch enforcement.
Both routes and the Angular module are gated by LaunchDarkly flag `my-clas-enabled` (name TBD per repo convention). Flag off ⇒ routes 404, sidebar item hidden, route guard redirects to Me dashboard.
specs/001-easycla-ss-integration-fable/m1-my-cla/tasks.md:68
- T018 explicitly states that there is no server-side flag gate, so this test requirement is impossible under the chosen implementation. Remove the flag-off API 404 case and cover flag behavior in the Angular guard/tab tests instead.
- [ ] T015 [P] [US1] Route/controller tests in `server/routes/__tests__/clas.route.spec.ts`: 401 without session; userID derived **only** from session (a request supplying user IDs is ignored); `GET /api/me/clas/:signatureId/pdf-url` returns **404** (never 403) for unknown, not-owned, and ECLA signature IDs; 502 `{ code: "UPSTREAM_ERROR" }` on upstream failure; flag off ⇒ 404 on both routes.
specs/001-easycla-ss-integration-fable/m1-my-cla/research.md:13
- R1 still declares the per-user signatures endpoint as the selected M1 API and rejects a bespoke endpoint, but the merged design uses consolidated
/v4/my-clasand owned-PDF endpoints. Since later tasks use R1 as a decision source, update the decision rather than leaving the obsolete architecture as normative.
**Decision**: `GET /v4/signatures/user/{userID}` (swagger `cla.v2.yaml` path `/signatures/user/{userID}`; handler `v2/signatures/handlers.go` `SignaturesGetUserSignaturesHandler`, paginated via `pageSize`/`nextKey`, also accepts an optional `userName` query param passed to the v1 service). Returns the user's signature records — ICLAs and ECLAs are distinguished by `signature_user_ccla_company_id` (present ⇒ ECLA) per the signatures data model (`signatures/models.go`).
**Rationale**: purpose-built per-user endpoint; no fan-out per project; v2 response models already include project/company references.
**Alternatives considered**: per-project queries (`/signatures/project/...`) — O(projects) calls, rejected; new bespoke endpoint — unnecessary.
specs/001-easycla-ss-integration-fable/m1-my-cla/mockups/my-clas-empty-state.html:139
- Acceptance scenario 3 requires the empty state to link to CLA documentation, but this reference provides only explanatory text and no link. Add the required documentation action so the mockup covers the complete empty-state behavior.
<h3>No CLAs on file yet</h3>
<p>We haven't found any signed ICLAs or ECLAs for your linked identities.</p>
specs/001-easycla-ss-integration-fable/m1-my-cla/quickstart.md:6
- The setup instruction names a configuration that the implementation explicitly dropped.
gatewayFetchderives the base URL fromAPI_GW_AUDIENCE, and identity enrichment also requires local NATS connectivity; settingCLA_SERVICE_BASE_URLwill not configure this flow and can leave requests unauthenticated.
- Network access to EasyCLA **dev**: lfx-gateway dev URL for `/cla-service/v3|v4` (set `CLA_SERVICE_BASE_URL`), plus whichever token config the R3 spike selected (user bearer passthrough, or exchange/M2M client credentials).
specs/001-easycla-ss-integration-fable/m1-my-cla/quickstart.md:26
- This parity check targets the superseded per-user signatures endpoint, which does not exercise the consolidated identity aggregation and validity evaluation used by M1. Compare against
/v4/my-claswith the same complete, session-derived identity parameter set that Self Serve forwards.
| US1-AS1: ICLAs listed + PDF works | Click Download PDF → browser gets S3 URL, file opens; compare list against `GET /v4/signatures/user/{userID}` called directly with a dev token |
specs/001-easycla-ss-integration-fable/m1-my-cla/quickstart.md:42
- These direct calls use all three superseded upstream APIs. They will not validate the implementation described at the top of the upstream contract; use the consolidated list and owned-PDF endpoints, passing the same identity parameters to each.
# identity
curl -H "Authorization: Bearer $TOK" "$GW/cla-service/v3/users/username/<lf-username>"
# agreements
curl -H "Authorization: Bearer $TOK" "$GW/cla-service/v4/signatures/user/<userID>"
# pdf url
curl -H "Authorization: Bearer $TOK" "$GW/cla-service/v4/signatures/<signatureID>/signed-document"
docs/easycla-ss-migration/role-mapping-feasibility.md:112
- This enforcement map still presents the unowned per-user signatures API as the M1 boundary. The merged M1 API is
/v4/my-clas, which performs token-anchored identity ownership enforcement inside EasyCLA; keeping the old row contradicts P10 and the upstream contract.
| `GET /signatures/user/{userID}` | No per-user ownership check | confirmed in M1 research (R3) | SS server must bind userID to session |
docs/easycla-ss-migration/architecture-proposal.md:87
- The rationale says client identity is irrelevant to the v4 chain, but P10 makes the Self Serve client
azpa required authorization boundary for My CLAs. Qualify this as the behavior of the existing v4 routes and call out the M1 exception; otherwise P3 directly contradicts the approved trust model.
| P3 | **Tokens: user-scoped access tokens (never ID tokens), via SS's existing api-gw-audience refresh-token exchange** — the same access/refresh authentication the me/project lenses use. No M2M by default, no token infrastructure work | The whole v4 chain keys on user identity, not client/audience — verified in code ([feasibility §4](role-mapping-feasibility.md)); two curl spikes remain. ID-token usage stays legacy-console-only; interim gateway/ACS/v4 support for both token types is acceptable during cutover. **Caller-identification + trust model for the "My CLAs" by-identity read endpoint: see P10.** |
specs/001-easycla-ss-integration-fable/m1-my-cla/contracts/upstream-easycla-api.md:3
- This contract only says the following sections are superseded, but the file still documents the obsolete
users/by-identity, per-user signatures, and signed-document APIs. The merged implementation exposes/v4/my-clasand/v4/my-clas/{signatureID}/pdfwith different identity parameters, response fields, validity handling, and ownership enforcement. As a contract artifact, this needs to describe those actual endpoints rather than redirecting readers while retaining contradictory normative sections.
**Implementation update (PR #5125):** the endpoints below are superseded — that PR implements two consolidated read endpoints, `GET /v4/my-clas` and `GET /v4/my-clas/{signatureID}/pdf` (identity resolution, aggregation, ownership enforcement and validity evaluation inside EasyCLA; SS forwards all session-derived identity keys); see [docs/MY_CLAS_API.md](../../../../docs/MY_CLAS_API.md).
specs/001-easycla-ss-integration-fable/m1-my-cla/quickstart.md:19
- The implemented UI is a Profile child route, so
/me/clasdoes not exist. This quickstart currently sends testers to a 404 instead of the feature.
2. Open **Me lens → My CLAs** (`/me/clas`).
specs/001-easycla-ss-integration-fable/m1-my-cla/tasks.md:60
- The goal names the removed
/me/clasroute even though T020 later records/profile/clasas the corrected placement. The phase goal should use the actual acceptance URL.
**Goal**: logged-in user sees all their signed ICLAs (any status, labeled) and currently valid ECLAs under Me lens `/me/clas`, downloads ICLA PDFs via short-lived links, sees the GitHub-link CTA when unlinked, and gets a clear empty state. Read-only; signing links out to Contributor Console.
specs/001-easycla-ss-integration-fable/m1-my-cla/research.md:3
- This opening claim is contradicted by the document itself: T001/T002/T003 remain open, token behavior is explicitly inconclusive, and the P10 hardening follow-ups are not complete. Calling all unknowns resolved can cause the remaining launch gates to be skipped.
All Technical Context unknowns resolved. Facts verified in code on 2026-07-11 unless noted.
specs/001-easycla-ss-integration-fable/m1-my-cla/mockups/my-clas-populated.html:149
- The populated design omits the validity/status field required by FR-001, despite the mockup being the UI reference and even defining
.statuspill. Add a Status column and values for each ICLA so the reference cannot lead the implementation to omit a required field.
<th>Project</th>
<th>Type</th>
<th>Signed</th>
<th>Document</th>
docs/easycla-ss-migration/README.md:8
- The proposal now contains P1–P10, so describing it as P1–P8 makes the reading guide omit the v1-ID audit and My CLAs trust-boundary decisions.
1. **[architecture-proposal.md](architecture-proposal.md)** — start here. Current state, milestones, what leadership already settled, the proposed architecture (P1–P8), top risks, and what the review should challenge.
docs/easycla-ss-migration/spike-runbook.md:68
- Spike 2 calls the old unowned per-user endpoint, so a 200 here does not prove that a role-less user can reach the actual M1
/v4/my-clasroute; ACS authorization is path-specific. Exercise the consolidated M1 endpoint instead, otherwise the spike can pass while production requests still receive 403.
```bash
USER_ID="<user-B-easycla-userID>"
curl -s -o /dev/null -w "%{http_code}\n" \
"https://api-gw.dev.platform.linuxfoundation.org/cla-service/v4/signatures/user/$USER_ID" \
docs/easycla-ss-migration/role-mapping-feasibility.md:228
- This spike still tests
/signatures/user/{userID}, but ACS decisions are path-specific and M1 now calls/my-clas. As written, its result cannot answer the stated M1 feasibility question; switch the spike to the actual route.
2. **Role-less user → M1 read path.** Same exchange for a user with no ACS roles; `GET /cla-service/v4/signatures/user/{their-userID}`. **Outcome unknown — this is the decision point**: 200 ⇒ M1 uses user tokens as designed; 403 ⇒ file the ACS policy addition or fall back to M2M + server-side subject binding.
docs/easycla-ss-migration/architecture-proposal.md:85
- The proposal now adds consolidated My CLAs APIs and deliberately moves identity resolution, aggregation, validity evaluation, and PDF ownership enforcement into EasyCLA. Describing M1 as using only existing APIs with no business logic added conflicts with P10 and the merged backend design; clarify that Self Serve does not duplicate the logic, while EasyCLA owns the new read endpoints.
This issue also appears on line 87 of the same file.
| P1 | **Strangler pattern: SS is a new client of the existing EasyCLA v4/v3 APIs.** One SS server-side `cla` module; no business logic reimplemented; enforcement (roles, approval lists, sanctions) stays in EasyCLA until M6 | One source of truth; follows the proven crowdfunding integration shape in SS |
docs/easycla-ss-migration/role-mapping-feasibility.md:18
- This blanket conclusion is no longer true for M1: the consolidated My CLAs endpoint and P10 hardening require EasyCLA changes, including caller-client verification. Qualify the statement as applying to existing role-protected APIs so it does not contradict the architecture proposal's M1 trust boundary.
This issue also appears on line 112 of the same file.
Self Serve can call the existing EasyCLA v4 APIs with user-scoped tokens, **without any gateway or EasyCLA changes**, because the entire v4 authorization chain keys on the **user's identity (LF username)** — not on which Auth0 client or audience minted the token:
specs/001-easycla-ss-integration-fable/m1-my-cla/mockups/my-clas-populated.html:159
- The sample rows are not sorted by
signedOndescending as required by the SS contract: the table begins with 2019, then 2021/2022, and places May 2024 before November 2023. Reorder the samples newest-first so this implementation reference demonstrates the specified behavior.
<td><span class="typepill icla">ICLA</span></td>
<td>Jul 15, 2019</td>
<td><a class="doclink" href="#">⬇ Download PDF</a></td>
specs/001-easycla-ss-integration-fable/m1-my-cla/mockups/README.md:12
- The reference description omits the ICLA status field required by FR-001, matching the missing column in the HTML mockup. Include status in the documented row contents so reviewers and implementers know it is part of the design.
- `my-clas-populated.html` — populated table: ICLA rows with **Download PDF**, ECLA rows
showing **Covered by Corporate CLA (CCLA)** (no PDF), project/type/signed-date columns.
Adds the architecture-review context for the EasyCLA → LFX Self Serve migration under docs/easycla-ss-migration/: the milestone proposal (M1–M6), the EasyCLA↔SS role-mapping feasibility memo, the spike runbook, and a README (which links the architecture-review Google Slides deck). Scoped to the architecture docs only. The M1 spec-kit set (specs/…/m1-my-cla/) is intentionally not carried here — M1 is shipped and its spec docs already live on dev via the My CLAs API PRs (#5125/#5128). Docs only — no code changes. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Signed-off-by: Michal Lehotsky <mlehotsky@linuxfoundation.org>
mlehotskylf
force-pushed
the
spec/easycla-ss-m1
branch
from
July 30, 2026 16:39
cf219ea to
99ce979
Compare
mlehotskylf
changed the title
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@docs/easycla-ss-migration/spike-runbook.md`:
- Around line 61-68: Update the Spike 2 command to use a defined token variable:
either reuse $TOKEN assigned by rerunning step 1, or explicitly assign the
exchanged token to TOKEN_B before the request. Keep the Authorization header
consistent with the selected variable.
- Line 39: Revise the Spike 1 checkpoint in the runbook so the username claim is
described as a prerequisite or intermediate validation, not as passing the
spike. Reserve “passed” for the complete gateway-to-ACS-to-v4 flow that returns
HTTP 200 from the secured cla-managers endpoint, and update the related lines
45–56 consistently.
🪄 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: 52d6cb12-f894-472c-931f-d52f6593687e
📒 Files selected for processing (4)
docs/easycla-ss-migration/README.mddocs/easycla-ss-migration/architecture-proposal.mddocs/easycla-ss-migration/role-mapping-feasibility.mddocs/easycla-ss-migration/spike-runbook.md
🚧 Files skipped from review as they are similar to previous changes (1)
- docs/easycla-ss-migration/architecture-proposal.md
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 4 out of 4 changed files in this pull request and generated no new comments.
Comments suppressed due to low confidence (8)
docs/easycla-ss-migration/README.md:8
- The reading-order summary stops at P8, but
architecture-proposal.mdnow defines P9 and P10. Update the range so readers do not overlook the identity-mapping and trust-boundary proposals.
1. **[architecture-proposal.md](architecture-proposal.md)** — start here. Current state, milestones, what leadership already settled, the proposed architecture (P1–P8), top risks, and what the review should challenge.
docs/easycla-ss-migration/README.md:12
- This link is broken on the PR's
devbase: the root programspec.mdis only proposed by the still-open #5132. Make #5132 an explicit merge prerequisite, include the target here, or defer this link; otherwise the architecture entry point lands with a 404.
Implementation-level specifications (milestone scopes, acceptance criteria, per-milestone plans — used by the Spec Kit workflow) live separately in [specs/001-easycla-ss-integration-fable/](../../specs/001-easycla-ss-integration-fable/spec.md). This folder is for evaluating the architecture; that folder is for building it.
docs/easycla-ss-migration/architecture-proposal.md:9
- The linked root
spec.mdis absent from the current base and exists only in open PR #5132. Declare that PR as a dependency/merge it first, or remove the deep-dive link until its target is available.
This document is self-contained for review purposes. Implementation-level specs (milestone scopes, acceptance criteria, M1 plan) live in [specs/001-easycla-ss-integration-fable/](../../specs/001-easycla-ss-integration-fable/spec.md) — linked below only for deep dives.
docs/easycla-ss-migration/architecture-proposal.md:67
- These milestone links point to files not present on
dev; they are introduced by the separate, still-open #5132. Merge that dependency first or avoid publishing these links, otherwise the core milestone table contains broken navigation (including M6 on the next row).
| [M1](../../specs/001-easycla-ss-integration-fable/01-milestone-read-only-me-lens-fable.md) | Read-only "My CLAs" (Me lens) | — | S |
| [M2](../../specs/001-easycla-ss-integration-fable/02-milestone-sign-icla-fable.md) | Sign ICLA in SS (a: GitHub, b: GitLab, c: Gerrit) | — | M |
| [M3](../../specs/001-easycla-ss-integration-fable/03-milestone-sign-ecla-fable.md) | Sign ECLA in SS (a–c per platform) | Contributor Console + landing page | L |
| [M4](../../specs/001-easycla-ss-integration-fable/04-milestone-ccla-org-lens-fable.md) | CCLA management (Organization lens) | Corporate Console + its BFF | XL |
| [M5](../../specs/001-easycla-ss-integration-fable/05-milestone-project-lens-pcc-fable.md) | EasyCLA admin (Project lens) — *decision-gated* | PCC EasyCLA module | L |
docs/easycla-ss-migration/role-mapping-feasibility.md:8
- All three cited source documents are absent from the current base and only exist in open PR #5132. Merge that PR first or replace/defer these links so the memo's stated evidence is reviewable after this PR lands.
**Basis**: [00-overview-fable.md](../../specs/001-easycla-ss-integration-fable/00-overview-fable.md) §2.4/§3, [04-milestone-ccla-org-lens-fable.md](../../specs/001-easycla-ss-integration-fable/04-milestone-ccla-org-lens-fable.md) role options; code citations verified 2026-07-15, re-checked 2026-07-20
**Answers**: the open engineering action from the 2026-07-15 leadership review ([spec.md](../../specs/001-easycla-ss-integration-fable/spec.md) "Program review outcomes")
docs/easycla-ss-migration/role-mapping-feasibility.md:228
- This spike still targets
/signatures/user/{userID}, but M1 now uses the composed/v4/my-clasendpoint with its own ACS resource/policy and subject binding. A result from the generic route cannot establish whether the actual M1 path admits role-less users, and the M2M fallback no longer matches P10. Update the spike to exercise/my-clasand treat a gateway 403 as an M1 policy deployment gap.
2. **Role-less user → M1 read path.** Same exchange for a user with no ACS roles; `GET /cla-service/v4/signatures/user/{their-userID}`. **Outcome unknown — this is the decision point**: 200 ⇒ M1 uses user tokens as designed; 403 ⇒ file the ACS policy addition or fall back to M2M + server-side subject binding.
docs/easycla-ss-migration/spike-runbook.md:68
- This cannot validate spike 2 as written: it calls the generic signatures route, whose ACS policy differs from M1's
/v4/my-clas, and uses$TOKEN_Balthough step 1 assigns$TOKEN. That can produce a blank-bearer 403 or a path-specific result unrelated to M1. Exercise the actual M1 endpoint with the token variable created by step 1.
"https://api-gw.dev.platform.linuxfoundation.org/cla-service/v4/signatures/user/$USER_ID" \
-H "Authorization: Bearer $TOKEN_B"
docs/easycla-ss-migration/spike-runbook.md:72
- The M2M fallback is stale after P10 selected user-scoped tokens plus an authenticated-user ACS policy for the read-only M1 endpoint. For the actual
/my-clastest, a gateway 403 indicates that the endpoint's ACS resource/policy is missing or not deployed in that environment; it should not redirect implementation to the rejected fallback.
- **200** → **role-less users pass the secured router.** M1/M3 use user tokens exactly as proposed (P3); no fallback needed. Best outcome.
- **403 at the gateway** → warden denies users with no CLA role on this path. M1 then needs **either** a small ACS policy entry admitting authenticated users to these read paths, **or** the M2M fallback (SS server calls with a service token and binds the session userID itself — still Option A). Capture the warden response so we can size the policy change.
Address review comments from copilot[bot], coderabbitai: - docs/easycla-ss-migration/spike-runbook.md: drop the "grab it from a debug log" refresh-token suggestion; retrieve via controlled session-store inspection or PKCE, and add do-not-log / revoke-after-spike guidance (per copilot[bot], coderabbitai) - docs/easycla-ss-migration/architecture-proposal.md: note the later P10 approval (2026-07-28) and confirmation (2026-07-30) dates in the header so it no longer reads as stale against the P10 cell (per coderabbitai) - docs/easycla-ss-migration/role-mapping-feasibility.md: use "Approved List" terminology in prose per repo convention; API path segments (/approval-list) left unchanged (per coderabbitai) Resolves 3 review threads. Signed-off-by: Michal Lehotsky <mlehotsky@linuxfoundation.org>
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 4 out of 4 changed files in this pull request and generated no new comments.
Comments suppressed due to low confidence (14)
docs/easycla-ss-migration/README.md:12
- This link is broken on the current
devbase: only the nestedm1-my-cla/spec set exists there, while the root program spec is still being added by the open PR #5132. Merge that dependency first or point this entry at documents that already exist.
Implementation-level specifications (milestone scopes, acceptance criteria, per-milestone plans — used by the Spec Kit workflow) live separately in [specs/001-easycla-ss-integration-fable/](../../specs/001-easycla-ss-integration-fable/spec.md). This folder is for evaluating the architecture; that folder is for building it.
docs/easycla-ss-migration/spike-runbook.md:36
- JWT segments use unpadded base64url, so plain
base64 -dcan reject valid payloads containing-/_or missing padding. Decode as base64url; otherwise this checkpoint can falsely report that a valid token has no claims.
echo "$TOKEN" | cut -d. -f2 | base64 -d 2>/dev/null | jq . # inspect claims
docs/easycla-ss-migration/role-mapping-feasibility.md:228
- This spike targets the superseded per-user-signatures route, so its result does not answer whether ACS admits role-less users to M1's path-specific
/my-clasresource. Run the decision test against the actual M1 endpoint.
2. **Role-less user → M1 read path.** Same exchange for a user with no ACS roles; `GET /cla-service/v4/signatures/user/{their-userID}`. **Outcome unknown — this is the decision point**: 200 ⇒ M1 uses user tokens as designed; 403 ⇒ file the ACS policy addition or fall back to M2M + server-side subject binding.
docs/easycla-ss-migration/spike-runbook.md:51
-o /dev/nulldiscards the response body, but the 403 branch below requires that body to distinguish a gateway denial from a v4 scope denial. Print both the body and status code so this spike records its decision evidence.
curl -s -o /dev/null -w "%{http_code}\n" \
docs/easycla-ss-migration/spike-runbook.md:67
- The shipped M1 implementation superseded
/signatures/user/{userID}with/my-clas. Because ACS policies are path-specific, exercising the legacy route cannot establish whether a role-less user can reach the actual M1 endpoint.
"https://api-gw.dev.platform.linuxfoundation.org/cla-service/v4/signatures/user/$USER_ID" \
docs/easycla-ss-migration/spike-runbook.md:68
- Step 1 assigns the exchanged token to
TOKEN, but this command sends the undefinedTOKEN_B, producing an empty bearer credential and an authentication failure unrelated to the role test.
-H "Authorization: Bearer $TOKEN_B"
docs/easycla-ss-migration/spike-runbook.md:66
- This command also discards the response body even though the decision branch below says to capture the warden response. Preserve the body so a 403 can be attributed correctly.
curl -s -o /dev/null -w "%{http_code}\n" \
docs/easycla-ss-migration/role-mapping-feasibility.md:112
- This note presents the legacy
/signatures/user/{userID}route as the SS authorization boundary, but M1 now uses/my-clas; the M1 contract explicitly marks the per-user route as superseded. Retain the enforcement fact, but label it legacy so readers do not design SS around userID binding.
| `GET /signatures/user/{userID}` | No per-user ownership check | confirmed in M1 research (R3) | SS server must bind userID to session |
docs/easycla-ss-migration/architecture-proposal.md:67
- These M1–M6 links do not resolve on the current
devbase. M1 currently lives underm1-my-cla/, while the root milestone files are part of open PR #5132; merge that PR first or avoid publishing links to unavailable files.
| [M1](../../specs/001-easycla-ss-integration-fable/01-milestone-read-only-me-lens-fable.md) | Read-only "My CLAs" (Me lens) | — | S |
| [M2](../../specs/001-easycla-ss-integration-fable/02-milestone-sign-icla-fable.md) | Sign ICLA in SS (a: GitHub, b: GitLab, c: Gerrit) | — | M |
| [M3](../../specs/001-easycla-ss-integration-fable/03-milestone-sign-ecla-fable.md) | Sign ECLA in SS (a–c per platform) | Contributor Console + landing page | L |
| [M4](../../specs/001-easycla-ss-integration-fable/04-milestone-ccla-org-lens-fable.md) | CCLA management (Organization lens) | Corporate Console + its BFF | XL |
| [M5](../../specs/001-easycla-ss-integration-fable/05-milestone-project-lens-pcc-fable.md) | EasyCLA admin (Project lens) — *decision-gated* | PCC EasyCLA module | L |
docs/easycla-ss-migration/README.md:10
- The reading order omits the newly added spike runbook, and the proposal summary stops at P8 even though
architecture-proposal.mdnow includes P9 and P10. Add the runbook and update the proposal range so this index covers the PR's full contents.
This issue also appears on line 12 of the same file.
1. **[architecture-proposal.md](architecture-proposal.md)** — start here. Current state, milestones, what leadership already settled, the proposed architecture (P1–P8), top risks, and what the review should challenge.
2. **[role-mapping-feasibility.md](role-mapping-feasibility.md)** — the supporting deep analysis for the roles/permissions bridge (P2/P3): how EasyCLA v4 authorization actually works, token paths, read paths, options assessment, and the spike list. All claims cite `file:line`.
3. **[Slide deck (Google Slides)](https://docs.google.com/presentation/d/1FQJOpiETIO_H10c6_eP2Zu-LM7qlvG_t7blhRm--2KA/edit)** — presentation for the review session.
docs/easycla-ss-migration/architecture-proposal.md:9
- The linked root
spec.mdis not present on the currentdevbase; it belongs to the still-open PR #5132. As written, this architecture entry point ships with a broken deep-dive link unless that PR is merged first.
This issue also appears on line 63 of the same file.
This document is self-contained for review purposes. Implementation-level specs (milestone scopes, acceptance criteria, M1 plan) live in [specs/001-easycla-ss-integration-fable/](../../specs/001-easycla-ss-integration-fable/spec.md) — linked below only for deep dives.
docs/easycla-ss-migration/role-mapping-feasibility.md:8
- All three cited spec targets are absent from the current
devbase and are being introduced separately by open PR #5132. These evidence links will be broken if this PR lands first, so establish that merge dependency or use existing sources.
This issue also appears in the following locations of the same file:
- line 112
- line 228
**Basis**: [00-overview-fable.md](../../specs/001-easycla-ss-integration-fable/00-overview-fable.md) §2.4/§3, [04-milestone-ccla-org-lens-fable.md](../../specs/001-easycla-ss-integration-fable/04-milestone-ccla-org-lens-fable.md) role options; code citations verified 2026-07-15, re-checked 2026-07-20
**Answers**: the open engineering action from the 2026-07-15 leadership review ([spec.md](../../specs/001-easycla-ss-integration-fable/spec.md) "Program review outcomes")
docs/easycla-ss-migration/spike-runbook.md:81
- The v4 handler returns
400 Bad Request, not 404, whenGetCompanyByIDcannot resolve this path's internal company ID (v2/company/handlers.go:136-140). Following this note would send operators looking for the wrong status.
- A **404** on the cla-managers call usually means wrong company-internal-id vs SFID — the path wants the internal company ID, not the Salesforce ID.
docs/easycla-ss-migration/spike-runbook.md:34
- On an OAuth error response,
jq -r .access_tokenreturns the literalnullinstead of surfacinginvalid_grant,access_denied, or the description that the checkpoint asks operators to capture. Make the extraction fail with the provider's error details so token-exchange failures are diagnosable.
This issue also appears in the following locations of the same file:
- line 36
- line 51
- line 66
- line 67
- line 68
| jq -r .access_token)
Address a second round of review comments from coderabbitai: - spike-runbook.md: reserve "spike 1 passed" for the full-chain 200 from the secured cla-managers endpoint; the token-claim checkpoint is a prerequisite - spike-runbook.md: Spike 2 used an undefined $TOKEN_B — use $TOKEN (set by re-running Step 1 with user B's refresh token) with a clarifying comment Resolves 2 review threads. Signed-off-by: Michal Lehotsky <mlehotsky@linuxfoundation.org>
Collaborator
Author
Review Feedback AddressedCommits: This PR was rescoped to the EasyCLA↔SS architecture docs only ( Changes Made
Declined
Threads ResolvedAll previously-unresolved threads addressed. The 44 threads anchored to the removed |
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 4 out of 4 changed files in this pull request and generated no new comments.
Comments suppressed due to low confidence (16)
docs/easycla-ss-migration/README.md:12
- This target does not exist in the branch: only
specs/001-easycla-ss-integration-fable/m1-my-cla/is present. The PR that adds the linked program-level files (#5132) is still open, so this link will 404 if this PR lands first. Either make this PR depend on #5132 or link the currently available M1 spec and describe the remaining specs as pending.
Implementation-level specifications (milestone scopes, acceptance criteria, per-milestone plans — used by the Spec Kit workflow) live separately in [specs/001-easycla-ss-integration-fable/](../../specs/001-easycla-ss-integration-fable/spec.md). This folder is for evaluating the architecture; that folder is for building it.
docs/easycla-ss-migration/spike-runbook.md:36
- JWT payloads are base64url-encoded and commonly omit padding; plain
base64 -dcan reject-/_or truncated padding, causing this prerequisite check to fail on valid tokens. Decode as base64url with restored padding.
echo "$TOKEN" | cut -d. -f2 | base64 -d 2>/dev/null | jq . # inspect claims
docs/easycla-ss-migration/spike-runbook.md:53
-o /dev/nulldiscards the response body, but the next instruction requires that body to distinguish a gateway 403 from a v4 403. Preserve it while printing the status.
curl -s -o /dev/null -w "%{http_code}\n" \
"https://api-gw.dev.platform.linuxfoundation.org/cla-service/v4/company/$COMPANY_ID/project/$PROJECT_SFID/cla-managers" \
-H "Authorization: Bearer $TOKEN"
docs/easycla-ss-migration/spike-runbook.md:68
- This tests the superseded
/signatures/user/{userID}route rather than M1's shipped/my-clasroute, so its path-specific warden result cannot validate M1. Call the actual M1 endpoint and preserve its body for diagnosis.
"https://api-gw.dev.platform.linuxfoundation.org/cla-service/v4/signatures/user/$USER_ID" \
docs/easycla-ss-migration/README.md:10
- The new spike runbook is not linked anywhere in the repository, so the README entry point leaves one of this PR's four deliverables undiscoverable. Add it to the reading order.
This issue also appears on line 12 of the same file.
3. **[Slide deck (Google Slides)](https://docs.google.com/presentation/d/1FQJOpiETIO_H10c6_eP2Zu-LM7qlvG_t7blhRm--2KA/edit)** — presentation for the review session.
docs/easycla-ss-migration/README.md:8
- This entry is stale: the proposal now defines P1–P10, so the advertised scope omits P9 and P10.
1. **[architecture-proposal.md](architecture-proposal.md)** — start here. Current state, milestones, what leadership already settled, the proposed architecture (P1–P8), top risks, and what the review should challenge.
docs/easycla-ss-migration/architecture-proposal.md:9
- The linked root
spec.mdis absent from this branch, as are all six milestone files linked below; they are being added by the still-open #5132. These deep-dive links will all 404 unless #5132 merges first, so declare/enforce that dependency or point to documents already present.
This document is self-contained for review purposes. Implementation-level specs (milestone scopes, acceptance criteria, M1 plan) live in [specs/001-easycla-ss-integration-fable/](../../specs/001-easycla-ss-integration-fable/spec.md) — linked below only for deep dives.
docs/easycla-ss-migration/role-mapping-feasibility.md:8
- All three cited program-level targets are absent from this branch and belong to the still-open #5132, so the memo's basis and decision links currently 404. Merge that dependency first or replace these with links to available material.
**Basis**: [00-overview-fable.md](../../specs/001-easycla-ss-integration-fable/00-overview-fable.md) §2.4/§3, [04-milestone-ccla-org-lens-fable.md](../../specs/001-easycla-ss-integration-fable/04-milestone-ccla-org-lens-fable.md) role options; code citations verified 2026-07-15, re-checked 2026-07-20
**Answers**: the open engineering action from the 2026-07-15 leadership review ([spec.md](../../specs/001-easycla-ss-integration-fable/spec.md) "Program review outcomes")
docs/easycla-ss-migration/role-mapping-feasibility.md:18
- The blanket “without any … EasyCLA changes” conclusion conflicts with P10 in this PR: the trusted My CLAs identity path explicitly requires an EasyCLA
azpallow-list and in-handler JWKS verification. Narrow this claim to the general bridge/token path and call out P10 as the exception; otherwise readers may omit required auth hardening.
Self Serve can call the existing EasyCLA v4 APIs with user-scoped tokens, **without any gateway or EasyCLA changes**, because the entire v4 authorization chain keys on the **user's identity (LF username)** — not on which Auth0 client or audience minted the token:
docs/easycla-ss-migration/role-mapping-feasibility.md:29
- M1 is no longer an unknown condition: the shipped
/my-claspaths are registeredanyRole: truefor authenticated users (docs/MY_CLAS_API.md:68-74). Keep the unknown scoped to concrete M3 paths, and treat M1 only as a rollout/sync verification.
1. ACS's gateway-level path authorization must admit **ordinary users (no CLA role)** to the v4 *read* endpoints M1/M3 need.
docs/easycla-ss-migration/role-mapping-feasibility.md:198
- This repeats the outdated “no EasyCLA auth change” conclusion despite P10 adding handler-level caller validation. Record the narrow P10 exception here so the options assessment is consistent with the approved architecture.
**A. Bridge — confirmed, cheaper than assumed.** The milestone-04 concern "v4 might not accept SS tokens without gateway changes" is resolved negatively: no gateway change, no EasyCLA auth change, no new token infrastructure (§4). Residual costs: the spikes' outcomes (possibly a small ACS policy addition), and inheriting today's failure modes unchanged — async ACS assignment (server-side retries stay in SS), the documented **one-company-at-a-time role limitation** (the bridge surfaces but cannot fix it), and the enforcement unevenness in §3, which SS must mirror, not mask.
docs/easycla-ss-migration/role-mapping-feasibility.md:228
- This is not the shipped M1 path. M1 uses
GET /v4/my-clas, while/signatures/user/{userID}is the superseded, ICLA-only API and has a different ACS resource/path; its result cannot answer whether M1 works. The M1 policy is also alreadyanyRole: true, so a 403 now indicates rollout/sync failure rather than an open architecture decision.
2. **Role-less user → M1 read path.** Same exchange for a user with no ACS roles; `GET /cla-service/v4/signatures/user/{their-userID}`. **Outcome unknown — this is the decision point**: 200 ⇒ M1 uses user tokens as designed; 403 ⇒ file the ACS policy addition or fall back to M2M + server-side subject binding.
docs/easycla-ss-migration/spike-runbook.md:34
- On an Auth0 error response,
.access_tokenis absent andjq -rsilently emits the literalnull, hiding theinvalid_grant/access_denieddetails this runbook says to diagnose. Makejqfail and surface Auth0's message.
This issue also appears in the following locations of the same file:
- line 36
- line 51
- line 68
| jq -r .access_token)
docs/easycla-ss-migration/spike-runbook.md:82
- The project CLA-managers handler returns
400 Bad Requestwhen the internal company lookup fails (v2/company/handlers.go:136-140), not 404. As written, this gotcha sends operators toward the wrong diagnosis/status.
- A **404** on the cla-managers call usually means wrong company-internal-id vs SFID — the path wants the internal company ID, not the Salesforce ID.
docs/easycla-ss-migration/spike-runbook.md:56
- Use the singular possessive for the numbered spike.
- **200** → the whole chain works: SS token → gateway (issuer check) → ACS warden allow → X-ACL injected → v4 scope check passes. Spikes 1's happy path confirmed.
docs/easycla-ss-migration/spike-runbook.md:73
- M1's ACS resources are already registered
anyRole: truefor authenticated users (docs/MY_CLAS_API.md:68-74), so this is no longer an open policy-vs-M2M decision. A gateway 403 now indicates that the M1 ACS registration was not synced or is stale in dev.
- **200** → **role-less users pass the secured router.** M1/M3 use user tokens exactly as proposed (P3); no fallback needed. Best outcome.
- **403 at the gateway** → warden denies users with no CLA role on this path. M1 then needs **either** a small ACS policy entry admitting authenticated users to these read paths, **or** the M2M fallback (SS server calls with a service token and binds the session userID itself — still Option A). Capture the warden response so we can size the policy change.
…point answer P10: the lfx-v2-auth-service currently rejects SS session tokens on user_identity.list (invalid audience, verified in dev) and reuses the caller JWT against the Auth0 Management API — a small auth-service change is a prerequisite for the #1216 condition-1 flow. P2: Eric confirmed (2026-07-31, ARCH-406) /v1/me/permissions/checks has no decommission timeline, closing the M4/M5 dependency question. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Signed-off-by: Michal Lehotsky <mlehotsky@linuxfoundation.org>
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 4 out of 4 changed files in this pull request and generated no new comments.
Suppressed comments (11)
docs/easycla-ss-migration/README.md:12
- This entry-point link is broken in this PR: the checkout contains only
specs/001-easycla-ss-integration-fable/m1-my-cla/, whilespec.mdand the M2–M6 documents remain in the separate, still-open #5132. Either merge that prerequisite first or point this page only at material that exists on the target branch.
Implementation-level specifications (milestone scopes, acceptance criteria, per-milestone plans — used by the Spec Kit workflow) live separately in [specs/001-easycla-ss-integration-fable/](../../specs/001-easycla-ss-integration-fable/spec.md). This folder is for evaluating the architecture; that folder is for building it.
docs/easycla-ss-migration/spike-runbook.md:36
- JWT segments use unpadded base64url, so plain
base64 -dcan fail on-/_characters or missing padding; stderr is then suppressed and this checkpoint produces no claims. Decode with a base64url-aware operation so valid tokens are inspected reliably.
echo "$TOKEN" | cut -d. -f2 | base64 -d 2>/dev/null | jq . # inspect claims
docs/easycla-ss-migration/role-mapping-feasibility.md:18
- This unqualified “without any … EasyCLA changes” conclusion conflicts with P10, which requires new in-handler
azp/JWKS enforcement in EasyCLA for the My CLAs read path. Scope this statement to the existing role-gated M3–M5 APIs so readers do not infer that M1 also needs no EasyCLA work.
Self Serve can call the existing EasyCLA v4 APIs with user-scoped tokens, **without any gateway or EasyCLA changes**, because the entire v4 authorization chain keys on the **user's identity (LF username)** — not on which Auth0 client or audience minted the token:
docs/easycla-ss-migration/spike-runbook.md:61
- This block calls the legacy user-signatures endpoint rather than the merged M1
/my-clasendpoint, so it does not test the route whose role-less-user policy matters. It also discards the 403 body that line 73 asks the operator to capture. Call/my-clasand preserve the response body.
Re-run step 1 with user B's refresh token, then hit the M1 read endpoint. First resolve B's EasyCLA userID (the SS server does this via identity resolution; for the spike, use a known dev userID for B):
docs/easycla-ss-migration/architecture-proposal.md:9
- The linked parent
spec.mdis absent from the target tree, as are all six milestone files linked below. Those files are currently supplied only by the separate open #5132, so this architecture document will land with dead navigation unless that PR is made a prerequisite or these links are retargeted.
This document is self-contained for review purposes. Implementation-level specs (milestone scopes, acceptance criteria, M1 plan) live in [specs/001-easycla-ss-integration-fable/](../../specs/001-easycla-ss-integration-fable/spec.md) — linked below only for deep dives.
docs/easycla-ss-migration/role-mapping-feasibility.md:8
- Both basis links are broken in the current tree: only the
m1-my-cla/subtree exists, while00-overview-fable.md,04-milestone-ccla-org-lens-fable.md, and the parentspec.mdare still in open PR #5132. Merge that dependency first or replace these references with available sources.
This issue also appears on line 18 of the same file.
**Basis**: [00-overview-fable.md](../../specs/001-easycla-ss-integration-fable/00-overview-fable.md) §2.4/§3, [04-milestone-ccla-org-lens-fable.md](../../specs/001-easycla-ss-integration-fable/04-milestone-ccla-org-lens-fable.md) role options; code citations verified 2026-07-15, re-checked 2026-07-20
**Answers**: the open engineering action from the 2026-07-15 leadership review ([spec.md](../../specs/001-easycla-ss-integration-fable/spec.md) "Program review outcomes")
docs/easycla-ss-migration/role-mapping-feasibility.md:228
- This is no longer the M1 read path: merged PR #5125 introduced
/v4/my-clas, while/v4/signatures/user/{userID}is the legacy endpoint and returns only ICLAs. Testing the legacy route cannot decide whether the shipped My CLAs route admits an ordinary user; update the spike to call/cla-service/v4/my-clas.
2. **Role-less user → M1 read path.** Same exchange for a user with no ACS roles; `GET /cla-service/v4/signatures/user/{their-userID}`. **Outcome unknown — this is the decision point**: 200 ⇒ M1 uses user tokens as designed; 403 ⇒ file the ACS policy addition or fall back to M2M + server-side subject binding.
docs/easycla-ss-migration/spike-runbook.md:53
- The command discards the response body with
-o /dev/null, but the immediately following 403 instructions require that body to distinguish gateway denial from a v4 denial. Keep the body in the output so the spike records the evidence it is designed to collect.
This issue also appears on line 61 of the same file.
curl -s -o /dev/null -w "%{http_code}\n" \
"https://api-gw.dev.platform.linuxfoundation.org/cla-service/v4/company/$COMPANY_ID/project/$PROJECT_SFID/cla-managers" \
-H "Authorization: Bearer $TOKEN"
docs/easycla-ss-migration/role-mapping-feasibility.md:113
- This Swagger citation no longer points at the public CLA-manager endpoint: lines 3829–3836 are now part of the company-delete/lookup section after the My CLAs additions. The endpoint and its
security: []declaration are at lines 3941–3947, so the evidence link should be updated.
| `GET /company/{id}/cla-group/{g}/cla-managers` | **No auth at all** (`security: []`; public router) | `v2/company/handlers.go:162-164`; `swagger/cla.v2.yaml:3829-3836` | Useful zero-auth read path (§5) |
docs/easycla-ss-migration/role-mapping-feasibility.md:238
- The verification ledger is stale in the current tree: v4 auth wiring is now at
cmd/server.go:483, not 480–481, and Swagger line 3797 is not a spike endpoint (the secured manager route starts at 3909). Recheck the ledger's local line references after the merged M1 changes so its “verified in code” claim remains actionable.
Gateway routing/middleware: `lfx-gateway/dynamic/services/cla-service.yaml:3-53`, `dynamic/middleware.yaml:21-70` · ACS plugin deny + header injection: `traefik-acs-authorizer-middleware/acs.go:328-331,378,399-419` · ACS warden/permission-check 30-min response cache: `acs/middleware/cache.go:39,52-54` · v4 auth wiring: `cla-backend-go/cmd/server.go:480-481` · X-ACL decode: `lfx-kit@v0.1.33/auth/handlers.go:55-82` · scope-not-role checks: `cla-backend-go/utils/utils_user_auth_lambda.go:33-268`, `lfx-kit/auth/user.go:91-269` · per-endpoint checks as tabled in §3 · dual manager bookkeeping: `v2/cla_manager/service.go:242,405` (+ auto-assigned `contact` role, `service.go:417-427`) · Auth0 claim gating by audience: `auth0-terraform/src/actions/custom_claims.js:387-395` · SS secondary token: `lfx-self-serve/apps/lfx-one/src/server/middleware/auth.middleware.ts:230-249`, `server/utils/refresh-token-exchange.util.ts:20-101`, `apps/lfx-one/.env.example:146` · console BFF token/permissions: `lfx-corp-cla-console/backend/src/data/cla-api.ts:23-32`, `backend/src/data/user-api.ts:27-44,160-164` · ACS roles hardcoded + read APIs: `acs/userrole/repository.go:101-102`, `acs/userrole/transport_http.go:38-43,106-111` · v3-only permissions table: `cla-backend-go/user/repository_dynamo.go:114`, `auth/authorizer.go:143-166` · SS-native grant paths: `org-lens-access.service.ts:60-102`, `lfx-v2-fga-sync/docs/fga-catalog.md` · v4 Lambda has no HTTP event (invoked lambda-to-lambda): `cla-backend-go/serverless.yml:334-352` · spike endpoint paths/methods/routers: `swagger/cla.v2.yaml:2407,2518,3797`, `lfx-gateway/dynamic/services/cla-service.yaml:4-53`.
docs/easycla-ss-migration/README.md:10
- The entry point omits the newly added spike runbook, so it is not discoverable from the reading order, and it describes the proposal as P1–P8 even though the proposal now contains P1–P10. Add the runbook and update the proposal range.
This issue also appears on line 12 of the same file.
1. **[architecture-proposal.md](architecture-proposal.md)** — start here. Current state, milestones, what leadership already settled, the proposed architecture (P1–P8), top risks, and what the review should challenge.
2. **[role-mapping-feasibility.md](role-mapping-feasibility.md)** — the supporting deep analysis for the roles/permissions bridge (P2/P3): how EasyCLA v4 authorization actually works, token paths, read paths, options assessment, and the spike list. All claims cite `file:line`.
3. **[Slide deck (Google Slides)](https://docs.google.com/presentation/d/1FQJOpiETIO_H10c6_eP2Zu-LM7qlvG_t7blhRm--2KA/edit)** — presentation for the review session.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What
Adds the architecture-review context for the EasyCLA → LFX Self Serve migration under
docs/easycla-ss-migration/:README.md— entry point; links the architecture-review Google Slides deckarchitecture-proposal.md— milestone proposal (M1–M6) with the P-notes (incl. P10 trust-SS "My CLAs" identity read)role-mapping-feasibility.md— EasyCLA ↔ Self Serve roles/permissions feasibility memospike-runbook.md— spike runbookDocs only — no code changes.
Scope
Scoped to the architecture docs. The M1 spec-kit set (
specs/…/m1-my-cla/) is not part of this PR — M1 is shipped, and those spec docs already live ondevvia the My CLAs API PRs (#5125 / #5128).Related
Previously scoped to include the M1 spec-kit docs; rescoped to architecture docs only now that M1 has shipped.
🤖 Generated with Claude Code