From ecb342e9f423614d7a954c202342d53251b27372 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=C5=81ukasz=20Gryglicki?= Date: Wed, 30 Sep 2026 15:18:19 +0200 Subject: [PATCH] Fix: re-adding a removed approval doesn't restore a not uuthorized acknowledgment SS #2980 MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Signed-off-by: Łukasz Gryglicki Assisted by [OpenAI](https://platform.openai.com/) Assisted by [GitHub Copilot](https://github.com/features/copilot) Assisted by [Claude](https://claude.ai) --- .../approval_list_readd_criteria_test.go | 282 ++++++++ .../approval_list_readd_e2e_test.go | 227 ++++++ .../signatures/approval_list_readd_test.go | 453 ++++++++++++ .../approval_list_readd_unit_test.go | 650 ++++++++++++++++++ .../signatures/approval_list_removal_test.go | 134 +++- cla-backend-go/signatures/dbmodels.go | 15 +- cla-backend-go/signatures/mocks/mock_repo.go | 46 ++ cla-backend-go/signatures/repository.go | 231 +++++++ cla-backend-go/signatures/service.go | 206 ++++++ .../v2/cla_manager/designee_test.go | 255 +++++++ cla-backend-go/v2/cla_manager/service.go | 3 +- docs/M3_ORG_LENS_API.md | 10 +- 12 files changed, 2507 insertions(+), 5 deletions(-) create mode 100644 cla-backend-go/signatures/approval_list_readd_criteria_test.go create mode 100644 cla-backend-go/signatures/approval_list_readd_e2e_test.go create mode 100644 cla-backend-go/signatures/approval_list_readd_test.go create mode 100644 cla-backend-go/signatures/approval_list_readd_unit_test.go create mode 100644 cla-backend-go/v2/cla_manager/designee_test.go diff --git a/cla-backend-go/signatures/approval_list_readd_criteria_test.go b/cla-backend-go/signatures/approval_list_readd_criteria_test.go new file mode 100644 index 000000000..fb814d728 --- /dev/null +++ b/cla-backend-go/signatures/approval_list_readd_criteria_test.go @@ -0,0 +1,282 @@ +// Copyright The Linux Foundation and each contributor to CommunityBridge. +// SPDX-License-Identifier: MIT + +package signatures + +import ( + "context" + "errors" + "fmt" + "testing" + + "github.com/linuxfoundation/easycla/cla-backend-go/gen/v1/models" + "github.com/linuxfoundation/easycla/cla-backend-go/utils" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +// TestUpdateApprovalListReAddCriteria restores through every approval criteria EvaluateUserApproval knows, +// exactly as it evaluates them: only a positive match against the entries in effect counts +func TestUpdateApprovalListReAddCriteria(t *testing.T) { + cases := []struct { + name string + ccla readdCCLA + user func(*models.User) + params *models.ApprovalList + orgs map[string][]string + orgErr error + restored bool + // storeKey is the active PR lookup proving the restored user reached the GitHub status refresh + storeKey string + wantLookups int + wantErr string + }{ + {name: "email, case-insensitively", ccla: readdCCLA{emails: []string{"keep@acme.test"}}, + user: func(u *models.User) { u.LfEmail = "Alice@Acme.Test" }, + params: &models.ApprovalList{AddEmailApprovalList: []string{"alice@acme.test"}}, restored: true, storeKey: "active_pr:e:Alice@Acme.Test"}, + {name: "secondary email", ccla: readdCCLA{emails: []string{"keep@acme.test"}}, + user: func(u *models.User) { u.LfEmail = "primary@acme.test"; u.Emails = []string{"alice@acme.test"} }, + params: &models.ApprovalList{AddEmailApprovalList: []string{"alice@acme.test"}}, restored: true}, + {name: "wildcard domain", ccla: readdCCLA{domains: []string{"other.example"}}, + params: &models.ApprovalList{AddDomainApprovalList: []string{"*.acme.test"}}, restored: true, storeKey: "active_pr:e:alice@sub.acme.test", + user: func(u *models.User) { u.LfEmail = "alice@sub.acme.test" }}, + {name: "plain domain", ccla: readdCCLA{domains: []string{"other.example"}}, + params: &models.ApprovalList{AddDomainApprovalList: []string{"acme.test"}}, restored: true, storeKey: "active_pr:e:alice@acme.test"}, + {name: "unrelated domain", ccla: readdCCLA{domains: []string{"other.example"}}, + params: &models.ApprovalList{AddDomainApprovalList: []string{"elsewhere.test"}}, restored: false}, + {name: "GitHub username, case-insensitively", ccla: readdCCLA{githubUsers: []string{"keeper"}}, + user: func(u *models.User) { u.GithubUsername = "AliceGH" }, + params: &models.ApprovalList{AddGithubUsernameApprovalList: []string{"alicegh"}}, restored: true, storeKey: "active_pr:u:AliceGH"}, + {name: "GitLab username", ccla: readdCCLA{gitlabUsers: []string{"keeper"}}, + user: func(u *models.User) { u.GitlabUsername = readdAliceGitLab }, + params: &models.ApprovalList{AddGitlabUsernameApprovalList: []string{"alice-gl"}}, restored: true}, + {name: "GitHub organization membership", ccla: readdCCLA{githubOrgs: []string{"keep-org"}}, + user: func(u *models.User) { u.GithubUsername = readdAliceGitHub }, + params: &models.ApprovalList{AddGithubOrgApprovalList: []string{"acme-org"}}, orgs: map[string][]string{"alicegh": {"acme-org"}}, + restored: true, storeKey: "active_pr:u:alicegh", wantLookups: 1}, + {name: "GitHub organization lookup failure is reported, restores nothing", ccla: readdCCLA{githubOrgs: []string{"keep-org"}}, + user: func(u *models.User) { u.GithubUsername = readdAliceGitHub }, + params: &models.ApprovalList{AddGithubOrgApprovalList: []string{"acme-org"}}, orgErr: errors.New("github is down"), restored: false, wantLookups: 1, + wantErr: "the GitHub organization membership lookup failed"}, + {name: "GitHub organization non-member", ccla: readdCCLA{githubOrgs: []string{"keep-org"}}, + user: func(u *models.User) { u.GithubUsername = readdAliceGitHub }, + params: &models.ApprovalList{AddGithubOrgApprovalList: []string{"acme-org"}}, orgs: map[string][]string{"alicegh": {"another-org"}}, restored: false, wantLookups: 1}, + {name: "GitHub organization without a GitHub user never looks up", ccla: readdCCLA{githubOrgs: []string{"keep-org"}}, + params: &models.ApprovalList{AddGithubOrgApprovalList: []string{"acme-org"}}, orgs: map[string][]string{"alicegh": {"acme-org"}}, restored: false, wantLookups: 0}, + {name: "GitLab group is not evaluated", ccla: readdCCLA{gitlabOrgs: []string{"keep-group"}}, + user: func(u *models.User) { u.GitlabUsername = readdAliceGitLab }, + params: &models.ApprovalList{AddGitlabOrgApprovalList: []string{"acme-group"}}, restored: false}, + } + for _, tc := range cases { + t.Run(tc.name, func(t *testing.T) { + h := newReaddHarness(t, []map[string]interface{}{tc.ccla.item(), readdRemovedRow(1, "alice@acme.test", utils.EmailCriteria), + readdDeliberateRow(2, "carol@acme.test")}) + alice := readdUser("user-001", "alice@acme.test") + if tc.user != nil { + tc.user(alice) + } + // the entries kept on the CCLA belong to somebody, so the edit never removes a whole list + keeper := readdUser("user-keep", "keep@acme.test") + keeper.GithubUsername, keeper.GitlabUsername = readdKeeper, readdKeeper + h.registry.add(alice, readdUser("user-002", "carol@acme.test"), keeper) + h.orgs.orgs, h.orgs.err = tc.orgs, tc.orgErr + + before := h.rows() + _, err := h.call(tc.params) + if tc.wantErr != "" { + require.Error(t, err) + assert.Contains(t, err.Error(), "re-add the entries to retry") + assert.Contains(t, err.Error(), tc.wantErr) + } else { + require.NoError(t, err) + } + if tc.restored { + h.assertRestored(t, before["sig-001"]) + h.assertRecoveryReads(t) + } else { + h.assertUntouched(t, before["sig-001"]) + } + h.assertUntouched(t, before["sig-002"]) + if tc.storeKey != "" { + assert.Contains(t, h.storeReads(), tc.storeKey, "the restored contributor reached the pull request status refresh") + } + assert.Equal(t, tc.wantLookups, h.orgs.lookups(), "GitHub organization lookups") + h.table.mu.Lock() + assert.Empty(t, h.table.puts) + assert.Zero(t, h.table.conditionFailures) + h.table.mu.Unlock() + }) + } +} + +// TestUpdateApprovalListReAddAtScale restores a whole department: every employee index page is walked +// without a Limit or filter, the rows are read consistently in batches with the unprocessed keys retried, +// and running the same edit again is a no-op +func TestUpdateApprovalListReAddAtScale(t *testing.T) { + const employees = 250 + items := []map[string]interface{}{readdCCLA{domains: []string{"other.example"}}.item()} + for position := 1; position <= employees; position++ { + items = append(items, readdRemovedRow(position, fmt.Sprintf("dev%03d@acme.test", position), utils.EmailDomainCriteria)) + } + table := &fakeSignaturesTable{items: items, invalidated: map[string]int{}, maxRawPage: 40, unprocessedOnce: 7} + h := newReaddHarnessWithTable(t, table) + for position := 1; position <= employees; position++ { + h.registry.add(readdUser(fmt.Sprintf("user-%03d", position), fmt.Sprintf("dev%03d@acme.test", position))) + } + + before := h.rows() + _, err := h.call(&models.ApprovalList{AddDomainApprovalList: []string{"acme.test"}}) + require.NoError(t, err) + for id, item := range before { + if id != readdCCLAID { + h.assertRestored(t, item) + } + } + h.assertRecoveryReads(t) + + h.table.mu.Lock() + pages := 0 + for _, query := range h.table.queries { + if query.indexName == fakeEmployeeIndex && query.filter == "" { + pages++ + assert.Zero(t, query.limit) + } + } + batches, retried := 0, 0 + for _, read := range h.table.reads { + if read.tableName == readdSignatures && len(read.signatureIDs) > 1 { + batches++ + if len(read.signatureIDs) == 7 { + retried++ + } + } + } + assert.Equal(t, len(h.table.invalidated), employees) + assert.Zero(t, h.table.conditionFailures) + assert.Empty(t, h.table.puts) + h.table.mu.Unlock() + assert.GreaterOrEqual(t, pages, 7, "every 40-row page of the employee index was walked") + assert.Equal(t, 4, batches, "3 batches of at most 100 keys plus the retry of the 7 unprocessed keys") + assert.Equal(t, 1, retried) + + // the same edit again finds nothing to restore + after := h.rows() + _, err = h.call(&models.ApprovalList{AddDomainApprovalList: []string{"acme.test"}}) + require.NoError(t, err) + for id, item := range after { + if id != readdCCLAID { + h.assertUntouched(t, item) + } + } +} + +// TestUpdateApprovalListReAddLosesToConcurrentWriters: whatever happens to the acknowledgment between the +// decision and the write - a deliberate invalidation, a deletion, a replacement, a signed or approved flip - +// the pinned condition fails, nothing is upserted and the edit still succeeds +func TestUpdateApprovalListReAddLosesToConcurrentWriters(t *testing.T) { + cases := []struct { + name string + hook func(item map[string]interface{}, table *fakeSignaturesTable) + approvedByHook bool + }{ + {"deliberate invalidation lands first", func(item map[string]interface{}, _ *fakeSignaturesTable) { + item["invalidation_reason"] = fakeS("left the company") + item["invalidated_by"] = fakeS("pcc-admin") + }, false}, + {"acknowledgment deleted", func(_ map[string]interface{}, table *fakeSignaturesTable) { table.removeLocked("sig-001") }, false}, + {"acknowledgment replaced by another user's", func(item map[string]interface{}, _ *fakeSignaturesTable) { + item["signature_reference_id"] = fakeS("user-999") + }, false}, + {"acknowledgment unsigned meanwhile", func(item map[string]interface{}, _ *fakeSignaturesTable) { item["signature_signed"] = fakeFalse() }, false}, + {"acknowledgment approved meanwhile", func(item map[string]interface{}, _ *fakeSignaturesTable) { item["signature_approved"] = fakeTrue() }, true}, + } + for _, tc := range cases { + t.Run(tc.name, func(t *testing.T) { + table := &fakeSignaturesTable{items: []map[string]interface{}{readdCCLA{emails: []string{"keep@acme.test"}}.item(), + readdRemovedRow(1, "alice@acme.test", utils.EmailCriteria)}, invalidated: map[string]int{}} + fired := false + table.beforeUpdate = func(item map[string]interface{}) { + if fired || fakeItemString(item, "signature_id") != readdAliceID { + return + } + fired = true + tc.hook(item, table) + } + h := newReaddHarnessWithTable(t, table) + h.registry.add(readdUser("user-001", "alice@acme.test"), readdUser("user-keep", "keep@acme.test")) + + _, err := h.call(&models.ApprovalList{AddEmailApprovalList: []string{"alice@acme.test"}}) + require.NoError(t, err) + h.table.mu.Lock() + defer h.table.mu.Unlock() + assert.True(t, fired, "the restore write was attempted") + assert.Equal(t, 1, h.table.conditionFailures, "the pinned condition rejected the write") + assert.Empty(t, h.table.upserts) + assert.Empty(t, h.table.invalidated) + if item := h.table.find("sig-001"); item != nil { + assert.Equal(t, readdRemovalNote(utils.EmailCriteria), fakeItemString(item, "note"), "note not rewritten") + assert.Equal(t, tc.approvedByHook, readdBool(item, "signature_approved")) + assert.False(t, readdHas(item, "date_modified") && fakeItemString(item, "date_modified") != "2023-01-01T00:00:01Z", "date_modified not rewritten") + } + }) + } +} + +// TestUpdateApprovalListReAddObservesListChangedDuringLookup: entries removed by somebody else while the GitHub +// organization membership of a candidate is being looked up are observed by the consistent re-read of the corporate +// signature - nothing is restored on the strength of a list no longer in effect, and a user is evaluated again +// against what remains of the added entries +func TestUpdateApprovalListReAddObservesListChangedDuringLookup(t *testing.T) { + cases := []struct { + name string + // remaining is the organization list another manager commits during the first lookup + remaining []string + restored bool + lookups int + }{ + {"the added organizations are removed", []string{"keep-org"}, false, 1}, + {"the whole list is removed", nil, false, 1}, + {"one added organization is removed, the user is evaluated again against the other", []string{"keep-org", "acme-org"}, true, 2}, + {"the organization the user belongs to is removed, the other added one stays", []string{"keep-org", "extra-org"}, false, 2}, + } + for _, tc := range cases { + t.Run(tc.name, func(t *testing.T) { + h := newReaddHarness(t, []map[string]interface{}{readdCCLA{githubOrgs: []string{"keep-org"}}.item(), + readdRemovedRow(1, "alice@acme.test", utils.GitHubOrgCriteria)}) + alice := readdUser("user-001", "alice@acme.test") + alice.GithubUsername = readdAliceGitHub + keeper := readdUser("user-keep", "keep@acme.test") + keeper.GithubUsername = readdKeeper + h.registry.add(alice, keeper) + lookups := 0 + listUserPublicOrgs = func(context.Context, string) ([]string, error) { + lookups++ + if lookups == 1 { + h.table.mu.Lock() + ccla := h.table.find("ccla-sig") + if len(tc.remaining) == 0 { + delete(ccla, "github_org_whitelist") + } else { + ccla["github_org_whitelist"] = fakeStringList(tc.remaining...) + } + h.table.mu.Unlock() + } + return []string{"acme-org"}, nil + } + + before := h.rows() + _, err := h.call(&models.ApprovalList{AddGithubOrgApprovalList: []string{"acme-org", "extra-org"}}) + require.NoError(t, err) + if tc.restored { + h.assertRestored(t, before["sig-001"]) + } else { + h.assertUntouched(t, before["sig-001"]) + } + assert.Equal(t, tc.lookups, lookups, "GitHub organization lookups") + h.table.mu.Lock() + defer h.table.mu.Unlock() + assert.Zero(t, h.table.conditionFailures) + assert.Empty(t, h.table.upserts) + }) + } +} diff --git a/cla-backend-go/signatures/approval_list_readd_e2e_test.go b/cla-backend-go/signatures/approval_list_readd_e2e_test.go new file mode 100644 index 000000000..19a2ce21e --- /dev/null +++ b/cla-backend-go/signatures/approval_list_readd_e2e_test.go @@ -0,0 +1,227 @@ +// Copyright The Linux Foundation and each contributor to CommunityBridge. +// SPDX-License-Identifier: MIT + +package signatures + +import ( + "fmt" + "sync/atomic" + "testing" + + "github.com/linuxfoundation/easycla/cla-backend-go/gen/v1/models" + "github.com/linuxfoundation/easycla/cla-backend-go/utils" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +// TestUpdateApprovalListReAddRestoresRemovalInvalidatedAcknowledgments is the #2980 scenario end to end +// through the service: an email removal invalidates the contributor's acknowledgment, re-adding the email +// re-approves that very record (no new one), while deliberate and unexplained invalidations stay as they are. +func TestUpdateApprovalListReAddRestoresRemovalInvalidatedAcknowledgments(t *testing.T) { + bob := readdRemovedRow(2, "bob@acme.test", utils.EmailCriteria) + bob["signature_type"] = fakeS("ecla") + bob["sig_type_signed_approved_id"] = fakeS("ecla#true#true#company-1") + items := []map[string]interface{}{ + readdCCLA{emails: []string{"alice@acme.test", "keep@acme.test"}, domains: []string{"other.example"}}.item(), + fakeEclaItem(1, "alice@acme.test"), + bob, + readdDeliberateRow(3, "carol@acme.test"), + fakeEclaItem(4, "dave@acme.test"), + } + items[4]["signature_approved"] = fakeFalse() + h := newReaddHarness(t, items) + h.registry.add(readdUser("user-001", "alice@acme.test"), readdUser("user-002", "bob@acme.test"), + readdUser("user-003", "carol@acme.test"), readdUser("user-004", "dave@acme.test"), readdUser("user-keep", "keep@acme.test")) + + // the removal + before := h.rows() + _, err := h.call(&models.ApprovalList{RemoveEmailApprovalList: []string{"alice@acme.test"}}) + require.NoError(t, err) + alice := h.row(t, "sig-001") + assert.False(t, readdBool(alice, "signature_approved"), "alice's acknowledgment was invalidated by the removal") + assert.Equal(t, readdRemovalNote(utils.EmailCriteria), fakeItemString(alice, "note")) + assert.Equal(t, ApprovalListRemovalReasonPrefix+utils.EmailCriteria+")", fakeItemString(alice, "invalidation_reason")) + assert.Equal(t, readdManager, fakeItemString(alice, "invalidated_by")) + assert.NotEmpty(t, fakeItemString(alice, "date_invalidated")) + assert.Empty(t, h.signatureReads(), "a remove-only edit reads no signature by key") + h.assertUntouched(t, before["sig-002"]) + h.assertUntouched(t, before["sig-003"]) + h.assertUntouched(t, before["sig-004"]) + + // the re-add (Bob's entry with a different case than his stored email) + before = h.rows() + logged := atomic.LoadInt64(h.events) + updated, err := h.call(&models.ApprovalList{AddEmailApprovalList: []string{"alice@acme.test", "Bob@acme.test"}}) + require.NoError(t, err) + require.NotNil(t, updated) + assert.ElementsMatch(t, []string{"keep@acme.test", "alice@acme.test", "Bob@acme.test"}, updated.EmailApprovalList) + + h.assertRestored(t, before["sig-001"]) + h.assertRestored(t, before["sig-002"]) + h.assertUntouched(t, before["sig-003"]) + h.assertUntouched(t, before["sig-004"]) + h.assertRecoveryReads(t) + + h.table.mu.Lock() + assert.Empty(t, h.table.puts, "no acknowledgment was created") + assert.Empty(t, h.table.upserts, "no acknowledgment was upserted") + assert.Zero(t, h.table.conditionFailures) + assert.Equal(t, map[string]int{"sig-001": 2, "sig-002": 1}, h.table.invalidated, "approval flips: alice's removal + both restores") + h.table.mu.Unlock() + assert.Greater(t, atomic.LoadInt64(h.events), logged, "the approval list update was logged") + h.emails.mu.Lock() + assert.Contains(t, h.emails.recipients, "manager@example.com") + h.emails.mu.Unlock() + assert.Subset(t, h.storeReads(), []string{"active_pr:e:alice@acme.test", "active_pr:e:bob@acme.test"}, + "the restored contributors had their pull request status refreshed") +} + +// TestUpdateApprovalListReAddWithAutoCreateRestoresBeforeCreating: with auto-create enabled the restore runs +// first, so the restored contributor keeps the original acknowledgment, a brand-new contributor gets a new +// one, and a deliberately invalidated contributor gets neither +func TestUpdateApprovalListReAddWithAutoCreateRestoresBeforeCreating(t *testing.T) { + keep := fakeEclaItem(9, "keep@acme.test") + items := []map[string]interface{}{ + readdCCLA{emails: []string{"keep@acme.test"}, domains: []string{"other.example"}, autoCreate: true}.item(), + readdRemovedRow(1, "alice@acme.test", utils.EmailCriteria), + readdDeliberateRow(3, "carol@acme.test"), + keep, + } + h := newReaddHarness(t, items) + h.registry.add(readdUser("user-001", "alice@acme.test"), readdUser("user-003", "carol@acme.test"), readdUser("user-009", "keep@acme.test")) + + before := h.rows() + _, err := h.call(&models.ApprovalList{AddEmailApprovalList: []string{"alice@acme.test", "new@acme.test", "carol@acme.test"}}) + require.NoError(t, err) + + h.assertRestored(t, before["sig-001"]) + h.assertUntouched(t, before["sig-003"]) + h.assertUntouched(t, before["sig-009"]) + h.table.mu.Lock() + defer h.table.mu.Unlock() + require.Len(t, h.table.puts, 1, "only the new contributor gets a new acknowledgment") + assert.Equal(t, "created-1", fakeItemString(h.table.puts[0], "signature_reference_id")) + assert.Equal(t, "cla-group-1", fakeItemString(h.table.puts[0], "signature_project_id")) + assert.Empty(t, h.table.upserts) + assert.Zero(t, h.table.conditionFailures) + assert.Equal(t, map[string]int{"sig-001": 1}, h.table.invalidated) +} + +// readdMatrixItems seeds one acknowledgment per case the restore must decide on +func readdMatrixItems() []map[string]interface{} { + otherCompany := readdRemovedRow(7, "grace@acme.test", utils.EmailCriteria) + otherCompany["signature_user_ccla_company_id"] = fakeS("company-2") + otherProject := readdRemovedRow(8, "heidi@acme.test", utils.EmailCriteria) + otherProject["signature_project_id"] = fakeS("cla-group-2") + eclaTyped := readdRemovedRow(9, "ivan@acme.test", utils.GitHubUsernameCriteria) + eclaTyped["signature_type"] = fakeS("ecla") + unsigned := readdRemovedRow(6, "frank@acme.test", utils.EmailCriteria) + unsigned["signature_signed"] = fakeFalse() + ambiguous := fakeEclaItem(5, "erin@acme.test") + ambiguous["signature_approved"] = fakeFalse() + legacyDeliberate := fakeEclaItem(4, "dave@acme.test") + legacyDeliberate["signature_approved"] = fakeFalse() + legacyDeliberate["note"] = fakeS(readdDeliberateNote) + return []map[string]interface{}{ + readdCCLA{emails: []string{"keep@acme.test", "nobody@acme.test"}, domains: []string{"other.example"}}.item(), + readdRemovedRow(1, "alice@acme.test", utils.EmailCriteria), + readdLegacyRemovedRow(2, "bob@acme.test", utils.EmailDomainCriteria), + readdDeliberateRow(3, "carol@acme.test"), + legacyDeliberate, + ambiguous, + unsigned, + otherCompany, + otherProject, + eclaTyped, + readdRemovedRow(10, "judy@acme.test", utils.EmailCriteria), + fakeEclaItem(11, "kate@acme.test"), + readdRemovedRow(12, "leo@acme.test", utils.EmailCriteria), + readdRemovedRow(13, "mia@acme.test", utils.GitHubOrgCriteria), + } +} + +func readdMatrixHarness(t *testing.T) *readdHarness { + t.Helper() + h := newReaddHarness(t, readdMatrixItems()) + for position, name := range []string{"alice", "bob", "carol", "dave", "erin", "frank", "grace", "heidi", "ivan", "judy", "kate"} { + h.registry.add(readdUser(fmt.Sprintf("user-%03d", position+1), name+"@acme.test")) + } + // leo (user-012) is unknown to the users table; mia is known + h.registry.add(readdUser("user-013", "mia@acme.test"), readdUser("user-keep", "keep@acme.test")) + return h +} + +// TestUpdateApprovalListReAddMatrix decides every seeded case in one edit: only signed, unapproved +// acknowledgments whose only invalidation evidence is an approval list removal, under this company and +// CLA group, whose user the re-added entries cover, are restored - whatever the criteria of the removal was +func TestUpdateApprovalListReAddMatrix(t *testing.T) { + h := readdMatrixHarness(t) + before := h.rows() + added := []string{"alice@acme.test", "bob@acme.test", "carol@acme.test", "dave@acme.test", "erin@acme.test", "frank@acme.test", + "grace@acme.test", "heidi@acme.test", "ivan@acme.test", "kate@acme.test", "leo@acme.test", "mia@acme.test"} + _, err := h.call(&models.ApprovalList{AddEmailApprovalList: added}) + require.NoError(t, err) + + for _, restored := range []string{"sig-001", "sig-002", "sig-009", "sig-013"} { + h.assertRestored(t, before[restored]) + } + for _, untouched := range []string{"sig-003", "sig-004", "sig-005", "sig-006", "sig-007", "sig-008", "sig-010", "sig-011", "sig-012"} { + h.assertUntouched(t, before[untouched]) + } + h.assertRecoveryReads(t) + h.table.mu.Lock() + assert.Empty(t, h.table.puts) + assert.Empty(t, h.table.upserts) + assert.Zero(t, h.table.conditionFailures) + assert.Equal(t, map[string]int{"sig-001": 1, "sig-002": 1, "sig-009": 1, "sig-013": 1}, h.table.invalidated) + h.table.mu.Unlock() + assert.Equal(t, 1, h.registry.lookups("user-012"), "the unknown user was looked up once and skipped") +} + +func TestUpdateApprovalListReAddNothingToRestore(t *testing.T) { + t.Run("remove-only edit restores nothing and reads no signature by key", func(t *testing.T) { + h := readdMatrixHarness(t) + before := h.rows() + _, err := h.call(&models.ApprovalList{RemoveEmailApprovalList: []string{"nobody@acme.test"}}) + require.NoError(t, err) + for id, item := range before { + if id != readdCCLAID { + h.assertUntouched(t, item) + } + } + assert.Empty(t, h.signatureReads()) + h.table.mu.Lock() + assert.Empty(t, h.table.invalidated) + h.table.mu.Unlock() + }) + t.Run("unrelated add evaluates the candidates and restores none", func(t *testing.T) { + h := readdMatrixHarness(t) + before := h.rows() + _, err := h.call(&models.ApprovalList{AddEmailApprovalList: []string{"zed@acme.test"}}) + require.NoError(t, err) + for id, item := range before { + if id != readdCCLAID { + h.assertUntouched(t, item) + } + } + h.assertRecoveryReads(t) + h.table.mu.Lock() + assert.Empty(t, h.table.invalidated) + assert.Empty(t, h.table.puts) + h.table.mu.Unlock() + }) + t.Run("adding and removing the same entry in one edit restores nothing", func(t *testing.T) { + h := readdMatrixHarness(t) + before := h.rows() + _, err := h.call(&models.ApprovalList{AddEmailApprovalList: []string{"alice@acme.test"}, RemoveEmailApprovalList: []string{"alice@acme.test"}}) + require.NoError(t, err) + h.assertUntouched(t, before["sig-001"]) + for _, read := range h.signatureReads() { + assert.Equal(t, []string{"ccla-sig"}, read.signatureIDs, "the entry is not in effect - no candidate is read") + } + h.table.mu.Lock() + assert.Equal(t, 1, h.table.conditionFailures, "the removal pass re-tried alice's already invalidated acknowledgment") + assert.Empty(t, h.table.invalidated) + h.table.mu.Unlock() + }) +} diff --git a/cla-backend-go/signatures/approval_list_readd_test.go b/cla-backend-go/signatures/approval_list_readd_test.go new file mode 100644 index 000000000..96d158a1a --- /dev/null +++ b/cla-backend-go/signatures/approval_list_readd_test.go @@ -0,0 +1,453 @@ +// Copyright The Linux Foundation and each contributor to CommunityBridge. +// SPDX-License-Identifier: MIT + +package signatures + +import ( + "context" + "errors" + "fmt" + "strings" + "sync" + "sync/atomic" + "testing" + "time" + + "github.com/LF-Engineering/lfx-kit/auth" + "github.com/aws/aws-sdk-go/service/dynamodb" + "github.com/go-openapi/strfmt" + "github.com/golang/mock/gomock" + mock_company "github.com/linuxfoundation/easycla/cla-backend-go/company/mocks" + "github.com/linuxfoundation/easycla/cla-backend-go/events" + eventsMock "github.com/linuxfoundation/easycla/cla-backend-go/events/mock" + "github.com/linuxfoundation/easycla/cla-backend-go/gen/v1/models" + "github.com/linuxfoundation/easycla/cla-backend-go/users" + mock_users "github.com/linuxfoundation/easycla/cla-backend-go/users/mocks" + "github.com/linuxfoundation/easycla/cla-backend-go/utils" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +// Shared fixtures and harness for the approval list re-add tests (#2980): re-adding approval list +// entries must re-approve the signed employee acknowledgments that only an earlier approval list +// removal had invalidated, and never the deliberately invalidated ones. + +const ( + readdManager = "manager-lf" + readdSignatures = "cla-test-signatures" + readdCCLAID = "ccla-sig" + readdAliceID = "sig-001" + readdAliceGitHub = "alicegh" + readdAliceGitLab = "alice-gl" + readdKeeper = "keeper" + readdRestorePrefix = "Re-enabled employee acknowledgment previously disabled by approval list removal via CLA Manager " + readdManager + " approval list edit on " + // the note the removal path writes (verifyUserApprovals keeps two spaces before "removal") + readdRemovalNoteFormat = "Signature invalidated (approved set to false) by " + readdManager + " due to %s removal" + readdDeliberateNote = "Signature invalidated (approved set to false) by pcc-admin for legacy-dev" +) + +func readdRemovalNote(criteria string) string { return fmt.Sprintf(readdRemovalNoteFormat, criteria) } + +// readdRemovedRow is a signed acknowledgment an approval list removal invalidated, with the M2 attribution +func readdRemovedRow(position int, email, criteria string) map[string]interface{} { + item := readdLegacyRemovedRow(position, email, criteria) + item["date_invalidated"] = fakeS("2026-09-01T10:11:12.123456+0000") + item["invalidated_by"] = fakeS(readdManager) + item["invalidation_reason"] = fakeS(ApprovalListRemovalReasonPrefix + criteria + ")") + return item +} + +// readdLegacyRemovedRow is a removal-invalidated acknowledgment carrying only the pre-M2 note +func readdLegacyRemovedRow(position int, email, criteria string) map[string]interface{} { + item := fakeEclaItem(position, email) + item["signature_approved"] = fakeFalse() + item["note"] = fakeS(readdRemovalNote(criteria)) + return item +} + +// readdDeliberateRow is an acknowledgment somebody invalidated on purpose +func readdDeliberateRow(position int, email string) map[string]interface{} { + item := fakeEclaItem(position, email) + item["signature_approved"] = fakeFalse() + item["note"] = fakeS(readdDeliberateNote) + item["date_invalidated"] = fakeS("2026-09-02T10:11:12.123456+0000") + item["invalidated_by"] = fakeS("pcc-admin") + item["invalidation_reason"] = fakeS("left the company") + item["invalidation_note"] = fakeS("manual") + return item +} + +// readdCCLA builds the company's signed and approved corporate signature with the given approval lists +type readdCCLA struct { + emails, domains, githubUsers, githubOrgs, gitlabUsers, gitlabOrgs []string + autoCreate bool +} + +func (c readdCCLA) item() map[string]interface{} { + item := map[string]interface{}{ + "signature_id": fakeS("ccla-sig"), + "signature_project_id": fakeS("cla-group-1"), + "signature_reference_id": fakeS("company-1"), + "signature_reference_type": fakeS("company"), + "signature_reference_name": fakeS("Acme"), + "signature_type": fakeS("ccla"), + "signature_approved": fakeTrue(), + "signature_signed": fakeTrue(), + "signature_acl": fakeStringList(readdManager), + "date_created": fakeS("2022-01-01T00:00:00Z"), + "date_modified": fakeS("2022-01-01T00:00:00Z"), + } + if c.autoCreate { + item["auto_create_ecla"] = fakeTrue() + } + for name, values := range map[string][]string{ + "email_whitelist": c.emails, "domain_whitelist": c.domains, "github_whitelist": c.githubUsers, + "github_org_whitelist": c.githubOrgs, "gitlab_username_approval_list": c.gitlabUsers, "gitlab_org_approval_list": c.gitlabOrgs, + } { + if len(values) > 0 { + item[name] = fakeStringList(values...) + } + } + return item +} + +// readdUser is an employee of company-1 known to the users table +func readdUser(id, email string) *models.User { + return &models.User{UserID: id, LfEmail: strfmt.Email(email), Username: id, CompanyID: "company-1"} +} + +// readdRegistry is the users table: lookups hand out copies, so the service may mutate them freely +type readdRegistry struct { + mu sync.Mutex + users map[string]*models.User + created int + getUserCalls []string +} + +func newReaddRegistry() *readdRegistry { + registry := &readdRegistry{users: map[string]*models.User{}} + registry.add(&models.User{UserID: "manager-user", LfUsername: readdManager, Username: readdManager, LfEmail: "manager@example.com", CompanyID: "company-1"}) + return registry +} + +func (r *readdRegistry) add(list ...*models.User) { + r.mu.Lock() + defer r.mu.Unlock() + for _, user := range list { + r.users[user.UserID] = user + } +} + +func (r *readdRegistry) find(match func(*models.User) bool) *models.User { + r.mu.Lock() + defer r.mu.Unlock() + for _, user := range r.users { + if match(user) { + clone := *user + return &clone + } + } + return nil +} + +func (r *readdRegistry) byID(id string) (*models.User, error) { + r.mu.Lock() + r.getUserCalls = append(r.getUserCalls, id) + r.mu.Unlock() + return r.find(func(u *models.User) bool { return u.UserID == id }), nil +} + +func (r *readdRegistry) byUserName(name string, _ bool) (*models.User, error) { + return r.find(func(u *models.User) bool { return u.LfUsername == name }), nil +} + +func readdUserHasEmail(u *models.User, email string) bool { + if strings.EqualFold(string(u.LfEmail), email) { + return true + } + for _, candidate := range u.Emails { + if strings.EqualFold(candidate, email) { + return true + } + } + return false +} + +func (r *readdRegistry) byEmail(email string) (*models.User, error) { + if user := r.find(func(u *models.User) bool { return readdUserHasEmail(u, email) }); user != nil { + return user, nil + } + return nil, &utils.UserNotFound{Message: "user not found", UserEmail: email} +} + +func (r *readdRegistry) byGitHub(login string) (*models.User, error) { + if user := r.find(func(u *models.User) bool { return u.GithubUsername != "" && strings.EqualFold(u.GithubUsername, login) }); user != nil { + return user, nil + } + return nil, errors.New("github user not found: " + login) +} + +func (r *readdRegistry) byGitLab(login string) (*models.User, error) { + if user := r.find(func(u *models.User) bool { return u.GitlabUsername != "" && strings.EqualFold(u.GitlabUsername, login) }); user != nil { + return user, nil + } + return nil, errors.New("gitlab user not found: " + login) +} + +func (r *readdRegistry) search(_ string, term string, _ bool) (*models.Users, error) { + result := &models.Users{Users: []models.User{}} + r.mu.Lock() + defer r.mu.Unlock() + for _, user := range r.users { + if readdUserHasEmail(user, term) { + result.Users = append(result.Users, *user) + } + } + return result, nil +} + +func (r *readdRegistry) updateCompany(userID, companyID, _ string) error { + r.mu.Lock() + defer r.mu.Unlock() + if user, ok := r.users[userID]; ok { + user.CompanyID = companyID + } + return nil +} + +func (r *readdRegistry) create(user *models.User) (*models.User, error) { + r.mu.Lock() + defer r.mu.Unlock() + r.created++ + clone := *user + clone.UserID = fmt.Sprintf("created-%d", r.created) + r.users[clone.UserID] = &clone + result := clone + return &result, nil +} + +func (r *readdRegistry) lookups(id string) int { + r.mu.Lock() + defer r.mu.Unlock() + count := 0 + for _, call := range r.getUserCalls { + if call == id { + count++ + } + } + return count +} + +// readdOrgStub stands in for the GitHub public organization lookup +type readdOrgStub struct { + mu sync.Mutex + orgs map[string][]string + err error + calls []string +} + +func (o *readdOrgStub) list(_ context.Context, login string) ([]string, error) { + o.mu.Lock() + defer o.mu.Unlock() + o.calls = append(o.calls, login) + if o.err != nil { + return nil, o.err + } + return o.orgs[login], nil +} + +func (o *readdOrgStub) lookups() int { + o.mu.Lock() + defer o.mu.Unlock() + return len(o.calls) +} + +type readdHarness struct { + table *fakeSignaturesTable + repo repository + svc service + registry *readdRegistry + approvals *fakeApprovalRepo + emails *recordingEmailSender + events *int64 + orgs *readdOrgStub +} + +func newReaddHarness(t *testing.T, items []map[string]interface{}) *readdHarness { + t.Helper() + return newReaddHarnessWithTable(t, &fakeSignaturesTable{items: items, invalidated: map[string]int{}}) +} + +// newReaddHarnessWithTable wires the real signature repository and service to the fake table, a users +// table registry, stubbed company/events dependencies, a recording email sender and an org stub +func newReaddHarnessWithTable(t *testing.T, table *fakeSignaturesTable) *readdHarness { + t.Helper() + sess, closeServer := newApprovalRemovalSession(t, table) + t.Cleanup(closeServer) + ctrl := gomock.NewController(t) + + registry := newReaddRegistry() + mockUsers := mock_users.NewMockUserRepository(ctrl) + mockUsers.EXPECT().GetUser(gomock.Any()).DoAndReturn(registry.byID).AnyTimes() + mockUsers.EXPECT().GetUserByUserName(gomock.Any(), gomock.Any()).DoAndReturn(registry.byUserName).AnyTimes() + mockUsers.EXPECT().GetUserByEmail(gomock.Any()).DoAndReturn(registry.byEmail).AnyTimes() + mockUsers.EXPECT().GetUserByGitHubUsername(gomock.Any()).DoAndReturn(registry.byGitHub).AnyTimes() + mockUsers.EXPECT().GetUserByGitLabUsername(gomock.Any()).DoAndReturn(registry.byGitLab).AnyTimes() + mockUsers.EXPECT().SearchUsers(gomock.Any(), gomock.Any(), gomock.Any()).DoAndReturn(registry.search).AnyTimes() + mockUsers.EXPECT().UpdateUserCompanyID(gomock.Any(), gomock.Any(), gomock.Any()).DoAndReturn(registry.updateCompany).AnyTimes() + mockUsers.EXPECT().CreateUser(gomock.Any()).DoAndReturn(registry.create).AnyTimes() + + mockCompany := mock_company.NewMockIRepository(ctrl) + mockCompany.EXPECT().GetCompany(gomock.Any(), gomock.Any()).Return(fakeCompany(), nil).AnyTimes() + + logged := new(int64) + mockEvents := eventsMock.NewMockService(ctrl) + mockEvents.EXPECT().LogEvent(gomock.Any()).Do(func(*events.LogEventArgs) { atomic.AddInt64(logged, 1) }).AnyTimes() + mockEvents.EXPECT().LogEventWithContext(gomock.Any(), gomock.Any()).Do(func(context.Context, *events.LogEventArgs) { atomic.AddInt64(logged, 1) }).AnyTimes() + + emails := &recordingEmailSender{} + previousSender := utils.GetEmailSender() + utils.SetEmailSender(emails) + t.Cleanup(func() { utils.SetEmailSender(previousSender) }) + + orgs := &readdOrgStub{orgs: map[string][]string{}} + previousOrgs := listUserPublicOrgs + listUserPublicOrgs = orgs.list + t.Cleanup(func() { listUserPublicOrgs = previousOrgs }) + + approvalsRepo := &fakeApprovalRepo{} + repo := repository{stage: "test", dynamoDBClient: dynamodb.New(sess), companyRepo: mockCompany, usersRepo: mockUsers, + eventsService: mockEvents, signatureTableName: readdSignatures, approvalRepo: approvalsRepo} + svc := service{repo: repo, usersService: users.NewService(mockUsers, mockEvents), eventsService: mockEvents} + return &readdHarness{table: table, repo: repo, svc: svc, registry: registry, approvals: approvalsRepo, emails: emails, events: logged, orgs: orgs} +} + +// call edits the approval list as the CLA manager through the service, the way the v4 handler does +func (h *readdHarness) call(params *models.ApprovalList) (*models.Signature, error) { + return h.svc.UpdateApprovalList(context.Background(), &auth.User{UserName: readdManager, Email: "manager@example.com"}, + &models.ClaGroup{ProjectID: "cla-group-1", ProjectName: "My Project", Version: "v2"}, fakeCompany(), "cla-group-1", params, "project-sfid") +} + +// row returns a shallow copy of the stored item, read directly so the table's read log stays untouched +func (h *readdHarness) row(t *testing.T, signatureID string) map[string]interface{} { + t.Helper() + h.table.mu.Lock() + defer h.table.mu.Unlock() + item := h.table.find(signatureID) + require.NotNil(t, item, "signature %s exists", signatureID) + return fakeCopyItem(item) +} + +// rows snapshots every stored item by signature ID +func (h *readdHarness) rows() map[string]map[string]interface{} { + h.table.mu.Lock() + defer h.table.mu.Unlock() + out := map[string]map[string]interface{}{} + for _, item := range h.table.items { + out[fakeItemString(item, "signature_id")] = fakeCopyItem(item) + } + return out +} + +func readdBool(item map[string]interface{}, name string) bool { + if attr, ok := item[name].(map[string]interface{}); ok { + if value, ok := attr["BOOL"].(bool); ok { + return value + } + } + return false +} + +func readdHas(item map[string]interface{}, name string) bool { + _, ok := item[name] + return ok +} + +// signatureReads returns the GetItem/BatchGetItem calls that hit the signatures table +func (h *readdHarness) signatureReads() []fakeCapturedRead { + h.table.mu.Lock() + defer h.table.mu.Unlock() + var reads []fakeCapturedRead + for _, read := range h.table.reads { + if read.tableName == readdSignatures { + reads = append(reads, read) + } + } + return reads +} + +// storeReads returns the keys looked up in the store table (the active PR metadata of a user) +func (h *readdHarness) storeReads() []string { + h.table.mu.Lock() + defer h.table.mu.Unlock() + var keys []string + for _, read := range h.table.reads { + if read.tableName != readdSignatures && len(read.signatureIDs) == 1 { + keys = append(keys, read.signatureIDs[0]) + } + } + return keys +} + +// assertRestored checks that the acknowledgment is approved again with the attribution cleared and the +// restore note appended to what was there, and that nothing else about it changed +func (h *readdHarness) assertRestored(t *testing.T, before map[string]interface{}) { + t.Helper() + signatureID := fakeItemString(before, "signature_id") + after := h.row(t, signatureID) + assert.True(t, readdBool(after, "signature_approved"), "%s approved again", signatureID) + assert.True(t, readdBool(after, "signature_signed"), "%s still signed", signatureID) + for _, attr := range []string{"date_invalidated", "invalidated_by", "invalidation_reason", "invalidation_note"} { + assert.False(t, readdHas(after, attr), "%s %s cleared", signatureID, attr) + } + note := fakeItemString(after, "note") + wantPrefix := strings.TrimSpace(fakeItemString(before, "note") + " " + readdRestorePrefix) + assert.True(t, strings.HasPrefix(note, wantPrefix), "%s note %q keeps the history and gets the restore note", signatureID, note) + assert.True(t, strings.HasSuffix(note, "Z."), "%s note ends with the edit timestamp: %q", signatureID, note) + modified, parseErr := time.Parse(time.RFC3339, fakeItemString(after, "date_modified")) + require.NoError(t, parseErr, "%s date_modified is a timestamp", signatureID) + assert.Less(t, time.Since(modified), time.Minute, "%s date_modified refreshed", signatureID) + for _, attr := range []string{"signature_id", "signature_project_id", "signature_reference_id", "signature_reference_type", "signature_type", + "signature_user_ccla_company_id", "user_email", "date_created"} { + assert.Equal(t, before[attr], after[attr], "%s %s unchanged", signatureID, attr) + } +} + +// assertUntouched checks that the stored item is exactly what it was +func (h *readdHarness) assertUntouched(t *testing.T, before map[string]interface{}) { + t.Helper() + signatureID := fakeItemString(before, "signature_id") + assert.Equal(t, before, h.row(t, signatureID), "%s untouched", signatureID) +} + +// assertRecoveryReads checks the candidate query shape and that every signatures-table read was strongly +// consistent, including the corporate signature GetItem and at least one BatchGetItem +func (h *readdHarness) assertRecoveryReads(t *testing.T) { + t.Helper() + reads := h.signatureReads() + require.NotEmpty(t, reads) + sawCCLA, sawBatch := false, false + for _, read := range reads { + assert.True(t, read.consistentRead, "read of %v is strongly consistent", read.signatureIDs) + if len(read.signatureIDs) == 1 && read.signatureIDs[0] == readdCCLAID { + sawCCLA = true + } + if len(read.signatureIDs) > 1 { + sawBatch = true + } + } + assert.True(t, sawCCLA, "the corporate signature was re-read consistently") + assert.True(t, sawBatch, "the candidates were read consistently in batch") + h.table.mu.Lock() + defer h.table.mu.Unlock() + found := false + for _, query := range h.table.queries { + if query.indexName != fakeEmployeeIndex || query.filter != "" { + continue + } + found = true + assert.Zero(t, query.limit, "candidate query has no Limit") + assert.ElementsMatch(t, []string{"signature_user_ccla_company_id", "signature_project_id", "signature_id"}, query.attributeNames) + } + assert.True(t, found, "the candidates were listed through the employee index without a filter") +} diff --git a/cla-backend-go/signatures/approval_list_readd_unit_test.go b/cla-backend-go/signatures/approval_list_readd_unit_test.go new file mode 100644 index 000000000..2ce7347a4 --- /dev/null +++ b/cla-backend-go/signatures/approval_list_readd_unit_test.go @@ -0,0 +1,650 @@ +// Copyright The Linux Foundation and each contributor to CommunityBridge. +// SPDX-License-Identifier: MIT + +package signatures + +import ( + "context" + "errors" + "strings" + "sync/atomic" + "testing" + + "github.com/golang/mock/gomock" + eventsMock "github.com/linuxfoundation/easycla/cla-backend-go/events/mock" + "github.com/linuxfoundation/easycla/cla-backend-go/gen/v1/models" + "github.com/linuxfoundation/easycla/cla-backend-go/users" + mock_users "github.com/linuxfoundation/easycla/cla-backend-go/users/mocks" + "github.com/linuxfoundation/easycla/cla-backend-go/utils" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +// readdFakeRepo answers the three restore calls of the service directly +type readdFakeRepo struct { + SignatureRepository + ccla *ItemSignature + cclaErr error + // later, when set, answers every consistent read of the corporate signature after the first one + later func(read int) (*ItemSignature, error) + candidates []*ItemSignature + candidatesErr error + restoreErr map[string]error + restoreSkipped map[string]bool + cclaCalls int + candidateCalls int + restored []string + notes []string +} + +func (f *readdFakeRepo) GetItemSignatureConsistent(_ context.Context, _ string) (*ItemSignature, error) { + f.cclaCalls++ + if f.cclaCalls > 1 && f.later != nil { + return f.later(f.cclaCalls) + } + return f.ccla, f.cclaErr +} + +func (f *readdFakeRepo) GetRemovalInvalidatedEmployeeSignatures(_ context.Context, _, _ string) ([]*ItemSignature, error) { + f.candidateCalls++ + return f.candidates, f.candidatesErr +} + +func (f *readdFakeRepo) RestoreRemovalInvalidatedEmployeeSignature(_ context.Context, snapshot *ItemSignature, note string) (bool, error) { + if err := f.restoreErr[snapshot.SignatureID]; err != nil { + return false, err + } + f.restored = append(f.restored, snapshot.SignatureID) + f.notes = append(f.notes, note) + return !f.restoreSkipped[snapshot.SignatureID], nil +} + +func readdCandidate(signatureID, userID string) *ItemSignature { + return &ItemSignature{SignatureID: signatureID, SignatureReferenceID: userID, SignatureProjectID: "cla-group-1", SignatureUserCompanyID: "company-1", + SignatureSigned: true, InvalidationReason: ApprovalListRemovalReasonPrefix + utils.EmailCriteria + ")"} +} + +// readdDirectService wires the service to the fake repository and a users table whose GetUser may fail per ID +func readdDirectService(t *testing.T, repo *readdFakeRepo, registry *readdRegistry, userErr map[string]error) service { + t.Helper() + ctrl := gomock.NewController(t) + mockUsers := mock_users.NewMockUserRepository(ctrl) + mockUsers.EXPECT().GetUser(gomock.Any()).DoAndReturn(func(id string) (*models.User, error) { + if err := userErr[id]; err != nil { + return nil, err + } + return registry.byID(id) + }).AnyTimes() + return service{repo: repo, usersService: users.NewService(mockUsers, eventsMock.NewMockService(ctrl))} +} + +func TestRestoreRemovalInvalidatedEmployeeSignaturesDirect(t *testing.T) { + manager := &models.User{UserID: "manager-user", Username: readdManager} + signedCCLA := func(emails ...string) *ItemSignature { + return &ItemSignature{SignatureID: "ccla-sig", SignatureSigned: true, SignatureApproved: true, EmailApprovalList: emails} + } + addAlice := &models.ApprovalList{AddEmailApprovalList: []string{"alice@acme.test"}} + run := func(t *testing.T, repo *readdFakeRepo, params *models.ApprovalList, userErr map[string]error) ([]*models.User, error) { + registry := newReaddRegistry() + registry.add(readdUser("user-001", "alice@acme.test"), readdUser("user-002", "bob@acme.test")) + svc := readdDirectService(t, repo, registry, userErr) + return svc.restoreRemovalInvalidatedEmployeeSignatures(context.Background(), manager, fakeClaGroup(), fakeCompany(), "ccla-sig", params) + } + + t.Run("nil params or no additions restore nothing without reading", func(t *testing.T) { + for _, params := range []*models.ApprovalList{nil, {}, {RemoveEmailApprovalList: []string{"alice@acme.test"}}} { + repo := &readdFakeRepo{ccla: signedCCLA("alice@acme.test"), candidates: []*ItemSignature{readdCandidate("sig-001", "user-001")}} + restored, err := run(t, repo, params, nil) + require.NoError(t, err) + assert.Nil(t, restored) + assert.Zero(t, repo.cclaCalls) + assert.Zero(t, repo.candidateCalls) + } + }) + t.Run("corporate signature load failure", func(t *testing.T) { + repo := &readdFakeRepo{cclaErr: errors.New("dynamo is down")} + restored, err := run(t, repo, addAlice, nil) + assert.EqualError(t, err, "unable to load corporate signature ccla-sig: dynamo is down") + assert.Nil(t, restored) + assert.Zero(t, repo.candidateCalls) + }) + t.Run("corporate signature gone, unsigned or unapproved", func(t *testing.T) { + for _, ccla := range []*ItemSignature{nil, {SignatureID: "ccla-sig", SignatureApproved: true, EmailApprovalList: []string{"alice@acme.test"}}, + {SignatureID: "ccla-sig", SignatureSigned: true, EmailApprovalList: []string{"alice@acme.test"}}} { + repo := &readdFakeRepo{ccla: ccla, candidates: []*ItemSignature{readdCandidate("sig-001", "user-001")}} + restored, err := run(t, repo, addAlice, nil) + require.NoError(t, err) + assert.Nil(t, restored) + assert.Zero(t, repo.candidateCalls) + } + }) + t.Run("added entry not in effect", func(t *testing.T) { + repo := &readdFakeRepo{ccla: signedCCLA("keep@acme.test"), candidates: []*ItemSignature{readdCandidate("sig-001", "user-001")}} + restored, err := run(t, repo, addAlice, nil) + require.NoError(t, err) + assert.Nil(t, restored) + assert.Zero(t, repo.candidateCalls) + }) + t.Run("candidates load failure", func(t *testing.T) { + repo := &readdFakeRepo{ccla: signedCCLA("alice@acme.test"), candidatesErr: errors.New("index is down")} + restored, err := run(t, repo, addAlice, nil) + assert.EqualError(t, err, "unable to load the removal-invalidated employee acknowledgments: index is down") + assert.Nil(t, restored) + }) + t.Run("no candidates", func(t *testing.T) { + repo := &readdFakeRepo{ccla: signedCCLA("alice@acme.test")} + restored, err := run(t, repo, addAlice, nil) + require.NoError(t, err) + assert.Nil(t, restored) + assert.Equal(t, 1, repo.candidateCalls) + }) + t.Run("restores the covered users, once each, with the attributed note", func(t *testing.T) { + repo := &readdFakeRepo{ccla: signedCCLA("alice@acme.test", "bob@acme.test"), candidates: []*ItemSignature{ + readdCandidate("sig-001", "user-001"), readdCandidate("sig-002", "user-002"), readdCandidate("sig-003", "user-001")}} + restored, err := run(t, repo, &models.ApprovalList{AddEmailApprovalList: []string{"alice@acme.test"}}, nil) + require.NoError(t, err) + require.Len(t, restored, 1, "bob's entry was not part of this edit") + assert.Equal(t, "user-001", restored[0].UserID) + assert.Equal(t, []string{"sig-001", "sig-003"}, repo.restored) + for _, note := range repo.notes { + assert.True(t, strings.HasPrefix(note, readdRestorePrefix) && strings.HasSuffix(note, "Z."), note) + } + }) + t.Run("the user of several acknowledgments is loaded once", func(t *testing.T) { + repo := &readdFakeRepo{ccla: signedCCLA("alice@acme.test"), candidates: []*ItemSignature{readdCandidate("sig-001", "user-001"), readdCandidate("sig-003", "user-001")}} + registry := newReaddRegistry() + registry.add(readdUser("user-001", "alice@acme.test")) + svc := readdDirectService(t, repo, registry, nil) + _, err := svc.restoreRemovalInvalidatedEmployeeSignatures(context.Background(), manager, fakeClaGroup(), fakeCompany(), "ccla-sig", addAlice) + require.NoError(t, err) + assert.Equal(t, 1, registry.lookups("user-001")) + }) + t.Run("unknown user is skipped without error", func(t *testing.T) { + repo := &readdFakeRepo{ccla: signedCCLA("alice@acme.test"), candidates: []*ItemSignature{readdCandidate("sig-009", "user-unknown"), readdCandidate("sig-001", "user-001")}} + restored, err := run(t, repo, addAlice, nil) + require.NoError(t, err) + assert.Equal(t, []string{"sig-001"}, repo.restored) + require.Len(t, restored, 1) + }) + t.Run("a restore reported as not done keeps the user off the list", func(t *testing.T) { + repo := &readdFakeRepo{ccla: signedCCLA("alice@acme.test"), candidates: []*ItemSignature{readdCandidate("sig-001", "user-001")}, restoreSkipped: map[string]bool{"sig-001": true}} + restored, err := run(t, repo, addAlice, nil) + require.NoError(t, err) + assert.Nil(t, restored) + assert.Equal(t, []string{"sig-001"}, repo.restored) + }) + t.Run("failures are collected and the rest is still restored", func(t *testing.T) { + // alice restores, bob's write fails, dave's user cannot be loaded, erin's evaluation trips over a broken domain pattern + repo := &readdFakeRepo{ccla: signedCCLA("alice@acme.test", "bob@acme.test"), candidates: []*ItemSignature{ + readdCandidate("sig-001", "user-001"), readdCandidate("sig-002", "user-002"), readdCandidate("sig-004", "user-004"), readdCandidate("sig-005", "user-005")}, + restoreErr: map[string]error{"sig-002": errors.New("write failed")}} + repo.ccla.EmailDomainApprovalList = []string{"acme.(test"} + params := &models.ApprovalList{AddEmailApprovalList: []string{"alice@acme.test", "bob@acme.test"}, AddDomainApprovalList: []string{"acme.(test"}} + registry := newReaddRegistry() + registry.add(readdUser("user-001", "alice@acme.test"), readdUser("user-002", "bob@acme.test"), readdUser("user-005", "erin@acme.test")) + svc := readdDirectService(t, repo, registry, map[string]error{"user-004": errors.New("users table is down")}) + restored, err := svc.restoreRemovalInvalidatedEmployeeSignatures(context.Background(), manager, fakeClaGroup(), fakeCompany(), "ccla-sig", params) + require.Error(t, err) + assert.Contains(t, err.Error(), "unable to restore signature sig-002: write failed") + assert.Contains(t, err.Error(), "unable to load user user-004 of signature sig-004: users table is down") + assert.Contains(t, err.Error(), "unable to evaluate user user-005 of signature sig-005: error parsing regexp") + require.Len(t, restored, 1) + assert.Equal(t, "user-001", restored[0].UserID) + assert.Equal(t, []string{"sig-001"}, repo.restored) + }) + t.Run("the corporate signature is re-read before every restore", func(t *testing.T) { + repo := &readdFakeRepo{ccla: signedCCLA("alice@acme.test", "bob@acme.test"), candidates: []*ItemSignature{ + readdCandidate("sig-001", "user-001"), readdCandidate("sig-002", "user-002"), readdCandidate("sig-003", "user-001")}} + restored, err := run(t, repo, &models.ApprovalList{AddEmailApprovalList: []string{"alice@acme.test", "bob@acme.test"}}, nil) + require.NoError(t, err) + assert.Len(t, restored, 2) + assert.Equal(t, []string{"sig-001", "sig-002", "sig-003"}, repo.restored) + assert.Equal(t, 4, repo.cclaCalls, "the initial read plus one per restore") + }) + t.Run("a removal committed meanwhile stops the restoring", func(t *testing.T) { + repo := &readdFakeRepo{ccla: signedCCLA("alice@acme.test", "bob@acme.test"), candidates: []*ItemSignature{ + readdCandidate("sig-001", "user-001"), readdCandidate("sig-002", "user-002")}} + repo.later = func(int) (*ItemSignature, error) { return signedCCLA("keep@acme.test"), nil } + restored, err := run(t, repo, &models.ApprovalList{AddEmailApprovalList: []string{"alice@acme.test", "bob@acme.test"}}, nil) + require.NoError(t, err) + assert.Nil(t, restored) + assert.Empty(t, repo.restored) + assert.Equal(t, 2, repo.cclaCalls, "the remaining candidates are not evaluated once the entries are gone") + }) + t.Run("a corporate signature deactivated or deleted meanwhile stops the restoring", func(t *testing.T) { + for _, later := range []*ItemSignature{nil, {SignatureID: "ccla-sig", SignatureSigned: true, EmailApprovalList: []string{"alice@acme.test"}}, + {SignatureID: "ccla-sig", SignatureApproved: true, EmailApprovalList: []string{"alice@acme.test"}}} { + repo := &readdFakeRepo{ccla: signedCCLA("alice@acme.test"), candidates: []*ItemSignature{readdCandidate("sig-001", "user-001"), readdCandidate("sig-003", "user-001")}} + repo.later = func(int) (*ItemSignature, error) { return later, nil } + restored, err := run(t, repo, addAlice, nil) + require.NoError(t, err) + assert.Nil(t, restored) + assert.Empty(t, repo.restored) + assert.Equal(t, 2, repo.cclaCalls) + } + }) + t.Run("a failed re-read is reported and stops the restoring", func(t *testing.T) { + repo := &readdFakeRepo{ccla: signedCCLA("alice@acme.test"), candidates: []*ItemSignature{readdCandidate("sig-001", "user-001"), readdCandidate("sig-003", "user-001")}} + repo.later = func(int) (*ItemSignature, error) { return nil, errors.New("dynamo is down") } + restored, err := run(t, repo, addAlice, nil) + assert.EqualError(t, err, "unable to reload corporate signature ccla-sig: dynamo is down") + assert.Nil(t, restored) + assert.Empty(t, repo.restored) + }) + t.Run("a list changed meanwhile is evaluated again, restoring only the users it still covers", func(t *testing.T) { + both := &models.ApprovalList{AddEmailApprovalList: []string{"alice@acme.test", "bob@acme.test"}} + for _, order := range [][]*ItemSignature{{readdCandidate("sig-001", "user-001"), readdCandidate("sig-002", "user-002")}, + {readdCandidate("sig-002", "user-002"), readdCandidate("sig-001", "user-001")}} { + repo := &readdFakeRepo{ccla: signedCCLA("alice@acme.test", "bob@acme.test"), candidates: order} + repo.later = func(int) (*ItemSignature, error) { return signedCCLA("alice@acme.test"), nil } + restored, err := run(t, repo, both, nil) + require.NoError(t, err) + require.Len(t, restored, 1) + assert.Equal(t, "user-001", restored[0].UserID) + assert.Equal(t, []string{"sig-001"}, repo.restored) + } + }) + t.Run("a list that keeps changing is reported instead of decided", func(t *testing.T) { + repo := &readdFakeRepo{ccla: signedCCLA("alice@acme.test", "bob@acme.test"), candidates: []*ItemSignature{readdCandidate("sig-001", "user-001")}} + repo.later = func(read int) (*ItemSignature, error) { + if read%2 == 0 { + return signedCCLA("alice@acme.test"), nil + } + return signedCCLA("alice@acme.test", "bob@acme.test"), nil + } + restored, err := run(t, repo, &models.ApprovalList{AddEmailApprovalList: []string{"alice@acme.test", "bob@acme.test"}}, nil) + assert.EqualError(t, err, "unable to evaluate user user-001 of signature sig-001: the approval list kept changing") + assert.Nil(t, restored) + assert.Empty(t, repo.restored) + assert.Equal(t, 1+restoreDecisionRounds, repo.cclaCalls) + }) + t.Run("an unanswered GitHub organization lookup is a failure, not a negative decision", func(t *testing.T) { + previous := listUserPublicOrgs + listUserPublicOrgs = func(context.Context, string) ([]string, error) { return nil, errors.New("github is down") } + t.Cleanup(func() { listUserPublicOrgs = previous }) + repo := &readdFakeRepo{ccla: &ItemSignature{SignatureID: "ccla-sig", SignatureSigned: true, SignatureApproved: true, GitHubOrgApprovalList: []string{"acme-org"}}, + candidates: []*ItemSignature{readdCandidate("sig-001", "user-001"), readdCandidate("sig-002", "user-002")}} + registry := newReaddRegistry() + alice, bob := readdUser("user-001", "alice@acme.test"), readdUser("user-002", "bob@acme.test") + alice.GithubUsername = readdAliceGitHub + registry.add(alice, bob) + svc := readdDirectService(t, repo, registry, nil) + restored, err := svc.restoreRemovalInvalidatedEmployeeSignatures(context.Background(), manager, fakeClaGroup(), fakeCompany(), "ccla-sig", + &models.ApprovalList{AddGithubOrgApprovalList: []string{"acme-org"}}) + assert.EqualError(t, err, "unable to evaluate user user-001 of signature sig-001: the GitHub organization membership lookup failed", + "bob has no GitHub username, so nothing was looked up for him") + assert.Nil(t, restored) + assert.Empty(t, repo.restored) + }) + t.Run("a direct match is decided without the failing GitHub organization lookup", func(t *testing.T) { + previous := listUserPublicOrgs + listUserPublicOrgs = func(context.Context, string) ([]string, error) { return nil, errors.New("github is down") } + t.Cleanup(func() { listUserPublicOrgs = previous }) + repo := &readdFakeRepo{ccla: &ItemSignature{SignatureID: "ccla-sig", SignatureSigned: true, SignatureApproved: true, + EmailApprovalList: []string{"alice@acme.test"}, GitHubOrgApprovalList: []string{"acme-org"}}, + candidates: []*ItemSignature{readdCandidate("sig-001", "user-001")}} + registry := newReaddRegistry() + alice := readdUser("user-001", "alice@acme.test") + alice.GithubUsername = readdAliceGitHub + registry.add(alice) + svc := readdDirectService(t, repo, registry, nil) + restored, err := svc.restoreRemovalInvalidatedEmployeeSignatures(context.Background(), manager, fakeClaGroup(), fakeCompany(), "ccla-sig", + &models.ApprovalList{AddEmailApprovalList: []string{"alice@acme.test"}, AddGithubOrgApprovalList: []string{"acme-org"}}) + require.NoError(t, err) + require.Len(t, restored, 1) + assert.Equal(t, []string{"sig-001"}, repo.restored) + }) +} + +// readdFailingCandidates wraps the real repository and breaks only the candidate listing +type readdFailingCandidates struct { + SignatureRepository + err error +} + +func (f *readdFailingCandidates) GetRemovalInvalidatedEmployeeSignatures(_ context.Context, _, _ string) ([]*ItemSignature, error) { + return nil, f.err +} + +// TestUpdateApprovalListReAddReportsRecoveryFailure: the edit itself is persisted and its side effects run, +// but an incomplete recovery is reported to the caller so the entries can be re-added to retry +func TestUpdateApprovalListReAddReportsRecoveryFailure(t *testing.T) { + h := newReaddHarness(t, []map[string]interface{}{readdCCLA{emails: []string{"keep@acme.test"}}.item(), readdRemovedRow(1, "alice@acme.test", utils.EmailCriteria)}) + h.registry.add(readdUser("user-001", "alice@acme.test"), readdUser("user-keep", "keep@acme.test")) + h.svc.repo = &readdFailingCandidates{SignatureRepository: h.repo, err: errors.New("index is down")} + before := h.rows() + logged := atomic.LoadInt64(h.events) + + updated, err := h.call(&models.ApprovalList{AddEmailApprovalList: []string{"alice@acme.test"}}) + require.Error(t, err) + assert.Contains(t, err.Error(), "re-add the entries to retry") + assert.Contains(t, err.Error(), "unable to load the removal-invalidated employee acknowledgments: index is down") + assert.Nil(t, updated) + h.assertUntouched(t, before["sig-001"]) + ccla := h.row(t, "ccla-sig") + assert.Equal(t, fakeStringList("keep@acme.test", "alice@acme.test"), ccla["email_whitelist"], "the list edit was persisted") + assert.Greater(t, atomic.LoadInt64(h.events), logged, "the edit was logged") + h.emails.mu.Lock() + assert.Contains(t, h.emails.recipients, "manager@example.com") + h.emails.mu.Unlock() + assert.Contains(t, h.storeReads(), "active_pr:e:alice@acme.test", "alice's pull request status was still refreshed") +} + +func TestAddedApprovalCriteria(t *testing.T) { + ccla := &ItemSignature{SignatureID: "ccla-sig", EmailApprovalList: []string{"a@x.test", "B@x.test"}, EmailDomainApprovalList: []string{"x.test"}, + GitHubUsernameApprovalList: []string{"gh-a"}, GitHubOrgApprovalList: []string{"org-a"}, GitlabUsernameApprovalList: []string{"gl-a"}, GitlabOrgApprovalList: []string{"group-a"}} + t.Run("entries in effect, trimmed, exact case, deduplicated", func(t *testing.T) { + added := addedApprovalCriteria(ccla, &models.ApprovalList{ + AddEmailApprovalList: []string{" a@x.test ", "a@x.test", "b@x.test", "missing@x.test", " "}, + AddDomainApprovalList: []string{"x.test", "y.test"}, + AddGithubUsernameApprovalList: []string{"GH-A", "gh-a"}, + AddGithubOrgApprovalList: []string{"org-a"}, + AddGitlabUsernameApprovalList: []string{"gl-a"}, + AddGitlabOrgApprovalList: []string{"group-a", "group-b"}, + }) + require.NotNil(t, added) + assert.Equal(t, "ccla-sig", added.SignatureID) + assert.Equal(t, []string{"a@x.test"}, added.EmailApprovalList) + assert.Equal(t, []string{"x.test"}, added.DomainApprovalList) + assert.Equal(t, []string{"gh-a"}, added.GithubUsernameApprovalList) + assert.Equal(t, []string{"org-a"}, added.GithubOrgApprovalList) + assert.Equal(t, []string{"gl-a"}, added.GitlabUsernameApprovalList) + assert.Equal(t, []string{"group-a"}, added.GitlabOrgApprovalList) + }) + t.Run("nothing in effect", func(t *testing.T) { + assert.Nil(t, addedApprovalCriteria(ccla, &models.ApprovalList{AddEmailApprovalList: []string{"b@x.test"}, AddDomainApprovalList: []string{"y.test"}})) + assert.Nil(t, addedApprovalCriteria(ccla, &models.ApprovalList{RemoveEmailApprovalList: []string{"a@x.test"}})) + assert.Nil(t, addedApprovalCriteria(ccla, &models.ApprovalList{})) + }) +} + +func TestAppendMissingUsers(t *testing.T) { + alice, bob := &models.User{UserID: "user-001"}, &models.User{UserID: "user-002"} + list := appendMissingUsers(nil, []*models.User{alice, nil, bob, alice}) + require.Len(t, list, 2) + assert.Same(t, alice, list[0]) + assert.Same(t, bob, list[1]) + list = appendMissingUsers(list, []*models.User{{UserID: "user-002"}, {UserID: "user-003"}}) + require.Len(t, list, 3) + assert.Equal(t, "user-003", list[2].UserID) +} + +func TestRestoreRemovalInvalidatedEmployeeSignatureWrite(t *testing.T) { + const note = "Re-enabled by the test." + pins := pinnedAs("#SG", ":csg") + pinnedAs("#RT", ":crt") + pinnedAs("#RID", ":crid") + pinnedAs("#PID", ":cpid") + pinnedAs("#CID", ":ccid") + cases := []struct { + name string + item map[string]interface{} + wantCondition string + }{ + {"attributed removal", readdRemovedRow(1, "alice@acme.test", utils.EmailCriteria), "attribute_exists(#ID)" + pinnedAs("#S", ":cs") + blankAs("#A", ":ca") + + pinnedAs("#DI", ":cdi") + pinnedAs("#IB", ":cib") + pinnedAs("#IR", ":cir") + blankAs("#IN", ":cin") + pins}, + {"legacy note-only removal", readdLegacyRemovedRow(1, "alice@acme.test", utils.GitHubUsernameCriteria), "attribute_exists(#ID)" + pinnedAs("#S", ":cs") + blankAs("#A", ":ca") + + blankAs("#DI", ":cdi") + blankAs("#IB", ":cib") + blankAs("#IR", ":cir") + blankAs("#IN", ":cin") + pins}, + } + for _, tc := range cases { + t.Run(tc.name, func(t *testing.T) { + h := newReaddHarness(t, []map[string]interface{}{tc.item}) + before, err := h.repo.GetItemSignature(context.Background(), "sig-001") + require.NoError(t, err) + require.True(t, before.InvalidatedByApprovalListRemoval()) + + done, err := h.repo.RestoreRemovalInvalidatedEmployeeSignature(context.Background(), before, note) + require.NoError(t, err) + assert.True(t, done) + assertReApproved(t, h.repo, "sig-001", before.Note+" "+note, before) + + h.table.mu.Lock() + defer h.table.mu.Unlock() + require.Len(t, h.table.updates, 1) + update := h.table.updates[0] + assert.Equal(t, "SET #A = :a, #S = :s, #M = :m REMOVE #DI, #IB, #IR, #IN", update.UpdateExpression) + assert.Equal(t, tc.wantCondition, update.ConditionExpression) + assert.Equal(t, map[string]string{"#ID": "signature_id", "#A": "signature_approved", "#S": "note", "#M": "date_modified", "#DI": "date_invalidated", + "#IB": "invalidated_by", "#IR": "invalidation_reason", "#IN": "invalidation_note", "#SG": "signature_signed", "#RT": "signature_reference_type", + "#RID": "signature_reference_id", "#PID": "signature_project_id", "#CID": "signature_user_ccla_company_id"}, update.ExpressionAttributeNames) + values := update.ExpressionAttributeValues + assert.True(t, *values[":a"].BOOL) + assert.Equal(t, before.Note+" "+note, *values[":s"].S) + assert.Equal(t, before.Note, *values[":cs"].S) + assert.False(t, *values[":ca"].BOOL) + assert.True(t, *values[":csg"].BOOL) + assert.Equal(t, before.DateInvalidated, *values[":cdi"].S) + assert.Equal(t, before.InvalidatedBy, *values[":cib"].S) + assert.Equal(t, before.InvalidationReason, *values[":cir"].S) + assert.Equal(t, "", *values[":cin"].S) + assert.Equal(t, "user", *values[":crt"].S) + assert.Equal(t, "user-001", *values[":crid"].S) + assert.Equal(t, "cla-group-1", *values[":cpid"].S) + assert.Equal(t, "company-1", *values[":ccid"].S) + assert.Empty(t, h.table.upserts) + assert.Zero(t, h.table.conditionFailures) + }) + } +} + +func TestRestoreRemovalInvalidatedEmployeeSignatureRefuses(t *testing.T) { + unsigned := readdRemovedRow(1, "alice@acme.test", utils.EmailCriteria) + unsigned["signature_signed"] = fakeFalse() + moved := readdCandidate("sig-001", "user-999") + manualNote := readdRemovedRow(1, "alice@acme.test", utils.EmailCriteria) + manualNote["note"] = fakeS("Signature invalidated (approved set to false) by pcc-admin for user-001") + withInvalidationNote := readdRemovedRow(1, "alice@acme.test", utils.EmailCriteria) + withInvalidationNote["invalidation_note"] = fakeS("manual") + companyReference := readdRemovedRow(1, "alice@acme.test", utils.EmailCriteria) + companyReference["signature_reference_type"] = fakeS("company") + cclaTyped := readdRemovedRow(1, "alice@acme.test", utils.EmailCriteria) + cclaTyped["signature_type"] = fakeS("ccla") + cases := []struct { + name string + item map[string]interface{} + snapshot *ItemSignature + }{ + {"already approved", fakeEclaItem(1, "alice@acme.test"), nil}, + {"deliberately invalidated", readdDeliberateRow(1, "alice@acme.test"), nil}, + {"unsigned", unsigned, nil}, + {"gone", readdRemovedRow(2, "bob@acme.test", utils.EmailCriteria), readdCandidate("sig-001", "user-001")}, + {"replaced since the decision", readdRemovedRow(1, "alice@acme.test", utils.EmailCriteria), moved}, + {"later manual note over the retained removal attribution", manualNote, nil}, + {"invalidation note present", withInvalidationNote, nil}, + {"not a user acknowledgment", companyReference, nil}, + {"not an individual or employee acknowledgment", cclaTyped, nil}, + } + for _, tc := range cases { + t.Run(tc.name, func(t *testing.T) { + h := newReaddHarness(t, []map[string]interface{}{tc.item}) + snapshot := tc.snapshot + if snapshot == nil { + var err error + snapshot, err = h.repo.GetItemSignature(context.Background(), "sig-001") + require.NoError(t, err) + } + before := h.rows() + done, err := h.repo.RestoreRemovalInvalidatedEmployeeSignature(context.Background(), snapshot, "note") + require.NoError(t, err) + assert.False(t, done) + for _, item := range before { + h.assertUntouched(t, item) + } + h.table.mu.Lock() + assert.Empty(t, h.table.updates, "nothing was written") + h.table.mu.Unlock() + reads := h.signatureReads() + require.Len(t, reads, 1, "one consistent re-read of the acknowledgment") + assert.True(t, reads[0].consistentRead) + }) + } +} + +func TestGetItemSignatureConsistent(t *testing.T) { + h := newReaddHarness(t, []map[string]interface{}{readdRemovedRow(1, "alice@acme.test", utils.EmailCriteria)}) + row, err := h.repo.GetItemSignatureConsistent(context.Background(), "sig-001") + require.NoError(t, err) + require.NotNil(t, row) + assert.Equal(t, "user-001", row.SignatureReferenceID) + assert.Equal(t, readdRemovalNote(utils.EmailCriteria), row.Note) + assert.True(t, row.InvalidatedByApprovalListRemoval()) + missing, err := h.repo.GetItemSignatureConsistent(context.Background(), "sig-404") + require.NoError(t, err) + assert.Nil(t, missing) + reads := h.signatureReads() + require.Len(t, reads, 2) + for _, read := range reads { + assert.True(t, read.consistentRead) + } + assert.Equal(t, []string{"sig-404"}, reads[1].signatureIDs) +} + +func TestGetRemovalInvalidatedEmployeeSignatures(t *testing.T) { + t.Run("classifies every row of the company under the CLA group, walking all index pages", func(t *testing.T) { + table := &fakeSignaturesTable{items: readdMatrixItems(), invalidated: map[string]int{}, maxRawPage: 5, unprocessedOnce: 2} + h := newReaddHarnessWithTable(t, table) + candidates, err := h.repo.GetRemovalInvalidatedEmployeeSignatures(context.Background(), "company-1", "cla-group-1") + require.NoError(t, err) + var ids []string + for _, candidate := range candidates { + ids = append(ids, candidate.SignatureID) + assert.True(t, candidate.SignatureSigned) + assert.False(t, candidate.SignatureApproved) + assert.NotEmpty(t, candidate.SignatureReferenceID, "the snapshot carries the full row, not the index projection") + } + assert.ElementsMatch(t, []string{"sig-001", "sig-002", "sig-009", "sig-010", "sig-012", "sig-013"}, ids) + + h.table.mu.Lock() + defer h.table.mu.Unlock() + require.Len(t, h.table.queries, 3, "11 index rows in pages of 5") + for i, query := range h.table.queries { + assert.Equal(t, fakeEmployeeIndex, query.indexName) + assert.Empty(t, query.filter) + assert.Zero(t, query.limit) + assert.ElementsMatch(t, []string{"signature_user_ccla_company_id", "signature_project_id", "signature_id"}, query.attributeNames) + assert.Equal(t, i > 0, query.startKey != "", "pages after the first continue from the last key") + } + require.Len(t, h.table.reads, 2, "one batch plus the retry of the unprocessed keys") + assert.Len(t, h.table.reads[0].signatureIDs, 11) + assert.Len(t, h.table.reads[1].signatureIDs, 2) + for _, read := range h.table.reads { + assert.Equal(t, readdSignatures, read.tableName) + assert.True(t, read.consistentRead) + } + }) + t.Run("nothing under the CLA group", func(t *testing.T) { + h := newReaddHarness(t, readdMatrixItems()) + candidates, err := h.repo.GetRemovalInvalidatedEmployeeSignatures(context.Background(), "company-1", "cla-group-9") + require.NoError(t, err) + assert.Empty(t, candidates) + assert.Empty(t, h.signatureReads(), "no keys, no batch read") + }) + t.Run("index query failure", func(t *testing.T) { + table := &fakeSignaturesTable{items: readdMatrixItems(), invalidated: map[string]int{}, failIndex: fakeEmployeeIndex} + h := newReaddHarnessWithTable(t, table) + _, err := h.repo.GetRemovalInvalidatedEmployeeSignatures(context.Background(), "company-1", "cla-group-1") + require.Error(t, err) + assert.Contains(t, err.Error(), "injected query failure") + }) + t.Run("keys still unprocessed after every attempt", func(t *testing.T) { + table := &fakeSignaturesTable{items: readdMatrixItems(), invalidated: map[string]int{}, unprocessedAlways: 1} + h := newReaddHarnessWithTable(t, table) + _, err := h.repo.GetRemovalInvalidatedEmployeeSignatures(context.Background(), "company-1", "cla-group-1") + assert.EqualError(t, err, "1 signatures still unprocessed after 5 batch read attempts") + assert.Len(t, h.signatureReads(), 5) + }) +} + +// TestGetRemovalInvalidatedEmployeeSignaturesEvidenceAndShape: the classification is taken on the consistent base +// row - a later invalidation over the retained removal attribution, an invalidation note or a row that is not a +// user's individual/employee acknowledgment is not a candidate +func TestGetRemovalInvalidatedEmployeeSignaturesEvidenceAndShape(t *testing.T) { + manualNote := readdRemovedRow(2, "bob@acme.test", utils.EmailCriteria) + manualNote["note"] = fakeS("Signature invalidated (approved set to false) by pcc-admin for user-002") + withInvalidationNote := readdRemovedRow(3, "carol@acme.test", utils.EmailCriteria) + withInvalidationNote["invalidation_note"] = fakeS("manual") + companyReference := readdRemovedRow(4, "dave@acme.test", utils.EmailCriteria) + companyReference["signature_reference_type"] = fakeS("company") + cclaTyped := readdRemovedRow(5, "erin@acme.test", utils.EmailCriteria) + cclaTyped["signature_type"] = fakeS("ccla") + eclaTyped := readdRemovedRow(6, "frank@acme.test", utils.EmailCriteria) + eclaTyped["signature_type"] = fakeS("ecla") + blankNote := readdRemovedRow(7, "grace@acme.test", utils.EmailCriteria) + blankNote["note"] = fakeS(" ") + noNote := readdRemovedRow(8, "heidi@acme.test", utils.EmailCriteria) + delete(noNote, "note") + h := newReaddHarness(t, []map[string]interface{}{readdCCLA{}.item(), readdRemovedRow(1, "alice@acme.test", utils.EmailCriteria), + manualNote, withInvalidationNote, companyReference, cclaTyped, eclaTyped, blankNote, noNote}) + candidates, err := h.repo.GetRemovalInvalidatedEmployeeSignatures(context.Background(), "company-1", "cla-group-1") + require.NoError(t, err) + var ids []string + for _, candidate := range candidates { + ids = append(ids, candidate.SignatureID) + } + assert.ElementsMatch(t, []string{"sig-001", "sig-006", "sig-007", "sig-008"}, ids) +} + +func TestRestorableEmployeeAcknowledgment(t *testing.T) { + removalOnly := func() *ItemSignature { + return &ItemSignature{SignatureID: "sig-001", SignatureReferenceID: "user-001", SignatureReferenceType: utils.SignatureReferenceTypeUser, + SignatureType: utils.SignatureTypeCLA, SignatureUserCompanyID: "company-1", SignatureProjectID: "cla-group-1", SignatureSigned: true, + InvalidationReason: ApprovalListRemovalReasonPrefix + utils.EmailCriteria + ")", Note: readdRemovalNote(utils.EmailCriteria)} + } + cases := []struct { + name string + change func(*ItemSignature) + want bool + }{ + {"removal-only employee acknowledgment", func(*ItemSignature) {}, true}, + {"ecla typed", func(s *ItemSignature) { s.SignatureType = utils.ClaTypeECLA }, true}, + {"legacy note-only attribution", func(s *ItemSignature) { s.InvalidationReason = "" }, true}, + {"attribution without a note", func(s *ItemSignature) { s.Note = "" }, true}, + {"other company", func(s *ItemSignature) { s.SignatureUserCompanyID = "company-2" }, false}, + {"other CLA group", func(s *ItemSignature) { s.SignatureProjectID = "cla-group-2" }, false}, + {"company reference", func(s *ItemSignature) { s.SignatureReferenceType = utils.SignatureReferenceTypeCompany }, false}, + {"ccla typed", func(s *ItemSignature) { s.SignatureType = utils.SignatureTypeCCLA }, false}, + {"unsigned", func(s *ItemSignature) { s.SignatureSigned = false }, false}, + {"not invalidated by a removal", func(s *ItemSignature) { s.InvalidationReason = "left the company" }, false}, + {"later manual note", func(s *ItemSignature) { + s.Note = "Signature invalidated (approved set to false) by pcc-admin for user-001" + }, false}, + {"invalidation note", func(s *ItemSignature) { s.InvalidationNote = "manual" }, false}, + } + for _, tc := range cases { + t.Run(tc.name, func(t *testing.T) { + row := removalOnly() + tc.change(row) + assert.Equal(t, tc.want, restorableEmployeeAcknowledgment(row, "company-1", "cla-group-1")) + }) + } + assert.False(t, restorableEmployeeAcknowledgment(nil, "company-1", "cla-group-1")) +} + +func TestInvalidatedOnlyByApprovalListRemoval(t *testing.T) { + cases := []struct { + name string + row ItemSignature + byRemoval bool + onlyByRemoval bool + }{ + {"attributed removal with its note", ItemSignature{InvalidationReason: ApprovalListRemovalReasonPrefix + utils.GitHubOrgCriteria + ")", + Note: readdRemovalNote(utils.GitHubOrgCriteria)}, true, true}, + {"attributed removal, note appended by a restore", ItemSignature{InvalidationReason: ApprovalListRemovalReasonPrefix + utils.EmailCriteria + ")", + Note: readdRemovalNote(utils.EmailCriteria) + " Re-enabled employee acknowledgment previously disabled by approval list removal."}, true, false}, + {"attributed removal without a note", ItemSignature{InvalidationReason: ApprovalListRemovalReasonPrefix + utils.EmailCriteria + ")"}, true, true}, + {"legacy note only", ItemSignature{Note: readdRemovalNote(utils.EmailDomainCriteria)}, true, true}, + {"legacy note with extra whitespace", ItemSignature{Note: " Signature invalidated (approved set to false) by manager-lf due to " + utils.GitlabUsernameCriteria + " removal "}, true, true}, + {"retained attribution, later manual note", ItemSignature{InvalidationReason: ApprovalListRemovalReasonPrefix + utils.EmailCriteria + ")", + Note: "Signature invalidated (approved set to false) by pcc-admin for user-001"}, true, false}, + {"retained attribution, CLA group deletion note", ItemSignature{InvalidationReason: ApprovalListRemovalReasonPrefix + utils.EmailCriteria + ")", + Note: "Signature invalidated (approved set to false) by pcc-admin due to CLA Group/Project: cla-group-1 deletion"}, true, false}, + {"retained attribution with an invalidation note", ItemSignature{InvalidationReason: ApprovalListRemovalReasonPrefix + utils.EmailCriteria + ")", + Note: readdRemovalNote(utils.EmailCriteria), InvalidationNote: "manual"}, true, false}, + {"deliberate", ItemSignature{InvalidationReason: "left the company", Note: readdDeliberateNote, InvalidationNote: "manual"}, false, false}, + {"legacy deliberate note", ItemSignature{Note: readdDeliberateNote}, false, false}, + {"nothing", ItemSignature{}, false, false}, + } + for _, tc := range cases { + t.Run(tc.name, func(t *testing.T) { + row := tc.row + assert.Equal(t, tc.byRemoval, row.InvalidatedByApprovalListRemoval(), "InvalidatedByApprovalListRemoval") + assert.Equal(t, tc.onlyByRemoval, row.InvalidatedOnlyByApprovalListRemoval(), "InvalidatedOnlyByApprovalListRemoval") + }) + } +} diff --git a/cla-backend-go/signatures/approval_list_removal_test.go b/cla-backend-go/signatures/approval_list_removal_test.go index 514a2ace6..6d87e0c8f 100644 --- a/cla-backend-go/signatures/approval_list_removal_test.go +++ b/cla-backend-go/signatures/approval_list_removal_test.go @@ -60,6 +60,12 @@ type fakeSignaturesTable struct { // failIndex makes every Query against that index fail (an outage isolated to one GSI) failIndex string beforeUpdate func(item map[string]interface{}) + // reads records every GetItem and BatchGetItem; unprocessedOnce makes the first BatchGetItem + // leave that many keys unprocessed, as DynamoDB may under throttling; unprocessedAlways does + // so on every BatchGetItem (a sustained throttle) + reads []fakeCapturedRead + unprocessedOnce int + unprocessedAlways int } // fakeIndexKeys lists the attributes DynamoDB puts into LastEvaluatedKey for each index @@ -79,6 +85,7 @@ type fakeCapturedQuery struct { indexName string limit int64 attributeNames []string + filter string // startKey is the signature_id of the ExclusiveStartKey, when the query continues a page startKey string } @@ -87,6 +94,31 @@ type fakeAttrValue struct { S *string N *string BOOL *bool + L []fakeAttrValue +} + +type fakeGetRequest struct { + TableName string + Key map[string]fakeAttrValue + ConsistentRead bool + ProjectionExpression string + ExpressionAttributeNames map[string]string +} + +type fakeKeysAndAttributes struct { + Keys []map[string]fakeAttrValue + ConsistentRead bool +} + +type fakeBatchGetRequest struct { + RequestItems map[string]fakeKeysAndAttributes +} + +// fakeCapturedRead records a GetItem (one key) or BatchGetItem (all its keys) and its consistency +type fakeCapturedRead struct { + tableName string + signatureIDs []string + consistentRead bool } type fakeQueryRequest struct { @@ -127,11 +159,105 @@ func (f *fakeSignaturesTable) ServeHTTP(w http.ResponseWriter, r *http.Request) f.handleUpdate(w, body) case "DynamoDB_20120810.PutItem": f.handlePut(w, body) + case "DynamoDB_20120810.GetItem": + f.handleGet(w, body) + case "DynamoDB_20120810.BatchGetItem": + f.handleBatchGet(w, body) + case "DynamoDB_20120810.DescribeTable": + f.mu.Lock() + count := len(f.items) + f.mu.Unlock() + fakeWriteJSON(w, map[string]interface{}{"Table": map[string]interface{}{"ItemCount": count, "TableStatus": "ACTIVE"}}) default: http.Error(w, "unsupported operation", http.StatusBadRequest) } } +// handleGet answers a GetItem by signature_id with a copy of the item (empty when missing, or +// when another table such as the store is asked - its key value is still recorded) +func (f *fakeSignaturesTable) handleGet(w http.ResponseWriter, body []byte) { + var req fakeGetRequest + if err := json.Unmarshal(body, &req); err != nil { + http.Error(w, err.Error(), http.StatusBadRequest) + return + } + signatureID, recorded := "", "" + if key, ok := req.Key["signature_id"]; ok && key.S != nil { + signatureID, recorded = *key.S, *key.S + } else if key, ok := req.Key["key"]; ok && key.S != nil { + recorded = *key.S + } + f.mu.Lock() + defer f.mu.Unlock() + f.reads = append(f.reads, fakeCapturedRead{tableName: req.TableName, signatureIDs: []string{recorded}, consistentRead: req.ConsistentRead}) + resp := map[string]interface{}{} + if item := f.find(signatureID); item != nil && signatureID != "" { + resp["Item"] = fakeProjectItem(fakeCopyItem(item), fakeProjectedNames(req.ProjectionExpression, req.ExpressionAttributeNames)) + } + fakeWriteJSON(w, resp) +} + +// handleBatchGet answers a BatchGetItem on the signatures table, honoring unprocessedOnce/unprocessedAlways +func (f *fakeSignaturesTable) handleBatchGet(w http.ResponseWriter, body []byte) { + var req fakeBatchGetRequest + if err := json.Unmarshal(body, &req); err != nil { + http.Error(w, err.Error(), http.StatusBadRequest) + return + } + f.mu.Lock() + defer f.mu.Unlock() + leaveUnprocessed := max(f.unprocessedOnce, f.unprocessedAlways) + responses := map[string]interface{}{} + unprocessed := map[string]interface{}{} + for tableName, request := range req.RequestItems { + captured := fakeCapturedRead{tableName: tableName, consistentRead: request.ConsistentRead} + var found []map[string]interface{} + var pending []map[string]fakeAttrValue + for i, key := range request.Keys { + signatureID := "" + if value, ok := key["signature_id"]; ok && value.S != nil { + signatureID = *value.S + } + captured.signatureIDs = append(captured.signatureIDs, signatureID) + if leaveUnprocessed > 0 && i >= len(request.Keys)-leaveUnprocessed { + pending = append(pending, key) + continue + } + if item := f.find(signatureID); item != nil { + found = append(found, fakeCopyItem(item)) + } + } + f.reads = append(f.reads, captured) + responses[tableName] = found + if len(pending) > 0 { + unprocessed[tableName] = map[string]interface{}{"Keys": fakeKeysToWire(pending), "ConsistentRead": request.ConsistentRead} + } + } + f.unprocessedOnce = 0 + fakeWriteJSON(w, map[string]interface{}{"Responses": responses, "UnprocessedKeys": unprocessed}) +} + +func fakeKeysToWire(keys []map[string]fakeAttrValue) []map[string]interface{} { + out := make([]map[string]interface{}, 0, len(keys)) + for _, key := range keys { + wire := map[string]interface{}{} + for name, value := range key { + wire[name] = fakeAttrToItem(value) + } + out = append(out, wire) + } + return out +} + +// fakeCopyItem returns a shallow copy so a response cannot alias the stored row +func fakeCopyItem(item map[string]interface{}) map[string]interface{} { + out := make(map[string]interface{}, len(item)) + for name, value := range item { + out[name] = value + } + return out +} + func (f *fakeSignaturesTable) handleQuery(w http.ResponseWriter, body []byte) { var req fakeQueryRequest if err := json.Unmarshal(body, &req); err != nil { @@ -139,7 +265,7 @@ func (f *fakeSignaturesTable) handleQuery(w http.ResponseWriter, body []byte) { return } - captured := fakeCapturedQuery{indexName: req.IndexName, limit: aws.Int64Value(req.Limit)} + captured := fakeCapturedQuery{indexName: req.IndexName, limit: aws.Int64Value(req.Limit), filter: req.FilterExpression} for _, name := range req.ExpressionAttributeNames { captured.attributeNames = append(captured.attributeNames, name) } @@ -344,6 +470,12 @@ func fakeAttrToItem(value fakeAttrValue) map[string]interface{} { return map[string]interface{}{"BOOL": *value.BOOL} case value.N != nil: return map[string]interface{}{"N": *value.N} + case value.L != nil: + list := make([]interface{}, 0, len(value.L)) + for _, element := range value.L { + list = append(list, fakeAttrToItem(element)) + } + return map[string]interface{}{"L": list} } return nil } diff --git a/cla-backend-go/signatures/dbmodels.go b/cla-backend-go/signatures/dbmodels.go index 16e7ab4a6..314e08330 100644 --- a/cla-backend-go/signatures/dbmodels.go +++ b/cla-backend-go/signatures/dbmodels.go @@ -94,7 +94,20 @@ func (s *ItemSignature) InvalidatedByApprovalListRemoval() bool { if s.InvalidationReason != "" { return strings.HasPrefix(s.InvalidationReason, ApprovalListRemovalReasonPrefix) } - note := strings.Join(strings.Fields(s.Note), " ") + return approvalListRemovalNote(s.Note) +} + +// InvalidatedOnlyByApprovalListRemoval reports whether an approval list removal is the only invalidation evidence on +// the record: a later invalidation recorded with first-write-wins attribution keeps the removal reason but replaces +// the note, and such a record must not be treated as removal-only +func (s *ItemSignature) InvalidatedOnlyByApprovalListRemoval() bool { + return s.InvalidatedByApprovalListRemoval() && s.InvalidationNote == "" && + (strings.TrimSpace(s.Note) == "" || approvalListRemovalNote(s.Note)) +} + +// approvalListRemovalNote reports whether the note is the one verifyUserApprovals writes +func approvalListRemovalNote(note string) bool { + note = strings.Join(strings.Fields(note), " ") if !strings.HasPrefix(note, legacyInvalidationNotePrefix) { return false } diff --git a/cla-backend-go/signatures/mocks/mock_repo.go b/cla-backend-go/signatures/mocks/mock_repo.go index 05b7c1cef..5bd02c54a 100644 --- a/cla-backend-go/signatures/mocks/mock_repo.go +++ b/cla-backend-go/signatures/mocks/mock_repo.go @@ -1,6 +1,7 @@ // Copyright The Linux Foundation and each contributor to CommunityBridge. // SPDX-License-Identifier: MIT // + // Code generated by MockGen. DO NOT EDIT. // Source: signatures/repository.go @@ -410,6 +411,21 @@ func (mr *MockSignatureRepositoryMockRecorder) GetItemSignature(ctx, signatureID return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "GetItemSignature", reflect.TypeOf((*MockSignatureRepository)(nil).GetItemSignature), ctx, signatureID) } +// GetItemSignatureConsistent mocks base method. +func (m *MockSignatureRepository) GetItemSignatureConsistent(ctx context.Context, signatureID string) (*signatures0.ItemSignature, error) { + m.ctrl.T.Helper() + ret := m.ctrl.Call(m, "GetItemSignatureConsistent", ctx, signatureID) + ret0, _ := ret[0].(*signatures0.ItemSignature) + ret1, _ := ret[1].(error) + return ret0, ret1 +} + +// GetItemSignatureConsistent indicates an expected call of GetItemSignatureConsistent. +func (mr *MockSignatureRepositoryMockRecorder) GetItemSignatureConsistent(ctx, signatureID interface{}) *gomock.Call { + mr.mock.ctrl.T.Helper() + return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "GetItemSignatureConsistent", reflect.TypeOf((*MockSignatureRepository)(nil).GetItemSignatureConsistent), ctx, signatureID) +} + // GetProjectCompanyEmployeeSignature mocks base method. func (m *MockSignatureRepository) GetProjectCompanyEmployeeSignature(ctx context.Context, companyModel *models.Company, claGroupModel *models.ClaGroup, employeeUserModel *models.User, wg *sync.WaitGroup, resultChannel chan<- *signatures0.EmployeeModel, errorChannel chan<- error) { m.ctrl.T.Helper() @@ -482,6 +498,21 @@ func (mr *MockSignatureRepositoryMockRecorder) GetProjectSignatures(ctx, params return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "GetProjectSignatures", reflect.TypeOf((*MockSignatureRepository)(nil).GetProjectSignatures), ctx, params) } +// GetRemovalInvalidatedEmployeeSignatures mocks base method. +func (m *MockSignatureRepository) GetRemovalInvalidatedEmployeeSignatures(ctx context.Context, companyID, claGroupID string) ([]*signatures0.ItemSignature, error) { + m.ctrl.T.Helper() + ret := m.ctrl.Call(m, "GetRemovalInvalidatedEmployeeSignatures", ctx, companyID, claGroupID) + ret0, _ := ret[0].([]*signatures0.ItemSignature) + ret1, _ := ret[1].(error) + return ret0, ret1 +} + +// GetRemovalInvalidatedEmployeeSignatures indicates an expected call of GetRemovalInvalidatedEmployeeSignatures. +func (mr *MockSignatureRepositoryMockRecorder) GetRemovalInvalidatedEmployeeSignatures(ctx, companyID, claGroupID interface{}) *gomock.Call { + mr.mock.ctrl.T.Helper() + return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "GetRemovalInvalidatedEmployeeSignatures", reflect.TypeOf((*MockSignatureRepository)(nil).GetRemovalInvalidatedEmployeeSignatures), ctx, companyID, claGroupID) +} + // GetSignature mocks base method. func (m *MockSignatureRepository) GetSignature(ctx context.Context, signatureID string) (*models.Signature, error) { m.ctrl.T.Helper() @@ -599,6 +630,21 @@ func (mr *MockSignatureRepositoryMockRecorder) RemoveCLAManager(ctx, signatureID return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "RemoveCLAManager", reflect.TypeOf((*MockSignatureRepository)(nil).RemoveCLAManager), ctx, signatureID, claManagerID) } +// RestoreRemovalInvalidatedEmployeeSignature mocks base method. +func (m *MockSignatureRepository) RestoreRemovalInvalidatedEmployeeSignature(ctx context.Context, snapshot *signatures0.ItemSignature, note string) (bool, error) { + m.ctrl.T.Helper() + ret := m.ctrl.Call(m, "RestoreRemovalInvalidatedEmployeeSignature", ctx, snapshot, note) + ret0, _ := ret[0].(bool) + ret1, _ := ret[1].(error) + return ret0, ret1 +} + +// RestoreRemovalInvalidatedEmployeeSignature indicates an expected call of RestoreRemovalInvalidatedEmployeeSignature. +func (mr *MockSignatureRepositoryMockRecorder) RestoreRemovalInvalidatedEmployeeSignature(ctx, snapshot, note interface{}) *gomock.Call { + mr.mock.ctrl.T.Helper() + return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "RestoreRemovalInvalidatedEmployeeSignature", reflect.TypeOf((*MockSignatureRepository)(nil).RestoreRemovalInvalidatedEmployeeSignature), ctx, snapshot, note) +} + // SaveOrUpdateSignature mocks base method. func (m *MockSignatureRepository) SaveOrUpdateSignature(ctx context.Context, signature *signatures0.ItemSignature) error { m.ctrl.T.Helper() diff --git a/cla-backend-go/signatures/repository.go b/cla-backend-go/signatures/repository.go index a8f63237f..71d5c37f4 100644 --- a/cla-backend-go/signatures/repository.go +++ b/cla-backend-go/signatures/repository.go @@ -74,6 +74,8 @@ type SignatureRepository interface { InvalidateProjectRecord(ctx context.Context, signatureID, note string) error InvalidateProjectRecordWithMetadata(ctx context.Context, signatureID, note string, metadata *InvalidationMetadata) error ReinvalidateProjectRecordWithMetadata(ctx context.Context, existing *ItemSignature, note string, metadata *InvalidationMetadata) error + GetRemovalInvalidatedEmployeeSignatures(ctx context.Context, companyID, claGroupID string) ([]*ItemSignature, error) + RestoreRemovalInvalidatedEmployeeSignature(ctx context.Context, snapshot *ItemSignature, note string) (bool, error) UpdateEnvelopeDetails(ctx context.Context, signatureID, envelopeID string, signURL *string) (*models.Signature, error) CreateSignature(ctx context.Context, signature *ItemSignature) error UpdateSignature(ctx context.Context, signatureID string, updates map[string]interface{}) error @@ -81,6 +83,7 @@ type SignatureRepository interface { GetSignature(ctx context.Context, signatureID string) (*models.Signature, error) GetItemSignature(ctx context.Context, signatureID string) (*ItemSignature, error) + GetItemSignatureConsistent(ctx context.Context, signatureID string) (*ItemSignature, error) GetActivePullRequestMetadata(ctx context.Context, gitHubAuthorUsername, gitHubAuthorEmail string) (*ActivePullRequest, error) GetIndividualSignature(ctx context.Context, claGroupID, userID string, approved, signed *bool) (*models.Signature, error) GetIndividualSignatures(ctx context.Context, claGroupID, userID string, approved, signed *bool) ([]*models.Signature, error) @@ -244,6 +247,36 @@ func (repo repository) GetItemSignature(ctx context.Context, signatureID string) return &signature, nil } +// GetItemSignatureConsistent returns the signature for the specified signature id using a strongly +// consistent base-table read, so a decision taken on it reflects every write that already completed +func (repo repository) GetItemSignatureConsistent(ctx context.Context, signatureID string) (*ItemSignature, error) { + f := logrus.Fields{ + "functionName": "v1.signatures.repository.GetItemSignatureConsistent", + utils.XREQUESTID: ctx.Value(utils.XREQUESTID), + "signatureID": signatureID, + } + + result, err := repo.dynamoDBClient.GetItemWithContext(ctx, &dynamodb.GetItemInput{ + TableName: aws.String(repo.signatureTableName), + Key: map[string]*dynamodb.AttributeValue{"signature_id": {S: aws.String(signatureID)}}, + ConsistentRead: aws.Bool(true), + }) + if err != nil { + log.WithFields(f).WithError(err).Warnf("error retrieving signature ID: %s", signatureID) + return nil, err + } + if result == nil || len(result.Item) == 0 { + return nil, nil + } + + var signature ItemSignature + if err := dynamodbattribute.UnmarshalMap(result.Item, &signature); err != nil { + log.WithFields(f).WithError(err).Warnf("error unmarshalling signature for ID: %s", signatureID) + return nil, err + } + return &signature, nil +} + // SaveOrUpdateSignature either creates or updates the signature record func (repo repository) SaveOrUpdateSignature(ctx context.Context, signature *ItemSignature) error { f := logrus.Fields{ @@ -2444,6 +2477,204 @@ func appendNote(existing, addition string) string { } } +// GetRemovalInvalidatedEmployeeSignatures returns the raw signed employee acknowledgments of the company +// under the CLA group - legacy cla and auto-created ecla rows alike - that are unapproved with approval +// list removal evidence only (#2980). The company index only supplies the signature ids: the rows are +// then read strongly consistently from the table and the classifier runs on each of them, so neither +// index lag nor per-user collapsing can hide an acknowledgment or a deliberate invalidation +func (repo repository) GetRemovalInvalidatedEmployeeSignatures(ctx context.Context, companyID, claGroupID string) ([]*ItemSignature, error) { + f := logrus.Fields{ + "functionName": "v1.signatures.repository.GetRemovalInvalidatedEmployeeSignatures", + utils.XREQUESTID: ctx.Value(utils.XREQUESTID), + "companyID": companyID, + "claGroupID": claGroupID, + } + + condition := expression.Key("signature_user_ccla_company_id").Equal(expression.Value(companyID)). + And(expression.Key("signature_project_id").Equal(expression.Value(claGroupID))) + expr, err := expression.NewBuilder().WithKeyCondition(condition).WithProjection(expression.NamesList(expression.Name("signature_id"))).Build() + if err != nil { + log.WithFields(f).WithError(err).Warn("error building the employee signature query") + return nil, err + } + + queryInput := &dynamodb.QueryInput{ + TableName: aws.String(repo.signatureTableName), + IndexName: aws.String("signature-user-ccla-company-index"), + KeyConditionExpression: expr.KeyCondition(), + ProjectionExpression: expr.Projection(), + ExpressionAttributeNames: expr.Names(), + ExpressionAttributeValues: expr.Values(), + } + + var signatureIDs []string + for { + results, queryErr := repo.dynamoDBClient.QueryWithContext(ctx, queryInput) + if queryErr != nil { + log.WithFields(f).WithError(queryErr).Warn("error retrieving the employee signatures") + return nil, queryErr + } + for _, item := range results.Items { + if id := item["signature_id"]; id != nil && aws.StringValue(id.S) != "" { + signatureIDs = append(signatureIDs, *id.S) + } + } + if len(results.LastEvaluatedKey) == 0 { + break + } + queryInput.ExclusiveStartKey = results.LastEvaluatedKey + } + + rows, err := repo.getItemSignaturesConsistent(ctx, signatureIDs) + if err != nil { + log.WithFields(f).WithError(err).Warn("error reading the employee signatures") + return nil, err + } + var candidates []*ItemSignature + for _, row := range rows { + if restorableEmployeeAcknowledgment(row, companyID, claGroupID) { + candidates = append(candidates, row) + } + } + log.WithFields(f).Debugf("found %d removal-invalidated employee signatures among %d", len(candidates), len(rows)) + return candidates, nil +} + +// restorableEmployeeAcknowledgment reports whether the current base-table row is a signed employee acknowledgment of +// the company under the CLA group whose only invalidation evidence is an approval list removal; the index the row was +// found through is eventually consistent, so its scope is decided on the row itself +func restorableEmployeeAcknowledgment(row *ItemSignature, companyID, claGroupID string) bool { + return row != nil && row.SignatureUserCompanyID == companyID && row.SignatureProjectID == claGroupID && + row.SignatureReferenceType == utils.SignatureReferenceTypeUser && + (row.SignatureType == utils.SignatureTypeCLA || row.SignatureType == utils.ClaTypeECLA) && + row.SignatureSigned && row.InvalidatedOnlyByApprovalListRemoval() +} + +// BatchGetItem accepts at most 100 keys per call; keys it leaves unprocessed are retried with a growing pause +const ( + batchGetItemSize = 100 + batchGetItemAttempts = 5 +) + +// getItemSignaturesConsistent reads the given signatures from the table with strongly consistent batch reads +func (repo repository) getItemSignaturesConsistent(ctx context.Context, signatureIDs []string) ([]*ItemSignature, error) { + var rows []*ItemSignature + for start := 0; start < len(signatureIDs); start += batchGetItemSize { + end := min(start+batchGetItemSize, len(signatureIDs)) + keys := make([]map[string]*dynamodb.AttributeValue, 0, end-start) + for _, signatureID := range signatureIDs[start:end] { + keys = append(keys, map[string]*dynamodb.AttributeValue{"signature_id": {S: aws.String(signatureID)}}) + } + requests := map[string]*dynamodb.KeysAndAttributes{ + repo.signatureTableName: {Keys: keys, ConsistentRead: aws.Bool(true)}, + } + for attempt := 0; ; attempt++ { + if attempt == batchGetItemAttempts { + return nil, fmt.Errorf("%d signatures still unprocessed after %d batch read attempts", len(requests[repo.signatureTableName].Keys), attempt) + } + if attempt > 0 { + time.Sleep(time.Duration(attempt) * 50 * time.Millisecond) + } + output, err := repo.dynamoDBClient.BatchGetItemWithContext(ctx, &dynamodb.BatchGetItemInput{RequestItems: requests}) + if err != nil { + return nil, err + } + var page []*ItemSignature + if err = dynamodbattribute.UnmarshalListOfMaps(output.Responses[repo.signatureTableName], &page); err != nil { + return nil, err + } + rows = append(rows, page...) + if pending := output.UnprocessedKeys[repo.signatureTableName]; pending == nil || len(pending.Keys) == 0 { + break + } + requests = output.UnprocessedKeys + } + } + return rows, nil +} + +// RestoreRemovalInvalidatedEmployeeSignature re-approves an employee acknowledgment whose user an approval +// list re-add covers again (#2980). The row is re-read consistently and must still be the signed, unapproved, +// removal-only acknowledgment the decision was taken on; the write clears the attribution and appends the +// note, pinned to that exact snapshot, so a concurrent deliberate invalidation, deletion or replacement wins +// and the acknowledgment is reported as not restored +func (repo repository) RestoreRemovalInvalidatedEmployeeSignature(ctx context.Context, snapshot *ItemSignature, note string) (bool, error) { + f := logrus.Fields{ + "functionName": "v1.signatures.repository.RestoreRemovalInvalidatedEmployeeSignature", + utils.XREQUESTID: ctx.Value(utils.XREQUESTID), + "signatureID": snapshot.SignatureID, + } + + existing, err := repo.GetItemSignatureConsistent(ctx, snapshot.SignatureID) + if err != nil { + log.WithFields(f).WithError(err).Warnf("error loading signature %s before restoring it", snapshot.SignatureID) + return false, err + } + switch { + case existing == nil: + log.WithFields(f).Warnf("signature %s no longer exists - not restoring", snapshot.SignatureID) + return false, nil + case existing.SignatureApproved: + log.WithFields(f).Debugf("signature %s is already approved - nothing to restore", snapshot.SignatureID) + return false, nil + case !restorableEmployeeAcknowledgment(existing, snapshot.SignatureUserCompanyID, snapshot.SignatureProjectID) || + existing.SignatureReferenceID != snapshot.SignatureReferenceID: + log.WithFields(f).Warnf("signature %s changed meanwhile - not restoring", snapshot.SignatureID) + return false, nil + } + + _, now := utils.CurrentTime() + expressionAttributeNames, expressionAttributeValues, updateExpression, conditionExpression := validationUpdateExpression(existing, appendNote(existing.Note, note), now, true) + conditionExpression += restorationPins(existing, expressionAttributeNames, expressionAttributeValues) + + input := &dynamodb.UpdateItemInput{ + Key: map[string]*dynamodb.AttributeValue{ + "signature_id": { + S: aws.String(existing.SignatureID), + }, + }, + ExpressionAttributeNames: expressionAttributeNames, + ExpressionAttributeValues: expressionAttributeValues, + UpdateExpression: &updateExpression, + ConditionExpression: &conditionExpression, + TableName: aws.String(repo.signatureTableName), + } + + if _, updateErr := repo.dynamoDBClient.UpdateItemWithContext(ctx, input); updateErr != nil { + if aerr, ok := updateErr.(awserr.Error); ok && aerr.Code() == dynamodb.ErrCodeConditionalCheckFailedException { + log.WithFields(f).Warnf("signature %s changed concurrently - not restoring", existing.SignatureID) + return false, nil + } + log.WithFields(f).WithError(updateErr).Warnf("error restoring signature_approved for signature_id: %s", existing.SignatureID) + return false, updateErr + } + log.WithFields(f).Infof("restored employee acknowledgment %s for user %s", existing.SignatureID, existing.SignatureReferenceID) + return true, nil +} + +// restorationPins extends the automatic validation condition with the signed state and identity of the +// acknowledgment, so the restore never lands on a replaced row +func restorationPins(existing *ItemSignature, names map[string]*string, values map[string]*dynamodb.AttributeValue) string { + names["#SG"] = aws.String("signature_signed") + values[":csg"] = &dynamodb.AttributeValue{BOOL: aws.Bool(true)} + condition := " AND #SG = :csg" + for _, pinned := range []struct{ name, placeholder, attribute, read string }{ + {"#RT", ":crt", "signature_reference_type", existing.SignatureReferenceType}, + {"#RID", ":crid", "signature_reference_id", existing.SignatureReferenceID}, + {"#PID", ":cpid", "signature_project_id", existing.SignatureProjectID}, + {"#CID", ":ccid", "signature_user_ccla_company_id", existing.SignatureUserCompanyID}, + } { + names[pinned.name] = aws.String(pinned.attribute) + values[pinned.placeholder] = &dynamodb.AttributeValue{S: aws.String(pinned.read)} + if pinned.read == "" { + condition += " AND (attribute_not_exists(" + pinned.name + ") OR " + pinned.name + " = " + pinned.placeholder + ")" + } else { + condition += " AND " + pinned.name + " = " + pinned.placeholder + } + } + return condition +} + // GetProjectCompanyEmployeeSignatures returns a list of employee signatures for the specified project and specified company func (repo repository) GetProjectCompanyEmployeeSignatures(ctx context.Context, params signatures.GetProjectCompanyEmployeeSignaturesParams, criteria *ApprovalCriteria) (*models.Signatures, error) { f := logrus.Fields{ diff --git a/cla-backend-go/signatures/service.go b/cla-backend-go/signatures/service.go index d4e3edccd..3b4519a60 100644 --- a/cla-backend-go/signatures/service.go +++ b/cla-backend-go/signatures/service.go @@ -8,6 +8,7 @@ import ( "errors" "fmt" "os" + "reflect" "regexp" "strconv" "strings" @@ -102,6 +103,8 @@ const ( errNilGitHubRepositoryOrOwner = "unable to get github repository - repository response is nil or owner is nil" githubStatusStateFailure = "failure" githubStatusMissingCLA = "Missing CLA Authorization." + // restoreDecisionRounds bounds the evaluate/re-read rounds of a restore decision while the approval list keeps changing + restoreDecisionRounds = 3 ) // listUserPublicOrgs is the indirection that lets unit tests for @@ -563,6 +566,14 @@ func (s service) UpdateApprovalList(ctx context.Context, authUser *auth.User, cl invalidated := github.ModelProjectUserCache.InvalidateByProject(claGroupModel.ProjectID) log.WithFields(f).Infof("invalidated %d ProjectUserCache entries for project %s after approval list update", invalidated, claGroupModel.ProjectID) + // Re-approve the signed employee acknowledgments that only an approval list removal had invalidated, when the + // entries just added cover their user again (#2980). The list edit already succeeded and the remaining side + // effects still run, but an incomplete recovery is reported to the caller - re-adding the entries retries it + restoredUsers, restoreErr := s.restoreRemovalInvalidatedEmployeeSignatures(ctx, userModel, claGroupModel, companyModel, corporateSigModel.SignatureID, params) + if restoreErr != nil { + log.WithFields(f).WithError(restoreErr).Warnf("problem restoring removal-invalidated employee acknowledgments for company ID: %s, project ID: %s, cla group ID: %s", companyModel.CompanyID, claGroupModel.ProjectID, claGroupID) + } + // If auto create ECLA is enabled for this Corporate Agreement, then create an ECLA for each employee that was added to the approval list // we get the complete user list as output from the processing of the approval list var userModelList []*models.User @@ -580,6 +591,7 @@ func (s service) UpdateApprovalList(ctx context.Context, authUser *auth.User, cl } userModelList = userList } + userModelList = appendMissingUsers(userModelList, restoredUsers) var wg sync.WaitGroup @@ -627,9 +639,203 @@ func (s service) UpdateApprovalList(ctx context.Context, authUser *auth.User, cl // Wait until all the go routines are done - if we don't wait, the behavior is undefined wg.Wait() + if restoreErr != nil { + return nil, fmt.Errorf("approval list updated, but re-enabling the employee acknowledgments disabled by an earlier approval list removal failed - re-add the entries to retry: %w", restoreErr) + } return updatedCorporateSignature, nil } +// restoreRemovalInvalidatedEmployeeSignatures re-approves the signed employee acknowledgments of the company under +// the CLA group that only an approval list removal had invalidated, when the entries just added cover their user +// again (#2980). Deliberate invalidations are never touched, a remove-only edit restores nothing, and only a +// positive match against the persisted post-edit entries counts; the corporate signature is read again before +// every guarded restore so a list change committed meanwhile is observed. Returns the users whose acknowledgment +// was restored plus every lookup/evaluation/write failure met on the way - an unanswered GitHub organization +// membership lookup is such a failure, not a negative decision +func (s service) restoreRemovalInvalidatedEmployeeSignatures(ctx context.Context, claManager *models.User, claGroupModel *models.ClaGroup, companyModel *models.Company, cclaSignatureID string, params *models.ApprovalList) ([]*models.User, error) { + f := logrus.Fields{ + "functionName": "v1.signatures.service.restoreRemovalInvalidatedEmployeeSignatures", + utils.XREQUESTID: ctx.Value(utils.XREQUESTID), + "claGroupID": claGroupModel.ProjectID, + "companyID": companyModel.CompanyID, + "signatureID": cclaSignatureID, + } + if params == nil || len(params.AddEmailApprovalList)+len(params.AddDomainApprovalList)+len(params.AddGithubUsernameApprovalList)+ + len(params.AddGithubOrgApprovalList)+len(params.AddGitlabUsernameApprovalList)+len(params.AddGitlabOrgApprovalList) == 0 { + return nil, nil + } + + // decide on the CCLA as actually written, not on the eventually consistent index read + cclaSignature, err := s.repo.GetItemSignatureConsistent(ctx, cclaSignatureID) + if err != nil { + return nil, fmt.Errorf("unable to load corporate signature %s: %w", cclaSignatureID, err) + } + if cclaSignature == nil || !cclaSignature.SignatureSigned || !cclaSignature.SignatureApproved { + log.WithFields(f).Warn("corporate signature is no longer signed and approved - not restoring employee acknowledgments") + return nil, nil + } + added := addedApprovalCriteria(cclaSignature, params) + if added == nil { + log.WithFields(f).Debug("none of the added approval list entries is in effect - nothing to restore") + return nil, nil + } + + candidates, err := s.repo.GetRemovalInvalidatedEmployeeSignatures(ctx, companyModel.CompanyID, claGroupModel.ProjectID) + if err != nil { + return nil, fmt.Errorf("unable to load the removal-invalidated employee acknowledgments: %w", err) + } + if len(candidates) == 0 { + return nil, nil + } + + note := fmt.Sprintf("Re-enabled employee acknowledgment previously disabled by approval list removal via CLA Manager %s approval list edit on %s.", + utils.GetBestUsername(claManager), utils.CurrentSimpleDateTimeString()) + decider := &restoreDecider{svc: s, ctx: ctx, f: f, cclaSignatureID: cclaSignatureID, params: params, added: added} + usersByID := map[string]*models.User{} + var restored []*models.User + var failures []error + for _, candidate := range candidates { + user, known := usersByID[candidate.SignatureReferenceID] + if !known { + user, err = s.usersService.GetUser(candidate.SignatureReferenceID) + if err != nil { + failures = append(failures, fmt.Errorf("unable to load user %s of signature %s: %w", candidate.SignatureReferenceID, candidate.SignatureID, err)) + continue + } + usersByID[candidate.SignatureReferenceID] = user + } + if user == nil { + log.WithFields(f).Warnf("user %s of signature %s not found - not restoring", candidate.SignatureReferenceID, candidate.SignatureID) + continue + } + restorable, stop, decideErr := decider.decide(user, candidate) + if decideErr != nil { + failures = append(failures, decideErr) + } + if stop { + break + } + if !restorable { + continue + } + done, restoreErr := s.repo.RestoreRemovalInvalidatedEmployeeSignature(ctx, candidate, note) + if restoreErr != nil { + failures = append(failures, fmt.Errorf("unable to restore signature %s: %w", candidate.SignatureID, restoreErr)) + continue + } + if done { + log.WithFields(f).Infof("restored employee acknowledgment %s of user %s", candidate.SignatureID, candidate.SignatureReferenceID) + restored = appendMissingUsers(restored, []*models.User{user}) + } + } + return restored, errors.Join(failures...) +} + +// restoreDecider decides, for one approval list edit, whether a candidate acknowledgment may be restored: its user +// must match the added entries in effect on the corporate signature as re-read after the evaluation (#2980) +type restoreDecider struct { + svc service + ctx context.Context + f logrus.Fields + cclaSignatureID string + params *models.ApprovalList + added *models.Signature +} + +// evaluate matches the user against the added entries - an unanswered GitHub organization membership lookup is a +// failure, not a negative decision +func (d *restoreDecider) evaluate(user *models.User, candidate *ItemSignature) (bool, error) { + approved, lookupFailed, err := d.svc.EvaluateUserApproval(d.ctx, user, d.added) + if err != nil { + return false, fmt.Errorf("unable to evaluate user %s of signature %s: %w", candidate.SignatureReferenceID, candidate.SignatureID, err) + } + if !approved && lookupFailed { + return false, fmt.Errorf("unable to evaluate user %s of signature %s: the GitHub organization membership lookup failed", candidate.SignatureReferenceID, candidate.SignatureID) + } + return approved, nil +} + +// decide reports (restorable, stop, failure): the corporate signature is re-read after every positive evaluation +// and the user evaluated again whenever the added entries in effect changed meanwhile, so the decision is made +// against the committed list; stop means the corporate signature or the added entries are gone +func (d *restoreDecider) decide(user *models.User, candidate *ItemSignature) (bool, bool, error) { + for round := 0; round < restoreDecisionRounds; round++ { + approved, err := d.evaluate(user, candidate) + if err != nil || !approved { + return false, false, err + } + current, reloadErr := d.svc.repo.GetItemSignatureConsistent(d.ctx, d.cclaSignatureID) + if reloadErr != nil { + return false, true, fmt.Errorf("unable to reload corporate signature %s: %w", d.cclaSignatureID, reloadErr) + } + if current == nil || !current.SignatureSigned || !current.SignatureApproved { + log.WithFields(d.f).Warn("corporate signature is no longer signed and approved - not restoring the remaining employee acknowledgments") + return false, true, nil + } + stillAdded := addedApprovalCriteria(current, d.params) + if stillAdded == nil { + log.WithFields(d.f).Warn("the added approval list entries are no longer in effect - not restoring the remaining employee acknowledgments") + return false, true, nil + } + if reflect.DeepEqual(stillAdded, d.added) { + return true, false, nil + } + d.added = stillAdded + } + return false, false, fmt.Errorf("unable to evaluate user %s of signature %s: the approval list kept changing", candidate.SignatureReferenceID, candidate.SignatureID) +} + +// addedApprovalCriteria returns, as an approval list to evaluate users against, the submitted additions that are +// in effect on the corporate signature after the edit (persisted entries are trimmed, exact case), or nil when +// none of them is +func addedApprovalCriteria(cclaSignature *ItemSignature, params *models.ApprovalList) *models.Signature { + inEffect := func(persisted, submitted []string) []string { + var out []string + for _, entry := range submitted { + entry = strings.TrimSpace(entry) + if entry != "" && utils.StringInSlice(entry, persisted) && !utils.StringInSlice(entry, out) { + out = append(out, entry) + } + } + return out + } + added := &models.Signature{ + SignatureID: cclaSignature.SignatureID, + EmailApprovalList: inEffect(cclaSignature.EmailApprovalList, params.AddEmailApprovalList), + DomainApprovalList: inEffect(cclaSignature.EmailDomainApprovalList, params.AddDomainApprovalList), + GithubUsernameApprovalList: inEffect(cclaSignature.GitHubUsernameApprovalList, params.AddGithubUsernameApprovalList), + GithubOrgApprovalList: inEffect(cclaSignature.GitHubOrgApprovalList, params.AddGithubOrgApprovalList), + GitlabUsernameApprovalList: inEffect(cclaSignature.GitlabUsernameApprovalList, params.AddGitlabUsernameApprovalList), + // GitLab group membership is not evaluated by EvaluateUserApproval - group-only re-adds restore nothing + GitlabOrgApprovalList: inEffect(cclaSignature.GitlabOrgApprovalList, params.AddGitlabOrgApprovalList), + } + if len(added.EmailApprovalList)+len(added.DomainApprovalList)+len(added.GithubUsernameApprovalList)+ + len(added.GithubOrgApprovalList)+len(added.GitlabUsernameApprovalList)+len(added.GitlabOrgApprovalList) == 0 { + return nil + } + return added +} + +// appendMissingUsers appends the users not yet on the list, by user ID +func appendMissingUsers(list []*models.User, users []*models.User) []*models.User { + for _, user := range users { + if user == nil { + continue + } + present := false + for _, existing := range list { + if existing != nil && existing.UserID == user.UserID { + present = true + break + } + } + if !present { + list = append(list, user) + } + } + return list +} + func (s service) createOrGetEmployeeModels(ctx context.Context, claGroupModel *models.ClaGroup, companyModel *models.Company, corporateSignatureModel *models.Signature) ([]*models.User, error) { // nolint gocyclomatic f := logrus.Fields{ "functionName": "v2.company.service.createOrGetEmployeeModels", diff --git a/cla-backend-go/v2/cla_manager/designee_test.go b/cla-backend-go/v2/cla_manager/designee_test.go new file mode 100644 index 000000000..a33899791 --- /dev/null +++ b/cla-backend-go/v2/cla_manager/designee_test.go @@ -0,0 +1,255 @@ +// Copyright The Linux Foundation and each contributor to CommunityBridge. +// SPDX-License-Identifier: MIT + +package cla_manager + +import ( + "context" + "errors" + "fmt" + "io" + "net/http" + "strings" + "sync" + "testing" + "time" + + "github.com/golang/mock/gomock" + "github.com/linuxfoundation/easycla/cla-backend-go/projects_cla_groups" + mock_projects_cla_groups "github.com/linuxfoundation/easycla/cla-backend-go/projects_cla_groups/mocks" + "github.com/linuxfoundation/easycla/cla-backend-go/token" + "github.com/linuxfoundation/easycla/cla-backend-go/utils" + organizationService "github.com/linuxfoundation/easycla/cla-backend-go/v2/organization-service" + v2UserService "github.com/linuxfoundation/easycla/cla-backend-go/v2/user-service" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +const ( + designeePlatformHost = "platform.designee.invalid" + designeeAuthHost = "auth.designee.invalid" + designeeCompanySFID = "0014100000DesignEE" + designeeUserSFID = "0034100000DesigneE" + designeeUserLFID = "designee-lf" + designeeClaGroupID = "cla-group-designee" +) + +// designeeHTTP answers the token endpoint, the user-service username lookup and the +// organization-service scope listing; anything else fails the test (no network) +type designeeHTTP struct { + t *testing.T + // users is the JSON returned for the user-service username lookup + users string + // scopesStatus overrides the status code of the scope listing when non-zero + scopesStatus int + // scopes holds the JSON scope listing per organization SFID + scopes string + + mu sync.Mutex + calls []string +} + +// token.Init spawns one asynchronous token request per call; it runs once per test binary and +// designeeTokenServed is closed once the fake transport has answered that request, so the +// goroutine no longer reads http.DefaultTransport when a subtest restores it +var ( + designeeTokenInit sync.Once + designeeTokenClose sync.Once + designeeTokenServed = make(chan struct{}) +) + +func (h *designeeHTTP) RoundTrip(r *http.Request) (*http.Response, error) { + h.mu.Lock() + h.calls = append(h.calls, r.Method+" "+r.URL.Path) + h.mu.Unlock() + response := &http.Response{StatusCode: http.StatusOK, Header: http.Header{"Content-Type": []string{"application/json"}}, Request: r} + switch { + case r.URL.Host == designeeAuthHost && r.URL.Path == "/oauth/token": + response.Body = io.NopCloser(strings.NewReader(`{"access_token":"unit-test-token","token_type":"Bearer","expires_in":3600}`)) + designeeTokenClose.Do(func() { close(designeeTokenServed) }) + case r.URL.Host == designeePlatformHost && r.URL.Path == "/user-service/v1/users": + response.Body = io.NopCloser(strings.NewReader(h.users)) + case r.URL.Host == designeePlatformHost && r.URL.Path == "/organization-service/v1/orgs/"+designeeCompanySFID+"/servicescopes": + if h.scopesStatus != 0 { + response.StatusCode = h.scopesStatus + } + response.Body = io.NopCloser(strings.NewReader(h.scopes)) + default: + h.t.Errorf("unexpected HTTP request (no network allowed): %s %s", r.Method, r.URL) + return nil, fmt.Errorf("unexpected HTTP request: %s %s", r.Method, r.URL) + } + return response, nil +} + +func (h *designeeHTTP) scopeCalls() int { + h.mu.Lock() + defer h.mu.Unlock() + n := 0 + for _, c := range h.calls { + if strings.HasSuffix(c, "/servicescopes") { + n++ + } + } + return n +} + +func setupDesigneeHTTP(t *testing.T, transport *designeeHTTP) { + t.Helper() + oldTransport, oldClient := http.DefaultTransport, http.DefaultClient + http.DefaultTransport = transport + http.DefaultClient = &http.Client{Transport: transport} + t.Cleanup(func() { + http.DefaultTransport, http.DefaultClient = oldTransport, oldClient + }) + designeeTokenInit.Do(func() { + token.Init("test-client", "test-secret", "https://"+designeeAuthHost+"/oauth/token", "test-audience") + }) + select { + case <-designeeTokenServed: + case <-time.After(5 * time.Second): + require.FailNow(t, "mock token initialization did not complete") + } + tok, err := token.GetToken() + require.NoError(t, err) + require.NotEmpty(t, tok) + organizationService.InitClient("https://"+designeePlatformHost, nil) + v2UserService.InitClient("https://"+designeePlatformHost, "api-key") +} + +func designeeUserJSON() string { + return `{"Data":[{"ID":"` + designeeUserSFID + `","Username":"` + designeeUserLFID + `"}],"Metadata":{"TotalSize":1}}` +} + +// designeeScopesJSON lists the user's cla-manager-designee scopes for the given project SFIDs +func designeeScopesJSON(projectSFIDs ...string) string { + scopes := make([]string, 0, len(projectSFIDs)) + for _, sfid := range projectSFIDs { + scopes = append(scopes, `{"ObjectTypeName":"project|organization","ObjectID":"`+sfid+`|`+designeeCompanySFID+`"}`) + } + return `{"userroles":[{"Contact":{"ID":"` + designeeUserSFID + `","Username":"` + designeeUserLFID + `"},` + + `"RoleScopes":[{"RoleName":"` + utils.CLADesigneeRole + `","Scopes":[` + strings.Join(scopes, ",") + `]}]}],` + + `"Metadata":{"Offset":0,"PageSize":1000,"TotalSize":1}}` +} + +func designeeMappings(projectSFIDs ...string) []*projects_cla_groups.ProjectClaGroup { + pcgs := make([]*projects_cla_groups.ProjectClaGroup, 0, len(projectSFIDs)) + for _, sfid := range projectSFIDs { + pcgs = append(pcgs, &projects_cla_groups.ProjectClaGroup{ProjectSFID: sfid, ClaGroupID: designeeClaGroupID}) + } + return pcgs +} + +func newDesigneeService(t *testing.T, mappings []*projects_cla_groups.ProjectClaGroup, mappingErr error) *service { + t.Helper() + ctrl := gomock.NewController(t) + t.Cleanup(ctrl.Finish) + pcgRepo := mock_projects_cla_groups.NewMockRepository(ctrl) + pcgRepo.EXPECT().GetProjectsIdsForClaGroup(gomock.Any(), designeeClaGroupID).Return(mappings, mappingErr).AnyTimes() + return &service{projectCGRepo: pcgRepo} +} + +func TestIsCLAManagerDesignee(t *testing.T) { + t.Run("no role for any project reports false", func(t *testing.T) { + // regression for lfx-self-serve#3008: every project check returning false used to fall through to true + transport := &designeeHTTP{t: t, users: designeeUserJSON(), scopes: designeeScopesJSON("project-other")} + setupDesigneeHTTP(t, transport) + svc := newDesigneeService(t, designeeMappings("project-a", "project-b", "project-c"), nil) + + status, err := svc.IsCLAManagerDesignee(context.Background(), designeeCompanySFID, designeeClaGroupID, designeeUserLFID) + + require.NoError(t, err) + require.NotNil(t, status) + require.NotNil(t, status.HasRole) + assert.False(t, *status.HasRole) + assert.Equal(t, designeeUserLFID, status.LfUsername) + assert.Equal(t, 3, transport.scopeCalls(), "every project mapping must be checked") + }) + + t.Run("no role scopes at all reports false", func(t *testing.T) { + transport := &designeeHTTP{t: t, users: designeeUserJSON(), scopes: `{"userroles":[],"Metadata":{"Offset":0,"PageSize":1000,"TotalSize":0}}`} + setupDesigneeHTTP(t, transport) + svc := newDesigneeService(t, designeeMappings("project-a", "project-b"), nil) + + status, err := svc.IsCLAManagerDesignee(context.Background(), designeeCompanySFID, designeeClaGroupID, designeeUserLFID) + + require.NoError(t, err) + require.NotNil(t, status.HasRole) + assert.False(t, *status.HasRole) + assert.Equal(t, 2, transport.scopeCalls()) + }) + + t.Run("role on the mapped project reports true", func(t *testing.T) { + transport := &designeeHTTP{t: t, users: designeeUserJSON(), scopes: designeeScopesJSON("project-a")} + setupDesigneeHTTP(t, transport) + svc := newDesigneeService(t, designeeMappings("project-a"), nil) + + status, err := svc.IsCLAManagerDesignee(context.Background(), designeeCompanySFID, designeeClaGroupID, designeeUserLFID) + + require.NoError(t, err) + require.NotNil(t, status.HasRole) + assert.True(t, *status.HasRole) + assert.Equal(t, designeeUserLFID, status.LfUsername) + assert.Equal(t, 1, transport.scopeCalls()) + }) + + t.Run("role on one of several projects reports true", func(t *testing.T) { + transport := &designeeHTTP{t: t, users: designeeUserJSON(), scopes: designeeScopesJSON("project-b")} + setupDesigneeHTTP(t, transport) + svc := newDesigneeService(t, designeeMappings("project-a", "project-b", "project-c"), nil) + + status, err := svc.IsCLAManagerDesignee(context.Background(), designeeCompanySFID, designeeClaGroupID, designeeUserLFID) + + require.NoError(t, err) + require.NotNil(t, status.HasRole) + assert.True(t, *status.HasRole) + }) + + t.Run("no project mappings reports false without role lookups", func(t *testing.T) { + transport := &designeeHTTP{t: t, users: designeeUserJSON(), scopes: designeeScopesJSON("project-a")} + setupDesigneeHTTP(t, transport) + svc := newDesigneeService(t, nil, nil) + + status, err := svc.IsCLAManagerDesignee(context.Background(), designeeCompanySFID, designeeClaGroupID, designeeUserLFID) + + require.NoError(t, err) + require.NotNil(t, status.HasRole) + assert.False(t, *status.HasRole) + assert.Equal(t, 0, transport.scopeCalls()) + }) + + t.Run("role lookup failure is returned", func(t *testing.T) { + transport := &designeeHTTP{t: t, users: designeeUserJSON(), scopes: `{"Message":"boom"}`, scopesStatus: http.StatusInternalServerError} + setupDesigneeHTTP(t, transport) + svc := newDesigneeService(t, designeeMappings("project-a"), nil) + + status, err := svc.IsCLAManagerDesignee(context.Background(), designeeCompanySFID, designeeClaGroupID, designeeUserLFID) + + require.Error(t, err) + assert.Nil(t, status) + }) + + t.Run("mapping lookup failure is returned", func(t *testing.T) { + transport := &designeeHTTP{t: t, users: designeeUserJSON(), scopes: designeeScopesJSON("project-a")} + setupDesigneeHTTP(t, transport) + mappingErr := errors.New("dynamodb unavailable") + svc := newDesigneeService(t, nil, mappingErr) + + status, err := svc.IsCLAManagerDesignee(context.Background(), designeeCompanySFID, designeeClaGroupID, designeeUserLFID) + + require.ErrorIs(t, err, mappingErr) + assert.Nil(t, status) + assert.Equal(t, 0, transport.scopeCalls()) + }) + + t.Run("unknown user is returned as an error", func(t *testing.T) { + transport := &designeeHTTP{t: t, users: `{"Data":[],"Metadata":{"TotalSize":0}}`, scopes: designeeScopesJSON("project-a")} + setupDesigneeHTTP(t, transport) + svc := newDesigneeService(t, designeeMappings("project-a"), nil) + + status, err := svc.IsCLAManagerDesignee(context.Background(), designeeCompanySFID, designeeClaGroupID, designeeUserLFID) + + require.ErrorIs(t, err, v2UserService.ErrUserNotFound) + assert.Nil(t, status) + assert.Equal(t, 0, transport.scopeCalls()) + }) +} diff --git a/cla-backend-go/v2/cla_manager/service.go b/cla-backend-go/v2/cla_manager/service.go index b8dedcb69..9097c4a11 100644 --- a/cla-backend-go/v2/cla_manager/service.go +++ b/cla-backend-go/v2/cla_manager/service.go @@ -572,8 +572,7 @@ func (s *service) IsCLAManagerDesignee(ctx context.Context, companySFID, claGrou }, nil } } - log.WithFields(f).Debugf("User %s has %s role at project level", userLFID, utils.CLADesigneeRole) - hasRole = true + log.WithFields(f).Debugf("User %s does not have %s role for any of the %d projects", userLFID, utils.CLADesigneeRole, len(pcgs)) } diff --git a/docs/M3_ORG_LENS_API.md b/docs/M3_ORG_LENS_API.md index 4cdfdc908..fcadbf002 100644 --- a/docs/M3_ORG_LENS_API.md +++ b/docs/M3_ORG_LENS_API.md @@ -255,7 +255,15 @@ instead of dropping the criterion while its members stay approved, while an add- invalidation evidence (any attribution attribute, or the legacy "Signature invalidated" note) is skipped, so re-adding someone to a list does not silently undo a CLA-manager or admin invalidation; the contributor re-acknowledges through the console instead, which -creates a fresh record. Probe: `utils/sanctioned_write_gate.sh` — +creates a fresh record. Re-adding an entry ([lfx-self-serve#2980](https://github.com/linuxfoundation/lfx-self-serve/issues/2980)) +re-approves, with or without `autoCreateECLA`, the signed acknowledgments that **only** an +approval list removal had invalidated (removal attribution or the legacy removal note, no other +note or `invalidation_note`) and whose user the re-added entries cover; deliberate invalidations +stay. The list edit succeeds even when that recovery is incomplete — the response is then 400 +`approval list updated, but re-enabling ... failed - re-add the entries to retry` (an unanswered +GitHub organization lookup counts as a failure). The CCLA is re-read before every restore, so an +entry removed meanwhile is observed; a GitLab-group-only re-add restores nothing (group +membership is not evaluated). Probe: `utils/sanctioned_write_gate.sh` — PASS = 403 `company_sanctioned` per op; payloads use bogus targets to limit the damage where the gate is broken, but a broken gate still runs the real write path: eclaAutoCreate has no bogus placeholder (skipped unless `ECLA_AUTO_CREATE_OK=1` — a broken gate persists