Two more SSS checks updates

[lukaszgryglicki]

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

Assisted by OpenAI

Assisted by GitHub Copilot

Assisted by Claude

[coderabbitai]

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info

⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

Run ID: 77d82a1e-d95c-4582-9833-d4dab21575d8

📥 Commits

Reviewing files that changed from the base of the PR and between 43a5371 and b8639ab.

📒 Files selected for processing (1)

• cla-backend-go/v2/sign/service.go

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

• cla-backend-go/v2/sign/service.go

---

Walkthrough

Two defensive input guards are added: checkCompanyCompliance in the sign service now short-circuits when CompanyExternalID is empty (returning an error in required mode or the persisted IsSanctioned value in optional mode), and NewClient in the SSS base client now trims whitespace from all config string fields after non-empty validation.

Changes

SSS Defensive Input Guards

Layer / File(s)	Summary
SSS client config whitespace trimming cla-sss-base/client.go	NewClient trims BaseURL, Auth0Domain, Auth0ClientID, Auth0ClientSecret, and Auth0Audience immediately after the non-empty check, preventing padded values from producing malformed token or request URLs.
checkCompanyCompliance early-exit and complianceUnavailable helper cla-backend-go/v2/sign/service.go	Adds an early return when company.CompanyExternalID is empty, delegating to new complianceUnavailable helper. In sssRequired mode it returns (false, error); in optional mode it logs a warning and returns the persisted company.IsSanctioned value, skipping domain/org resolution and the SSS call.
Tests for empty ExternalID handling cla-backend-go/v2/sign/service_sss_test.go	Added newTestSSSClient helper and three test cases validating checkCompanyCompliance behavior when CompanyExternalID is missing: required mode must block with error, optional mode must not error and must not block, and optional mode must still block if a persisted SSS-origin sanction exists.

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~12 minutes

Possibly related PRs

• linuxfoundation/easycla#5087: Both PRs modify the same checkCompanyCompliance SSS compliance enforcement path in cla-backend-go/v2/sign/service.go around mode-dependent behavior and persisted IsSanctioned handling.
• linuxfoundation/easycla#5078: Implements the core checkCompanyCompliance SSS flow and external-ID handling that this PR adds a defensive guard and mode-aware helper on top of.
• linuxfoundation/easycla#5058: Introduces the underlying sss.Client and Auth0 token logic that this PR's config trimming and SSS call integration depend on.

🚥 Pre-merge checks | ✅ 2 | ❌ 3

❌ Failed checks (1 warning, 2 inconclusive)

Check name	Status	Explanation	Resolution
Docstring Coverage	⚠️ Warning	Docstring coverage is 14.29% which is insufficient. The required threshold is 80.00%.	Write docstrings for the functions missing them to satisfy the coverage threshold.
Title check	❓ Inconclusive	The title 'Two more SSS checks updates' is vague and generic, using non-descriptive language that doesn't convey what specific SSS checks were modified or why.	Revise the title to be more specific about the changes, such as 'Handle missing CompanyExternalID in SSS compliance checks' to clearly indicate the main changes.
Description check	❓ Inconclusive	The description contains only attribution information without explaining what changes were made, why they were made, or how they affect the codebase.	Add a meaningful description of the changes, such as explaining the CompanyExternalID handling improvements, config validation updates, and new test coverage.

✅ Passed checks (2 passed)

Check name	Status	Explanation
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 docstrings

• Create stacked PR
• Commit on current branch

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Commit unit tests in branch unicron-sss-easycla-fix

---

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

[Copilot]

Pull request overview

This PR hardens Sanctions Screening Service (SSS) integration by normalizing SSS client configuration values and adding an explicit early-exit path when a company lacks an external (SFDC) ID required for domain resolution in the compliance check.

Changes:

• Trim and persist sanitized SSS client config fields in NewClient to avoid padded config values causing malformed URLs/requests.
• In checkCompanyCompliance, short-circuit when CompanyExternalID is missing, returning an error in required mode and falling back to the persisted sanction state in optional mode.

Reviewed changes

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

File	Description
cla-sss-base/client.go	Trims and stores config values to prevent whitespace/padding from corrupting downstream HTTP requests.
cla-backend-go/v2/sign/service.go	Adds an early return when a company has no external ID, avoiding unnecessary upstream lookups and making required/optional behavior explicit.

[coderabbitai]

🧹 Nitpick comments (1)

cla-backend-go/v2/sign/service_sss_test.go (1)

98-112: ⚡ Quick win

Add coverage for persisted sanctioned state on empty external ID in optional mode.

Line 98-Line 112 only covers IsSanctioned=false. Please add a sibling case with IsSanctioned=true (and empty CompanyExternalID) to verify this new early-exit path returns blocked=true, err=nil when SSS is optional.

Proposed test addition

+func TestCheckCompanyComplianceOptionalMissingExternalIDHonorsPersistedSanction(t *testing.T) {
+	svc := &service{sssRequired: false, sssClient: newTestSSSClient(t)}
+
+	blocked, err := svc.checkCompanyCompliance(context.Background(), &models.Company{
+		CompanyID:         "company-id",
+		CompanyName:       "Company",
+		CompanyExternalID: "",
+		IsSanctioned:      true,
+	})
+	if err != nil {
+		t.Fatalf("expected optional SSS to continue when external ID is missing, got %v", err)
+	}
+	if !blocked {
+		t.Fatal("expected optional SSS to honor persisted sanction state when external ID is missing")
+	}
+}

🤖 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 98 - 112, Add a new
test function that is a sibling to
TestCheckCompanyComplianceOptionalAllowsMissingExternalID to cover the case
where optional SSS mode is enabled but the Company has IsSanctioned=true with an
empty CompanyExternalID. The new test should invoke checkCompanyCompliance with
this Company configuration and verify that it returns blocked=true with err=nil,
ensuring the early-exit path for persisted sanctioned state works correctly in
optional mode.

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

Nitpick comments:
In `@cla-backend-go/v2/sign/service_sss_test.go`:
- Around line 98-112: Add a new test function that is a sibling to
TestCheckCompanyComplianceOptionalAllowsMissingExternalID to cover the case
where optional SSS mode is enabled but the Company has IsSanctioned=true with an
empty CompanyExternalID. The new test should invoke checkCompanyCompliance with
this Company configuration and verify that it returns blocked=true with err=nil,
ensuring the early-exit path for persisted sanctioned state works correctly in
optional mode.

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

Run ID: d3443548-ba80-48f2-9f1c-d02802c64922

📥 Commits

Reviewing files that changed from the base of the PR and between b87aa51 and 98ae496.

📒 Files selected for processing (2)

• cla-backend-go/v2/sign/service.go
• cla-backend-go/v2/sign/service_sss_test.go

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

• cla-backend-go/v2/sign/service.go

[Copilot]

Pull request overview

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