M3 followup after testing prod candidate on dev

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 {

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),

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)
+}

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)

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:

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))

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
+}