chore: address comments from PR

Author: gwossum

authorization/hasher.go

@@ -69,7 +69,7 @@ func NewAuthorizationHasher(opts ...AuthorizationHasherOption) (*AuthorizationHa
 	// Create the hasher used for hashing new tokens before storage.
 	hasher, err := influxdb2_algo.New(influxdb2_algo.WithVariant(options.hasherVariant))
 	if err != nil {
-		return nil, fmt.Errorf("creating hasher for AuthorizationHasher: %w", err)
+		return nil, fmt.Errorf("creating hasher %s for AuthorizationHasher: %w", options.hasherVariant.Prefix(), err)
 	}
 
 	// Create decoder and register all requested decoder variants.

authorization/http_server_test.go

@@ -191,7 +191,7 @@ func TestService_handlePostAuthorization(t *testing.T) {
 					require.Equal(t, tt.wants.contentType, contentType)
 				}
 				diff, err := jsonDiff(string(body), tt.wants.body)
-				require.NoError(t, err)
+				require.NoError(t, err, "jsonDiff failed")
 				require.Empty(t, diff, "authorization endpoint returned unexpected result")
 			})
 		}
@@ -355,7 +355,7 @@ func TestService_handleGetAuthorization(t *testing.T) {
 				require.Equal(t, tt.wants.contentType, contentType)
 			}
 			diff, err := jsonDiff(string(body), tt.wants.body)
-			require.NoError(t, err)
+			require.NoError(t, err, "jsonDiff failed")
 			require.Empty(t, diff, "authorization endpoint returned unexpected result")
 		})
 	}
@@ -740,7 +740,7 @@ func TestService_handleGetAuthorizations(t *testing.T) {
 					require.Equal(t, tt.wants.contentType, contentType)
 				}
 				diff, err := jsonDiff(string(body), tt.wants.body)
-				require.NoError(t, err)
+				require.NoError(t, err, "jsonDiff failed")
 				require.Empty(t, diff, "authorization endpoint returned unexpected results")
 			})
 		}
@@ -840,7 +840,7 @@ func TestService_handleDeleteAuthorization(t *testing.T) {
 
 			if tt.wants.body != "" {
 				diff, err := jsonDiff(string(body), tt.wants.body)
-				require.NoError(t, err)
+				require.NoError(t, err, "jsonDiff failed")
 				require.Empty(t, diff, "authorization endpoint returned unexpected results")
 			}
 		})

authorization/service_test.go

@@ -64,7 +64,7 @@ func initAuthService(s kv.Store, f influxdbtesting.AuthorizationFields, useHashe
 
 func TestBoltAuthService(t *testing.T) {
 	t.Parallel()
-	for _, useHashedTokens := range []bool{true} {
+	for _, useHashedTokens := range []bool{false, true} {
 		init := func(f influxdbtesting.AuthorizationFields, t *testing.T) (influxdb.AuthorizationService, string, func()) {
 			return initBoltAuthService(f, useHashedTokens, t)
 		}

authorization/storage.go

@@ -298,7 +298,7 @@ func (s *Store) hashedTokenMigration(ctx context.Context) error {
 			// Now update them. This really seems too simple, but s.UpdateAuthorization() is magical.
 			for _, a := range batch {
 				if _, err := s.UpdateAuthorization(ctx, tx, a.ID, a); err != nil {
-					return err
+					return fmt.Errorf("failed to update authorization for %d (%s): %w", a.ID, a.Description, err)
 				}
 			}
 			return nil

authorization/storage_authorization.go

@@ -17,6 +17,7 @@ import (
 )
 
 var (
+	ErrNilAuthorization    = goerrors.New("authorization cannot be nil")
 	ErrHashedTokenMismatch = goerrors.New("HashedToken does not match Token")
 	ErrIncorrectToken      = goerrors.New("token is incorrect for authorization")
 	ErrNoTokenAvailable    = goerrors.New("no token available for authorization")
@@ -108,7 +109,7 @@ func (s *Store) transformToken(a *influxdb.Authorization) error {
 			// Note that even if a.HashedToken is set, we will regenerate it here. This ensures
 			// that a.HashedToken will be stored using the currently configured hashing algorithm.
 			if hashedToken, err := s.hasher.Hash(a.Token); err != nil {
-				return fmt.Errorf("error hashing token: %w", err)
+				return fmt.Errorf("error hashing token for token %d (%s): %w", a.ID, a.Description, err)
 			} else {
 				a.HashedToken = hashedToken
 			}
@@ -122,11 +123,17 @@ func (s *Store) transformToken(a *influxdb.Authorization) error {
 }
 
 // CreateAuthorization takes an Authorization object and saves it in storage using its token
-// using its token property as an index
+// using its token property as an index. The contents of a should be considered invalid if an
+// error occurs.
 func (s *Store) CreateAuthorization(ctx context.Context, tx kv.Tx, a *influxdb.Authorization) (retErr error) {
 	defer func() {
 		retErr = errors.ErrInternalServiceError(retErr, errors.WithErrorOp(influxdb.OpCreateAuthorization))
 	}()
+
+	if a == nil {
+		return ErrNilAuthorization
+	}
+
 	// if the provided ID is invalid, or already maps to an existing Auth, then generate a new one
 	if !a.ID.Valid() {
 		id, err := s.generateSafeID(ctx, tx, authBucket)
@@ -193,7 +200,7 @@ func (s *Store) validateToken(auth *influxdb.Authorization, token string) (bool,
 	if auth.HashedToken != "" {
 		match, err := s.hasher.Match(auth.HashedToken, token)
 		if err != nil {
-			return false, fmt.Errorf("error matching hashed token for validation: %w", err)
+			return false, fmt.Errorf("error matching hashed token %d (%s) for validation: %w", auth.ID, auth.Description, err)
 		}
 		return match, nil
 	}
@@ -437,6 +444,10 @@ func (s *Store) UpdateAuthorization(ctx context.Context, tx kv.Tx, id platform.I
 		retErr = errors.ErrInternalServiceError(retErr, errors.WithErrorOp(influxdb.OpUpdateAuthorization))
 	}()
 
+	if a == nil {
+		return nil, ErrNilAuthorization
+	}
+
 	initialToken := a.Token
 	initialHashedToken := a.HashedToken
 
@@ -610,16 +621,28 @@ func (s *Store) authorizationsPredicateFn(f influxdb.AuthorizationFilter) kv.Cur
 			// but we'll still look at the unhashed Token if it is available.
 		}
 		return func(_, value []byte) bool {
-			// it is assumed that token never has escaped string data
+			// Check if "token" matches. It is assumed that token never has escaped string data.
 			if got, _, _, err := jsonparser.Get(value, "token"); err == nil {
-				return string(got) == token
+				if len(got) > 0 {
+					return string(got) == token
+				}
+			} else {
+				return true // predicate must return true on errors
 			}
+
+			// Check if "hashedToken" matches, if applicable.
 			if len(allHashes) > 0 {
 				if got, _, _, err := jsonparser.Get(value, "hashedToken"); err == nil {
-					return slices.Contains(allHashes, string(got))
+					if len(got) > 0 {
+						return slices.Contains(allHashes, string(got))
+					}
+				} else {
+					return true // predicate must return true on errors
 				}
 			}
-			return true
+
+			// No match on "token" or "hashedToken", do not include this record.
+			return false
 		}
 	}
 

authorizer/task_test.go

@@ -38,7 +38,7 @@ func TestOnboardingValidation(t *testing.T) {
 				Bucket:                 "holder",
 				RetentionPeriodSeconds: 1,
 			})
-			require.NoError(t, err)
+			require.NoError(t, err, "OnboardInitialUser failed")
 
 			ctx := pctx.SetAuthorizer(context.Background(), r.Auth)
 
@@ -142,18 +142,16 @@ func runTestValidations(useHashedTokens bool, t *testing.T) {
 		Bucket:                 "holder",
 		RetentionPeriodSeconds: 1,
 	})
-	require.NoError(t, err)
+	require.NoError(t, err, "OnboardInitialUser failed")
 
-	err = svc.CreateOrganization(context.Background(), otherOrg)
-	require.NoError(t, err)
+	require.NoError(t, svc.CreateOrganization(context.Background(), otherOrg))
 
 	otherBucket := &influxdb.Bucket{
 		Name:  "other_bucket",
 		OrgID: otherOrg.ID,
 	}
 
-	err = svc.CreateBucket(context.Background(), otherBucket)
-	require.NoError(t, err)
+	require.NoError(t, svc.CreateBucket(context.Background(), otherBucket))
 
 	var (
 		orgID            = r.Org.ID
@@ -603,8 +601,7 @@ func newStore(t *testing.T) kv.Store {
 
 	store := inmem.NewKVStore()
 
-	err := all.Up(context.Background(), zaptest.NewLogger(t), store)
-	require.NoError(t, err)
+	require.NoError(t, all.Up(context.Background(), zaptest.NewLogger(t), store))
 
 	return store
 }

cmd/influxd/launcher/launcher.go

@@ -295,7 +295,7 @@ func (m *Launcher) run(ctx context.Context, opts *InfluxdOpts) (err error) {
 		hasherVariantName := authorization.DefaultHashVariantName // This value could come from opts in the future.
 		authStore, err := authorization.NewStore(ctx, m.kvStore, opts.UseHashedTokens, authorization.WithAuthorizationHashVariantName(hasherVariantName), authorization.WithLogger(m.log))
 		if err != nil {
-			m.log.Error("Failed creating new authorization store", zap.Error(err))
+			m.log.Error("Failed creating new authorization store", zap.Error(err), zap.Bool("UseHashedTokens", opts.UseHashedTokens), zap.String("hasherVariant", hasherVariantName))
 			return err
 		}
 		authSvc = authorization.NewService(authStore, ts)