Unicron fix easycla outage dev

[lukaszgryglicki]

Fixes prod outage (which is already reverted, but we want to retry porting py->go). (dev).

[coderabbitai]

Walkthrough

This PR introduces a new GitHub public organization lookup API, refactors approval checks to use it, and systematically updates DynamoDB persistence layers in legacy services to work directly with typed AttributeValue maps instead of converting to/from interface maps. The changes span GitHub API integration, OAuth user persistence, and project handler updates.

Changes

GitHub Organization Lookup and Approval Check

Layer / File(s)	Summary
GitHub Public Org Lookup API cla-backend-go/github/github_org.go	New ListUserPublicOrgs(ctx, user) fetches GitHub organizations where a user is publicly visible via paginated API, collects org logins, and handles nil entries.
Org Approval Check Refactoring cla-backend-go/signatures/service.go	UserIsApproved changes from per-org membership queries to fetching all user public orgs once via ListUserPublicOrgs and matching against allowlist with case-insensitive comparison.
Related Organization Member Lookup cla-backend-go/github/github_org.go	GetOrganizationMembers continues to list org members via GitHub App client without changes.

OAuth User DynamoDB AttributeValue Refactoring

Layer / File(s)	Summary
User Lookup AttributeValue Handling cla-backend-legacy/internal/api/github_oauth.go	User lookup by GitHub ID and email fallback now store DynamoDB items as raw types.AttributeValue maps (userAV) instead of converting to interface maps.
Existing User Update Path cla-backend-legacy/internal/api/github_oauth.go	Existing user updates mutate the AttributeValue map directly for fields (username, name, emails, github id, modified date), persist via PutItem, then convert back while preserving user_github_id as int64.
New User Creation Path cla-backend-legacy/internal/api/github_oauth.go	New users are created by directly building a DynamoDB AttributeValue item map with explicit version, timestamps, and attributes, persisting via PutItem, then converting back with user_github_id as int64.

Project Handler DynamoDB AttributeValue Refactoring

Layer / File(s)	Summary
Project Creation Item Construction cla-backend-legacy/internal/api/handlers.go	PostProjectV1 now builds DynamoDB items directly as typed AttributeValue maps with all required attributes (enabled booleans, ACL, timestamps, version) instead of converting through interface maps.
Project Update In-Place Modification cla-backend-legacy/internal/api/handlers.go	PutProjectV1 loads existing DynamoDB items, applies edits in-place using typed AttributeValues, and persists with PutItem, replacing the prior round-trip conversion pattern.

🎯 4 (Complex) | ⏱️ ~45 minutes

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 inconclusive)

Check name	Status	Explanation	Resolution
Title check	❓ Inconclusive	The title 'Unicron fix easycla outage dev' is vague and unclear; 'Unicron' appears unrelated to the changeset, and it lacks specificity about what was fixed or the scope of changes.	Revise the title to clearly describe the main changes, e.g., 'Port GitHub org lookup and PR/merge-group status updates to Go' or 'Add ListUserPublicOrgs and GitHub webhook handlers'.

✅ Passed checks (4 passed)

Check name	Status	Explanation
Description check	✅ Passed	The description mentions fixing a prod outage and porting Python to Go, which broadly aligns with the changeset's focus on adding new Go functions and refactoring DynamoDB operations.
Docstring Coverage	✅ Passed	No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
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-fix-easycla-outage-dev

Warning

There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure.

🔧 golangci-lint (2.12.1)

level=error msg="[linters_context] typechecking error: pattern ./...: directory prefix . does not contain main module or its selected dependencies"

---

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

[Copilot]

Pull request overview

This PR retries a previously reverted prod-outage fix by avoiding DynamoDB type coercion regressions in the legacy API layer (py→go parity), and by correcting GitHub org-based approval checks in the Go backend to mirror the pre-cutover Python behavior.

Changes:

• Legacy API: stop round-tripping through store.InterfaceMapToItem for project/user upserts to prevent digit-only strings being coerced into DynamoDB N types.
• Go signatures service: replace per-org membership checks (403-prone) with listing a user’s public orgs and matching against the approval list.
• Go GitHub client: add ListUserPublicOrgs helper to support the new org-approval logic.

Reviewed changes

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

File	Description
cla-backend-legacy/internal/api/handlers.go	Builds DynamoDB AttributeValue maps directly for project create/update to avoid numeric-string coercion.
cla-backend-legacy/internal/api/github_oauth.go	Avoids InterfaceMapToItem for GitHub OAuth user upserts; preserves legacy wire shape for user_github_id.
cla-backend-go/signatures/service.go	Fixes GitHub org approval matching by using publicly visible org memberships instead of forbidden membership endpoints.
cla-backend-go/github/github_org.go	Adds ListUserPublicOrgs to fetch a user’s public org logins via GitHub API pagination.

[coderabbitai]

Actionable comments posted: 2

Caution

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

⚠️ Outside diff range comments (1)

cla-backend-go/signatures/service.go (1)

1033-1044: ⚠️ Potential issue | 🟠 Major

Add explicit repository-type filtering to these webhook repository lookups.

Both UpdateGitHubChangeRequest and UpdateGitHubMergeGroup resolve repositories using GetRepositoryByExternalID(...) without type scoping. The underlying v1 repository method queries only on external ID without filtering by provider type. If external IDs are reused across providers (GitHub and GitLab may use overlapping ID spaces), these lookups could bind a GitHub webhook to the wrong repository record.

Use the v2 pattern (explicit type filter on queries) or the legacy pattern (GetByExternalIDAndType(ctx, id, "github")) to ensure GitHub webhooks only match GitHub repositories.

Affected code locations

• cla-backend-go/signatures/service.go lines 1033-1044 (UpdateGitHubChangeRequest)
• cla-backend-go/signatures/service.go lines 1110-1121 (UpdateGitHubMergeGroup)

🤖 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/signatures/service.go` around lines 1033 - 1044, The
repository lookup in UpdateGitHubChangeRequest (repo, err :=
s.repositoryService.GetRepositoryByExternalID(...)) and the similar lookup in
UpdateGitHubMergeGroup must be scoped to provider type to avoid cross-provider
ID collisions; instead of calling GetRepositoryByExternalID use the v2 style or
the legacy helper that includes the type (e.g., GetByExternalIDAndType(ctx,
strconv.FormatInt(repositoryID,10), "github") or the repositoryService v2 query
that accepts a provider/type filter) so that only GitHub repositories are
returned; update both calls and any related nil/error handling to use the typed
lookup.

🧹 Nitpick comments (1)

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

5846-5859: ⚡ Quick win

Add a regression test for digit-only project names and external IDs.

This fix is subtle and outage-related. Please cover both create and update paths with values like "12345" for project_name and project_external_id, and assert they round-trip as strings rather than DynamoDB numbers.

Also applies to: 5961-6004

🤖 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 5846 - 5859, Add
regression tests that verify digit-only project_name and project_external_id
stay strings through create and update: write tests calling the project creation
and update handlers (the code path that builds the AttributeValue map where
"project_name" and "project_external_id" are set as types.AttributeValueMemberS)
using values like "12345", then fetch the stored/returned project and assert
those fields are returned as strings (not numbers) and round-trip unchanged;
duplicate the same check for the update path (the handler that modifies
date_modified/version and builds AttributeValue inputs) to ensure updates also
preserve string types.

🤖 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/signatures/service.go`:
- Around line 1649-1652: The current handling of github.ListUserPublicOrgs
swallows API errors and falls through to treating the user as not org-approved;
change it so transient GitHub API failures (e.g., HTTP 403, 429, 5xx) returned
by github.ListUserPublicOrgs are propagated as an error instead of returning
false,nil. Modify the block around github.ListUserPublicOrgs (where userOrgs is
assigned and log.WithFields(f).Warnf is called) to detect transient/rate-limit
status from the error (by inspecting the error type/status) and return that
error (or a wrapped retryable error) so callers get an error response instead of
silently denying org-approved users.

In `@cla-backend-legacy/internal/api/handlers.go`:
- Around line 5961-5990: The loaded DynamoDB item may contain project_name or
project_external_id stored as numeric AttributeValue types from the prior bug;
before calling h.projects.PutItem ensure those keys are normalized to string
AttributeValueMemberS (convert any AttributeValueMemberN to a string value) so
updates that only touch booleans don't preserve the broken type; modify the code
surrounding item, req.ProjectName, req.ProjectExternalID and the pre-PutItem
logic to detect and replace numeric AttributeValue entries for "project_name"
and "project_external_id" with AttributeValueMemberS containing the string
representation (then continue to set updated values if the request provides
them) so h.projects.PutItem always receives S types for those fields.

---

Outside diff comments:
In `@cla-backend-go/signatures/service.go`:
- Around line 1033-1044: The repository lookup in UpdateGitHubChangeRequest
(repo, err := s.repositoryService.GetRepositoryByExternalID(...)) and the
similar lookup in UpdateGitHubMergeGroup must be scoped to provider type to
avoid cross-provider ID collisions; instead of calling GetRepositoryByExternalID
use the v2 style or the legacy helper that includes the type (e.g.,
GetByExternalIDAndType(ctx, strconv.FormatInt(repositoryID,10), "github") or the
repositoryService v2 query that accepts a provider/type filter) so that only
GitHub repositories are returned; update both calls and any related nil/error
handling to use the typed lookup.

---

Nitpick comments:
In `@cla-backend-legacy/internal/api/handlers.go`:
- Around line 5846-5859: Add regression tests that verify digit-only
project_name and project_external_id stay strings through create and update:
write tests calling the project creation and update handlers (the code path that
builds the AttributeValue map where "project_name" and "project_external_id" are
set as types.AttributeValueMemberS) using values like "12345", then fetch the
stored/returned project and assert those fields are returned as strings (not
numbers) and round-trip unchanged; duplicate the same check for the update path
(the handler that modifies date_modified/version and builds AttributeValue
inputs) to ensure updates also preserve string types.

🪄 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: 6178b405-c35e-45b2-b61a-c1e043d3a837

📥 Commits

Reviewing files that changed from the base of the PR and between 96e6104 and c663a3a.

📒 Files selected for processing (4)

• cla-backend-go/github/github_org.go
• cla-backend-go/signatures/service.go
• cla-backend-legacy/internal/api/github_oauth.go
• cla-backend-legacy/internal/api/handlers.go

[coderabbitai]

⚠️ Potential issue | 🟠 Major | ⚡ Quick win

Don't silently deny org-approved users on GitHub API failures.

If ListUserPublicOrgs hits a transient 403/429/5xx, this path falls through to false, nil, so org-only-approved contributors get a "not approved" result instead of a retryable error. That turns a GitHub outage or rate-limit event into a user-visible authorization failure.

Suggested fix

-			userOrgs, err := github.ListUserPublicOrgs(ctx, login)
-			if err != nil {
-				log.WithFields(f).Warnf("could not list public orgs for github user %s; treating as no org-approval match: %v", login, err)
-			} else {
-				for _, approvedOrg := range githubOrgApprovalList {
-					approvedOrgTrim := strings.TrimSpace(approvedOrg)
-					matched := false
-					for _, userOrg := range userOrgs {
-						if strings.EqualFold(approvedOrgTrim, userOrg) {
-							matched = true
-							break
-						}
-					}
-					if matched {
-						log.WithFields(f).Debugf("found matching github organization: %s for user: %s", approvedOrg, login)
-						return true, nil
-					}
-					log.WithFields(f).Debugf("user: %s is not in the organization: %s", login, approvedOrg)
-				}
-			}
+			userOrgs, err := github.ListUserPublicOrgs(ctx, login)
+			if err != nil {
+				log.WithFields(f).WithError(err).Warnf("could not list public orgs for github user %s", login)
+				return false, err
+			}
+			for _, approvedOrg := range githubOrgApprovalList {
+				approvedOrgTrim := strings.TrimSpace(approvedOrg)
+				matched := false
+				for _, userOrg := range userOrgs {
+					if strings.EqualFold(approvedOrgTrim, userOrg) {
+						matched = true
+						break
+					}
+				}
+				if matched {
+					log.WithFields(f).Debugf("found matching github organization: %s for user: %s", approvedOrg, login)
+					return true, nil
+				}
+				log.WithFields(f).Debugf("user: %s is not in the organization: %s", login, approvedOrg)
+			}

📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change

	userOrgs, err := github.ListUserPublicOrgs(ctx, login)
	if err != nil {
	break
	}
	if membership != nil {
	log.WithFields(f).Debugf("found matching github organization: %s for user: %s", org, user.GithubUsername)
	return true, nil
	log.WithFields(f).Warnf("could not list public orgs for github user %s; treating as no org-approval match: %v", login, err)
	} else {
	userOrgs, err := github.ListUserPublicOrgs(ctx, login)
	if err != nil {
	log.WithFields(f).WithError(err).Warnf("could not list public orgs for github user %s", login)
	return false, err
	}
	for _, approvedOrg := range githubOrgApprovalList {
	approvedOrgTrim := strings.TrimSpace(approvedOrg)
	matched := false
	for _, userOrg := range userOrgs {
	if strings.EqualFold(approvedOrgTrim, userOrg) {
	matched = true
	break
	}
	}
	if matched {
	log.WithFields(f).Debugf("found matching github organization: %s for user: %s", approvedOrg, login)
	return true, nil
	}
	log.WithFields(f).Debugf("user: %s is not in the organization: %s", login, approvedOrg)
	}

🤖 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/signatures/service.go` around lines 1649 - 1652, The current
handling of github.ListUserPublicOrgs swallows API errors and falls through to
treating the user as not org-approved; change it so transient GitHub API
failures (e.g., HTTP 403, 429, 5xx) returned by github.ListUserPublicOrgs are
propagated as an error instead of returning false,nil. Modify the block around
github.ListUserPublicOrgs (where userOrgs is assigned and
log.WithFields(f).Warnf is called) to detect transient/rate-limit status from
the error (by inspecting the error type/status) and return that error (or a
wrapped retryable error) so callers get an error response instead of silently
denying org-approved users.

[coderabbitai]

⚠️ Potential issue | 🟠 Major | ⚡ Quick win

Normalize previously-corrupted string fields before saving.

This prevents new project_name / project_external_id coercion, but Line 5961 now writes the loaded DynamoDB item back as-is. If an existing row already has either field stored as N from the earlier bug, updating only a boolean here will preserve the broken type and keep the project broken after save.

Suggested fix

  if !stringSliceContainsExact(projectACL, authUser.Username) {
  	// Python raises falcon.HTTPForbidden via project_acl_verify.
  	respond.JSON(w, http.StatusForbidden, map[string]any{
  		"title":       "Unauthorized",
  		"description": "Provided Token credentials does not have sufficient permissions to access resource",
  	})
  	return
  }

+	// Self-heal rows written by the old numeric-string coercion path.
+	for _, key := range []string{"project_external_id", "project_name"} {
+		if av, ok := item[key].(*types.AttributeValueMemberN); ok {
+			item[key] = &types.AttributeValueMemberS{Value: av.Value}
+		}
+	}
+
 	// Patch the AttributeValue map directly so we never round-trip pynamodb
 	// types through InterfaceMapToItem's isNumericString heuristic, which
 	// can silently coerce digit-only S fields to N.
 	updatedString := " "

🤖 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 5961 - 5990, The
loaded DynamoDB item may contain project_name or project_external_id stored as
numeric AttributeValue types from the prior bug; before calling
h.projects.PutItem ensure those keys are normalized to string
AttributeValueMemberS (convert any AttributeValueMemberN to a string value) so
updates that only touch booleans don't preserve the broken type; modify the code
surrounding item, req.ProjectName, req.ProjectExternalID and the pre-PutItem
logic to detect and replace numeric AttributeValue entries for "project_name"
and "project_external_id" with AttributeValueMemberS containing the string
representation (then continue to set updated values if the request provides
them) so h.projects.PutItem always receives S types for those fields.