Skip to content

Commit

Permalink
[#4348] Bug Icla and Company check
Browse files Browse the repository at this point in the history 
- Resolved authorization cla endpoint with users on a non existant company

Signed-off-by: Harold Wanyama <[email protected]>
  • Loading branch information
@nickmango
nickmango committed Jul 11, 2024
1 parent 536b294 commit 898f1f4
Show file tree
Hide file tree
Showing 4 changed files with 89 additions and 33 deletions.
32 changes: 16 additions & 16 deletions cla-backend-go/users/models.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -5,22 +5,22 @@ package users

// DBUser data model
type DBUser struct {
UserID string `json:"user_id"`
UserExternalID string `json:"user_external_id"`
LFEmail string `json:"lf_email"`
Admin bool `json:"admin"`
LFUsername string `json:"lf_username"`
DateCreated string `json:"date_created"`
DateModified string `json:"date_modified"`
UserName string `json:"user_name"`
Version string `json:"version"`
UserEmails UserEmails `json:"user_emails"`
UserGithubID string `json:"user_github_id"`
UserGithubUsername string `json:"user_github_username"`
UserGitlabID string `json:"user_gitlab_id"`
UserGitlabUsername string `json:"user_gitlab_username"`
UserCompanyID string `json:"user_company_id"`
Note string `json:"note"`
UserID string `json:"user_id"`
UserExternalID string `json:"user_external_id"`
LFEmail string `json:"lf_email"`
Admin bool `json:"admin"`
LFUsername string `json:"lf_username"`
DateCreated string `json:"date_created"`
DateModified string `json:"date_modified"`
UserName string `json:"user_name"`
Version string `json:"version"`
UserEmails []string `json:"user_emails"`
UserGithubID string `json:"user_github_id"`
UserGithubUsername string `json:"user_github_username"`
UserGitlabID string `json:"user_gitlab_id"`
UserGitlabUsername string `json:"user_gitlab_username"`
UserCompanyID string `json:"user_company_id"`
Note string `json:"note"`
}

type UserEmails struct {
Expand Down
7 changes: 1 addition & 6 deletions cla-backend-go/users/repository.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -1254,11 +1254,6 @@ func (repo repository) UpdateUserCompanyID(userID, companyID, note string) error

// convertDBUserModel translates a dyanamoDB data model into a service response model
func convertDBUserModel(user DBUser) *models.User {
var emails []string
if user.UserEmails.SS != nil {
emails = user.UserEmails.SS
}

return &models.User{
UserID: user.UserID,
UserExternalID: user.UserExternalID,
Expand All @@ -1269,7 +1264,7 @@ func convertDBUserModel(user DBUser) *models.User {
DateModified: user.DateModified,
Username: user.UserName,
Version: user.Version,
Emails: emails,
Emails: user.UserEmails,
GithubID: user.UserGithubID,
GithubUsername: user.UserGithubUsername,
GitlabID: user.UserGitlabID,
Expand Down
11 changes: 6 additions & 5 deletions cla-backend-go/v2/signatures/service.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -496,6 +496,8 @@ func (s *Service) IsUserAuthorized(ctx context.Context, lfid, claGroupId string)
log.WithFields(f).Debug("user has signed ICLA")
response.ICLA = true
hasSigned = true
} else {
log.WithFields(f).Debug("user has not signed ICLA")
}

// fetch company
Expand All @@ -505,13 +507,12 @@ func (s *Service) IsUserAuthorized(ctx context.Context, lfid, claGroupId string)
} else {
log.WithFields(f).Debug("fetching company")
companyModel, err := s.v1CompanyService.GetCompany(ctx, user.CompanyID)
if err != nil {
if companyErr, ok := err.(*utils.CompanyNotFound); ok {
log.WithFields(f).WithError(companyErr).Debug("company not found")
response.CompanyAffiliation = false
} else if err != nil {
log.WithFields(f).WithError(err).Debug("unable to fetch company")
return nil, err
}
if companyModel == nil {
log.WithFields(f).Debug("company not found")
response.CompanyAffiliation = false
} else {
log.WithFields(f).Debug("company found")
response.CompanyAffiliation = true
Expand Down
72 changes: 66 additions & 6 deletions cla-backend-go/v2/signatures/service_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -6,11 +6,11 @@ package signatures
import (
"context"
"errors"
"fmt"
"testing"

v1Models "github.com/communitybridge/easycla/cla-backend-go/gen/v1/models"
"github.com/communitybridge/easycla/cla-backend-go/gen/v2/models"
"github.com/communitybridge/easycla/cla-backend-go/utils"

// mock_signatures "github.com/communitybridge/easycla/cla-backend-go/v2/signatures/mock_v1_signatures"
mock_company "github.com/communitybridge/easycla/cla-backend-go/company/mocks"
Expand All @@ -24,6 +24,7 @@ import (

func TestService_IsUserAuthorized(t *testing.T) {
type testCase struct {
name string
lfid string
projectID string
userID string
Expand All @@ -40,10 +41,13 @@ func TestService_IsUserAuthorized(t *testing.T) {
expectedICLA bool
expectedCCLA bool
expectedCompanyAffiliation bool
getCompanyResult *v1Models.Company
getCompanyError error
}

cases := []testCase{
{
name: "claGroupRequiresICLA",
lfid: "foobar_1",
projectID: "project-123",
userID: "user-123",
Expand All @@ -66,8 +70,13 @@ func TestService_IsUserAuthorized(t *testing.T) {
expectedICLA: true,
expectedCCLA: true,
expectedCompanyAffiliation: true,
getCompanyResult: &v1Models.Company{
CompanyID: "company-123",
},
getCompanyError: nil,
},
{
name: "claGroupDoesNotRequireICLA",
lfid: "foobar_2",
projectID: "project-123",
userID: "user-123",
Expand All @@ -90,8 +99,13 @@ func TestService_IsUserAuthorized(t *testing.T) {
expectedICLA: true,
expectedCCLA: true,
expectedCompanyAffiliation: true,
getCompanyResult: &v1Models.Company{
CompanyID: "company-123",
},
getCompanyError: nil,
},
{
name: "icla signature found",
lfid: "foobar_3",
projectID: "project-123",
userID: "user-123",
Expand All @@ -114,8 +128,13 @@ func TestService_IsUserAuthorized(t *testing.T) {
expectedICLA: true,
expectedCCLA: false,
expectedCompanyAffiliation: true,
getCompanyResult: &v1Models.Company{
CompanyID: "company-123",
},
getCompanyError: nil,
},
{
name: "icla signature not found",
lfid: "foobar_4",
projectID: "project-123",
userID: "user-123",
Expand All @@ -136,8 +155,13 @@ func TestService_IsUserAuthorized(t *testing.T) {
expectedICLA: false,
expectedCCLA: true,
expectedCompanyAffiliation: true,
getCompanyResult: &v1Models.Company{
CompanyID: "company-123",
},
getCompanyError: nil,
},
{
name: "individual signature error",
lfid: "foobar_5",
projectID: "project-123",
userID: "user-123",
Expand All @@ -157,8 +181,13 @@ func TestService_IsUserAuthorized(t *testing.T) {
expectedICLA: false,
expectedCCLA: false,
expectedCompanyAffiliation: true,
getCompanyResult: &v1Models.Company{
CompanyID: "company-123",
},
getCompanyError: nil,
},
{
name: "user has not signed ccla and icla",
lfid: "foobar_6",
projectID: "project-123",
userID: "user-123",
Expand All @@ -171,11 +200,42 @@ func TestService_IsUserAuthorized(t *testing.T) {
expectedICLA: false,
expectedCCLA: false,
expectedCompanyAffiliation: false,
getCompanyResult: &v1Models.Company{
CompanyID: "company-123",
},
getCompanyError: nil,
},
{
name: "user has icla and has company id that does not exist",
lfid: "foobar_7",
projectID: "project-123",
userID: "user-123",
companyID: "company-123",
getUserByLFUsernameResult: &v1Models.User{
UserID: "user-123",
CompanyID: "company-123",
},
getUserByLFUsernameError: nil,
claGroupRequiresICLA: false,
expectedAuthorized: true,
expectedCCLARequiresICLA: false,
expectedICLA: true,
expectedCCLA: false,
expectedCompanyAffiliation: false,
getCompanyResult: nil,
getCompanyError: &utils.CompanyNotFound{
Message: "no company matching company record",
CompanyID: "company-123",
},
getIndividualSignatureResult: &v1Models.Signature{
SignatureID: "signature-123",
},
getIndividualSignatureError: nil,
},
}

for _, tc := range cases {
t.Run(fmt.Sprintf("LFID=%s ProjectID=%s", tc.lfid, tc.projectID), func(t *testing.T) {
t.Run(tc.name, func(t *testing.T) {
ctrl := gomock.NewController(t)
defer ctrl.Finish()

Expand Down Expand Up @@ -204,12 +264,12 @@ func TestService_IsUserAuthorized(t *testing.T) {
signed := true
mockSignatureService.EXPECT().GetIndividualSignature(context.Background(), tc.projectID, tc.userID, &approved, &signed).Return(tc.getIndividualSignatureResult, tc.getIndividualSignatureError)

mockSignatureService.EXPECT().ProcessEmployeeSignature(context.Background(), gomock.Any(), gomock.Any(), gomock.Any()).Return(tc.processEmployeeSignatureResult, tc.processEmployeeSignatureError)
if tc.getCompanyError == nil {
mockSignatureService.EXPECT().ProcessEmployeeSignature(context.Background(), gomock.Any(), gomock.Any(), gomock.Any()).Return(tc.processEmployeeSignatureResult, tc.processEmployeeSignatureError)
}

mockCompanyService := mock_company.NewMockIService(ctrl)
mockCompanyService.EXPECT().GetCompany(context.Background(), tc.companyID).Return(&v1Models.Company{
CompanyID: tc.companyID,
}, nil)
mockCompanyService.EXPECT().GetCompany(context.Background(), tc.companyID).Return(tc.getCompanyResult, tc.getCompanyError)

service := NewService(awsSession, "", mockProjectService, mockCompanyService, mockSignatureService, nil, nil, mockUserService, nil)

Expand Down

0 comments on commit 898f1f4

Please sign in to comment.