diff --git a/cla-backend-go/cmd/refresh_stored_username_test.go b/cla-backend-go/cmd/refresh_stored_username_test.go new file mode 100644 index 000000000..a1ea20eca --- /dev/null +++ b/cla-backend-go/cmd/refresh_stored_username_test.go @@ -0,0 +1,213 @@ +// Copyright The Linux Foundation and each contributor to CommunityBridge. +// SPDX-License-Identifier: MIT + +package cmd + +import ( + "strings" + "testing" + + "github.com/go-openapi/strfmt" + "github.com/linuxfoundation/easycla/cla-backend-go/gen/v1/models" + "github.com/linuxfoundation/easycla/cla-backend-go/user" +) + +type stubUsersService struct { + called bool + calledWith string + updates map[string]interface{} +} + +func (s *stubUsersService) UpdateUser(userID string, updates map[string]interface{}) (*models.User, error) { + s.called = true + s.calledWith = userID + s.updates = updates + return &models.User{UserID: userID}, nil +} + +func (s *stubUsersService) CreateUser(*models.User, *user.CLAUser) (*models.User, error) { + return nil, nil +} + +func (s *stubUsersService) Save(*models.UserUpdate, *user.CLAUser) (*models.User, error) { + return nil, nil +} + +func (s *stubUsersService) Delete(string, *user.CLAUser) error { return nil } +func (s *stubUsersService) GetUser(string) (*models.User, error) { return nil, nil } +func (s *stubUsersService) GetUserByLFUserName(string) (*models.User, error) { return nil, nil } +func (s *stubUsersService) GetUserByUserName(string, bool) (*models.User, error) { return nil, nil } +func (s *stubUsersService) GetUserByEmail(string) (*models.User, error) { return nil, nil } +func (s *stubUsersService) GetUsersByEmail(string) ([]*models.User, error) { return nil, nil } +func (s *stubUsersService) GetUsersByLFEmail(string) ([]*models.User, error) { return nil, nil } +func (s *stubUsersService) GetUserByGitHubID(string) (*models.User, error) { return nil, nil } +func (s *stubUsersService) GetUserByGitHubUsername(string) (*models.User, error) { return nil, nil } +func (s *stubUsersService) GetUserByGitlabID(int) (*models.User, error) { return nil, nil } +func (s *stubUsersService) GetUserByGitLabUsername(string) (*models.User, error) { return nil, nil } +func (s *stubUsersService) SearchUsers(string, string, bool) (*models.Users, error) { return nil, nil } +func (s *stubUsersService) UpdateUserCompanyID(string, string, string) error { return nil } +func (s *stubUsersService) ConvertUserModelToUserCompatModel(*models.User) (*models.UserCompat, error) { + return nil, nil +} + +const testUserID = "user-id-123" + +func makeStoredUser(email string) *models.User { + return &models.User{UserID: testUserID, Username: "Alice", LfEmail: strfmt.Email(email)} +} + +func makeTestCLAUser(name, email string) *user.CLAUser { + return &user.CLAUser{Name: name, LFEmail: email} +} + +func TestRefreshStoredUserIdentity(t *testing.T) { + cases := []struct { + desc string + stored *models.User + claUser *user.CLAUser + wantCalled bool + wantNameKey bool + wantEmailKey bool + wantEmailVal string + }{ + { + desc: "nil userModel → no-op", + stored: nil, + claUser: makeTestCLAUser("Alice", "alice@example.com"), + wantCalled: false, + }, + { + desc: "nil claUser → no-op", + stored: makeStoredUser("alice@example.com"), + claUser: nil, + wantCalled: false, + }, + { + desc: "name unchanged, email unchanged → no-op", + stored: makeStoredUser("alice@example.com"), + claUser: makeTestCLAUser("Alice", "alice@example.com"), + wantCalled: false, + }, + { + desc: "name changed only → only user_name updated", + stored: makeStoredUser("alice@example.com"), + claUser: makeTestCLAUser("Alice Smith", "alice@example.com"), + wantCalled: true, + wantNameKey: true, + wantEmailKey: false, + }, + { + desc: "email changed only → only lf_email updated", + stored: makeStoredUser("alice-old@example.com"), + claUser: makeTestCLAUser("Alice", "alice-new@example.com"), + wantCalled: true, + wantNameKey: false, + wantEmailKey: true, + wantEmailVal: "alice-new@example.com", + }, + { + desc: "both changed → both updated", + stored: makeStoredUser("alice-old@example.com"), + claUser: makeTestCLAUser("Alice Smith", "alice-new@example.com"), + wantCalled: true, + wantNameKey: true, + wantEmailKey: true, + wantEmailVal: "alice-new@example.com", + }, + { + desc: "stored email differs only in case → normalize on write", + stored: makeStoredUser("Alice@Example.COM"), + claUser: makeTestCLAUser("Alice", "alice@example.com"), + wantCalled: true, + wantEmailKey: true, + wantEmailVal: "alice@example.com", + }, + { + desc: "incoming email differs only in case from already-normalized stored → no-op", + stored: makeStoredUser("alice@example.com"), + claUser: makeTestCLAUser("Alice", "ALICE@EXAMPLE.COM"), + wantCalled: false, + }, + { + desc: "email uppercased in JWT → lowercased on write", + stored: makeStoredUser("old@example.com"), + claUser: makeTestCLAUser("Alice", "NEW@Example.COM"), + wantCalled: true, + wantEmailKey: true, + wantEmailVal: "new@example.com", + }, + { + desc: "incoming email has whitespace → trimmed and lowercased", + stored: makeStoredUser("old@example.com"), + claUser: makeTestCLAUser("Alice", " NEW@Example.COM "), + wantCalled: true, + wantEmailKey: true, + wantEmailVal: "new@example.com", + }, + { + desc: "empty name in claUser → name not touched, email still synced", + stored: makeStoredUser("old@example.com"), + claUser: makeTestCLAUser("", "new@example.com"), + wantCalled: true, + wantNameKey: false, + wantEmailKey: true, + }, + { + desc: "empty email in claUser → email not touched, name still synced", + stored: makeStoredUser("old@example.com"), + claUser: makeTestCLAUser("Alice Smith", ""), + wantCalled: true, + wantNameKey: true, + wantEmailKey: false, + }, + { + desc: "both empty in claUser → no-op", + stored: makeStoredUser("old@example.com"), + claUser: makeTestCLAUser("", ""), + wantCalled: false, + }, + } + + for _, tc := range cases { + t.Run(tc.desc, func(t *testing.T) { + svc := &stubUsersService{} + result := refreshStoredUserIdentity(svc, tc.stored, tc.claUser) + + if svc.called != tc.wantCalled { + t.Fatalf("UpdateUser called=%v, want %v", svc.called, tc.wantCalled) + } + if !svc.called { + return + } + if svc.calledWith != testUserID { + t.Errorf("UpdateUser userID=%q, want %q", svc.calledWith, testUserID) + } + _, hasName := svc.updates["user_name"] + if hasName != tc.wantNameKey { + t.Errorf("user_name in updates=%v, want %v", hasName, tc.wantNameKey) + } + emailVal, hasEmail := svc.updates["lf_email"] + if hasEmail != tc.wantEmailKey { + t.Errorf("lf_email in updates=%v, want %v", hasEmail, tc.wantEmailKey) + } + if hasEmail { + got, ok := emailVal.(string) + if !ok { + t.Fatalf("lf_email value is not string: %T", emailVal) + } + if got != strings.ToLower(got) { + t.Errorf("lf_email not lowercased: %q", got) + } + if tc.wantEmailVal != "" && got != tc.wantEmailVal { + t.Errorf("lf_email=%q, want %q", got, tc.wantEmailVal) + } + } + if _, hasDate := svc.updates["date_modified"]; !hasDate { + t.Error("date_modified must always be present when updating") + } + if result == nil { + t.Error("result must not be nil on success") + } + }) + } +} diff --git a/cla-backend-go/cmd/root.go b/cla-backend-go/cmd/root.go index 7f52b6c81..8d8b23880 100644 --- a/cla-backend-go/cmd/root.go +++ b/cla-backend-go/cmd/root.go @@ -7,6 +7,7 @@ import ( "flag" "fmt" "os" + "testing" ini "github.com/linuxfoundation/easycla/cla-backend-go/init" log "github.com/linuxfoundation/easycla/cla-backend-go/logging" @@ -52,6 +53,14 @@ func init() { // when this action is called directly. rootCmd.Flags().BoolP("toggle", "t", false, "Help message for toggle") + // Skip AWS/SSM init when running under `go test`: ini.Init() fatals on + // missing DYNAMODB_AWS_REGION (and other env vars) which are not set in + // CI test environments, and unit tests in this package don't exercise + // that code path. + if testing.Testing() { + return + } + // Initialize our common stuff ini.Init() } diff --git a/cla-backend-go/cmd/server.go b/cla-backend-go/cmd/server.go index 902e2e254..678035e70 100644 --- a/cla-backend-go/cmd/server.go +++ b/cla-backend-go/cmd/server.go @@ -908,21 +908,40 @@ func responseLoggingMiddleware(next http.Handler) http.Handler { }) } -func refreshStoredUserName(usersService users.Service, userModel *models.User, claUser *user.CLAUser) *models.User { - if userModel == nil || claUser == nil || claUser.Name == "" || userModel.Username == claUser.Name { +func refreshStoredUserIdentity(usersService users.Service, userModel *models.User, claUser *user.CLAUser) *models.User { + if userModel == nil || claUser == nil { return userModel } - updatedUser, err := usersService.UpdateUser(userModel.UserID, map[string]interface{}{ - "user_name": claUser.Name, + // Normalize the incoming email before compare/persist: the lf-email-index + // query path (users/repository.go:GetUsersByLFEmail) lowercases the + // search key, so a mixed-case stored value would be orphaned from that + // index. Compare byte-wise against the stored value to detect that + // situation and rewrite to the normalized form. + normalizedIncomingEmail := strings.ToLower(strings.TrimSpace(claUser.LFEmail)) + nameChanged := claUser.Name != "" && userModel.Username != claUser.Name + emailChanged := normalizedIncomingEmail != "" && string(userModel.LfEmail) != normalizedIncomingEmail + if !nameChanged && !emailChanged { + return userModel + } + + updates := map[string]interface{}{ "date_modified": time.Now().UTC().Format(time.RFC3339), - }) + } + if nameChanged { + updates["user_name"] = claUser.Name + } + if emailChanged { + updates["lf_email"] = normalizedIncomingEmail + } + + updatedUser, err := usersService.UpdateUser(userModel.UserID, updates) if err != nil { log.WithFields(logrus.Fields{ - "functionName": "cmd.refreshStoredUserName", + "functionName": "cmd.refreshStoredUserIdentity", "userID": userModel.UserID, "lfUsername": claUser.LFUsername, - }).WithError(err).Warn("unable to refresh stored user_name from current identity claims") + }).WithError(err).Warn("unable to refresh stored user identity from current identity claims") return userModel } @@ -973,7 +992,7 @@ func createUserFromRequest(authorizer auth.Authorizer, usersService users.Servic } // If found - refresh the stored display name and return if userModel != nil { - userModel = refreshStoredUserName(usersService, userModel, claUser) + userModel = refreshStoredUserIdentity(usersService, userModel, claUser) if !needToStoreUser { return r } @@ -993,7 +1012,7 @@ func createUserFromRequest(authorizer auth.Authorizer, usersService users.Servic } // If found - refresh the stored display name and return if userModel != nil { - userModel = refreshStoredUserName(usersService, userModel, claUser) + userModel = refreshStoredUserIdentity(usersService, userModel, claUser) if !needToStoreUser { return r } diff --git a/cla-backend-go/company/repository.go b/cla-backend-go/company/repository.go index ccb61f96f..0d848bc71 100644 --- a/cla-backend-go/company/repository.go +++ b/cla-backend-go/company/repository.go @@ -348,6 +348,12 @@ func (repo repository) GetCompany(ctx context.Context, companyID string) (*model utils.XREQUESTID: ctx.Value(utils.XREQUESTID), "companyID": companyID, } + + if companyID == "" { + log.WithFields(f).Warn("companyID is empty, skipping DynamoDB lookup") + return nil, &utils.CompanyNotFound{Message: "company_id cannot be empty", CompanyID: companyID} + } + companyTableData, err := repo.dynamoDBClient.GetItem(&dynamodb.GetItemInput{ TableName: aws.String(repo.companyTableName), Key: map[string]*dynamodb.AttributeValue{ diff --git a/cla-backend-go/emails/params.go b/cla-backend-go/emails/params.go index 82eafea4d..53643a9ec 100644 --- a/cla-backend-go/emails/params.go +++ b/cla-backend-go/emails/params.go @@ -80,6 +80,10 @@ type CLAGroupTemplateParams struct { // GetProjectNameOrFoundation returns if the foundationName is set it gets back // the foundation Name otherwise the ProjectName is returned func (claParams CLAGroupTemplateParams) GetProjectNameOrFoundation() string { + if len(claParams.Projects) == 0 { + log.Warnf("GetProjectNameOrFoundation called with empty Projects slice (CLAGroupName=%q); rendering as empty string", claParams.CLAGroupName) + return "" + } project := claParams.Projects[0] if claParams.ChildProjectCount == 1 { return claParams.Projects[0].ExternalProjectName @@ -96,6 +100,10 @@ func (claParams CLAGroupTemplateParams) GetProjectNameOrFoundation() string { // Project is used generally in v1 templates because the matching there was 1:1 // it will returns the first element from the projects list func (claParams CLAGroupTemplateParams) Project() CLAProjectParams { + if len(claParams.Projects) == 0 { + log.Warnf("Project called with empty Projects slice (CLAGroupName=%q); rendering as zero CLAProjectParams", claParams.CLAGroupName) + return CLAProjectParams{} + } return claParams.Projects[0] } diff --git a/cla-backend-go/events/service.go b/cla-backend-go/events/service.go index e9738b5c1..4546afdd1 100644 --- a/cla-backend-go/events/service.go +++ b/cla-backend-go/events/service.go @@ -439,6 +439,15 @@ func (s *service) LogEventWithContext(ctx context.Context, args *LogEventArgs) { return } + // loadDetails resolves UserID from LfUsername via DDB lookup; if the user is + // not found in EasyCLA's user table (e.g. external contractor whose LFX profile + // predates their EasyCLA record) UserID stays empty and CreateEvent would fail + // with ErrUserIDRequired. Log at warn and skip instead of surfacing a level=error. + if args.UserID == "" { + log.WithFields(f).Warnf("skipping event %s: UserID could not be resolved for LfUsername=%s", args.EventType, args.LfUsername) + return + } + eventData, containsPII := args.EventData.GetEventDetailsString(args) eventSummary, _ := args.EventData.GetEventSummaryString(args) event := models.Event{ diff --git a/cla-backend-go/signatures/converters.go b/cla-backend-go/signatures/converters.go index 84d9235e0..57258288e 100644 --- a/cla-backend-go/signatures/converters.go +++ b/cla-backend-go/signatures/converters.go @@ -57,12 +57,16 @@ func (repo repository) buildProjectSignatureModels(ctx context.Context, results claType = utils.ClaTypeICLA } - // Use the signedOn field if possible, for older signatures that are missing it, use the date created value as the default/fallback + // Use the signedOn field if possible, for older signatures that are missing it, use the date created value as the default/fallback. + // Both fields can be empty for legacy signatures predating the SignedOn column — leave signedOn as "" in that case + // rather than calling FormatTimeString("") and emitting a spurious warning. signedOn := dbSignature.DateCreated if dbSignature.SignedOn != "" { signedOn = dbSignature.SignedOn } - signedOn = utils.FormatTimeString(signedOn) + if signedOn != "" { + signedOn = utils.FormatTimeString(signedOn) + } sig := &models.Signature{ SignatureID: dbSignature.SignatureID,