From fd771bcc8f697348e1011af3a0e446641aaee81b Mon Sep 17 00:00:00 2001 From: Harold Wanyama Date: Tue, 30 Jul 2024 17:29:11 +0300 Subject: [PATCH] [#4348] Bug/Ecla Check - Resolved bug for ecla checks against emails in the email approval list Signed-off-by: Harold Wanyama --- .../mocks}/mock_service.go | 28 ++---- cla-backend-go/signatures/repository.go | 10 ++- cla-backend-go/signatures/service.go | 34 ++++++-- cla-backend-go/signatures/service_test.go | 85 +++++++++++++++++++ cla-backend-go/v2/signatures/service.go | 1 + cla-backend-go/v2/signatures/service_test.go | 3 +- 6 files changed, 126 insertions(+), 35 deletions(-) rename cla-backend-go/{v2/signatures/mock_v1_signatures => signatures/mocks}/mock_service.go (94%) create mode 100644 cla-backend-go/signatures/service_test.go diff --git a/cla-backend-go/v2/signatures/mock_v1_signatures/mock_service.go b/cla-backend-go/signatures/mocks/mock_service.go similarity index 94% rename from cla-backend-go/v2/signatures/mock_v1_signatures/mock_service.go rename to cla-backend-go/signatures/mocks/mock_service.go index 714990cd3..1f70a6caf 100644 --- a/cla-backend-go/v2/signatures/mock_v1_signatures/mock_service.go +++ b/cla-backend-go/signatures/mocks/mock_service.go @@ -504,31 +504,17 @@ func (mr *MockSignatureServiceMockRecorder) UpdateSignature(ctx, signatureID, up return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "UpdateSignature", reflect.TypeOf((*MockSignatureService)(nil).UpdateSignature), ctx, signatureID, updates) } -// createOrGetEmployeeModels mocks base method. -func (m *MockSignatureService) createOrGetEmployeeModels(ctx context.Context, claGroupModel *models.ClaGroup, companyModel *models.Company, corporateSignatureModel *models.Signature) ([]*models.User, error) { +// UserIsApproved mocks base method. +func (m *MockSignatureService) UserIsApproved(ctx context.Context, user *models.User, cclaSignature *models.Signature) (bool, error) { m.ctrl.T.Helper() - ret := m.ctrl.Call(m, "createOrGetEmployeeModels", ctx, claGroupModel, companyModel, corporateSignatureModel) - ret0, _ := ret[0].([]*models.User) + ret := m.ctrl.Call(m, "UserIsApproved", ctx, user, cclaSignature) + ret0, _ := ret[0].(bool) ret1, _ := ret[1].(error) return ret0, ret1 } -// createOrGetEmployeeModels indicates an expected call of createOrGetEmployeeModels. -func (mr *MockSignatureServiceMockRecorder) createOrGetEmployeeModels(ctx, claGroupModel, companyModel, corporateSignatureModel interface{}) *gomock.Call { - mr.mock.ctrl.T.Helper() - return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "createOrGetEmployeeModels", reflect.TypeOf((*MockSignatureService)(nil).createOrGetEmployeeModels), ctx, claGroupModel, companyModel, corporateSignatureModel) -} - -// handleGitHubStatusUpdate mocks base method. -func (m *MockSignatureService) handleGitHubStatusUpdate(ctx context.Context, employeeUserModel *models.User) error { - m.ctrl.T.Helper() - ret := m.ctrl.Call(m, "handleGitHubStatusUpdate", ctx, employeeUserModel) - ret0, _ := ret[0].(error) - return ret0 -} - -// handleGitHubStatusUpdate indicates an expected call of handleGitHubStatusUpdate. -func (mr *MockSignatureServiceMockRecorder) handleGitHubStatusUpdate(ctx, employeeUserModel interface{}) *gomock.Call { +// UserIsApproved indicates an expected call of UserIsApproved. +func (mr *MockSignatureServiceMockRecorder) UserIsApproved(ctx, user, cclaSignature interface{}) *gomock.Call { mr.mock.ctrl.T.Helper() - return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "handleGitHubStatusUpdate", reflect.TypeOf((*MockSignatureService)(nil).handleGitHubStatusUpdate), ctx, employeeUserModel) + return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "UserIsApproved", reflect.TypeOf((*MockSignatureService)(nil).UserIsApproved), ctx, user, cclaSignature) } diff --git a/cla-backend-go/signatures/repository.go b/cla-backend-go/signatures/repository.go index cf103f4e7..fa52dde05 100644 --- a/cla-backend-go/signatures/repository.go +++ b/cla-backend-go/signatures/repository.go @@ -2368,14 +2368,14 @@ func (repo repository) GetProjectCompanyEmployeeSignature(ctx context.Context, c } // This is the keys we want to match - condition := expression.Key("signature_user_ccla_company_id").Equal(expression.Value(companyModel.CompanyID)).And( - expression.Key("signature_project_id").Equal(expression.Value(claGroupModel.ProjectID))) + condition := expression.Key("signature_reference_id").Equal(expression.Value(employeeUserModel.UserID)) var filterAdded bool var filter expression.ConditionBuilder // Check for approved signatures - filter = addAndCondition(filter, expression.Name("signature_reference_id").Equal(expression.Value(employeeUserModel.UserID)), &filterAdded) + filter = addAndCondition(filter, expression.Name("signature_user_ccla_company_id").Equal(expression.Value(companyModel.CompanyID)), &filterAdded) + filter = addAndCondition(filter, expression.Name("signature_project_id").Equal(expression.Value(claGroupModel.ProjectID)), &filterAdded) log.WithFields(f).Debugf("running employee signature query on table: %s", repo.signatureTableName) expr, err := expression.NewBuilder().WithKeyCondition(condition).WithFilter(filter).WithProjection(buildProjection()).Build() @@ -2394,7 +2394,7 @@ func (repo repository) GetProjectCompanyEmployeeSignature(ctx context.Context, c FilterExpression: expr.Filter(), ProjectionExpression: expr.Projection(), TableName: aws.String(repo.signatureTableName), - IndexName: aws.String("signature-user-ccla-company-index"), // Name of a secondary index to scan + IndexName: aws.String("reference-signature-index"), // Name of a secondary index to scan Limit: aws.Int64(10), } @@ -2406,7 +2406,9 @@ func (repo repository) GetProjectCompanyEmployeeSignature(ctx context.Context, c errorChannel <- errQuery return } + if results == nil || len(results.Items) == 0 { + log.WithFields(f).Debug("No ecla records found!") resultChannel <- &EmployeeModel{ Signature: nil, User: employeeUserModel, diff --git a/cla-backend-go/signatures/service.go b/cla-backend-go/signatures/service.go index 5feae4a02..f4295df4c 100644 --- a/cla-backend-go/signatures/service.go +++ b/cla-backend-go/signatures/service.go @@ -77,6 +77,7 @@ type SignatureService interface { UpdateEnvelopeDetails(ctx context.Context, signatureID, envelopeID string, signURL *string) (*models.Signature, error) // handleGitHubStatusUpdate(ctx context.Context, employeeUserModel *models.User) error ProcessEmployeeSignature(ctx context.Context, companyModel *models.Company, claGroupModel *models.ClaGroup, user *models.User) (*bool, error) + UserIsApproved(ctx context.Context, user *models.User, cclaSignature *models.Signature) (bool, error) } type service struct { @@ -1199,7 +1200,7 @@ func (s service) HasUserSigned(ctx context.Context, user *models.User, projectID func (s service) ProcessEmployeeSignature(ctx context.Context, companyModel *models.Company, claGroupModel *models.ClaGroup, user *models.User) (*bool, error) { f := logrus.Fields{ - "functionName": "v2.signatures.service.processEmployeeSignature", + "functionName": "v2.signatures.service.ProcessEmployeeSignature", utils.XREQUESTID: ctx.Value(utils.XREQUESTID), "companyID": companyModel.CompanyID, "projectID": claGroupModel.ProjectID, @@ -1227,7 +1228,8 @@ func (s service) ProcessEmployeeSignature(ctx context.Context, companyModel *mod if result != nil { employeeSignature := result.Signature if employeeSignature != nil { - log.WithFields(f).Debugf("ECLA Signature check - located employee acknowledgement - signature id: %s", employeeSignature.SignatureID) + // log.WithFields(f).Debugf("ECLA Signature check - located employee acknowledgement - signature id: %s", employeeSignature.SignatureID) + log.WithFields(f).Debugf("ecla signature check - :%+v", employeeSignature) // Get corporate ccla signature of company to access the approval list cclaSignature, cclaErr := s.GetCorporateSignature(ctx, projectID, companyID, &approved, &signed) @@ -1237,7 +1239,8 @@ func (s service) ProcessEmployeeSignature(ctx context.Context, companyModel *mod } if cclaSignature != nil { - userApproved, approvedErr := s.userIsApproved(ctx, user, cclaSignature) + log.WithFields(f).Debug("found ccla signature") + userApproved, approvedErr := s.UserIsApproved(ctx, user, cclaSignature) if approvedErr != nil { log.WithFields(f).WithError(approvedErr).Warnf("problem determining if user: %s is approved for project: %s", user.UserID, projectID) return &hasSigned, approvedErr @@ -1247,6 +1250,8 @@ func (s service) ProcessEmployeeSignature(ctx context.Context, companyModel *mod if userApproved { log.WithFields(f).Debugf("user: %s is in the approval list for signature: %s", user.UserID, employeeSignature.SignatureID) hasSigned = true + } else { + log.WithFields(f).Debugf("user: %s is not in the approval list for signature: %s", user.UserID, employeeSignature.SignatureID) } } } else { @@ -1264,14 +1269,24 @@ func (s service) ProcessEmployeeSignature(ctx context.Context, companyModel *mod } -func (s service) userIsApproved(ctx context.Context, user *models.User, cclaSignature *models.Signature) (bool, error) { - emails := append(user.Emails, string(user.LfEmail)) - +func (s service) UserIsApproved(ctx context.Context, user *models.User, cclaSignature *models.Signature) (bool, error) { + // add lf email to emails f := logrus.Fields{ - "functionName": "v1.signatures.service.userIsApproved", + "functionName": "v1.signatures.service.UserIsApproved", + } + + emails := user.Emails + + if user.LfEmail != "" { + log.WithFields(f).Debugf("adding lf email: %s to emails", user.LfEmail) + emails = append(emails, string(user.LfEmail)) + // remove duplicates + log.WithFields(f).Debug("removing duplicates") + emails = utils.RemoveDuplicates(emails) } // check GitHub username approval list + log.WithFields(f).Debug("checking if user is in the approval list") gitHubUsernameApprovalList := cclaSignature.GithubUsernameApprovalList if len(gitHubUsernameApprovalList) > 0 { for _, gitHubUsername := range gitHubUsernameApprovalList { @@ -1297,9 +1312,12 @@ func (s service) userIsApproved(ctx context.Context, user *models.User, cclaSign // check email email approval list emailApprovalList := cclaSignature.EmailApprovalList + log.WithFields(f).Debugf("checking if user is in the email approval list: %+v with emails :%v", emailApprovalList, emails) if len(emailApprovalList) > 0 { for _, email := range emails { - if strings.EqualFold(email, strings.TrimSpace(user.LfUsername)) { + log.WithFields(f).Debugf("checking email: %s", email) + if utils.StringInSlice(email, emailApprovalList) { + log.WithFields(f).Debugf("found matching email: %s in the email approval list", email) return true, nil } } diff --git a/cla-backend-go/signatures/service_test.go b/cla-backend-go/signatures/service_test.go new file mode 100644 index 000000000..9d35f9da2 --- /dev/null +++ b/cla-backend-go/signatures/service_test.go @@ -0,0 +1,85 @@ +// Copyright The Linux Foundation and each contributor to CommunityBridge. +// SPDX-License-Identifier: MIT + +package signatures + +import ( + "context" + "testing" + + v1Models "github.com/communitybridge/easycla/cla-backend-go/gen/v1/models" + "github.com/stretchr/testify/assert" +) + +func TestUserIsApproved(t *testing.T) { + ctx := context.Background() + + testCases := []struct { + name string + user *v1Models.User + cclaSignature *v1Models.Signature + expectedIsApproved bool + }{ + { + name: "User in GitHub username approval list", + user: &v1Models.User{ + GithubUsername: "approved-user", + }, + cclaSignature: &v1Models.Signature{ + GithubUsernameApprovalList: []string{"approved-user"}, + }, + expectedIsApproved: true, + }, + { + name: "User not in GitHub username approval list", + user: &v1Models.User{ + GithubUsername: "unapproved-user", + }, + cclaSignature: &v1Models.Signature{ + GithubUsernameApprovalList: []string{"approved-user"}, + }, + expectedIsApproved: false, + }, + { + name: "User in Email approval list", + user: &v1Models.User{ + Emails: []string{"foo@gmail.com"}, + }, + cclaSignature: &v1Models.Signature{ + EmailApprovalList: []string{"foo@gmail.com"}, + }, + expectedIsApproved: true, + }, + { + name: "User not in Email approval list", + user: &v1Models.User{ + Emails: []string{"unapproved@gmail.com"}, + }, + cclaSignature: &v1Models.Signature{ + EmailApprovalList: []string{"approved@gmail.com"}, + }, + expectedIsApproved: false, + }, + { + name: "User in Domain approval list", + user: &v1Models.User{ + Emails: []string{"approved@samsung.com"}, + }, + cclaSignature: &v1Models.Signature{ + DomainApprovalList: []string{"samsung.com"}, + }, + expectedIsApproved: true, + }, + } + + for _, tc := range testCases { + t.Run(tc.name, func(t *testing.T) { + service := NewService(nil, nil, nil, nil, false, nil, nil, nil, nil, "", "", "") + + isApproved, err := service.UserIsApproved(ctx, tc.user, tc.cclaSignature) + + assert.Nil(t, err) + assert.Equal(t, tc.expectedIsApproved, isApproved) + }) + } +} diff --git a/cla-backend-go/v2/signatures/service.go b/cla-backend-go/v2/signatures/service.go index 5d7b6a3e3..581bd6433 100644 --- a/cla-backend-go/v2/signatures/service.go +++ b/cla-backend-go/v2/signatures/service.go @@ -522,6 +522,7 @@ func (s *Service) IsUserAuthorized(ctx context.Context, lfid, claGroupId string) log.WithFields(f).WithError(err).Debug("unable to process ecla") return nil, err } + log.WithFields(f).Debugf("ecla value: %b", ecla) if ecla != nil && *ecla { log.WithFields(f).Debug("user has signed ECLA") hasSigned = true diff --git a/cla-backend-go/v2/signatures/service_test.go b/cla-backend-go/v2/signatures/service_test.go index c5d62a4af..b65885db2 100644 --- a/cla-backend-go/v2/signatures/service_test.go +++ b/cla-backend-go/v2/signatures/service_test.go @@ -16,8 +16,8 @@ import ( mock_company "github.com/communitybridge/easycla/cla-backend-go/company/mocks" ini "github.com/communitybridge/easycla/cla-backend-go/init" mock_project "github.com/communitybridge/easycla/cla-backend-go/project/mocks" + mock_v1_signatures "github.com/communitybridge/easycla/cla-backend-go/signatures/mocks" mock_users "github.com/communitybridge/easycla/cla-backend-go/v2/signatures/mock_users" - mock_v1_signatures "github.com/communitybridge/easycla/cla-backend-go/v2/signatures/mock_v1_signatures" "github.com/golang/mock/gomock" "github.com/stretchr/testify/assert" ) @@ -257,7 +257,6 @@ func TestService_IsUserAuthorized(t *testing.T) { mockUserService.EXPECT().GetUserByLFUserName(tc.lfid).Return(tc.getUserByLFUsernameResult, tc.getUserByLFUsernameError) if tc.getUserByLFUsernameResult != nil { - mockSignatureService := mock_v1_signatures.NewMockSignatureService(ctrl) approved := true