chore: improve error logs (argoproj#10592) (argoproj#19260)

* chore: wrapped all errors and add context in account.go Signed-off-by: KangManJoo <[email protected]> * chore: rollback error msg Signed-off-by: KangManJoo <[email protected]> * chore: To align the test code error messages with the actual error message returned. Signed-off-by: KangManJoo <[email protected]> --------- Signed-off-by: KangManJoo <[email protected]>
scm-manager · Aug 8, 2024 · 1a94032 · 1a94032
1 parent 74bbc4f
commit 1a94032
Show file tree

Hide file tree

Showing 2 changed files with 18 additions and 17 deletions.
diff --git a/server/account/account.go b/server/account/account.go
@@ -48,7 +48,7 @@ func (s *Server) UpdatePassword(ctx context.Context, q *account.UpdatePasswordRe
 	// assuming user is trying to update someone else if username is different or issuer is not Argo CD
 	if updatedUsername != username || issuer != session.SessionManagerClaimsIssuer {
 		if err := s.enf.EnforceErr(ctx.Value("claims"), rbacpolicy.ResourceAccounts, rbacpolicy.ActionUpdate, q.Name); err != nil {
-			return nil, err
+			return nil, fmt.Errorf("permission denied: %w", err)
 		}
 	}
 
@@ -70,7 +70,7 @@ func (s *Server) UpdatePassword(ctx context.Context, q *account.UpdatePasswordRe
 
 		iat, err := session.Iat(ctx)
 		if err != nil {
-			return nil, err
+			return nil, fmt.Errorf("failed to get issue time: %w", err)
 		}
 		if time.Since(iat) > common.ChangePasswordSSOTokenMaxAge {
 			return nil, errors.New("SSO token is too old. Please use 'argocd relogin' to get a new token.")
@@ -80,12 +80,12 @@ func (s *Server) UpdatePassword(ctx context.Context, q *account.UpdatePasswordRe
 	// Need to validate password complexity with regular expression
 	passwordPattern, err := s.settingsMgr.GetPasswordPattern()
 	if err != nil {
-		return nil, err
+		return nil, fmt.Errorf("failed to get password pattern: %w", err)
 	}
 
 	validPasswordRegexp, err := regexp.Compile(passwordPattern)
 	if err != nil {
-		return nil, err
+		return nil, fmt.Errorf("failed to compile password regex: %w", err)
 	}
 
 	if !validPasswordRegexp.Match([]byte(q.NewPassword)) {
@@ -95,7 +95,7 @@ func (s *Server) UpdatePassword(ctx context.Context, q *account.UpdatePasswordRe
 
 	hashedPassword, err := password.HashPassword(q.NewPassword)
 	if err != nil {
-		return nil, err
+		return nil, fmt.Errorf("failed to hash password: %w", err)
 	}
 
 	err = s.settingsMgr.UpdateAccount(updatedUsername, func(acc *settings.Account) error {
@@ -105,7 +105,7 @@ func (s *Server) UpdatePassword(ctx context.Context, q *account.UpdatePasswordRe
 		return nil
 	})
 	if err != nil {
-		return nil, err
+		return nil, fmt.Errorf("failed to update account password: %w", err)
 	}
 
 	if updatedUsername == username {
@@ -132,7 +132,7 @@ func (s *Server) CanI(ctx context.Context, r *account.CanIRequest) (*account.Can
 	if r.Resource == "logs" {
 		serverRBACLogEnforceEnable, err := s.settingsMgr.GetServerRBACLogEnforceEnable()
 		if err != nil {
-			return nil, err
+			return nil, fmt.Errorf("failed to get server RBAC log enforcement setting: %w", err)
 		}
 
 		if !serverRBACLogEnforceEnable {
@@ -174,7 +174,7 @@ func (s *Server) ensureHasAccountPermission(ctx context.Context, action string,
 		return nil
 	}
 	if err := s.enf.EnforceErr(ctx.Value("claims"), rbacpolicy.ResourceAccounts, action, account); err != nil {
-		return err
+		return fmt.Errorf("permission denied for account %s with action %s: %w", account, action, err)
 	}
 	return nil
 }
@@ -184,7 +184,7 @@ func (s *Server) ListAccounts(ctx context.Context, r *account.ListAccountRequest
 	resp := account.AccountsList{}
 	accounts, err := s.settingsMgr.GetAccounts()
 	if err != nil {
-		return nil, err
+		return nil, fmt.Errorf("failed to get accounts: %w", err)
 	}
 	for name, a := range accounts {
 		if err := s.ensureHasAccountPermission(ctx, rbacpolicy.ActionGet, name); err == nil {
@@ -200,26 +200,26 @@ func (s *Server) ListAccounts(ctx context.Context, r *account.ListAccountRequest
 // GetAccount returns an account
 func (s *Server) GetAccount(ctx context.Context, r *account.GetAccountRequest) (*account.Account, error) {
 	if err := s.ensureHasAccountPermission(ctx, rbacpolicy.ActionGet, r.Name); err != nil {
-		return nil, err
+		return nil, fmt.Errorf("permission denied to get account %s: %w", r.Name, err)
 	}
 	a, err := s.settingsMgr.GetAccount(r.Name)
 	if err != nil {
-		return nil, err
+		return nil, fmt.Errorf("failed to get account %s: %w", r.Name, err)
 	}
 	return toApiAccount(r.Name, *a), nil
 }
 
 // CreateToken creates a token
 func (s *Server) CreateToken(ctx context.Context, r *account.CreateTokenRequest) (*account.CreateTokenResponse, error) {
 	if err := s.ensureHasAccountPermission(ctx, rbacpolicy.ActionUpdate, r.Name); err != nil {
-		return nil, err
+		return nil, fmt.Errorf("permission denied to create token for account %s: %w", r.Name, err)
 	}
 
 	id := r.Id
 	if id == "" {
 		uniqueId, err := uuid.NewRandom()
 		if err != nil {
-			return nil, err
+			return nil, fmt.Errorf("failed to generate unique ID: %w", err)
 		}
 		id = uniqueId.String()
 	}
@@ -252,15 +252,15 @@ func (s *Server) CreateToken(ctx context.Context, r *account.CreateTokenRequest)
 		return nil
 	})
 	if err != nil {
-		return nil, err
+		return nil, fmt.Errorf("failed to update account with new token: %w", err)
 	}
 	return &account.CreateTokenResponse{Token: tokenString}, nil
 }
 
 // DeleteToken deletes a token
 func (s *Server) DeleteToken(ctx context.Context, r *account.DeleteTokenRequest) (*account.EmptyResponse, error) {
 	if err := s.ensureHasAccountPermission(ctx, rbacpolicy.ActionUpdate, r.Name); err != nil {
-		return nil, err
+		return nil, fmt.Errorf("permission denied to delete account %s: %w", r.Name, err)
 	}
 
 	err := s.settingsMgr.UpdateAccount(r.Name, func(account *settings.Account) error {
@@ -271,7 +271,7 @@ func (s *Server) DeleteToken(ctx context.Context, r *account.DeleteTokenRequest)
 		return status.Errorf(codes.NotFound, "token with id '%s' does not exist", r.Id)
 	})
 	if err != nil {
-		return nil, err
+		return nil, fmt.Errorf("failed to delete account %s: %w", r.Name, err)
 	}
 	return &account.EmptyResponse{}, nil
 }
diff --git a/server/account/account_test.go b/server/account/account_test.go
@@ -292,7 +292,8 @@ func TestCreateToken_UserSpecifiedID(t *testing.T) {
 
 	_, err = accountServer.CreateToken(ctx, &account.CreateTokenRequest{Name: "account1", Id: "test"})
 	require.Error(t, err)
-	assert.Contains(t, "account already has token with id 'test'", err.Error())
+	assert.Contains(t, err.Error(), "failed to update account with new token:")
+	assert.Contains(t, err.Error(), "account already has token with id 'test'")
 }
 
 func TestDeleteToken_SuccessfullyRemoved(t *testing.T) {