diff --git a/internal/controller/group_controller.go b/internal/controller/group_controller.go index a38e99be..94ca405f 100644 --- a/internal/controller/group_controller.go +++ b/internal/controller/group_controller.go @@ -61,6 +61,12 @@ const ( requeueAfter = 8 * time.Hour ) +type gitlabUserToVerify struct { + user string + email string + cachedID string +} + // GroupReconciler reconciles a Group object type GroupReconciler struct { client.Client @@ -860,6 +866,10 @@ func (r *GroupReconciler) createUsersInBackendAndCache(ctx context.Context, // NOTE: CacheMutex is already held by caller (Reconcile) backendKey := backendName + "_" + backendType + var gitlabUsersToVerify []gitlabUserToVerify + var usersToCreate []string + + // Identify users to verify or create (Lock held) for _, user := range users { userDetails := r.allLdapUserData[user] if userDetails == nil { @@ -876,10 +886,27 @@ func (r *GroupReconciler) createUsersInBackendAndCache(ctx context.Context, // Check if user already has ID for this backend if userID, exists := userBackends[backendKey]; exists && userID != "" { - r.backendLogger.WithField("user", user).Debug("user already exists in cache") - continue + if backendType == "gitlab" { + gitlabUsersToVerify = append(gitlabUsersToVerify, gitlabUserToVerify{ + user: user, + email: userDetails.GetEmail(), + cachedID: userID, + }) + } else { + r.backendLogger.WithField("user", user).Info("user already exists in cache") + } + } else { + usersToCreate = append(usersToCreate, user) } + } + + if err := r.verifyAndUpdateGitLabUserCache(ctx, gitlabUsersToVerify, backendKey, backendClient); err != nil { + return err + } + // Create new users (Lock held) + for _, user := range usersToCreate { + userDetails := r.allLdapUserData[user] // if user details are not found in cache, create a new user in backend // Standardize first/last names for backends (e.g. Fivetran) that do not support ., (, ), or , in names newUser, err := backendClient.CreateUser(ctx, &structs.User{ @@ -906,6 +933,61 @@ func (r *GroupReconciler) createUsersInBackendAndCache(ctx context.Context, return nil } +// verifyAndUpdateGitLabUserCache verifies that cached GitLab user IDs are still +// valid by fetching from the backend, and updates the cache if the ID has changed +// (e.g. after a user re-login). Caller must hold CacheMutex; this method temporarily +// releases it during network calls and re-acquires it before returning. +func (r *GroupReconciler) verifyAndUpdateGitLabUserCache(ctx context.Context, + usersToVerify []gitlabUserToVerify, + backendKey string, + backendClient clients.Client) error { + + if len(usersToVerify) == 0 { + return nil + } + + // Release the lock before making network calls + r.CacheMutex.Unlock() + + type verifyResult struct { + gitlabUserToVerify + fetchedID string + err error + } + results := make([]verifyResult, 0, len(usersToVerify)) + + for _, u := range usersToVerify { + fetched, err := backendClient.FetchUserDetails(ctx, u.cachedID) + res := verifyResult{gitlabUserToVerify: u, err: err} + if err == nil { + res.fetchedID = fetched.ID + } + results = append(results, res) + } + + r.CacheMutex.Lock() + + for _, res := range results { + if res.err != nil { + r.backendLogger.WithField("user", res.user).WithError(res.err).Error("error fetching user details") + return res.err + } + + if res.fetchedID == res.cachedID { + r.backendLogger.WithField("user", res.user).Info("user already exists in cache") + continue + } + + r.backendLogger.WithField("user", res.user).Info("user has logged in, updating cache with new userID") + if err := r.Store.User.SetBackend(ctx, res.email, backendKey, res.fetchedID); err != nil { + r.backendLogger.WithField("user", res.user).WithError(err).Error("error updating user details in cache") + return err + } + } + + return nil +} + func (r *GroupReconciler) fetchOrCreateTeam(ctx context.Context, groupName string, backendClient clients.Client, backendParams *structs.BackendParams) (string, error) { diff --git a/internal/controller/group_controller_test.go b/internal/controller/group_controller_test.go index 91e576d9..c623277e 100644 --- a/internal/controller/group_controller_test.go +++ b/internal/controller/group_controller_test.go @@ -18,6 +18,10 @@ package controller import ( "context" + "encoding/json" + "fmt" + "net/http" + "net/http/httptest" "os" "path/filepath" "sync" @@ -82,6 +86,14 @@ var _ = Describe("Group Controller", func() { CleanupInterval: int32(-1), }, }, + Pattern: map[string][]config.PatternEntry{ + "default": { + { + Input: "(.*)", + Output: "$1", + }, + }, + }, } Cache, err := cache.New(&appConfig.Cache) @@ -351,5 +363,133 @@ var _ = Describe("Group Controller", func() { Expect(status.Status).To(BeFalse()) Expect(status.Message).To(ContainSubstring("missing required connection parameters")) }) + + It("should update user ID in cache when gitlab backend returns a different ID", func() { + By("simulating a user login scenario where cache has old ID (username) but backend has new ID (integer)") + + cleanup := setupSafeTestConfig() + defer cleanup() + + userEmail := "usernauttestuser@example.com" + oldID := "usernauttestuser" // to be stored in cache (username) + newID := 737373 // to be returned by backend (as userID as integer) + + // Setup Mock Server + ts := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + w.Header().Set("Content-Type", "application/json") + + // Mock User Details Fetch via Username (GET /api/v4/users?username=usernauttestuser) + // This happens because "usernauttestuser" is not an integer, so the client falls back to username search + if r.Method == http.MethodGet && r.URL.Path == "/api/v4/users" && r.URL.Query().Get("username") == oldID { + // Return the user with the NEW numeric ID + _ = json.NewEncoder(w).Encode([]map[string]interface{}{{ + "id": newID, + "username": oldID, + "name": "Usernaut Test User", + "email": userEmail, + "state": "active", + }}) + return + } + + // Mock Team Creation/Fetch (needed so the flow reaches user processing) + if r.Method == http.MethodGet && r.URL.Path == "/api/v4/groups/test-group-gitlab" { + w.WriteHeader(http.StatusNotFound) + return + } + if r.Method == http.MethodPost && r.URL.Path == "/api/v4/groups" { + _ = json.NewEncoder(w).Encode(map[string]interface{}{ + "id": 1628041, + "name": "test-group-gitlab", + "path": "test-group-gitlab", + }) + return + } + // Mock Team Members Fetch + if r.Method == http.MethodGet && + (r.URL.Path == "/api/v4/groups/1628041/members" || r.URL.Path == "/api/v4/groups/1628041/members/all") { + _ = json.NewEncoder(w).Encode([]interface{}{}) + return + } + + // Mock Add Member + if r.Method == http.MethodPost && r.URL.Path == "/api/v4/groups/1628041/members" { + w.WriteHeader(http.StatusCreated) + _ = json.NewEncoder(w).Encode(map[string]interface{}{ + "id": newID, + "username": "usernaut-test-user", + "name": "Test User", + "state": "active", + }) + return + } + + // Default fallback + w.WriteHeader(http.StatusNotFound) + })) + defer ts.Close() + + gitlabBackend := config.Backend{ + Name: "gitlab", + Type: "gitlab", + Enabled: true, + Connection: map[string]interface{}{ + keyUrl: ts.URL, + keyParentGroupId: 123456, + "token": "mock-token", + }, + } + reconciler, ldapClient := setupTestReconciler([]config.Backend{gitlabBackend}) + + // Set initial cache state + backendKey := "gitlab_gitlab" + err := reconciler.Store.User.SetBackend(ctx, userEmail, backendKey, oldID) + Expect(err).NotTo(HaveOccurred()) + + ldapClient.EXPECT().GetUserLDAPData(gomock.Any(), gomock.Any()).Return(map[string]interface{}{ + "cn": "CN-test", + "sn": "SN-test", + "displayName": "Usernaut Test User", + "mail": userEmail, + "uid": "uid-test", + }, nil).AnyTimes() + + groupName := "test-group-github" + group := &usernautdevv1alpha1.Group{ + ObjectMeta: metav1.ObjectMeta{ + Name: groupName, + Namespace: "default", + }, + Spec: usernautdevv1alpha1.GroupSpec{ + GroupName: groupName, + Members: usernautdevv1alpha1.Members{ + Users: []string{"testuser"}, + }, + Backends: []usernautdevv1alpha1.Backend{ + {Name: "gitlab", Type: "gitlab"}, + }, + }, + } + Expect(k8sClient.Create(ctx, group)).To(Succeed()) + defer func() { _ = k8sClient.Delete(ctx, group) }() + + // Manually acquire lock to simulate Reconcile flow, then release it + // This test verifies the logic inside createUsersInBackendAndCache which expects the lock to be held initially + // However, since we are calling Reconcile() which handles locking internally, we don't need to manually lock here. + // The key verification is that the operation succeeds (meaning the unlock/lock sequence inside worked correctly) + // and that the cache is updated. + + _, err = reconciler.Reconcile(ctx, reconcile.Request{ + NamespacedName: types.NamespacedName{Name: groupName, Namespace: "default"}, + }) + // Expect no error if the mock server handles everything correctly + Expect(err).NotTo(HaveOccurred()) + + // Verify Cache Update + backends, err := reconciler.Store.User.GetBackends(ctx, userEmail) + Expect(err).NotTo(HaveOccurred()) + // The ID should be updated to the new one (737373) + Expect(backends[backendKey]).To(Equal(fmt.Sprintf("%d", newID))) + }) }) }) diff --git a/pkg/clients/gitlab/users.go b/pkg/clients/gitlab/users.go index d68cfdbf..edb6b302 100644 --- a/pkg/clients/gitlab/users.go +++ b/pkg/clients/gitlab/users.go @@ -106,13 +106,12 @@ func (g *GitlabClient) FetchUserDetails(ctx context.Context, userID string) (*st return nil, listErr } if len(users) > 0 && resp.StatusCode == http.StatusOK { - log.Infof("found user %s details in gitlab backend", userID) + log.Infof("found user %s details in gitlab backend with userID %d", userID, users[0].ID) return userDetails(users[0]), nil } else { // this handles the case where user never logged in to gitlab // so user details like userID is not found in gitlab - // TODO: need to handle the case when the user login in gitlab - log.Warnf("unable to find user %s details in gitlab backend", userID) + log.Warnf("user never logged in to gitlab, unable to find user %s details in gitlab backend", userID) return &structs.User{ ID: userID, UserName: userID,