diff --git a/cla-backend-go/company/repository_external_id.go b/cla-backend-go/company/repository_external_id.go index 799734ed7..1b77e5c80 100644 --- a/cla-backend-go/company/repository_external_id.go +++ b/cla-backend-go/company/repository_external_id.go @@ -93,7 +93,7 @@ func (repo repository) EnsureCompanyForExternalID(ctx context.Context, externalI if _, notFound := err.(*utils.CompanyNotFound); !notFound { return nil, false, err } - } else if len(rows) > 0 { + } else if len(rows) > 0 && canonicalSigningEntity(rows[0].CompanyName, rows[0].SigningEntityName) == "" { existing = rows[0] } } else { diff --git a/cla-backend-go/company/repository_external_id_test.go b/cla-backend-go/company/repository_external_id_test.go index d5028aa0d..0dd86b0c1 100644 --- a/cla-backend-go/company/repository_external_id_test.go +++ b/cla-backend-go/company/repository_external_id_test.go @@ -330,6 +330,18 @@ func TestEnsureCompanyForExternalID(t *testing.T) { assert.Equal(t, 0, table.puts) }) + t.Run("a parent request with only signing-entity rows creates the parent", func(t *testing.T) { + repo, table := newCompanyRepo(t, &fakeCompaniesTable{items: map[string]map[string]interface{}{ + "child": fakeCompanyItem("child", "Acme Inc", "Acme GmbH", sfid), + }}) + comp, created, err := repo.EnsureCompanyForExternalID(context.Background(), sfid, "Acme Inc", "") + require.NoError(t, err) + assert.True(t, created) + assert.Equal(t, deterministicCompanyID(sfid, ""), comp.CompanyID) + assert.Equal(t, "Acme Inc", comp.SigningEntityName) + assert.Equal(t, 1, table.puts) + }) + t.Run("existing named signing entity row is reused", func(t *testing.T) { repo, table := newCompanyRepo(t, &fakeCompaniesTable{items: map[string]map[string]interface{}{ "parent": fakeCompanyItem("parent", "Acme Inc", "Acme Inc", sfid), diff --git a/cla-backend-go/signatures/approval_list_removal_test.go b/cla-backend-go/signatures/approval_list_removal_test.go index 6d87e0c8f..ba6b74a63 100644 --- a/cla-backend-go/signatures/approval_list_removal_test.go +++ b/cla-backend-go/signatures/approval_list_removal_test.go @@ -2069,3 +2069,80 @@ func TestUpdateApprovalListPendingOrganizationChangesDriveTheRemainingCoverage(t assert.Contains(t, emailSender.recipients, "hank@corp.example") assert.NotContains(t, emailSender.recipients, "gina@corp.example") } + +// a CLA group without enabled GitHub repositories has no organization members to re-check, so +// removing an organization criterion from its CCLA must still succeed and drop the criterion +func TestUpdateApprovalListGitHubOrgRemovalSucceedsWhenTheCLAGroupHasNoRepositories(t *testing.T) { + items := []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(), + "github_org_whitelist": fakeStringList("removed-org"), + "signature_acl": fakeStringList("manager-lf"), + "date_created": fakeS("2023-01-01T00:00:00Z"), + "date_modified": fakeS("2023-01-01T00:00:00Z"), + }, fakeEclaItem(1, "alice@corp.example")} + + table := &fakeSignaturesTable{items: items, invalidated: map[string]int{}} + awsSession, closeServer := newApprovalRemovalSession(t, table) + defer closeServer() + + ctrl := gomock.NewController(t) + defer ctrl.Finish() + mockUsers := mock_users.NewMockUserRepository(ctrl) + mockUsers.EXPECT().GetUser("user-001"). + Return(&models.User{UserID: "user-001", GithubUsername: "alice", LfEmail: "alice@corp.example"}, nil).AnyTimes() + mockUsers.EXPECT().GetUserByUserName("manager-lf", true). + Return(&models.User{LfUsername: "manager-lf", LfEmail: "manager@example.com"}, nil).AnyTimes() + mockCompanyRepo := mock_company.NewMockIRepository(ctrl) + mockCompanyRepo.EXPECT().GetCompany(gomock.Any(), "company-1"). + Return(&models.Company{CompanyID: "company-1", CompanyName: "Acme"}, nil).AnyTimes() + mockRepositories := mock.NewMockRepositoryInterface(ctrl) + mockRepositories.EXPECT().GitHubGetRepositoriesByCLAGroup(gomock.Any(), "cla-group-1", true). + Return(nil, &utils.GitHubRepositoryNotFound{Message: "no repositories found associated with CLA Group ID: cla-group-1 that is enabled"}) + mockGitHubOrgs := githubOrgMock.NewMockRepositoryInterface(ctrl) + originalMembers := getOrganizationMembers + getOrganizationMembers = func(context.Context, string, int64) ([]string, error) { + return nil, errors.New("must not be called") + } + defer func() { getOrganizationMembers = originalMembers }() + stubListUserPublicOrgs(t, nil, errors.New("must not be called")) + mockEvents := eventsMock.NewMockService(ctrl) + approvalRepo := &fakeApprovalRepo{} + + repo := repository{ + stage: "test", + dynamoDBClient: dynamodb.New(awsSession), + companyRepo: mockCompanyRepo, + usersRepo: mockUsers, + eventsService: mockEvents, + repositoriesRepo: mockRepositories, + ghOrgRepo: mockGitHubOrgs, + signatureTableName: "cla-test-signatures", + approvalRepo: approvalRepo, + } + + updated, err := repo.UpdateApprovalList(context.Background(), + &models.User{LfUsername: "manager-lf", LfEmail: "manager@example.com"}, + &models.ClaGroup{ProjectID: "cla-group-1", ProjectName: "My Project", Version: "v2"}, + "company-1", + &models.ApprovalList{RemoveGithubOrgApprovalList: []string{"removed-org"}}, + &events.LogEventArgs{EventType: events.InvalidatedSignature}) + require.NoError(t, err) + require.NotNil(t, updated) + assert.Empty(t, updated.GithubOrgApprovalList) + + table.mu.Lock() + defer table.mu.Unlock() + assert.Empty(t, table.invalidated, "nobody can be a member of an organization the CLA group does not use") + require.Len(t, table.ccla, 1) + assert.Contains(t, table.ccla[0], "REMOVE") + require.Len(t, approvalRepo.added, 1) + assert.Equal(t, "removed-org", approvalRepo.added[0].ApprovalName) + assert.False(t, approvalRepo.added[0].Active) +} diff --git a/cla-backend-go/signatures/repository.go b/cla-backend-go/signatures/repository.go index 71d5c37f4..6ab6eb15d 100644 --- a/cla-backend-go/signatures/repository.go +++ b/cla-backend-go/signatures/repository.go @@ -4356,7 +4356,10 @@ func (repo repository) UpdateApprovalList(ctx context.Context, claManager *model approvalList.Version = claGroupModel.Version // Get repositories by CLAGroup repositories, getRepoByCLAGroupErr := repo.repositoriesRepo.GitHubGetRepositoriesByCLAGroup(ctx, projectID, true) - if getRepoByCLAGroupErr != nil { + var noRepositories *utils.GitHubRepositoryNotFound + if errors.As(getRepoByCLAGroupErr, &noRepositories) { + repositories = nil + } else if getRepoByCLAGroupErr != nil { msg := fmt.Sprintf("unable to fetch repositories for cla group ID: %s ", projectID) log.WithFields(f).WithError(getRepoByCLAGroupErr).Warn(msg) return nil, errors.New(msg) @@ -5062,7 +5065,10 @@ func (repo repository) gitHubOrgRemovalTargets(ctx context.Context, projectID, c // Get repositories by CLAGroup repositories, getRepoByCLAGroupErr := repo.repositoriesRepo.GitHubGetRepositoriesByCLAGroup(ctx, projectID, true) - if getRepoByCLAGroupErr != nil { + var noRepositories *utils.GitHubRepositoryNotFound + if errors.As(getRepoByCLAGroupErr, &noRepositories) { + repositories = nil + } else if getRepoByCLAGroupErr != nil { msg := fmt.Sprintf("unable to fetch repositories for cla group ID: %s ", projectID) log.WithFields(f).WithError(getRepoByCLAGroupErr).Warn(msg) return nil, nil, errors.New(msg) diff --git a/cla-backend-go/swagger/cla.v2.yaml b/cla-backend-go/swagger/cla.v2.yaml index 2a0c07700..31737a8cb 100644 --- a/cla-backend-go/swagger/cla.v2.yaml +++ b/cla-backend-go/swagger/cla.v2.yaml @@ -3309,6 +3309,8 @@ paths: $ref: '#/responses/forbidden-or-company-sanctioned' '404': $ref: '#/responses/not-found' + '409': + $ref: '#/responses/conflict' '500': $ref: '#/responses/internal-server-error' tags: @@ -3346,6 +3348,8 @@ paths: $ref: '#/responses/forbidden-or-company-sanctioned' '404': $ref: '#/responses/not-found' + '409': + $ref: '#/responses/conflict' '500': $ref: '#/responses/internal-server-error' tags: diff --git a/cla-backend-go/v2/cla_manager/handlers.go b/cla-backend-go/v2/cla_manager/handlers.go index feabb8fe7..0a37545b0 100644 --- a/cla-backend-go/v2/cla_manager/handlers.go +++ b/cla-backend-go/v2/cla_manager/handlers.go @@ -584,6 +584,11 @@ func Configure(api *operations.EasyclaAPI, service Service, v1CompanyService v1C log.WithFields(f).Warn(msg) return cla_manager.NewApproveCLAManagerRequestNotFound().WithXRequestID(reqID).WithPayload(utils.ErrorResponseNotFound(reqID, msg)) } + if errors.Is(err, ErrCLAManagerRequestAlreadyDecided) { + msg := fmt.Sprintf("CLA Manager request already decided for Company ID: %s, Project ID: %s, Request ID: %s", params.CompanyID, params.ProjectSFID, params.RequestID) + log.WithFields(f).Warn(msg) + return cla_manager.NewApproveCLAManagerRequestConflict().WithXRequestID(reqID).WithPayload(utils.ErrorResponseConflict(reqID, msg)) + } msg := fmt.Sprintf("unable to approve CLA Manager request for Company ID: %s, Project ID: %s, Request ID: %s", params.CompanyID, params.ProjectSFID, params.RequestID) log.WithFields(f).WithError(err).Warn(msg) return cla_manager.NewApproveCLAManagerRequestInternalServerError().WithXRequestID(reqID).WithPayload(utils.ErrorResponseInternalServerErrorWithError(reqID, msg, err)) @@ -640,6 +645,11 @@ func Configure(api *operations.EasyclaAPI, service Service, v1CompanyService v1C log.WithFields(f).Warn(msg) return cla_manager.NewDenyCLAManagerRequestNotFound().WithXRequestID(reqID).WithPayload(utils.ErrorResponseNotFound(reqID, msg)) } + if errors.Is(err, ErrCLAManagerRequestAlreadyDecided) { + msg := fmt.Sprintf("CLA Manager request already decided for Company ID: %s, Project ID: %s, Request ID: %s", params.CompanyID, params.ProjectSFID, params.RequestID) + log.WithFields(f).Warn(msg) + return cla_manager.NewDenyCLAManagerRequestConflict().WithXRequestID(reqID).WithPayload(utils.ErrorResponseConflict(reqID, msg)) + } msg := fmt.Sprintf("unable to deny CLA Manager request for Company ID: %s, Project ID: %s, Request ID: %s", params.CompanyID, params.ProjectSFID, params.RequestID) log.WithFields(f).WithError(err).Warn(msg) return cla_manager.NewDenyCLAManagerRequestInternalServerError().WithXRequestID(reqID).WithPayload(utils.ErrorResponseInternalServerErrorWithError(reqID, msg, err)) diff --git a/cla-backend-go/v2/cla_manager/requests.go b/cla-backend-go/v2/cla_manager/requests.go index 1a9e77d0a..2de384840 100644 --- a/cla-backend-go/v2/cla_manager/requests.go +++ b/cla-backend-go/v2/cla_manager/requests.go @@ -79,6 +79,9 @@ func (s *service) ApproveCLAManagerRequest(ctx context.Context, authUser *auth.U if existingRequest == nil || existingRequest.CompanyID != companyModel.CompanyID || existingRequest.ProjectID != claGroupID { return nil, errRequestNotFound } + if existingRequest.Status != pendingRequestStatus { + return nil, ErrCLAManagerRequestAlreadyDecided + } claGroupModel, err := s.projectService.GetCLAGroupByID(ctx, claGroupID) if err != nil { @@ -108,12 +111,25 @@ func (s *service) ApproveCLAManagerRequest(ctx context.Context, authUser *auth.U request, err := s.managerService.ApproveRequest(companyModel.CompanyID, claGroupID, requestID) if err != nil { + if _, revertErr := s.managerService.PendingRequest(companyModel.CompanyID, claGroupID, requestID); revertErr != nil { + log.WithFields(f).WithError(revertErr).Warnf("unable to revert request %s to pending after the status update failed: %v", requestID, err) + } return nil, err } - _, aclErr := s.signatureService.AddCLAManager(ctx, sigModel.SignatureID, request.UserID) - if aclErr != nil { - return nil, aclErr + if !aclContainsUser(claManagers, request.UserID) { + _, aclErr := s.signatureService.AddCLAManager(ctx, sigModel.SignatureID, request.UserID) + if aclErr != nil { + stored, readErr := s.signatureService.GetSignature(ctx, sigModel.SignatureID) + if readErr == nil && stored != nil && aclContainsUser(stored.SignatureACL, request.UserID) { + log.WithFields(f).WithError(aclErr).Warn("ACL update reported an error but the requester is in the signature ACL - keeping the approval") + } else { + if _, revertErr := s.managerService.PendingRequest(companyModel.CompanyID, claGroupID, requestID); revertErr != nil { + log.WithFields(f).WithError(revertErr).Warnf("unable to revert request %s to pending after the ACL update failed: %v", requestID, aclErr) + } + return nil, aclErr + } + } } s.eventService.LogEventWithContext(ctx, &events.LogEventArgs{ @@ -175,6 +191,9 @@ func (s *service) DenyCLAManagerRequest(ctx context.Context, authUser *auth.User if existingRequest == nil || existingRequest.CompanyID != companyModel.CompanyID || existingRequest.ProjectID != claGroupID { return nil, errRequestNotFound } + if existingRequest.Status != pendingRequestStatus { + return nil, ErrCLAManagerRequestAlreadyDecided + } claGroupModel, err := s.projectService.GetCLAGroupByID(ctx, claGroupID) if err != nil { @@ -204,6 +223,9 @@ func (s *service) DenyCLAManagerRequest(ctx context.Context, authUser *auth.User request, err := s.managerService.DenyRequest(companyModel.CompanyID, claGroupID, requestID) if err != nil { + if _, revertErr := s.managerService.PendingRequest(companyModel.CompanyID, claGroupID, requestID); revertErr != nil { + log.WithFields(f).WithError(revertErr).Warnf("unable to revert request %s to pending after the status update failed: %v", requestID, err) + } return nil, err } @@ -355,3 +377,12 @@ func sendRequestDeniedEmailToRequester(emailSvc emails.EmailTemplateService, ema log.Debugf("sent email with subject: %s to recipients: %+v", subject, recipients) } } + +func aclContainsUser(acl []v1Models.User, userID string) bool { + for _, manager := range acl { + if manager.UserID == userID || manager.LfUsername == userID { + return true + } + } + return false +} diff --git a/cla-backend-go/v2/cla_manager/requests_test.go b/cla-backend-go/v2/cla_manager/requests_test.go index 014ef0863..0e9df26b2 100644 --- a/cla-backend-go/v2/cla_manager/requests_test.go +++ b/cla-backend-go/v2/cla_manager/requests_test.go @@ -38,6 +38,8 @@ type fakeManagerService struct { getCalls []string approveCalls [][]string denyCalls [][]string + pendingCalls [][]string + pendingErr error } func (f *fakeManagerService) GetRequests(companyID, claGroupID string) (*v1Models.ClaManagerRequestList, error) { @@ -60,6 +62,11 @@ func (f *fakeManagerService) DenyRequest(companyID, claGroupID, requestID string return f.denied, f.denyErr } +func (f *fakeManagerService) PendingRequest(companyID, claGroupID, requestID string) (*v1Models.ClaManagerRequest, error) { + f.pendingCalls = append(f.pendingCalls, []string{companyID, claGroupID, requestID}) + return f.request, f.pendingErr +} + type fakeProjectService struct { service2.Service claGroup *v1Models.ClaGroup @@ -76,6 +83,14 @@ type fakeSignatureService struct { err error addCalls [][]string addErr error + stored *v1Models.Signature + storedErr error + getCalls []string +} + +func (f *fakeSignatureService) GetSignature(ctx context.Context, signatureID string) (*v1Models.Signature, error) { + f.getCalls = append(f.getCalls, signatureID) + return f.stored, f.storedErr } func (f *fakeSignatureService) GetProjectCompanySignatures(ctx context.Context, params sigAPI.GetProjectCompanySignaturesParams) (*v1Models.Signatures, error) { @@ -303,13 +318,15 @@ func TestGetCLAManagerRequest(t *testing.T) { } } +const approvedStatus = "approved" + func TestApproveCLAManagerRequest(t *testing.T) { authUser := &auth.User{UserName: "manager-user", Email: "manager@example.com"} companyModel := acmeCompany() t.Run("approve flips status, updates the signature ACL and notifies managers and requester", func(t *testing.T) { approvedRequest := pendingRequest() - approvedRequest.Status = "approved" + approvedRequest.Status = approvedStatus mgr := &fakeManagerService{request: pendingRequest(), approved: approvedRequest} sigs := &fakeSignatureService{signatures: ccalSignatures()} ev := &fakeEventsService{} @@ -330,6 +347,8 @@ func TestApproveCLAManagerRequest(t *testing.T) { assert.Equal(t, "comp-sfid", result.CompanyExternalID, "enriched from the company model - the v1 read projection drops it") assert.Equal(t, [][]string{{"company-1", "cla-group-1", "req-1"}}, mgr.approveCalls) assert.Equal(t, [][]string{{"sig-1", "user-9"}}, sigs.addCalls, "the requester is added to the CCLA signature ACL") + assert.Empty(t, mgr.pendingCalls) + assert.Empty(t, sigs.getCalls) if assert.Len(t, ev.logged, 1) { logged := ev.logged[0] @@ -390,19 +409,148 @@ func TestApproveCLAManagerRequest(t *testing.T) { assert.Empty(t, mgr.approveCalls) }) - t.Run("approve error skips the ACL update", func(t *testing.T) { - mgr := &fakeManagerService{request: pendingRequest(), approveErr: errors.New("status update failed")} - sigs := &fakeSignatureService{signatures: ccalSignatures()} + t.Run("re-approving a requester already in the ACL skips the ACL add instead of failing after the status flip", func(t *testing.T) { + approvedRequest := pendingRequest() + approvedRequest.Status = approvedStatus + mgr := &fakeManagerService{request: pendingRequest(), approved: approvedRequest} + sigs := ccalSignatures() + sigs.Signatures[0].SignatureACL = append(sigs.Signatures[0].SignatureACL, v1Models.User{UserID: "uuid-9", LfUsername: "user-9", Username: "requester", LfEmail: strfmt.Email("requester@example.com")}) + sigSvc := &fakeSignatureService{signatures: sigs, addErr: errors.New("manager already in signature ACL")} s := &service{ - managerService: mgr, - projectService: &fakeProjectService{claGroup: &v1Models.ClaGroup{ProjectName: "My Project"}}, - signatureService: sigs, + managerService: mgr, + projectService: &fakeProjectService{claGroup: &v1Models.ClaGroup{ProjectName: "My Project"}}, + signatureService: sigSvc, + eventService: &fakeEventsService{}, + emailTemplateService: &fakeEmailTemplateService{}, } + installEmailSender(t) result, err := s.ApproveCLAManagerRequest(context.Background(), authUser, companyModel, "cla-group-1", "req-1") - assert.Nil(t, result) - assert.EqualError(t, err, "status update failed") - assert.Empty(t, sigs.addCalls) + assert.Nil(t, err) + if assert.NotNil(t, result) { + assert.Equal(t, "approved", result.Status) + } + assert.Equal(t, [][]string{{"company-1", "cla-group-1", "req-1"}}, mgr.approveCalls) + assert.Empty(t, sigSvc.addCalls) + }) + + t.Run("a failed ACL add reverts the request to pending so the approval can be retried", func(t *testing.T) { + for name, reconcile := range map[string]*fakeSignatureService{ + "requester absent from the stored ACL": {stored: ccalSignatures().Signatures[0]}, + "stored ACL cannot be read": {storedErr: errors.New("read failed")}, + } { + approvedRequest := pendingRequest() + approvedRequest.Status = approvedStatus + mgr := &fakeManagerService{request: pendingRequest(), approved: approvedRequest} + aclErr := errors.New("acl write failed") + sigSvc := &fakeSignatureService{signatures: ccalSignatures(), addErr: aclErr, stored: reconcile.stored, storedErr: reconcile.storedErr} + ev := &fakeEventsService{} + s := &service{ + managerService: mgr, + projectService: &fakeProjectService{claGroup: &v1Models.ClaGroup{ProjectName: "My Project"}}, + signatureService: sigSvc, + eventService: ev, + emailTemplateService: &fakeEmailTemplateService{}, + } + + sender := installEmailSender(t) + result, err := s.ApproveCLAManagerRequest(context.Background(), authUser, companyModel, "cla-group-1", "req-1") + assert.Nil(t, result, name) + assert.Equal(t, aclErr, err, name) + assert.Equal(t, [][]string{{"company-1", "cla-group-1", "req-1"}}, mgr.approveCalls, name) + assert.Equal(t, [][]string{{"sig-1", "user-9"}}, sigSvc.addCalls, name) + assert.Equal(t, []string{"sig-1"}, sigSvc.getCalls, name) + assert.Equal(t, [][]string{{"company-1", "cla-group-1", "req-1"}}, mgr.pendingCalls, name) + assert.Empty(t, ev.logged, name) + assert.Empty(t, sender.sent, name) + } + }) + + t.Run("an ACL write that committed but failed its read-back keeps the approval", func(t *testing.T) { + approvedRequest := pendingRequest() + approvedRequest.Status = approvedStatus + mgr := &fakeManagerService{request: pendingRequest(), approved: approvedRequest} + stored := ccalSignatures().Signatures[0] + stored.SignatureACL = append(stored.SignatureACL, v1Models.User{UserID: "uuid-9", LfUsername: "user-9", Username: "requester", LfEmail: strfmt.Email("requester@example.com")}) + sigSvc := &fakeSignatureService{signatures: ccalSignatures(), addErr: errors.New("read-back failed"), stored: stored} + ev := &fakeEventsService{} + s := &service{ + managerService: mgr, + projectService: &fakeProjectService{claGroup: &v1Models.ClaGroup{ProjectName: "My Project"}}, + signatureService: sigSvc, + eventService: ev, + emailTemplateService: &fakeEmailTemplateService{}, + } + + sender := installEmailSender(t) + result, err := s.ApproveCLAManagerRequest(context.Background(), authUser, companyModel, "cla-group-1", "req-1") + assert.Nil(t, err) + if assert.NotNil(t, result) { + assert.Equal(t, "approved", result.Status) + } + assert.Equal(t, [][]string{{"sig-1", "user-9"}}, sigSvc.addCalls) + assert.Equal(t, []string{"sig-1"}, sigSvc.getCalls) + assert.Empty(t, mgr.pendingCalls) + if assert.Len(t, ev.logged, 1) { + assert.Equal(t, events.ClaManagerAccessRequestApproved, ev.logged[0].EventType) + } + assert.Len(t, sender.sent, 3, "two managers and the requester are notified as on the happy path") + }) + + t.Run("aclContainsUser matches hydrated and raw ACL entries by LF username", func(t *testing.T) { + assert.False(t, aclContainsUser([]v1Models.User{{UserID: "uuid-9"}}, "user-9")) + assert.True(t, aclContainsUser([]v1Models.User{{UserID: "uuid-9", LfUsername: "user-9"}}, "user-9")) + assert.True(t, aclContainsUser([]v1Models.User{{LfUsername: "user-9"}}, "user-9")) + assert.False(t, aclContainsUser([]v1Models.User{{UserID: "uuid-1", LfUsername: "other"}}, "user-9")) + }) + + t.Run("approving an already decided request is a conflict and writes nothing", func(t *testing.T) { + for _, status := range []string{approvedStatus, "denied"} { + decided := pendingRequest() + decided.Status = status + mgr := &fakeManagerService{request: decided} + sigs := &fakeSignatureService{signatures: ccalSignatures()} + emailSvc := &fakeEmailTemplateService{} + s := &service{ + managerService: mgr, + projectService: &fakeProjectService{claGroup: &v1Models.ClaGroup{ProjectName: "My Project"}}, + signatureService: sigs, + eventService: &fakeEventsService{}, + emailTemplateService: emailSvc, + } + sender := installEmailSender(t) + result, err := s.ApproveCLAManagerRequest(context.Background(), authUser, companyModel, "cla-group-1", "req-1") + assert.Nil(t, result, status) + assert.ErrorIs(t, err, ErrCLAManagerRequestAlreadyDecided, status) + assert.Empty(t, mgr.approveCalls, status) + assert.Empty(t, sigs.addCalls, status) + assert.Empty(t, emailSvc.renderCalls, status) + assert.Empty(t, sender.sent, status) + } + }) + + t.Run("approve error skips the ACL update and reverts the request to pending", func(t *testing.T) { + for name, pendingErr := range map[string]error{"revert succeeds": nil, "revert fails": errors.New("revert failed")} { + mgr := &fakeManagerService{request: pendingRequest(), approveErr: errors.New("status update failed"), pendingErr: pendingErr} + sigs := &fakeSignatureService{signatures: ccalSignatures()} + ev := &fakeEventsService{} + emailSvc := &fakeEmailTemplateService{} + s := &service{ + managerService: mgr, + projectService: &fakeProjectService{claGroup: &v1Models.ClaGroup{ProjectName: "My Project"}}, + signatureService: sigs, + eventService: ev, + emailTemplateService: emailSvc, + } + + result, err := s.ApproveCLAManagerRequest(context.Background(), authUser, companyModel, "cla-group-1", "req-1") + assert.Nil(t, result, name) + assert.EqualError(t, err, "status update failed", name) + assert.Empty(t, sigs.addCalls, name) + assert.Equal(t, [][]string{{"company-1", "cla-group-1", "req-1"}}, mgr.pendingCalls, name) + assert.Empty(t, ev.logged, name) + assert.Empty(t, emailSvc.renderCalls, name) + } }) } @@ -453,6 +601,31 @@ func TestDenyCLAManagerRequest(t *testing.T) { } }) + t.Run("denying an already decided request is a conflict and writes nothing", func(t *testing.T) { + for _, status := range []string{approvedStatus, "denied"} { + decided := pendingRequest() + decided.Status = status + mgr := &fakeManagerService{request: decided} + sigs := &fakeSignatureService{signatures: ccalSignatures()} + emailSvc := &fakeEmailTemplateService{} + s := &service{ + managerService: mgr, + projectService: &fakeProjectService{claGroup: &v1Models.ClaGroup{ProjectName: "My Project"}}, + signatureService: sigs, + eventService: &fakeEventsService{}, + emailTemplateService: emailSvc, + } + sender := installEmailSender(t) + result, err := s.DenyCLAManagerRequest(context.Background(), authUser, companyModel, "cla-group-1", "req-1") + assert.Nil(t, result, status) + assert.ErrorIs(t, err, ErrCLAManagerRequestAlreadyDecided, status) + assert.Empty(t, mgr.denyCalls, status) + assert.Empty(t, sigs.addCalls, status) + assert.Empty(t, emailSvc.renderCalls, status) + assert.Empty(t, sender.sent, status) + } + }) + t.Run("missing request maps to not found", func(t *testing.T) { mgr := &fakeManagerService{} s := &service{managerService: mgr} @@ -462,6 +635,32 @@ func TestDenyCLAManagerRequest(t *testing.T) { assert.ErrorIs(t, err, errRequestNotFound) assert.Empty(t, mgr.denyCalls) }) + + t.Run("deny error reverts the request to pending so the denial can be retried", func(t *testing.T) { + for name, pendingErr := range map[string]error{"revert succeeds": nil, "revert fails": errors.New("revert failed")} { + mgr := &fakeManagerService{request: pendingRequest(), denyErr: errors.New("status update failed"), pendingErr: pendingErr} + sigs := &fakeSignatureService{signatures: ccalSignatures()} + ev := &fakeEventsService{} + emailSvc := &fakeEmailTemplateService{} + s := &service{ + managerService: mgr, + projectService: &fakeProjectService{claGroup: &v1Models.ClaGroup{ProjectName: "My Project"}}, + signatureService: sigs, + eventService: ev, + emailTemplateService: emailSvc, + } + sender := installEmailSender(t) + result, err := s.DenyCLAManagerRequest(context.Background(), authUser, companyModel, "cla-group-1", "req-1") + assert.Nil(t, result, name) + assert.EqualError(t, err, "status update failed", name) + assert.Equal(t, [][]string{{"company-1", "cla-group-1", "req-1"}}, mgr.denyCalls, name) + assert.Equal(t, [][]string{{"company-1", "cla-group-1", "req-1"}}, mgr.pendingCalls, name) + assert.Empty(t, sigs.addCalls, name) + assert.Empty(t, ev.logged, name) + assert.Empty(t, emailSvc.renderCalls, name) + assert.Empty(t, sender.sent, name) + } + }) } func TestClaManagerRequestJSONContract(t *testing.T) { diff --git a/cla-backend-go/v2/cla_manager/service.go b/cla-backend-go/v2/cla_manager/service.go index 9097c4a11..96fdcabb8 100644 --- a/cla-backend-go/v2/cla_manager/service.go +++ b/cla-backend-go/v2/cla_manager/service.go @@ -66,8 +66,12 @@ var ( ErrClaGroupBadRequest = errors.New("cla group bad request") errRequestNotFound = errors.New("cla manager request not found") + // ErrCLAManagerRequestAlreadyDecided when the request is no longer pending + ErrCLAManagerRequestAlreadyDecided = errors.New("cla manager request already decided") ) +const pendingRequestStatus = "pending" + const ( // used for filtering when fetching contributor email excludedNoReplyEmails = "noreply.github.com" diff --git a/cla-backend-legacy/internal/store/companies.go b/cla-backend-legacy/internal/store/companies.go index 741142184..ac2a19e0f 100644 --- a/cla-backend-legacy/internal/store/companies.go +++ b/cla-backend-legacy/internal/store/companies.go @@ -146,6 +146,14 @@ func PickParentCompany(items []map[string]types.AttributeValue) map[string]types func companyRowIsOlder(it, winner map[string]types.AttributeValue) bool { c, w := attrString(it, "date_created"), attrString(winner, "date_created") + if ct, okC := parsePynamoDateTimeString(c); okC { + if wt, okW := parsePynamoDateTimeString(w); okW { + if !ct.Equal(wt) { + return ct.Before(wt) + } + return attrString(it, "company_id") < attrString(winner, "company_id") + } + } if c != w { return c < w } diff --git a/cla-backend-legacy/internal/store/companies_test.go b/cla-backend-legacy/internal/store/companies_test.go index 702fa0492..57d98f31e 100644 --- a/cla-backend-legacy/internal/store/companies_test.go +++ b/cla-backend-legacy/internal/store/companies_test.go @@ -129,4 +129,12 @@ func TestPickParentCompany(t *testing.T) { if got := attrString(PickParentCompany([]map[string]types.AttributeValue{parent, tie}), "company_id"); got != "a" { t.Fatalf("expected the smallest company_id on a date tie, got %s", got) } + pynamo := companyRow("e", "Acme", "", "2021-01-01T00:00:00.100000+0000") + if got := attrString(PickParentCompany([]map[string]types.AttributeValue{pynamo, parent}), "company_id"); got != "b" { + t.Fatalf("expected the older RFC3339 row b over the later pynamo-formatted row, got %s", got) + } + sameInstant := companyRow("f", "Acme", "", "2021-01-01T00:00:00.000000+0000") + if got := attrString(PickParentCompany([]map[string]types.AttributeValue{sameInstant, parent}), "company_id"); got != "b" { + t.Fatalf("expected the smallest company_id when differently formatted dates are the same instant, got %s", got) + } } diff --git a/docs/M3_ORG_LENS_API.md b/docs/M3_ORG_LENS_API.md index bfa5a7370..ac7048ad6 100644 --- a/docs/M3_ORG_LENS_API.md +++ b/docs/M3_ORG_LENS_API.md @@ -206,7 +206,9 @@ the CCLA signature ACL and emails the CLA managers + requester; deny flips it to and emails without touching the ACL. Responses reuse the id-complete `cla-manager-request` shape (`requestID`, `companyID`/`companyExternalID`, `projectID`/`projectExternalID`, `userID`/`userExternalID`, names, emails, `status`, -`created`/`updated`). A request belonging to another company or CLA group returns 404. +`created`/`updated`). A request belonging to another company or CLA group returns 404. Only +`pending` requests can be decided: `approve`/`deny` on an already approved or denied request +returns 409 (`conflict`) and writes nothing. `PUT /v4/cla-group/{claGroupID}/ecla/{signatureID}/invalidate` invalidates one employee acknowledgment, mirroring the ICLA invalidate internals: sets