Skip to content

Commit 80cf2e6

Browse files
committed
chore: improve comments, error handling, logging, and edge cases
Changes in addition to minor cleanups: - `authentication.Store` can now log info and warnings - Improved logic in `UpdateAuthorization` when both Token and HashedToken are set. Added supporting test cases.
1 parent 9d008ea commit 80cf2e6

File tree

9 files changed

+270
-79
lines changed

9 files changed

+270
-79
lines changed

authorization/hasher.go

Lines changed: 6 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -50,7 +50,7 @@ func WithDecoderVariants(variants []influxdb2_algo.Variant) AuthorizationHasherO
5050

5151
// NewAuthorizationHasher creates an AuthorizationHasher for influxdb2 algorithm hashed tokens.
5252
// variantName specifies which token hashing variant to use, with blank indicating to use the default
53-
// hashing variant. By defaults, all variants of the influxdb2 hashing scheme are supported for
53+
// hashing variant. By default, all variants of the influxdb2 hashing scheme are supported for
5454
// maximal compatibility.
5555
func NewAuthorizationHasher(opts ...AuthorizationHasherOption) (*AuthorizationHasher, error) {
5656
options := authorizationHasherOptions{
@@ -80,8 +80,8 @@ func NewAuthorizationHasher(opts ...AuthorizationHasherOption) (*AuthorizationHa
8080
}
8181
}
8282

83-
// Create all variant hashers needed for requested decoder variants. This is for operations where all
84-
// potential variations of a raw token must be hashed.
83+
// Create all variant hashers needed for requested decoder variants. This is required for operations where
84+
// all potential variations of a raw token must be hashed, such as looking up a hash in the hashed token index.
8585
var allHashers []algorithm.Hash
8686
for _, variant := range options.decoderVariants {
8787
h, err := influxdb2_algo.New(influxdb2_algo.WithVariant(variant))
@@ -110,11 +110,11 @@ func (h *AuthorizationHasher) Hash(token string) (string, error) {
110110
// AllHashes generates a list of PHC-encoded hashes of token for all deterministic (i.e. non-salted) supported hashes.
111111
func (h *AuthorizationHasher) AllHashes(token string) ([]string, error) {
112112
hashes := make([]string, len(h.allHashers))
113-
for idx, h := range h.allHashers {
114-
digest, err := h.Hash(token)
113+
for idx, hasher := range h.allHashers {
114+
digest, err := hasher.Hash(token)
115115
if err != nil {
116116
variantName := "N/A"
117-
if influxdb_hasher, ok := h.(*influxdb2_algo.Hasher); ok {
117+
if influxdb_hasher, ok := hasher.(*influxdb2_algo.Hasher); ok {
118118
variantName = influxdb_hasher.Variant().Prefix()
119119
}
120120
return nil, fmt.Errorf("hashing raw token failed (variant=%s): %w", variantName, err)

authorization/http_server_test.go

Lines changed: 13 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -183,15 +183,16 @@ func TestService_handlePostAuthorization(t *testing.T) {
183183

184184
res := w.Result()
185185
contentType := res.Header.Get("Content-Type")
186-
body, _ := io.ReadAll(res.Body)
186+
body, err := io.ReadAll(res.Body)
187+
require.NoError(t, err)
187188

188189
require.Equalf(t, tt.wants.statusCode, res.StatusCode, "headers: %v body: %s", res.Header, body)
189190
if tt.wants.contentType != "" {
190191
require.Equal(t, tt.wants.contentType, contentType)
191192
}
192193
diff, err := jsonDiff(string(body), tt.wants.body)
193194
require.NoError(t, err)
194-
require.Empty(t, diff)
195+
require.Empty(t, diff, "authorization endpoint returned unexpected result")
195196
})
196197
}
197198
}
@@ -346,15 +347,16 @@ func TestService_handleGetAuthorization(t *testing.T) {
346347

347348
res := w.Result()
348349
contentType := res.Header.Get("Content-Type")
349-
body, _ := io.ReadAll(res.Body)
350+
body, err := io.ReadAll(res.Body)
351+
require.NoError(t, err)
350352

351353
require.Equalf(t, tt.wants.statusCode, res.StatusCode, "headers: %v body: %s", res.Header, body)
352354
if tt.wants.contentType != "" {
353355
require.Equal(t, tt.wants.contentType, contentType)
354356
}
355357
diff, err := jsonDiff(string(body), tt.wants.body)
356358
require.NoError(t, err)
357-
require.Empty(t, diff)
359+
require.Empty(t, diff, "authorization endpoint returned unexpected result")
358360
})
359361
}
360362
}
@@ -730,15 +732,16 @@ func TestService_handleGetAuthorizations(t *testing.T) {
730732

731733
res := w.Result()
732734
contentType := res.Header.Get("Content-Type")
733-
body, _ := io.ReadAll(res.Body)
735+
body, err := io.ReadAll(res.Body)
736+
require.NoError(t, err)
734737

735738
require.Equal(t, tt.wants.statusCode, res.StatusCode)
736739
if tt.wants.contentType != "" {
737740
require.Equal(t, tt.wants.contentType, contentType)
738741
}
739742
diff, err := jsonDiff(string(body), tt.wants.body)
740743
require.NoError(t, err)
741-
require.Empty(t, diff)
744+
require.Empty(t, diff, "authorization endpoint returned unexpected results")
742745
})
743746
}
744747
}
@@ -827,17 +830,18 @@ func TestService_handleDeleteAuthorization(t *testing.T) {
827830

828831
res := w.Result()
829832
contentType := res.Header.Get("Content-Type")
830-
body, _ := io.ReadAll(res.Body)
833+
body, err := io.ReadAll(res.Body)
834+
require.NoError(t, err)
831835

832836
require.Equal(t, tt.wants.statusCode, res.StatusCode)
833837
if tt.wants.contentType != "" {
834-
require.Equal(t, tt.wants.contentType, contentType)
838+
require.Equal(t, tt.wants.contentType, contentType, "handleDeleteAuthorization")
835839
}
836840

837841
if tt.wants.body != "" {
838842
diff, err := jsonDiff(string(body), tt.wants.body)
839843
require.NoError(t, err)
840-
require.Empty(t, diff)
844+
require.Empty(t, diff, "authorization endpoint returned unexpected results")
841845
}
842846
})
843847
}

authorization/storage.go

Lines changed: 24 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -14,6 +14,7 @@ import (
1414
"github.com/influxdata/influxdb/v2/kv"
1515
influxdb2_algo "github.com/influxdata/influxdb/v2/pkg/crypt/algorithm/influxdb2"
1616
"github.com/influxdata/influxdb/v2/snowflake"
17+
"go.uber.org/zap"
1718
)
1819

1920
/*---
@@ -126,6 +127,9 @@ type Store struct {
126127
// Indicates if tokens should be stored in hashed PHC format.
127128
useHashedTokens bool
128129

130+
// Logger
131+
log *zap.Logger
132+
129133
// Indicates if Store is read-only.
130134
readOnly bool
131135

@@ -153,15 +157,21 @@ func WithAuthorizationHashVariantName(name string) StoreOption {
153157
}
154158
}
155159

156-
func WithReadOnly(readOnly bool) StoreOption {
160+
func WithIgnoreMissingHashIndex(allowMissing bool) StoreOption {
157161
return func(s *storePlusOptions) {
158-
s.readOnly = readOnly
162+
s.ignoreMissingHashIndex = allowMissing
159163
}
160164
}
161165

162-
func WithIgnoreMissingHashIndex(allowMissing bool) StoreOption {
166+
func WithLogger(log *zap.Logger) StoreOption {
163167
return func(s *storePlusOptions) {
164-
s.ignoreMissingHashIndex = allowMissing
168+
s.log = log
169+
}
170+
}
171+
172+
func WithReadOnly(readOnly bool) StoreOption {
173+
return func(s *storePlusOptions) {
174+
s.readOnly = readOnly
165175
}
166176
}
167177

@@ -180,6 +190,10 @@ func NewStore(ctx context.Context, kvStore kv.Store, useHashedTokens bool, opts
180190
o(s)
181191
}
182192

193+
if s.log == nil {
194+
s.log = zap.NewNop()
195+
}
196+
183197
if err := s.setup(ctx); err != nil {
184198
return nil, err
185199
}
@@ -264,8 +278,12 @@ func (s *Store) hashedTokenMigration(ctx context.Context) error {
264278
var authsNeedingUpdate []*influxdb.Authorization
265279
err := s.View(ctx, func(tx kv.Tx) error {
266280
s.forEachAuthorization(ctx, tx, nil, func(a *influxdb.Authorization) bool {
267-
if a.HashedToken == "" && a.Token != "" {
268-
authsNeedingUpdate = append(authsNeedingUpdate, a)
281+
if a.HashedToken == "" {
282+
if a.Token != "" {
283+
authsNeedingUpdate = append(authsNeedingUpdate, a)
284+
} else {
285+
s.log.Warn("found authorization without any token set during hashed token migration", zap.Uint64("ID", uint64(a.ID)), zap.String("description", a.Description))
286+
}
269287
}
270288
return true
271289
})

authorization/storage_authorization.go

Lines changed: 49 additions & 38 deletions
Original file line numberDiff line numberDiff line change
@@ -13,6 +13,7 @@ import (
1313
"github.com/influxdata/influxdb/v2/kit/platform/errors"
1414
"github.com/influxdata/influxdb/v2/kv"
1515
jsonp "github.com/influxdata/influxdb/v2/pkg/jsonparser"
16+
"go.uber.org/zap"
1617
)
1718

1819
var (
@@ -80,38 +81,42 @@ func decodeAuthorization(b []byte, a *influxdb.Authorization) error {
8081
return nil
8182
}
8283

83-
// verifyTokensMatch returns an error if a.Token and a.HashedToken are set
84-
// but do not match.
85-
func (s *Store) verifyTokensMatch(a *influxdb.Authorization) error {
86-
if a.Token == "" || a.HashedToken == "" {
87-
return nil
88-
}
89-
90-
// If both Token and HashedToken are set, make sure they are equivalent before continuing.
91-
match, err := s.hasher.Match(a.HashedToken, a.Token)
92-
if err != nil {
93-
return fmt.Errorf("error matching tokens: %w", err)
94-
}
95-
if !match {
96-
return ErrHashedTokenMismatch
97-
}
98-
return nil
99-
}
100-
101-
// hashToken hashes a.Token to a.HashedToken, if needed.
102-
func (s *Store) hashToken(a *influxdb.Authorization) error {
103-
if !s.useHashedTokens || a.HashedToken != "" || a.Token == "" {
104-
// Either we're not using token hashing, the token has already been hashed,
105-
// or there's no token to be hashed.
106-
return nil
84+
// transformToken updates a.Token and a.HashedToken to match configuration state,
85+
// if needed. If needed, transformToken generates the a.HashedToken from a.Token when
86+
// token hashing is enabled. transformToken will also clear a.HashedToken if token
87+
// hashing is turned off and a.Token is set to the matching token. If a.HashedToken and
88+
// a.Token are both set but do not match (a.HashedToken is a hash of a.Token), then an
89+
// error is returned.
90+
func (s *Store) transformToken(a *influxdb.Authorization) error {
91+
// Verify Token and HashedToken match if both are set.
92+
if a.Token != "" && a.HashedToken != "" {
93+
match, err := s.hasher.Match(a.HashedToken, a.Token)
94+
if err != nil {
95+
return fmt.Errorf("error matching tokens: %w", err)
96+
}
97+
if !match {
98+
return ErrHashedTokenMismatch
99+
}
107100
}
108101

109-
// Hash the token. Redaction of the hashed token takes place when the record is written.
110-
hashedToken, err := s.hasher.Hash(a.Token)
111-
if err != nil {
112-
return fmt.Errorf("error hashing token: %w", err)
102+
if a.Token != "" {
103+
if s.useHashedTokens {
104+
// Need to generate HashedToken from Token. Redaction of the hashed token takes
105+
// place when the record is written to the KV store. In some cases the client
106+
// code that triggered commit needs access to the raw Token, such as when a
107+
// token is initially created so it can be shown to the user.
108+
// Note that even if a.HashedToken is set, we will regenerate it here. This ensures
109+
// that a.HashedToken will be stored using the currently configured hashing algoirithm.
110+
if hashedToken, err := s.hasher.Hash(a.Token); err != nil {
111+
return fmt.Errorf("error hashing token: %w", err)
112+
} else {
113+
a.HashedToken = hashedToken
114+
}
115+
} else {
116+
// Token hashing disabled, a.Token is available, clear a.HashedToken if set.
117+
a.HashedToken = ""
118+
}
113119
}
114-
a.HashedToken = hashedToken
115120

116121
return nil
117122
}
@@ -177,7 +182,7 @@ func (s *Store) GetAuthorizationByID(ctx context.Context, tx kv.Tx, id platform.
177182
return a, nil
178183
}
179184

180-
// validateToken checks if token matches tht token stored in auth. If auth.Token is set, that is
185+
// validateToken checks if token matches that token stored in auth. If auth.Token is set, that is
181186
// compared first. Otherwise, auth.HashedToken is used to verify token. If neither field in auth is set, then
182187
// the comparison fails.
183188
func (s *Store) validateToken(auth *influxdb.Authorization, token string) (bool, error) {
@@ -349,11 +354,7 @@ func (s *Store) forEachAuthorization(ctx context.Context, tx kv.Tx, pred kv.Curs
349354
// and makes sure indices point to it. It does not delete any indices. The updated authorization is
350355
// returned on success.
351356
func (s *Store) commitAuthorization(ctx context.Context, tx kv.Tx, a *influxdb.Authorization) error {
352-
if err := s.verifyTokensMatch(a); err != nil {
353-
return errors.ErrInternalServiceError(err, errors.WithErrorCode(errors.EInvalid))
354-
}
355-
356-
if err := s.hashToken(a); err != nil {
357+
if err := s.transformToken(a); err != nil {
357358
return errors.ErrInternalServiceError(err, errors.WithErrorCode(errors.EInternal))
358359
}
359360

@@ -602,7 +603,12 @@ func (s *Store) authorizationsPredicateFn(f influxdb.AuthorizationFilter) kv.Cur
602603

603604
if f.Token != nil {
604605
token := *f.Token
605-
allHashes, _ := s.hasher.AllHashes(token) // on error, allHashes is empty and we'll ignore hashedToken
606+
allHashes, err := s.hasher.AllHashes(token)
607+
if err != nil {
608+
s.log.Error("error generating hashes in authorizationsPredicateFn", zap.Error(err))
609+
// On error, continue onward. allHashes is empty and we'll effectively ignore hashedToken,
610+
// but we'll still look at the unhashed Token if it is available.
611+
}
606612
return func(_, value []byte) bool {
607613
// it is assumed that token never has escaped string data
608614
if got, _, _, err := jsonparser.Get(value, "token"); err == nil {
@@ -652,8 +658,13 @@ func (s *Store) filterAuthorizationsFn(filter influxdb.AuthorizationFilter) func
652658

653659
if filter.Token != nil {
654660
token := *filter.Token
655-
// if AllHashes returns an error, allHashes will be empty and we will ignore a.HashedToken.
656-
allHashes, _ := s.hasher.AllHashes(token)
661+
allHashes, err := s.hasher.AllHashes(token)
662+
if err != nil {
663+
s.log.Error("error generating hashes in filterPredicateFn", zap.Error(err))
664+
// On error, continue onward. allHashes is empty and we'll effectively ignore hashedToken,
665+
// but we'll still look at the unhashed Token if it is available.
666+
}
667+
657668
return func(a *influxdb.Authorization) bool {
658669
if a.Token == token {
659670
return true

0 commit comments

Comments
 (0)