Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
86 changes: 84 additions & 2 deletions internal/controller/group_controller.go
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -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 {
Expand All @@ -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" {
Comment thread
sunando94 marked this conversation as resolved.
gitlabUsersToVerify = append(gitlabUsersToVerify, gitlabUserToVerify{
user: user,
email: userDetails.GetEmail(),
cachedID: userID,
})
} else {
r.backendLogger.WithField("user", user).Info("user already exists in cache")
}
Comment thread
sunando94 marked this conversation as resolved.
} 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{
Expand All @@ -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) {
Expand Down
140 changes: 140 additions & 0 deletions internal/controller/group_controller_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,10 @@ package controller

import (
"context"
"encoding/json"
"fmt"
"net/http"
"net/http/httptest"
"os"
"path/filepath"
"sync"
Expand Down Expand Up @@ -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)
Expand Down Expand Up @@ -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"
Comment thread
maxkashyap41 marked this conversation as resolved.
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)))
})
})
})
5 changes: 2 additions & 3 deletions pkg/clients/gitlab/users.go
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand Down
Loading