feat(sign): integrate sanctions screening service enforcement#5078
feat(sign): integrate sanctions screening service enforcement#5078psrsingh wants to merge 7 commits into
Conversation
|
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 SSS configuration and startup wiring, extends the v2 sign service to accept an ChangesSanctions Screening Service Integration
Sequence DiagramsequenceDiagram
participant RequestCorporateSig as RequestCorporateSignature
participant CheckCompliance as checkCompanyCompliance
participant OrgService as organizationService
participant SSS as sss.Client
participant CompRepo as companyRepository
RequestCorporateSig->>CheckCompliance: checkCompanyCompliance(ctx, company)
CheckCompliance->>OrgService: Get organization (Link, Domains)
OrgService-->>CheckCompliance: organization data
CheckCompliance->>SSS: GetOrganizationStatus(domain, SFDCID?)
SSS-->>CheckCompliance: OrganizationStatus (Flagged/Clean)
alt Status == Flagged
CheckCompliance->>CompRepo: UpdateCompanySanctionStatus(companyID, true)
CompRepo-->>CheckCompliance: persisted
end
CheckCompliance-->>RequestCorporateSig: (sanctioned bool, error)
alt sanctioned == true
RequestCorporateSig-->>RequestCorporateSig: return "company is sanctioned" error
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 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 `@cla-backend-go/cmd/server.go`:
- Line 451: The v2SignService is being constructed with a nil SSS client which
leaves service.sssClient unset and causes checkCompanyCompliance to
short-circuit; replace the trailing nil in the sign.NewService(...) call (where
v2SignService is created) with the real SSS client instance (e.g., sssClient or
sanctionsService) that you initialize elsewhere in the server startup, ensuring
sign.NewService receives the proper SSS client so service.sssClient is set and
checkCompanyCompliance can perform sanctions checks.
In `@cla-backend-go/v2/sign/service.go`:
- Around line 2980-2989: The current logic only strips scheme and trailing
slashes from org.Link, which can leave paths, ports, or query strings in the
domain sent to sss.OrganizationStatusRequest; update the normalization to parse
org.Link with net/url (url.Parse) and use the parsed URL's hostname
(u.Hostname()) as the Domain for the sss.OrganizationStatusRequest (falling back
to the original org.Link trimmed if parsing fails), so
OrganizationStatusRequest.Domain is a bare hostname; update the code around
domain := org.Link and where req := sss.OrganizationStatusRequest{ Domain: ... }
to use the parsed hostname.
- Around line 249-264: The code currently skips compliance checks when comp ==
nil but later dereferences comp.CompanyID and calls requestCorporateSignature
with comp, which can panic if the company lookup returned (nil, nil); update the
guard in the sign flow to detect a missing company (comp == nil) and return a
clear not-found error (include input.CompanySfid if present for context) instead
of falling through; ensure this happens before any use of comp and before
calling checkCompanyCompliance/requestCorporateSignature so functions like
checkCompanyCompliance and requestCorporateSignature never receive a nil comp.
🪄 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: a559fa3a-3e68-4373-a0ff-cd157c5a1d8a
📒 Files selected for processing (4)
cla-backend-go/cmd/s3_upload/main.gocla-backend-go/cmd/server.gocla-backend-go/v2/sign/helpers.gocla-backend-go/v2/sign/service.go
337cc98 to
fcd1463
Compare
|
@lukaszgryglicki |
ec72938 to
bb11b46
Compare
|
There are some questions/decision points: #4985 (comment). I'm currently on the review... |
|
Regarding code rabbit and |
lukaszgryglicki
left a comment
There was a problem hiding this comment.
Action items
-
[Blocking] Won't compile:
checkCompanyCompliancecallss.companyRepo.UpdateCompanySanctionStatus(...), which exists on neithercompany.IRepository(company/repository.go:35) nor any implementation. Add it to the interface + DynamoDB repo (mirrorUpdateCompanyAccessList,repository.go:1239, updating theis_sanctionedattribute). -
[Blocking] Feature is inert: the SSS client is
nilatcmd/server.go:451andcmd/s3_upload/main.go:60, so nothing is screened in any environment. Construct it from config and pass it. Ready on branchunicron-sss-ecla-check: config keyscla-sss-base-url-<stage>+cla-sss-auth0-audience-<stage>(reusing the existingAuth0PlatformM2M creds) andsss.NewClientFromPlatformCredentials(...). Wire it once this PR adds thesssClientparam tosign.NewService. I'm implementing this in #5082 -
[High] Don't drop the legacy flag, and don't let SSS clear a manual block (D3): keep "block if
comp.IsSanctionedOR SSS flagged"; when persisting, only ever set the flag totruefrom SSS — never overwrite atruewith afalsefrom a (possibly stale) SSS "clean". -
[High] Make SSS unavailability explicit: branch on
*sss.RetryableError/*sss.TimeoutError/*sss.AuthErrorvs a definitiveflagged; implement the chosen fail-open/closed policy; and stop mapping SSS outages to HTTP 400 (handlers.goonly special-cases"is sanctioned") — return a 5xx with a clear "screening temporarily unavailable" message. -
[High] SSS required, not optional via flag: when SSS is required for the stage, a nil/misconfigured client must be an error, not a silent skip in
checkCompanyCompliance. Pair with acla-sss-required-<stage>gate so rollout can provision SSM params first. -
[High] PR-gating hot path + negative cache:
hasUserSignedruns per commit author (concurrently) and now adds an org-serviceGetOrganization+ SSS round-trip each; on error,GetCommitAuthorSignedStatus(github/github_repository.go) marks the author unsigned and writes a negative-TTL cache entry, so a transient SSS failure pins a contributor as unsigned past recovery, and already-"signed" authors (no-TTL positive cache) are never re-screened. Prefer gating on the stored flag here and doing the live SSS call only at sign time; never negative-cache an SSS-error outcome. Eventually some caching mechanism at least in current lambda scope so no org check happens twice - check once and then use memory cache. -
[Medium] Domain source: read the org-service
Domainsfield first (organization-service/models/organization.go:66); only parseLinkas a fallback, and when you do, prependhttps://beforeurl.ParsesoHostname()works on scheme-less values, and strip a leadingwww.. -
[Medium] Persistence churn:
UpdateCompanySanctionStatusis called on every check, including unchanged/clean → a DynamoDB write per signing/recheck. Write only on change. -
**[Medium] Caching: Org-service calls are a concern, cache the resolved domain on the company record; the SSS status itself should stay authoritative via the flag (updated on writes), with staleness handled on the SSS side.
-- **[Medium]: actual ECLA check happen in cla-backend-legacy request_employee_signature / check_and_prepare_employee_signature - this is where it should block ECLA flow IMHO. This is even before we start preparing ECLA.
Thanks for the detailed review. I agree with keeping I'll address the missing repository method, wire up the SSS client from config, switch domain resolution to use For PR-gating, I agree that introducing a live SSS dependency on the hot path is risky. Using the persisted flag there and limiting live SSS checks to signing flows seems like the better approach. |
|
|
|
Also note that |
I'll add a small in-memory cache keyed by company/org so each organization is checked at most once per execution window (or with a short TTL of around 5 minutes), which should avoid repeated org-service and SSS calls on the hot path. |
|
You don't need to care about staleness inside SSS - this will be handled as a separate feature - like we will probably have some vendor maintained queue that contains updates since last check which SSS will poll for changes periodically. This will be a new feature. |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
cla-backend-go/v2/sign/service_sss_test.go (1)
63-72: ⚡ Quick winAlign the test name with the error type under test.
Line 63 says this covers availability errors, but Line 66 uses
sss.AuthError. As written, it won't catch regressions in optional-mode retryable/timeout handling. Either rename the test to auth-specific behavior or switch the fixture tosss.RetryableError.🤖 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 `@cla-backend-go/v2/sign/service_sss_test.go` around lines 63 - 72, The test TestHandleSSSErrorOptionalAllowsAvailabilityErrors mismatches its name and fixture: it uses sss.AuthError but intends to cover availability/retryable errors. Update the test either by renaming TestHandleSSSErrorOptionalAllowsAvailabilityErrors to reflect auth behavior (e.g., TestHandleSSSErrorOptionalAllowsAuthErrors) or change the injected error in the call to svc.handleSSSError to an availability-type error such as sss.RetryableError (or sss.TimeoutError) so the test actually exercises optional-mode retryable/timeout handling; ensure the assertion logic remains the same.
🤖 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 `@cla-backend-go/company/repository.go`:
- Around line 1290-1332: Current code blindly flips is_sanctioned based on the
caller value, which allows a false update to clear a manual block; update the
logic so clears cannot override manual/other-source blocks: either 1) only allow
setting true (if sanctioned==false return nil early), or 2) make the flag
source-aware by adding/using a source attribute and a conditional UpdateItem
that only allows clearing when the existing source is the SSS-managed source.
Concretely, modify the path around repo.GetCompany/currentCompany.IsSanctioned
to (a) return early when sanctioned==false if you choose the "only-set-true"
approach, or (b) when supporting sources add an attribute like
"is_sanctioned_source" (or read an existing source field on currentCompany) and
change the UpdateItem to include a ConditionExpression that prevents setting ":s
= false" unless the existing source equals the SSS source; update the
UpdateExpression/ExpressionAttributeNames/Values and the call to
repo.dynamoDBClient.UpdateItem accordingly.
---
Nitpick comments:
In `@cla-backend-go/v2/sign/service_sss_test.go`:
- Around line 63-72: The test TestHandleSSSErrorOptionalAllowsAvailabilityErrors
mismatches its name and fixture: it uses sss.AuthError but intends to cover
availability/retryable errors. Update the test either by renaming
TestHandleSSSErrorOptionalAllowsAvailabilityErrors to reflect auth behavior
(e.g., TestHandleSSSErrorOptionalAllowsAuthErrors) or change the injected error
in the call to svc.handleSSSError to an availability-type error such as
sss.RetryableError (or sss.TimeoutError) so the test actually exercises
optional-mode retryable/timeout handling; ensure the assertion logic remains the
same.
🪄 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: 78a5478f-96f0-4fe0-a53e-ae9abfd6cc90
📒 Files selected for processing (8)
cla-backend-go/cmd/s3_upload/main.gocla-backend-go/cmd/server.gocla-backend-go/company/repository.gocla-backend-go/config/config.gocla-backend-go/config/local.gocla-backend-go/config/ssm.gocla-backend-go/v2/sign/service.gocla-backend-go/v2/sign/service_sss_test.go
🚧 Files skipped from review as they are similar to previous changes (1)
- cla-backend-go/cmd/s3_upload/main.go
|
@lukaszgryglicki PTAL, I've pushed the requested changes and addressed the remaining comments. |
|
Need to finish a few items, will get to this soon. |
okay |
|
I'm on it now, is this rebased with current |
Signed-off-by: psrsingh <psr.singh336@gmail.com>
Signed-off-by: psrsingh <psr.singh336@gmail.com>
lukaszgryglicki
left a comment
There was a problem hiding this comment.
-
[Blocking] Rebase + reconcile with the already-merged #5082. This PR is based on
devfrom before #5082 landed: patch 3 adds a secondtype SSS structandSSS SSSfield toconfig/config.go, rewritesloadSSMConfig, and re-wires the client incmd/server.go— butdevalready hasconfig.SSS{BaseURL, Audience},config/ssm.goloadOptionalSSSConfig(loadscla-sss-base-url-<stage>+cla-sss-auth0-audience-<stage>), andsss/from_config.go(NewClientFromPlatformCredentials). On rebase this is a duplicate-type/field compile error and overlapping edits. Collapse to ONE config approach (keep the merged one; see next two items). -
[Blocking] SSS is never actually screened in production.
checkCompanyCompliancepasses*organization-service/models.OrganizationtoresolveDomain, which doesorg.(interface{ GetDomains() []string })andorg.(interface{ GetLink() string }). That model has fieldsDomains stringandLink stringand NOGetDomains()/GetLink()methods (verified: none inv2/organization-service/models/), so both assertions fail at runtime,resolveDomainreturns"", and the SSS call is skipped for every org.service_sss_test.gopasses only becausetestOrgimplements those methods. Fix: read the concrete fields —org.Domains(astring; split on,, take first non-empty), fall back to parsingorg.Link— keeping the prepend-https://+Hostname()+ strip-www.logic. Drop theinterface{}indirection. -
[Blocking] Use the shared platform M2M credentials, not new dedicated SSS creds. Per
auth0-terraformthe SSS audience grant is on the existing"LF CLA Lambda Service"client (grants_sanctions_screening.tf:6); there is no dedicated EasyCLA-SSS Auth0 client, and nocla-sss-auth0-client-id/secret/domainexist in SSM or lfx-secrets-management. This PR'sSSS.{Auth0Domain,Auth0ClientID,Auth0ClientSecret}would therefore be empty in every deployed env and the client would never build. ReuseAuth0Platform.{ClientID,ClientSecret,URL}(exactly what the mergedsss.NewClientFromPlatformCredentialsalready does); onlyBaseURL+Auth0Audienceare SSS-specific and both are already loaded from SSM ondev. -
[Blocking] This PR's
loadSSMConfigloads onlycla-sss-required-<stage>—BaseURL/Auth0*are never read from SSM, so a deployed Lambda builds no client even before the conflict above. Resolved automatically by adopting the merged loader per items 1 and 3; otherwise add the missing SSM reads. -
[High] A nil client ignores
sssRequired.checkCompanyCompliancedoesif s.sssClient == nil { return false, nil }, allowing the action even whensssRequired == true— which defeats "required". When required and the client is nil it must block (error). Alsocmd/server.gosilently leavessssClient = nilwhen SSS config is incomplete with no warning; whenRequiredis true it should fail fast (or at least log loudly) at startup, not silently disable screening. -
[High] The contributor ECLA path is still not screened live. The real employee-acknowledgement endpoints are in
cla-backend-legacy(request_employee_signature/check_and_prepare_employee_signature), which still gate only on the storedis_sanctionedflag (handlers.go:8865). This PR'shasUserSignedchange is the GitHub PR-status path incla-backend-go, not the contributor ECLA endpoint. For full coverage, add the live SSS check in the legacy endpoints (around the existing flag check). Notecla-backend-legacyis a separate module with no SSS client today — decide: copy a minimal client / shared module / call v3 — or scope it as an explicit follow-up. Ideally bothcla-backend-legacyandcla-backend-goshoudl us ethe same shared client for them both. -
[Medium] Caching hard errors for 5 min extends outages.
complianceCachestoreserrand serves it forcomplianceCacheTTL(5m), so in a required stage a transient SSS timeout keeps blocking an org for up to 5 min after SSS recovers. Cache only definitive clean/flagged results; don't cache transient/availability errors (or give errors a much shorter TTL). No negative results cache please. -
[Medium]
handleSSSErrorblocks onBadRequesteven when not required. A 400 (our own malformed request, e.g. a bad domain) returns an error unconditionally; whensssRequired == falseit should route through the sameallowWhenOptionalpath rather than blocking the user for our bug. -
[Low]
complianceCacheis unbounded — one entry per distinct org for the warm Lambda's lifetime, evicted only on access-after-expiry. Fine short-term; add a size cap or periodic sweep if org volume is high. -
[Low] Confirm
orgClient.GetOrganization(ctx, company.CompanyExternalID)is keyed correctly (Salesforce org SFID) and thatCompanyExternalIDis populated for companies reaching this path; it's used both as the lookup id and asSFDCID. -
[Sync check] EasyCLA must not add its own long result cache — keep the in-process cache short and never persist a stale "clean".
Signed-off-by: psrsingh <psr.singh336@gmail.com>
Signed-off-by: psrsingh <psr.singh336@gmail.com>
907f5a0 to
ab0669f
Compare
Signed-off-by: psrsingh <psr.singh336@gmail.com>
fed873d to
ac79c85
Compare
I've gone through the feedback and updated the branch. The changes now use the SSS implementation already merged on One thing I wasn't completely sure about is required-mode enforcement after removing the old |
|
Regarding |
Signed-off-by: psrsingh <psr.singh336@gmail.com>
Thanks, I updated the implementation to use the SSM-backed required flag and pushed the latest changes. Would appreciate another look when you have time. |
|
Checking. |
lukaszgryglicki
left a comment
There was a problem hiding this comment.
-
[Main remaining gap] Share the SSS client across both backends and add the ECLA prepare-signature check. The contributor ECLA endpoints live in
cla-backend-legacy(request_employee_signature, plus its gerrit variant / thecheck_and_prepareprecheck) and still gate only on the storedis_sanctionedflag. To get the check at BOTH levels as intended — prepare-signature (CCLA ✓ inRequestCorporateSignature, ECLA ✗) and PR-gate (✓ inhasUserSigned):- (a) Extract
cla-backend-go/sssinto a small stdlib-only shared module (it has zero internal deps, verified) that BOTH backends import — rather than duplicating it or having legacy pull all of cla-backend-go. - (b) Initialize the client in
cla-backend-legacystartup the same way as cla-backend-go:sss.NewClientFromPlatformCredentials(...)from the sharedcla-sss-base-url-<stage>/cla-sss-auth0-audience-<stage>+ reusedAuth0Platformcreds, with the sameRequiredflag. - (c) Call it in
request_employee_signature(around the existingis_sanctionedflag check, before preparing the ECLA), applying the required-flag semantics below and persistingis_sanctioned=trueon a flagged result so the consoles, CCLA, and ECLA paths all stay consistent.
- (a) Extract
-
[Required-flag semantics — lock this and apply identically in both backends]
required=true→ block UNLESS SSS runs and returns a clear "not sanctioned". I.e. block on: nil/unconfigured client, org-lookup failure, missing domain, ANY SSS error (timeout / auth / retryable / bad-request / etc.), AND any non-cleanoutcome; allow ONLY on an explicit SSSclean.required=false→ block ONLY when SSS runs and returnsflagged; otherwise allow (nil client, SSS down/broken, any error, no domain → allow).- Current PR:
required=falsematches exactly.required=truematches for nil-client / org errors / missing-domain / SSS errors / flagged / clean — EXCEPT two cases that currently allow regardless of the flag and must block when required: (1)*sss.NotFoundError(404) returnsfalse, nilunconditionally — route it throughallowWhenOptional; (2) an unexpected 200 status that is neithercleannorflaggedis treated as clean — in required mode treat only an explicitcleanas "not sanctioned" and block everything else. - Implication to keep in mind (intended, not a bug): with
required=truethis strictness also applies to the PR-gate path (hasUserSigned), so an SSS outage will block PR re-checks (authors marked unsigned + negative-cached). That's the strict-compliance behavior, so set the flag per stage/rollout (startfalse, flip totrueonce SSS is provisioned and stable).
-
[Note — rollout]
Requireddefaults to false whencla-sss-required-<stage>is absent, and the client only builds whencla-sss-base-url-<stage>+cla-sss-auth0-audience-<stage>are set. So screening stays effectively off (fail-open + skip) until ops set all three. Rollout order: provision base-url + audience, verify a live call, then flipcla-sss-required-<stage>=true.
This is new, but I think will be very useful - just an idea during review:
-
[Requested behavior — track origin and let SSS clear only its own blocks] EasyCLA should actively clear
is_sanctionedback tofalse, but ONLY when the currenttruewas set by SSS — never an admin/manual block. Code should handle sanctioning like this:- Add a
sanction_originattribute set to"sss"ONLY when SSS setsis_sanctioned=true(inUpdateCompanySanctionStatus, cla-backend-go). Any company with nosanction_origin(admin-set or pre-existing) is treated as manual and is never auto-cleared. (The clear'sConditionExpression: sanction_origin = "sss"is already false when the attribute is absent, so those rows are protected without an extra existence check.) - On a confirmed SSS
cleanresult, clear the flag with a CONDITIONAL update — setis_sanctioned=falseonly whensanction_origin="sss"(DynamoDBConditionExpression); a failed condition (manual/absent) is a no-op (swallowConditionalCheckFailedException). Onflagged, setis_sanctioned=true+sanction_origin="sss". (ExtendUpdateCompanySanctionStatusto carry origin, or add a companion conditional-clear method.) - Make the manual-block short-circuit origin-aware. Today
checkCompanyCompliancedoesif company.IsSanctioned { return true }, which blocks AND skips SSS entirely. Keep that short-circuit ONLY for manual/non-sssorigin. For an existingsss-origin block, do NOT short-circuit — call SSS so a now-cleanresult can clear it; otherwise an SSS-flagged org can never recover after being de-listed. Manual/admin blocks stay sticky (SSS never clears them; admins can still clear manually anytime). - Clear ONLY on a definitive
cleanfrom a successful call — never on an SSS error/timeout/unavailable/missing-domain (leave the flag untouched there; the block-vs-allow decision then follows the required-flag semantics above). This guarantees SSS only ever clears a block it can positively confirm is clean. - This replaces the current blanket "only ever set true, never clear" guard (patch 4) and subsumes CodeRabbit's source-aware suggestion.
- Add a
-
[Nit]
resolveDomain(f, org interface{})+reflect.FieldByNameworks but is fragile (silently returns "" if a field is renamed — no compile error). Prefer passing the concrete*organization-service/models.Organizationand readingorg.Domains/org.Linkdirectly; keep theinterface{}only if the test fake truly needs it (it doesn't — it can use the real type). -
[Nit]
complianceCacheEntry.erris now always nil (errors aren't cached), andgetComplianceCachereturnscached.errwhich is therefore always nil — drop theerrfield/param for clarity. -
[Nit]
complianceCacheis unbounded (one entry per org for the warm Lambda's lifetime, evicted only on access-after-expiry); add a size cap or periodic sweep if org volume is high. Or add TTL which is quite short, like 5 mins. -
[Low] Confirm
orgClient.GetOrganization(ctx, company.CompanyExternalID)keys on the correct id (Salesforce org SFID) and thatCompanyExternalIDis populated for companies reaching this path; it's used both as the lookup key and asSFDCID.
|
Sorry for multiple changes requests but we're really getting closer with this - and this PR si actually big and implement a lot of very useful features. |
its fine , btw i have exam tomorrow lol !! |
Signed-off-by: psrsingh <psr.singh336@gmail.com>
1c3583c to
3918235
Compare
|
@lukaszgryglicki I've pushed an updated version of the branch that includes the shared SSS module, ECLA screening integration, required-mode handling updates, and aligned configuration across both backends. |
Summary
Integrates Sanctions Screening Service (SSS) enforcement into EasyCLA signing flows.
Changes
UpdateCompanySanctionStatus.company_external_idstarts with001.Notes
Closes #4988