Skip to content

Commit

Permalink
add a user hash function to serve as ID
Browse files Browse the repository at this point in the history
  • Loading branch information
alichaddad committed Sep 29, 2022
1 parent 2b7f3c8 commit ac6b04d
Show file tree
Hide file tree
Showing 4 changed files with 23 additions and 6 deletions.
6 changes: 3 additions & 3 deletions core/clustersmngr/factory.go
Original file line number Diff line number Diff line change
Expand Up @@ -460,8 +460,8 @@ func (cf *clustersManager) UpdateUserNamespaces(ctx context.Context, user *auth.
wg.Wait()
}

func (cf *clustersManager) UserLock(userID string) *sync.Mutex {
actual, _ := cf.usersLock.LoadOrStore(userID, &sync.Mutex{})
func (cf *clustersManager) UserLock(user *auth.UserPrincipal) *sync.Mutex {

This comment has been minimized.

Copy link
@bigkevmcd

bigkevmcd Oct 3, 2022

Contributor

This might be better named LockUser as it doesn't just return the lock, but it locks it too...

But this perhaps points to a nicer pattern...

func func (cf *clustersManager) WithUserLock(user *auth.UserPrincipal, f func() error) error {
  // take the lock, defer unlock, call f and return the error if necesary
}
actual, _ := cf.usersLock.LoadOrStore(user.Hash(), &sync.Mutex{})
lock := actual.(*sync.Mutex)
lock.Lock()
return lock
Expand All @@ -472,7 +472,7 @@ func (cf *clustersManager) GetUserNamespaces(user *auth.UserPrincipal) map[strin
}

func (cf *clustersManager) userNsList(ctx context.Context, user *auth.UserPrincipal) map[string][]v1.Namespace {
userLock := cf.UserLock(user.ID)
userLock := cf.UserLock(user)
defer userLock.Unlock()

userNamespaces := cf.GetUserNamespaces(user)
Expand Down
2 changes: 1 addition & 1 deletion core/clustersmngr/factory_caches.go
Original file line number Diff line number Diff line change
Expand Up @@ -151,5 +151,5 @@ func (un *UsersNamespaces) Clear() {
}

func (un UsersNamespaces) cacheKey(user *auth.UserPrincipal, cluster string) uint64 {
return ttlcache.StringKey(fmt.Sprintf("%s:%s", user.ID, cluster))
return ttlcache.StringKey(fmt.Sprintf("%s:%s", user.Hash(), cluster))
}
5 changes: 5 additions & 0 deletions pkg/server/auth/auth.go
Original file line number Diff line number Diff line change
Expand Up @@ -98,6 +98,11 @@ func (p *UserPrincipal) String() string {
return fmt.Sprintf("id=%q groups=%v", p.ID, p.Groups)
}

// Hash returns a unique string using user id,token and groups.
func (p *UserPrincipal) Hash() string {

This comment has been minimized.

Copy link
@bigkevmcd

bigkevmcd Oct 3, 2022

Contributor

This is a bit confusingly named, it doesn't use a Hash, maybe it should to avoid the token being leaked in the event of an error?

return fmt.Sprintf("%s/%s/%v", p.ID, p.Token(), p.Groups)
}

func (p *UserPrincipal) Valid() bool {
if p.ID == "" && p.Token() == "" {
return false
Expand Down
16 changes: 14 additions & 2 deletions pkg/server/auth/auth_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -331,7 +331,19 @@ func TestUserPrincipal_String(t *testing.T) {
// principal is logged out.
p := auth.NewUserPrincipal(auth.ID("testing"), auth.Groups([]string{"group1", "group2"}), auth.Token("test-token"))

if s := p.String(); s != `id="testing" groups=[group1 group2]` {
t.Fatalf("principal.String() got %s, want %s", s, `id="testing" groups=[group1 group2]`)
want := `id="testing" groups=[group1 group2]`
if s := p.String(); s != want {
t.Fatalf("principal.String() got %s, want %s", s, want)
}
}

func TestUserPrincipal_Hash(t *testing.T) {
// This is primarily to guard against leaking the auth token if the

This comment has been minimized.

Copy link
@bigkevmcd

bigkevmcd Oct 3, 2022

Contributor

I'm not sure this comment is right?

// principal is logged out.
p := auth.NewUserPrincipal(auth.ID("testing"), auth.Groups([]string{"group1", "group2"}), auth.Token("test-token"))

want := "testing/test-token/[group1 group2]"
if s := p.Hash(); s != want {
t.Fatalf("principal.String() got %s, want %s", s, want)
}
}

0 comments on commit ac6b04d

Please sign in to comment.