feat(sql/auth): base authentication storage implementation #1095

GeorgeMac · 2022-10-25T13:42:24Z

Fixes: FLI-36

Initial implementation for storing Authentication instances into the backing relational DBs.
This PR:

Creates a new package internal/storage/sql/auth.
Creates an *auth.Store implementation specifically for Authentication associated state.
Adds the necessary migrations for each DB driver.
Implements CreateAuthentication.
Implements GetAuthenticationByClientToken.
Create and Get both perform client token hashing using SHA256.
Adds create and read tests for Authentication instances.

codecov-commenter · 2022-10-25T13:44:36Z

Codecov Report

Merging #1095 (28b9a6d) into authentication (57da7f2) will increase coverage by 0.42%.
The diff coverage is 84.29%.

❗ Current head 28b9a6d differs from pull request most recent head afd9f67. Consider uploading reports for the commit afd9f67 to get more accurate results

@@                Coverage Diff                 @@
##           authentication    #1095      +/-   ##
==================================================
+ Coverage           80.52%   80.94%   +0.42%     
==================================================
  Files                  26       29       +3     
  Lines                1884     2125     +241     
==================================================
+ Hits                 1517     1720     +203     
- Misses                287      318      +31     
- Partials               80       87       +7

Impacted Files	Coverage Δ
internal/storage/sql/migrator.go	`23.45% <10.00%> (-2.94%)`	⬇️
internal/storage/sql/auth/fields.go	`69.44% <69.44%> (ø)`
internal/storage/sql/auth/store.go	`87.76% <87.76%> (ø)`
internal/storage/sql/errors.go	`98.24% <98.24%> (ø)`

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

… types

…p table in down

…ntation

cmd/flipt/import.go

internal/storage/sql/auth/fields.go

markphelps · 2022-10-27T14:56:14Z

internal/storage/sql/auth/store.go

+		builder: builder,
+		now:     timestamppb.Now,
+		generateID: func() string {
+			return uuid.Must(uuid.NewV4()).String()


are we worried about (highly unlikely) potential for panic here?

I think the only case uuid panics occur is when your system runs out of entropy (I could be mistaken).
I believe the advice in this situation is to panic for reals. So, I wouldn't be too worried about not capturing this.

internal/storage/sql/auth/fields.go

markphelps · 2022-10-27T15:06:28Z

internal/storage/sql/auth/store.go

+			&authentication.Id,
+			&hashedToken,
+			ptr(method(authentication.Method)),
+			&jsonField[map[string]string]{authentication.Metadata},


for variant.attachment I think we just represented it as a string and compacted it before saving, ie:

flipt/internal/storage/sql/common/flag.go

Line 265 in 28ed354

if attachment != nil {

is there a reason to do it this way instead?

The jsonField container just marshals which type it contains to and from a string to make that operations a one-liner. I could definitely use json.Compact in the Driver implementation too. But this is the same thing.

cool, i wonder if we should move to be consistent everywhere

Let me see if I can drop Attachment in, and it just works...
I just moved JSONField to the sql package, so it is reachable now.

Ahh I see, Attachment moves around as a string in the protobuf itself. My mistake.

Whereas, Metadata is actually a map<string, string> in protobuf:

flipt/rpc/flipt/auth/auth.proto

Line 45 in 35eec68

map<string, string> metadata = 6;

Here, JSONField allows for the one-lining of (un)marshalling to and from arbitrary Go types like this to string for persistence.

Whereas, Attachment using json.Compact to first unmarshal a string which assumed to contain JSON and back into a compact JSON form.

Seems these are two relatively different concepts, and I see why they're both different.
With Attachments, the data's schema is controlled by the end user.
We don't need to know the structure of the JSON document.
It is compacted to keep storage cost minimized.

With Metadata, the data's schema is strictly map<string, string>.
We use that structure internally to allow each auth method to populate the map in their respective ways (e.g. static tokens store their name and description under controlled keys).
JSON is merely a convenient serialization form to stash the structured object in a TEXT field.

markphelps · 2022-10-27T15:10:06Z

internal/storage/sql/errors.go

+)
+
+// AdaptError converts specific known-driver errors into wrapped storage errors.
+func (d Driver) AdaptError(err error) error {


oh i like this. is the idea to replace the specific sql/x subpackages that override the common implementation with this for existing flag/segment/etc CRUD operations?

Yeah, 💯 I think we might be able to do away with having to add adaptors for each driver.
I can port the rest of Flipt to it in a later PR too if we like how it works.

…teger

markphelps

heyooooo 🚀

markphelps · 2022-10-27T20:27:58Z

internal/storage/sql/auth/store_test.go

+	}
+}
+
+func FuzzHashClientToken(f *testing.F) {


ooo love the fuzz

Honestly, first time I have given it a proper go. It is really neat.
I ran a long fuzz of this function. Roughly ~7M calls.

It is mostly just base64 and sha256, so not testing much.
However, it was nice to get some confidence it was producing base64 encoded non-zero length tokens.

fix(sql/auth): use crypto/rand.Reader in place of math/rand.New fix(sql/auth): capture test parameters in loop-body variables chore(migrations): drop index before dropping authentications table fix(import): run down migrations on --drop fix(migrations/sqlite3): syntax error in uniqueness constraint on temp table in down test(sql/auth): fuzz hashClientToken chore(sql/auth): more fuzzing seeds fix(sql/auth): map driver constraint errors to internal error representation chore(sql/auth): remove underscore from Fuzz test name chore(storage/sql): remove dead code refactor(migrations): change authentications method from string to integer refactor(storage/sql): move common field utilities into sql package chore(storage/sql): use keyed field in struct literals

GeorgeMac changed the base branch from authentication to gm/authentication-protobuf October 25, 2022 13:42

GeorgeMac force-pushed the gm/authentication-storage branch from b9d2341 to 5ebcb8a Compare October 26, 2022 09:45

GeorgeMac force-pushed the gm/authentication-protobuf branch from 8fe812d to ade17a7 Compare October 26, 2022 12:21

GeorgeMac changed the title ~~WIP: storage implementation~~ WIP: authentication storage implementation Oct 26, 2022

Base automatically changed from gm/authentication-protobuf to authentication October 26, 2022 13:52

GeorgeMac mentioned this pull request Oct 26, 2022

feat: token-based authentication #1097

Merged

feat(sql/auth): add package for creating and receiving Authentication…

c62399b

… types

GeorgeMac force-pushed the gm/authentication-storage branch from 5ebcb8a to c62399b Compare October 26, 2022 14:28

GeorgeMac changed the title ~~WIP: authentication storage implementation~~ feat(sql/auth): base authentication storage implementation Oct 26, 2022

GeorgeMac added 9 commits October 26, 2022 15:53

fix(sql/auth): use crypto/rand.Reader in place of math/rand.New

d29bdb2

fix(sql/auth): capture test parameters in loop-body variables

b38585f

chore(migrations): drop index before dropping authentications table

391e464

fix(import): run down migrations on --drop

9c111fe

fix(migrations/sqlite3): syntax error in uniqueness constraint on tem…

b141aee

…p table in down

test(sql/auth): fuzz hashClientToken

faf49b9

chore(sql/auth): more fuzzing seeds

d0b24e0

fix(sql/auth): map driver constraint errors to internal error represe…

206a3cf

…ntation

chore(sql/auth): remove underscore from Fuzz test name

e856f34

GeorgeMac marked this pull request as ready for review October 27, 2022 13:02

GeorgeMac requested a review from markphelps October 27, 2022 13:02

chore(storage/sql): remove dead code

28b9a6d

markphelps reviewed Oct 27, 2022

View reviewed changes

GeorgeMac added 2 commits October 27, 2022 16:53

refactor(migrations): change authentications method from string to in…

b818412

…teger

refactor(storage/sql): move common field utilities into sql package

afd9f67

GeorgeMac requested a review from markphelps October 27, 2022 15:54

chore(storage/sql): use keyed field in struct literals

35eec68

markphelps approved these changes Oct 27, 2022

View reviewed changes

GeorgeMac merged commit 979b1c5 into authentication Oct 28, 2022

GeorgeMac deleted the gm/authentication-storage branch October 28, 2022 08:35

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(sql/auth): base authentication storage implementation #1095

feat(sql/auth): base authentication storage implementation #1095

GeorgeMac commented Oct 25, 2022 •

edited

Loading

codecov-commenter commented Oct 25, 2022 •

edited

Loading

markphelps Oct 27, 2022

GeorgeMac Oct 27, 2022

markphelps Oct 27, 2022

GeorgeMac Oct 27, 2022

markphelps Oct 27, 2022

GeorgeMac Oct 27, 2022

GeorgeMac Oct 27, 2022

markphelps Oct 27, 2022

GeorgeMac Oct 27, 2022

markphelps left a comment

markphelps Oct 27, 2022

GeorgeMac Oct 28, 2022

feat(sql/auth): base authentication storage implementation #1095

feat(sql/auth): base authentication storage implementation #1095

Conversation

GeorgeMac commented Oct 25, 2022 • edited Loading

codecov-commenter commented Oct 25, 2022 • edited Loading

Codecov Report

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

markphelps left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

GeorgeMac commented Oct 25, 2022 •

edited

Loading

codecov-commenter commented Oct 25, 2022 •

edited

Loading