chore: variety of minor fixes

Address PR issues on comments, error handling, and add final token
matching check in `Store.GetAuthorizationByToken`.

Author: gwossum

authorization/hasher.go

@@ -113,7 +113,11 @@ func (h *AuthorizationHasher) AllHashes(token string) ([]string, error) {
 	for idx, h := range h.allHashers {
 		digest, err := h.Hash(token)
 		if err != nil {
-			return nil, fmt.Errorf("hashing raw token failed: %w", err)
+			variantName := "N/A"
+			if influxdb_hasher, ok := h.(*influxdb2_algo.Hasher); ok {
+				variantName = influxdb_hasher.Variant().Prefix()
+			}
+			return nil, fmt.Errorf("hashing raw token failed (variant=%s): %w", variantName, err)
 		}
 		hashes[idx] = digest.Encode()
 	}

authorization/http_server_test.go

@@ -158,13 +158,10 @@ func TestService_handlePostAuthorization(t *testing.T) {
 				router.Mount(handler.Prefix(), handler)
 
 				req, err := newPostAuthorizationRequest(tt.args.authorization)
-				if err != nil {
-					t.Fatalf("failed to create new authorization request: %v", err)
-				}
+				require.NoError(t, err)
+
 				b, err := json.Marshal(req)
-				if err != nil {
-					t.Fatalf("failed to unmarshal authorization: %v", err)
-				}
+				require.NoError(t, err)
 
 				r := httptest.NewRequest("GET", "http://any.url", bytes.NewReader(b))
 				r = r.WithContext(context.WithValue(
@@ -185,21 +182,16 @@ func TestService_handlePostAuthorization(t *testing.T) {
 				handler.handlePostAuthorization(w, r)
 
 				res := w.Result()
-				content := res.Header.Get("Content-Type")
+				contentType := res.Header.Get("Content-Type")
 				body, _ := io.ReadAll(res.Body)
 
-				if res.StatusCode != tt.wants.statusCode {
-					t.Logf("headers: %v body: %s", res.Header, body)
-					t.Errorf("%q. handlePostAuthorization() = %v, want %v", tt.name, res.StatusCode, tt.wants.statusCode)
-				}
-				if tt.wants.contentType != "" && content != tt.wants.contentType {
-					t.Errorf("%q. handlePostAuthorization() = %v, want %v", tt.name, content, tt.wants.contentType)
-				}
-				if diff, err := jsonDiff(string(body), tt.wants.body); diff != "" {
-					t.Errorf("%q. handlePostAuthorization() = ***%s***", tt.name, diff)
-				} else if err != nil {
-					t.Errorf("%q, handlePostAuthorization() error: %v", tt.name, err)
+				require.Equalf(t, tt.wants.statusCode, res.StatusCode, "headers: %v body: %s", res.Header, body)
+				if tt.wants.contentType != "" {
+					require.Equal(t, tt.wants.contentType, contentType)
 				}
+				diff, err := jsonDiff(string(body), tt.wants.body)
+				require.NoError(t, err)
+				require.Empty(t, diff)
 			})
 		}
 	}
@@ -353,21 +345,16 @@ func TestService_handleGetAuthorization(t *testing.T) {
 			handler.handleGetAuthorization(w, r)
 
 			res := w.Result()
-			content := res.Header.Get("Content-Type")
+			contentType := res.Header.Get("Content-Type")
 			body, _ := io.ReadAll(res.Body)
 
-			if res.StatusCode != tt.wants.statusCode {
-				t.Logf("headers: %v body: %s", res.Header, body)
-				t.Errorf("%q. handleGetAuthorization() = %v, want %v", tt.name, res.StatusCode, tt.wants.statusCode)
-			}
-			if tt.wants.contentType != "" && content != tt.wants.contentType {
-				t.Errorf("%q. handleGetAuthorization() = %v, want %v", tt.name, content, tt.wants.contentType)
-			}
-			if diff, err := jsonDiff(string(body), tt.wants.body); err != nil {
-				t.Errorf("%q, handleGetAuthorization. error unmarshalling json %v", tt.name, err)
-			} else if tt.wants.body != "" && diff != "" {
-				t.Errorf("%q. handleGetAuthorization() = -got/+want %s**", tt.name, diff)
+			require.Equalf(t, tt.wants.statusCode, res.StatusCode, "headers: %v body: %s", res.Header, body)
+			if tt.wants.contentType != "" {
+				require.Equal(t, tt.wants.contentType, contentType)
 			}
+			diff, err := jsonDiff(string(body), tt.wants.body)
+			require.NoError(t, err)
+			require.Empty(t, diff)
 		})
 	}
 }
@@ -719,9 +706,7 @@ func TestService_handleGetAuthorizations(t *testing.T) {
 				s := itesting.NewTestInmemStore(t)
 
 				storage, err := NewStore(context.Background(), s, useHashedTokens)
-				if err != nil {
-					t.Fatal(err)
-				}
+				require.NoError(t, err)
 
 				svc := NewService(storage, tt.fields.TenantService)
 
@@ -744,21 +729,16 @@ func TestService_handleGetAuthorizations(t *testing.T) {
 				handler.handleGetAuthorizations(w, r)
 
 				res := w.Result()
-				content := res.Header.Get("Content-Type")
+				contentType := res.Header.Get("Content-Type")
 				body, _ := io.ReadAll(res.Body)
 
-				if res.StatusCode != tt.wants.statusCode {
-					t.Errorf("%q. handleGetAuthorizations() = %v, want %v", tt.name, res.StatusCode, tt.wants.statusCode)
-				}
-				if tt.wants.contentType != "" && content != tt.wants.contentType {
-					t.Errorf("%q. handleGetAuthorizations() = %v, want %v", tt.name, content, tt.wants.contentType)
+				require.Equal(t, tt.wants.statusCode, res.StatusCode)
+				if tt.wants.contentType != "" {
+					require.Equal(t, tt.wants.contentType, contentType)
 				}
-				if diff, err := jsonDiff(string(body), tt.wants.body); diff != "" {
-					t.Errorf("%q. handleGetAuthorizations() = ***%s***", tt.name, diff)
-				} else if err != nil {
-					t.Errorf("%q, handleGetAuthorizations() error: %v", tt.name, err)
-				}
-
+				diff, err := jsonDiff(string(body), tt.wants.body)
+				require.NoError(t, err)
+				require.Empty(t, diff)
 			})
 		}
 	}
@@ -846,22 +826,18 @@ func TestService_handleDeleteAuthorization(t *testing.T) {
 			handler.handleDeleteAuthorization(w, r)
 
 			res := w.Result()
-			content := res.Header.Get("Content-Type")
+			contentType := res.Header.Get("Content-Type")
 			body, _ := io.ReadAll(res.Body)
 
-			if res.StatusCode != tt.wants.statusCode {
-				t.Errorf("%q. handleDeleteAuthorization() = %v, want %v", tt.name, res.StatusCode, tt.wants.statusCode)
-			}
-			if tt.wants.contentType != "" && content != tt.wants.contentType {
-				t.Errorf("%q. handleDeleteAuthorization() = %v, want %v", tt.name, content, tt.wants.contentType)
+			require.Equal(t, tt.wants.statusCode, res.StatusCode)
+			if tt.wants.contentType != "" {
+				require.Equal(t, tt.wants.contentType, contentType)
 			}
 
 			if tt.wants.body != "" {
-				if diff, err := jsonDiff(string(body), tt.wants.body); err != nil {
-					t.Errorf("%q, handleDeleteAuthorization(). error unmarshalling json %v", tt.name, err)
-				} else if diff != "" {
-					t.Errorf("%q. handleDeleteAuthorization() = ***%s***", tt.name, diff)
-				}
+				diff, err := jsonDiff(string(body), tt.wants.body)
+				require.NoError(t, err)
+				require.Empty(t, diff)
 			}
 		})
 	}

authorization/storage.go

@@ -54,7 +54,7 @@ The implementation has the following behaviors under different scenarios:
     the hash algorithm (e.g. `SHA-512``) instead of the hashed token value.
 * Downgrading to an older InfluxDB without hashed token support with a BoltDB containing hashed tokens.
   * Downgrading requires a manual `influxd downgrade` command. If hashed tokens are found in the
-    BoltDB, user configuration is required. The user can also list impacted tokens. When downgrade is
+    BoltDB, user confirmation is required. The user can also list impacted tokens. When downgrade is
 	complete, all hashed tokens have been deleted from BoltDB along with their indices. The tokens with
 	deleted hashes are no longer useable.
   * Operations are as usual in InfluxDB 2.7.
@@ -78,7 +78,7 @@ When token hashing is enabled and a backup is restored, raw tokens are hashed be
 into BoltDB. Raw tokens are not stored.
 
 To verify tokens when hashed tokens are enabled, the presented token's hash is calculated and used
-for the token index lookup. The rest of the authorization flow is unchanged.
+for token index lookup. The rest of the authorization flow is unchanged.
 
 To verify tokens when hashed tokens are disabled, the an attempt is made to parse the presented token as
 PHC. If the parse succeeds, the access is denied. This prevents an attack described below. After this check,
@@ -100,7 +100,7 @@ can not be used.
 A potential future security would be optionally storing "peppered" hashes. This would require retrieving
 the pepper key from outside of BoltDB, for example from Vault.
 
-When listing tokens, hashed tokens are listed of "REDACTED" instead of the hashed
+When listing tokens, hashed tokens are listed as "REDACTED" instead of the hashed
 token value. Raw token values are returned as in previous versions.
 
 ---*/
@@ -277,7 +277,7 @@ func (s *Store) hashedTokenMigration(ctx context.Context) error {
 
 	for batch := range slices.Chunk(authsNeedingUpdate, 100) {
 		err := s.Update(ctx, func(tx kv.Tx) error {
-			// Now update them. This really seems too simple, but s.UpdateJAuthorization() is magical.
+			// 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

authorization/storage_authorization.go

@@ -17,6 +17,8 @@ import (
 
 var (
 	ErrHashedTokenMismatch = goerrors.New("HashedToken does not match Token")
+	ErrIncorrectToken      = goerrors.New("token is incorrect for authorization")
+	ErrNoTokenAvailable    = goerrors.New("no token available for authorization")
 )
 
 func authIndexKey(n string) []byte {
@@ -175,6 +177,25 @@ func (s *Store) GetAuthorizationByID(ctx context.Context, tx kv.Tx, id platform.
 	return a, nil
 }
 
+// validateToken checks if token matches tht token stored in auth. If auth.Token is set, that is
+// compared first. Otherwise, auth.HashedToken is used to verify token. If neither field in auth is set, then
+// the comparison fails.
+func (s *Store) validateToken(auth *influxdb.Authorization, token string) (bool, error) {
+	if auth.Token != "" {
+		return auth.Token == token, nil
+	}
+
+	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 match, nil
+	}
+
+	return false, ErrNoTokenAvailable
+}
+
 // GetAuthorizationsByToken searches for an authorization by its raw (unhashed) token value. It will also search
 // for entires with equivalent hashed tokens if the raw token is not directly found.
 func (s *Store) GetAuthorizationByToken(ctx context.Context, tx kv.Tx, token string) (auth *influxdb.Authorization, retErr error) {
@@ -246,7 +267,26 @@ func (s *Store) GetAuthorizationByToken(ctx context.Context, tx kv.Tx, token str
 		}
 	}
 
-	return s.GetAuthorizationByID(ctx, tx, id)
+	// Verify that the token stored in auth matches the requested token. This should be superfluous check, but
+	// we will just in case somehow the authorization record got out of sync with the index.
+	auth, err = s.GetAuthorizationByID(ctx, tx, id)
+	if err != nil {
+		return nil, &errors.Error{
+			Code: errors.EInternal,
+			Err:  err,
+		}
+	}
+	match, err := s.validateToken(auth, token)
+	if err != nil {
+		return nil, &errors.Error{
+			Code: errors.EInternal,
+			Err:  err,
+		}
+	}
+	if !match {
+		return nil, errors.EIncorrectPassword
+	}
+	return auth, nil
 }
 
 // ListAuthorizations returns all the authorizations matching a set of FindOptions. This function is used for
@@ -310,11 +350,11 @@ func (s *Store) forEachAuthorization(ctx context.Context, tx kv.Tx, pred kv.Curs
 // returned on success.
 func (s *Store) commitAuthorization(ctx context.Context, tx kv.Tx, a *influxdb.Authorization) error {
 	if err := s.verifyTokensMatch(a); err != nil {
-		return err
+		return errors.ErrInternalServiceError(err, errors.WithErrorCode(errors.EInvalid))
 	}
 
 	if err := s.hashToken(a); err != nil {
-		return err
+		return errors.ErrInternalServiceError(err, errors.WithErrorCode(errors.EInternal))
 	}
 
 	v, err := s.encodeAuthorization(a)
@@ -330,33 +370,33 @@ func (s *Store) commitAuthorization(ctx context.Context, tx kv.Tx, a *influxdb.A
 	if !s.useHashedTokens && a.Token != "" {
 		idx, err := authIndexBucket(tx)
 		if err != nil {
-			return err
+			return errors.ErrInternalServiceError(err, errors.WithErrorCode(errors.EInternal))
 		}
 
 		if err := idx.Put(authIndexKey(a.Token), encodedID); err != nil {
-			return err
+			return errors.ErrInternalServiceError(err, errors.WithErrorCode(errors.EInternal))
 		}
 	}
 
 	if a.HashedToken != "" {
 		idx, err := hashedAuthIndexBucket(tx)
 		// Don't ignore a missing index here, we want an error.
 		if err != nil {
-			return err
+			return errors.ErrInternalServiceError(err, errors.WithErrorCode(errors.EInternal))
 		}
 
 		if err := idx.Put(hashedAuthIndexKey(a.HashedToken), encodedID); err != nil {
-			return err
+			return errors.ErrInternalServiceError(err, errors.WithErrorCode(errors.EInternal))
 		}
 	}
 
 	b, err := tx.Bucket(authBucket)
 	if err != nil {
-		return err
+		return errors.ErrInternalServiceError(err, errors.WithErrorCode(errors.EInternal))
 	}
 
 	if err := b.Put(encodedID, v); err != nil {
-		return err
+		return errors.ErrInternalServiceError(err, errors.WithErrorCode(errors.EInternal))
 	}
 
 	return nil

authorization/storage_authorization_test.go

@@ -465,6 +465,7 @@ func TestAuthorizationStore_HashingConfigChanges(t *testing.T) {
 
 				return nil
 			})
+			require.NoError(t, err)
 		})
 
 	}

authorizer/task_test.go

@@ -38,9 +38,7 @@ func TestOnboardingValidation(t *testing.T) {
 				Bucket:                 "holder",
 				RetentionPeriodSeconds: 1,
 			})
-			if err != nil {
-				t.Fatal(err)
-			}
+			require.NoError(t, err)
 
 			ctx := pctx.SetAuthorizer(context.Background(), r.Auth)
 
@@ -53,9 +51,7 @@ func TestOnboardingValidation(t *testing.T) {
 }
 from(bucket:"holder") |> range(start:-5m) |> to(bucket:"holder", org:"thing")`,
 			})
-			if err != nil {
-				t.Fatal(err)
-			}
+			require.NoError(t, err)
 		})
 	}
 }
@@ -146,22 +142,18 @@ func runTestValidations(useHashedTokens bool, t *testing.T) {
 		Bucket:                 "holder",
 		RetentionPeriodSeconds: 1,
 	})
-	if err != nil {
-		t.Fatal(err)
-	}
+	require.NoError(t, err)
 
-	if err := svc.CreateOrganization(context.Background(), otherOrg); err != nil {
-		t.Fatal(err)
-	}
+	err = svc.CreateOrganization(context.Background(), otherOrg)
+	require.NoError(t, err)
 
 	otherBucket := &influxdb.Bucket{
 		Name:  "other_bucket",
 		OrgID: otherOrg.ID,
 	}
 
-	if err = svc.CreateBucket(context.Background(), otherBucket); err != nil {
-		t.Fatal(err)
-	}
+	err = svc.CreateBucket(context.Background(), otherBucket)
+	require.NoError(t, err)
 
 	var (
 		orgID            = r.Org.ID
@@ -611,9 +603,8 @@ func newStore(t *testing.T) kv.Store {
 
 	store := inmem.NewKVStore()
 
-	if err := all.Up(context.Background(), zaptest.NewLogger(t), store); err != nil {
-		t.Fatal(err)
-	}
+	err := all.Up(context.Background(), zaptest.NewLogger(t), store)
+	require.NoError(t, err)
 
 	return store
 }