Followup SSS integration

[lukaszgryglicki]

Signed-off-by: Lukasz Gryglicki lgryglicki@cncf.io

Assisted by OpenAI

Assisted by GitHub Copilot

Assisted by Claude

[coderabbitai]

Note

Reviews paused

It 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 reviews.auto_review.auto_pause_after_reviewed_commits setting.

Use the following commands to manage reviews:

• @coderabbitai resume to resume automatic reviews.
• @coderabbitai review to trigger a single review.

Use the checkboxes below for quick actions:

• ▶️ Resume reviews
• 🔍 Trigger review

Walkthrough

Enforces sanctions across signature flows, prevents SSS-origin updates from overwriting manual/admin blocks via conditional DynamoDB writes, and strengthens SSS compliance checks and logging with mode-aware fallback behavior.

Changes

Sanctions enforcement and conditional clearing

Layer / File(s)	Summary
Storage and data model layer cla-backend-go/company/projections.go, cla-backend-go/company/repository.go, cla-backend-legacy/internal/store/companies.go, cla-backend-legacy/internal/api/handlers.go	sanction_origin added to projections. UpdateCompanySanctionStatus now uses conditional writes for SSS-origin updates and REMOVE sanction_origin for manual/admin updates; conditional-check failures preserve existing manual/admin blocks.
ECLA creation and auto-create gates cla-backend-go/signatures/service.go, cla-backend-go/v2/signatures/handlers.go	Block ECLA creation/authorization when company IsSanctioned; ProcessEmployeeSignature short-circuits authorization for sanctioned companies. SignaturesEclaAutoCreateHandler rejects enabling auto-create for sanctioned companies.
Authorization and integration gates cla-backend-go/v2/gitlab-activity/service.go, cla-backend-go/v2/gitlab_organizations/service.go, cla-backend-go/v2/sign/service.go	MR participant signed checks now record specific signed-check errors and return false+error when the participant's company is sanctioned. Signature initiation skips activation for sanctioned companies. SignedCorporateCallback re-checks compliance and aborts finalization if sanctioned or check fails.
Compliance screening and error handling cla-backend-go/v2/sign/service.go, cla-backend-legacy/internal/api/handlers.go	checkCompanyCompliance computes sssMode (optional/required), enriches logs around SSS calls, tightens optional-mode fallbacks to honor persisted sanction state when live checks cannot run, treats ClearCompanySanctionStatusIfSSS failures as errors in required mode, and updates in-memory company.IsSanctioned/SanctionOrigin.

Sequence Diagram(s)

sequenceDiagram
  participant Client
  participant SignService
  participant SSSClient
  participant CompanyStore
  participant DynamoDB

  Client->>SignService: SignedCorporateCallback / initiate compliance check
  SignService->>SSSClient: GetOrganizationStatus(request)
  SSSClient-->>SignService: status (sanctioned|clean)
  SignService->>CompanyStore: Clear/UpdateCompanySanctionStatus based on status
  CompanyStore->>DynamoDB: UpdateItem (conditional or REMOVE sanction_origin)
  DynamoDB-->>CompanyStore: Success / ConditionalCheckFailedException
  CompanyStore-->>SignService: Result (error or nil)
  SignService-->>Client: finalize or abort based on compliance

Loading

Estimated code review effort

🎯 4 (Complex) | ⏱️ ~45 minutes

Possibly related PRs

• linuxfoundation/easycla#5078: Introduces the initial SSS gate and calls to UpdateCompanySanctionStatus, which this PR extends with conditional-clearing logic and enhanced error handling.
• linuxfoundation/easycla#5058: Adds related SSS client and dependencies that checkCompanyCompliance relies on.

🚥 Pre-merge checks | ✅ 3 | ❌ 2

❌ Failed checks (1 warning, 1 inconclusive)

Check name	Status	Explanation	Resolution
Description check	⚠️ Warning	The pull request description contains only sign-off and attribution information with no actual description of the changes or objectives.	Provide a meaningful description explaining the purpose of this PR, the problems it solves, and the key changes made (e.g., sanctions gating, compliance checks, SSS conditional updates).
Title check	❓ Inconclusive	The title 'Followup SSS integration' is vague and does not clearly convey what changes are included or their significance.	Revise the title to be more specific about the key changes, such as 'Add sanctions gate and compliance checks for SSS integration' or similar.

✅ Passed checks (3 passed)

Check name	Status	Explanation
Docstring Coverage	✅ Passed	Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%.
Linked Issues check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check	✅ Passed	Check skipped because no linked issues were found for this pull request.

_{✏️ Tip: You can configure your own custom pre-merge checks in the settings.}

✨ Finishing Touches

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Commit unit tests in branch unicron-sss-integration-followup

---

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

[Copilot]

Pull request overview

This PR continues the Sanctions Screening Service (SSS) integration by strengthening how sanction state is persisted (including origin tracking), ensuring SSS updates do not overwrite manual/admin blocks, and enforcing sanction gates in additional signing/authorization flows across both legacy and Go backends.

Changes:

• Add conditional DynamoDB updates so SSS-origin blocks cannot overwrite existing manual/admin blocks, and treat conditional-check failures as “preserve existing block.”
• Add additional SSS screening logging (including required vs optional mode) and tighten required-mode behavior when a persisted “clean” result cannot be saved.
• Enforce persisted sanction gates in more runtime paths (DocuSign corporate callback, GitLab MR authorization checks, and employee signature auto-create / authorization checks).

Reviewed changes

Copilot reviewed 7 out of 7 changed files in this pull request and generated no comments.

Show a summary per file

File	Description
cla-backend-legacy/internal/store/companies.go	Adds conditional update logic to prevent SSS from overwriting manual/admin sanctions and preserves existing blocks on conditional failure.
cla-backend-legacy/internal/api/handlers.go	Improves SSS screening observability and fails closed in required mode when a persisted “clean” cannot be cleared.
cla-backend-go/v2/sign/service.go	Re-screens company sanctions before finalizing corporate CLAs and improves SSS screening logging/required-mode persistence handling.
cla-backend-go/v2/gitlab-activity/service.go	Blocks GitLab authorization for users affiliated with sanctioned companies using the persisted sanction gate.
cla-backend-go/signatures/service.go	Prevents auto-creating ECLAs for sanctioned companies and ensures ECLA authorization fails for sanctioned companies.
cla-backend-go/company/repository.go	Adds conditional DynamoDB update logic mirroring legacy behavior to preserve manual/admin blocks when SSS flags a company.
cla-backend-go/company/projections.go	Ensures sanction_origin is included in projected company reads so origin-aware logic can function correctly.

[coderabbitai]

Actionable comments posted: 1

🤖 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-legacy/internal/api/handlers.go`:
- Around line 8995-9000: The optional SSS path can leave a durable
false-positive because when h.companies.ClearCompanySanctionStatusIfSSS(ctx,
companyID) fails and h.sssRequired is false you still proceed while the DB keeps
is_sanctioned=true with sanction_origin="sss", which later pre-SSS fallback
logic will honor; update checkCompanyCompliance so that on optional-mode clear
failures you either retry/persist the clear (e.g., retry
ClearCompanySanctionStatusIfSSS with backoff or enqueue a durable job) before
returning success, or change the pre-SSS fallback to ignore/override DB records
where sanction_origin=="sss" when h.sssRequired==false; locate and modify the
ClearCompanySanctionStatusIfSSS call and the pre-SSS fallback branch in
checkCompanyCompliance to implement one of these fixes.

🪄 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: d0d2fd71-a043-4569-ae42-93efd52df86a

📥 Commits

Reviewing files that changed from the base of the PR and between 2b40b7c and 02b15e7.

📒 Files selected for processing (7)

• cla-backend-go/company/projections.go
• cla-backend-go/company/repository.go
• cla-backend-go/signatures/service.go
• cla-backend-go/v2/gitlab-activity/service.go
• cla-backend-go/v2/sign/service.go
• cla-backend-legacy/internal/api/handlers.go
• cla-backend-legacy/internal/store/companies.go

[coderabbitai]

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (3)

cla-backend-legacy/internal/api/handlers.go (3)

5221-5249: ⚠️ Potential issue | 🟠 Major | ⚡ Quick win

Restrict sticky sanction overrides to privileged callers.

Line 5222 still authorizes any company_acl member, and Lines 5246-5249 now turn an is_sanctioned edit into a manual/admin-controlled state by deleting sanction_origin. That lets a regular CLA manager clear an SSS block or create a durable manual block through PUT /v1/company, which is broader than the new "manual/admin" semantics described in this PR.

---

8940-8947: ⚠️ Potential issue | 🟠 Major | ⚡ Quick win

Guard the Salesforce dependency before the optional fallback.

Line 8941 unconditionally dereferences h.salesforce, but NewHandlers explicitly allows dependencies to be nil when misconfigured. In optional mode that turns "screening unavailable" into a panic instead of the new false, nil fallback.

🤖 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-legacy/internal/api/handlers.go` around lines 8940 - 8947, The
code unconditionally calls h.salesforce.GetOrganization which can dereference a
nil dependency; before calling GetOrganization (in the method containing this
block, e.g., checkCompanyCompliance), add a nil check for h.salesforce and
handle the optional fallback the same way as when GetOrganization errors: if
h.salesforce == nil log a warning referencing companyExternalID and then if
h.sssRequired return false with a wrapped error, otherwise return false, nil;
only call h.salesforce.GetOrganization after confirming h.salesforce is non-nil.

---

8908-8918: ⚠️ Potential issue | 🟠 Major

Backfill legacy sanction rows that lack sanction_origin

checkCompanyCompliance short-circuits with return true when is_sanctioned is true and sanction_origin != "sss"—including when sanction_origin is missing (empty string). ClearCompanySanctionStatusIfSSS can only auto-clear when sanction_origin == "sss", so legacy rows persisted before sanction_origin existed can never be re-screened/cleared by a future SSS clean result. Ensure a migration/backfill (or an explicit one-time re-screen path) sets sanction_origin="sss" for pre-existing SSS-backed sanctions, or adjust the gate to handle missing origin safely.

🤖 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-legacy/internal/api/handlers.go` around lines 8908 - 8918,
checkCompanyCompliance currently treats a missing sanction_origin as non-SSS and
short-circuits, preventing ClearCompanySanctionStatusIfSSS from ever clearing
legacy rows; fix by backfilling legacy sanction rows to set
sanction_origin="sss" for any is_sanctioned records that were created by the old
SSS flow (preferred), or alternatively change the gate in checkCompanyCompliance
to only short-circuit when sanctionOrigin != "" && sanctionOrigin != "sss" so
empty origin does not block re-screening; update/migrate data and/or adjust the
logic in checkCompanyCompliance (and ensure ClearCompanySanctionStatusIfSSS
semantics remain consistent) so pre-existing SSS-backed sanctions can be
auto-cleared.

🤖 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.

Outside diff comments:
In `@cla-backend-legacy/internal/api/handlers.go`:
- Around line 8940-8947: The code unconditionally calls
h.salesforce.GetOrganization which can dereference a nil dependency; before
calling GetOrganization (in the method containing this block, e.g.,
checkCompanyCompliance), add a nil check for h.salesforce and handle the
optional fallback the same way as when GetOrganization errors: if h.salesforce
== nil log a warning referencing companyExternalID and then if h.sssRequired
return false with a wrapped error, otherwise return false, nil; only call
h.salesforce.GetOrganization after confirming h.salesforce is non-nil.
- Around line 8908-8918: checkCompanyCompliance currently treats a missing
sanction_origin as non-SSS and short-circuits, preventing
ClearCompanySanctionStatusIfSSS from ever clearing legacy rows; fix by
backfilling legacy sanction rows to set sanction_origin="sss" for any
is_sanctioned records that were created by the old SSS flow (preferred), or
alternatively change the gate in checkCompanyCompliance to only short-circuit
when sanctionOrigin != "" && sanctionOrigin != "sss" so empty origin does not
block re-screening; update/migrate data and/or adjust the logic in
checkCompanyCompliance (and ensure ClearCompanySanctionStatusIfSSS semantics
remain consistent) so pre-existing SSS-backed sanctions can be auto-cleared.

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

Run ID: aaf2a37f-bb70-4f1f-9f0c-45dc5ba40942

📥 Commits

Reviewing files that changed from the base of the PR and between 02b15e7 and 5d15e53.

📒 Files selected for processing (6)

• cla-backend-go/company/repository.go
• cla-backend-go/v2/gitlab-activity/service.go
• cla-backend-go/v2/gitlab_organizations/service.go
• cla-backend-go/v2/signatures/handlers.go
• cla-backend-legacy/internal/api/handlers.go
• cla-backend-legacy/internal/store/companies.go

🚧 Files skipped from review as they are similar to previous changes (3)

• cla-backend-go/v2/gitlab-activity/service.go
• cla-backend-legacy/internal/store/companies.go
• cla-backend-go/company/repository.go

[Copilot]

Pull request overview

Copilot reviewed 9 out of 9 changed files in this pull request and generated 2 comments.

[coderabbitai]

♻️ Duplicate comments (1)

cla-backend-legacy/internal/api/handlers.go (1)

8921-8964: ⚠️ Potential issue | 🟠 Major | ⚡ Quick win

Optional fallbacks still treat stale SSS blocks as authoritative.

This is still the unresolved false-positive path from the earlier review: if ClearCompanySanctionStatusIfSSS fails once at Lines 9003-9008 in optional mode, the row stays is_sanctioned=true with sanction_origin="sss", and these new fallback returns will keep blocking later requests whenever live screening is unavailable. Keep the optional-mode fallbacks at return false, nil, or explicitly ignore persisted sss-origin blocks here.

🤖 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-legacy/internal/api/handlers.go` around lines 8921 - 8964, The
optional-mode fallback is incorrectly honoring persisted SSS-origin blocks and
returning isSanctioned (which keeps false-positives blocking); in
checkCompanyCompliance update the early-return paths that currently do "return
isSanctioned, nil" when h.sssRequired is false (the branches after
h.sssClient==nil, missing companyExternalID, failed GetOrganization, nil org, or
unresolved domain) to instead treat SSS-origin sanctions as non-blocking in
optional mode — either return false, nil directly in these optional fallbacks or
explicitly check the persisted sanction origin (e.g., sanction_origin == "sss")
and clear/ignore it before returning; ensure this aligns with
ClearCompanySanctionStatusIfSSS logic so transient
ClearCompanySanctionStatusIfSSS failures don't leave SSS-origin rows blocking
future requests.

🤖 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.

Duplicate comments:
In `@cla-backend-legacy/internal/api/handlers.go`:
- Around line 8921-8964: The optional-mode fallback is incorrectly honoring
persisted SSS-origin blocks and returning isSanctioned (which keeps
false-positives blocking); in checkCompanyCompliance update the early-return
paths that currently do "return isSanctioned, nil" when h.sssRequired is false
(the branches after h.sssClient==nil, missing companyExternalID, failed
GetOrganization, nil org, or unresolved domain) to instead treat SSS-origin
sanctions as non-blocking in optional mode — either return false, nil directly
in these optional fallbacks or explicitly check the persisted sanction origin
(e.g., sanction_origin == "sss") and clear/ignore it before returning; ensure
this aligns with ClearCompanySanctionStatusIfSSS logic so transient
ClearCompanySanctionStatusIfSSS failures don't leave SSS-origin rows blocking
future requests.

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

Run ID: 0fc2d9fa-4fd0-4656-b40e-01104f28d7ce

📥 Commits

Reviewing files that changed from the base of the PR and between 5d15e53 and d51af38.

📒 Files selected for processing (2)

• cla-backend-go/v2/sign/service.go
• cla-backend-legacy/internal/api/handlers.go

[Copilot]

Pull request overview

Copilot reviewed 9 out of 9 changed files in this pull request and generated 1 comment.

[Copilot]

Pull request overview

Copilot reviewed 9 out of 10 changed files in this pull request and generated 1 comment.

Files not reviewed (1)

• cla-backend-go/company/mocks/mock_repo.go: Language not supported

[Copilot]

Pull request overview

Copilot reviewed 10 out of 11 changed files in this pull request and generated no new comments.

Files not reviewed (1)

• cla-backend-go/company/mocks/mock_repo.go: Language not supported