[management] Remove redundant get account calls in GetAccountFromToken #2615

bcmmbaga · 2024-09-18T13:51:35Z

Describe your changes

Issue ticket number and link

Checklist

Is it a bug fix
Is a typo/documentation fix
Is a feature enhancement
It is a refactor
Created tests that fail without the change (if possible)
Extended the README / documentation, if necessary

Signed-off-by: bcmmbaga <[email protected]>

management/server/account.go

pascal-fischer · 2024-09-23T12:00:02Z

management/server/account.go

+	unlock := am.Store.AcquireWriteLockByUID(ctx, accountID)
+	defer unlock()
+
+	account, err := am.Store.GetAccount(ctx, accountID)
+	if err != nil {
+		return err
+	}
+
+	jwtGroupsNames := extractJWTGroups(ctx, settings.JWTGroupsClaimName, claims)
+
+	oldGroups := make([]string, len(user.AutoGroups))
+	copy(oldGroups, user.AutoGroups)
+
+	// Update the account if group membership changes
+	if account.SetJWTGroups(claims.UserId, jwtGroupsNames) {
+		addNewGroups := difference(user.AutoGroups, oldGroups)
+		removeOldGroups := difference(oldGroups, user.AutoGroups)
+
+		if settings.GroupsPropagationEnabled {
+			account.UserGroupsAddToPeers(claims.UserId, addNewGroups...)
+			account.UserGroupsRemoveFromPeers(claims.UserId, removeOldGroups...)
+			account.Network.IncSerial()
 		}
-		if claim, ok := claims.Raw[account.Settings.JWTGroupsClaimName]; ok {
-			if slice, ok := claim.([]interface{}); ok {
-				var groupsNames []string
-				for _, item := range slice {
-					if g, ok := item.(string); ok {
-						groupsNames = append(groupsNames, g)
-					} else {
-						log.WithContext(ctx).Errorf("JWT claim %q is not a string: %v", account.Settings.JWTGroupsClaimName, item)
-					}
-				}

-				oldGroups := make([]string, len(user.AutoGroups))
-				copy(oldGroups, user.AutoGroups)
-				// if groups were added or modified, save the account
-				if account.SetJWTGroups(claims.UserId, groupsNames) {
-					if account.Settings.GroupsPropagationEnabled {
-						if user, err := account.FindUser(claims.UserId); err == nil {
-							addNewGroups := difference(user.AutoGroups, oldGroups)
-							removeOldGroups := difference(oldGroups, user.AutoGroups)
-							account.UserGroupsAddToPeers(claims.UserId, addNewGroups...)
-							account.UserGroupsRemoveFromPeers(claims.UserId, removeOldGroups...)
-							account.Network.IncSerial()
-							if err := am.Store.SaveAccount(ctx, account); err != nil {
-								log.WithContext(ctx).Errorf("failed to save account: %v", err)
-							} else {
-								log.WithContext(ctx).Tracef("user %s: JWT group membership changed, updating account peers", claims.UserId)
-								am.updateAccountPeers(ctx, account)
-								unlock()
-								alreadyUnlocked = true
-								for _, g := range addNewGroups {
-									if group := account.GetGroup(g); group != nil {
-										am.StoreEvent(ctx, user.Id, user.Id, account.Id, activity.GroupAddedToUser,
-											map[string]any{
-												"group":           group.Name,
-												"group_id":        group.ID,
-												"is_service_user": user.IsServiceUser,
-												"user_name":       user.ServiceUserName})
-									}
-								}
-								for _, g := range removeOldGroups {
-									if group := account.GetGroup(g); group != nil {
-										am.StoreEvent(ctx, user.Id, user.Id, account.Id, activity.GroupRemovedFromUser,
-											map[string]any{
-												"group":           group.Name,
-												"group_id":        group.ID,
-												"is_service_user": user.IsServiceUser,
-												"user_name":       user.ServiceUserName})
-									}
-								}
-							}
-						}
-					} else {
-						if err := am.Store.SaveAccount(ctx, account); err != nil {
-							log.WithContext(ctx).Errorf("failed to save account: %v", err)
-						}
-					}
-				}
-			} else {
-				log.WithContext(ctx).Debugf("JWT claim %q is not a string array", account.Settings.JWTGroupsClaimName)
+		if err := am.Store.SaveAccount(ctx, account); err != nil {
+			log.WithContext(ctx).Errorf("failed to save account: %v", err)
+			return nil
+		}


Idealy this would get wrapped in a transaction and we completly get rid of the lock and the getAccount

pascal-fischer · 2024-09-23T12:46:40Z

management/server/account.go

+		unlockAccount := am.Store.AcquireWriteLockByUID(ctx, userAccountID)
 		defer unlockAccount()
-		account, err = am.Store.GetAccountByUser(ctx, claims.UserId)
+		account, err := am.Store.GetAccountByUser(ctx, claims.UserId)
 		if err != nil {
-			return nil, err
+			return "", err
 		}
 		// If there is no primary domain account yet, we set the account as primary for the domain. Otherwise,
 		// we compare the account's ID with the domain account ID, and if they don't match, we set the account as
 		// non-primary account for the domain. We don't merge accounts at this stage, because of cases when a domain
 		// was previously unclassified or classified as public so N users that logged int that time, has they own account
 		// and peers that shouldn't be lost.
-		primaryDomain := domainAccount == nil || account.Id == domainAccount.Id
-
-		err = am.handleExistingUserAccount(ctx, account, primaryDomain, claims)
-		if err != nil {
-			return nil, err
+		primaryDomain := domainAccountID == "" || account.Id == domainAccountID
+		if err = am.handleExistingUserAccount(ctx, account, primaryDomain, claims); err != nil {
+			return "", err
 		}
-		return account, nil
+
+		return account.Id, nil


We should not do a getAccount and pass it down. We should do the updates in handleExistingUserAccount within a transaction inside that method

Can go into another PR

pascal-fischer · 2024-09-23T12:47:54Z

management/server/account.go

+		var domainAccount *Account
+		if domainAccountID != "" {
+			unlockAccount := am.Store.AcquireWriteLockByUID(ctx, domainAccountID)
 			defer unlockAccount()
 			domainAccount, err = am.Store.GetAccountByPrivateDomain(ctx, claims.Domain)
 			if err != nil {
-				return nil, err
+				return "", err
 			}
 		}
-		return am.handleNewUserAccount(ctx, domainAccount, claims)
+
+		account, err := am.handleNewUserAccount(ctx, domainAccount, claims)
+		if err != nil {
+			return "", err
+		}
+		return account.Id, nil


Same. We should not get the account here and do the update with transactions inside the handleNewUserAccount

Can go into another PR

management/server/group.go

pascal-fischer · 2024-09-23T13:52:24Z

management/server/http/peers_handler.go

+		account, err := h.accountManager.GetAccountByID(r.Context(), accountID, userID)
+		if err != nil {
+			util.WriteError(r.Context(), err, w)
+			return
+		}
+
+		if r.Method == http.MethodGet {
+			h.getPeer(r.Context(), account, peerID, userID, w)
+		} else {
+			h.updatePeer(r.Context(), account, userID, peerID, w, r)
+		}


We should not need to get the whole account to update or get a single peer

also make the switch nicely split between http.MethodGet, http.MethodPut without the additional if

pascal-fischer · 2024-09-23T13:58:21Z

management/server/http/peers_handler.go

 	if err != nil {
 		util.WriteError(r.Context(), err, w)
 		return
 	}

-	peers, err := h.accountManager.GetPeers(r.Context(), account.Id, user.Id)
+	account, err := h.accountManager.GetAccountByID(r.Context(), accountID, userID)


we should not get account here, we should only get peer and pass accountID into the GetValidatedPeers and do seperate db calls for the tables there (probably also in a transaction)

management/server/http/policies_handler.go

pascal-fischer · 2024-09-23T14:03:20Z

management/server/policy.go

@@ -350,15 +351,17 @@ func (am *DefaultAccountManager) SavePolicy(ctx context.Context, accountID, user
 		return err
 	}

-	exists := am.savePolicy(account, policy)
+	if err = am.savePolicy(account, policy, isUpdate); err != nil {


this should not load the whole account. Instead have a transaction and update only the fields

Signed-off-by: bcmmbaga <[email protected]>

# Conflicts: # management/server/sql_store.go

management/server/sql_store.go

Signed-off-by: bcmmbaga <[email protected]>

sonarcloud · 2024-09-26T20:37:59Z

Quality Gate passed

Issues
0 New issues
9 Accepted issues

Measures
0 Security Hotspots
0.0% Coverage on New Code
0.7% Duplication on New Code

See analysis details on SonarCloud

bcmmbaga added 10 commits September 16, 2024 13:33

refactor access control middleware and user access by JWT groups

258b30c

Signed-off-by: bcmmbaga <[email protected]>

refactor jwt groups extractor

3cf1b02

Signed-off-by: bcmmbaga <[email protected]>

refactor handlers to get account when necessary

e5d55d3

Signed-off-by: bcmmbaga <[email protected]>

refactor getAccountFromToken

ccab3b4

Signed-off-by: bcmmbaga <[email protected]>

refactor getAccountWithAuthorizationClaims

720d36a

Signed-off-by: bcmmbaga <[email protected]>

Merge branch 'main' into refactor-get-account-by-token

a4c4158

fix merge

021fc8f

Signed-off-by: bcmmbaga <[email protected]>

revert handles change

f60a423

Signed-off-by: bcmmbaga <[email protected]>

remove GetUserByID from account manager

8f9c54f

Signed-off-by: bcmmbaga <[email protected]>

fix tests

9631cb4

Signed-off-by: bcmmbaga <[email protected]>

bcmmbaga force-pushed the refactor-get-account-by-token branch from f1e2712 to 9631cb4 Compare September 18, 2024 16:05

bcmmbaga marked this pull request as ready for review September 18, 2024 16:30

mlsmaycon mentioned this pull request Sep 19, 2024

[management] Restrict accessible peers to user-owned peers for non-admins #2618

Merged

6 tasks

bcmmbaga force-pushed the refactor-get-account-by-token branch 2 times, most recently from 4b8680c to 9631cb4 Compare September 20, 2024 08:49

bcmmbaga added 3 commits September 20, 2024 14:07

refactor getAccountWithAuthorizationClaims to return account id

4d9bb7e

Signed-off-by: bcmmbaga <[email protected]>

Merge branch 'main' into refactor-get-account-by-token

26dd045

refactor handlers to use GetAccountIDFromToken

8f98add

Signed-off-by: bcmmbaga <[email protected]>

bcmmbaga force-pushed the refactor-get-account-by-token branch 2 times, most recently from 7d870f6 to 1d6f4a4 Compare September 22, 2024 20:23

fix tests

7601a17

Signed-off-by: bcmmbaga <[email protected]>

bcmmbaga force-pushed the refactor-get-account-by-token branch from 1d6f4a4 to 7601a17 Compare September 22, 2024 20:44

pascal-fischer requested changes Sep 23, 2024

View reviewed changes

bcmmbaga added 4 commits September 23, 2024 20:12

remove locks

d9f612d

Signed-off-by: bcmmbaga <[email protected]>

refactor

2884038

Signed-off-by: bcmmbaga <[email protected]>

add GetGroupByName from store

1ffe89d

Signed-off-by: bcmmbaga <[email protected]>

add GetGroupByID from store and refactor

7561706

Signed-off-by: bcmmbaga <[email protected]>

bcmmbaga force-pushed the refactor-get-account-by-token branch from f903fce to 7561706 Compare September 24, 2024 16:55

bcmmbaga added 2 commits September 24, 2024 21:57

Refactor retrieval of policy and posture checks

eab8564

Signed-off-by: bcmmbaga <[email protected]>

Refactor user permissions and retrieves PAT

d14b855

Signed-off-by: bcmmbaga <[email protected]>

bcmmbaga added 6 commits September 25, 2024 12:52

Refactor route, setupkey, nameserver and dns to get record(s) from store

16174f0

Signed-off-by: bcmmbaga <[email protected]>

Refactor store

41b212f

Signed-off-by: bcmmbaga <[email protected]>

fix lint

b815393

Signed-off-by: bcmmbaga <[email protected]>

fix tests

c384874

Signed-off-by: bcmmbaga <[email protected]>

fix add missing policy source posture checks

dc82c2d

Signed-off-by: bcmmbaga <[email protected]>

Merge branch 'main' into refactor-get-account-by-token

871595d

# Conflicts: # management/server/sql_store.go

pascal-fischer reviewed Sep 26, 2024

View reviewed changes

management/server/sql_store.go Outdated Show resolved Hide resolved

management/server/sql_store.go Outdated Show resolved Hide resolved

management/server/sql_store.go Outdated Show resolved Hide resolved

management/server/sql_store.go Outdated Show resolved Hide resolved

add store lock

4575ae2

Signed-off-by: bcmmbaga <[email protected]>

pascal-fischer previously approved these changes Sep 26, 2024

View reviewed changes

fix tests

b1b2b0a

Signed-off-by: bcmmbaga <[email protected]>

bcmmbaga dismissed pascal-fischer’s stale review via b1b2b0a September 26, 2024 16:48

bcmmbaga force-pushed the refactor-get-account-by-token branch from f2ed2ff to b1b2b0a Compare September 26, 2024 16:58

add get account

e90d9ce

Signed-off-by: bcmmbaga <[email protected]>

pascal-fischer approved these changes Sep 27, 2024

View reviewed changes

bcmmbaga merged commit acb73bd into main Sep 27, 2024
21 checks passed

bcmmbaga deleted the refactor-get-account-by-token branch September 27, 2024 14:10

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[management] Remove redundant get account calls in GetAccountFromToken #2615

[management] Remove redundant get account calls in GetAccountFromToken #2615

bcmmbaga commented Sep 18, 2024

pascal-fischer Sep 23, 2024

pascal-fischer Sep 23, 2024

pascal-fischer Sep 23, 2024

pascal-fischer Sep 23, 2024

pascal-fischer Sep 23, 2024

pascal-fischer Sep 23, 2024

pascal-fischer Sep 23, 2024

pascal-fischer Sep 23, 2024

pascal-fischer Sep 23, 2024

sonarcloud bot commented Sep 26, 2024

[management] Remove redundant get account calls in GetAccountFromToken #2615

[management] Remove redundant get account calls in GetAccountFromToken #2615

Conversation

bcmmbaga commented Sep 18, 2024

Describe your changes

Issue ticket number and link

Checklist

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sonarcloud bot commented Sep 26, 2024

Quality Gate passed