Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat(authentication): gRPC service definition for AuthenticationMethodTokenService #1102

Merged
merged 10 commits into from
Nov 3, 2022

Conversation

GeorgeMac
Copy link
Contributor

@GeorgeMac GeorgeMac commented Oct 28, 2022

Supports #779

FIxes: FLI-35

Another one for the token-based auth series of changes.

This change includes the gRPC definitions for static token creation.
It implements the bridge between the creation token transport to the authentication storage.

Additionally, I did a little shuffling.
I moved the auth related storage up from storage/sql/auth to storage/auth.
Then beneath storage/auth I nested two storage types sql and memory.

I am exploring the approach of not using mocking, rather, using in-memory representations of dependencies.
Would like to hear your thoughts. I had a colleague previously who was passionately anti-mock.
That just testing against an in-memory implementation that was maintained to be functionality equivalent, gave a more representative test suite.
I am unsure, but I thought I could give it a go here.

Finally, this PR wires up main with the gRPC service and gateway implementations.
This is guarded by the new authentication.enabled config flag.

@codecov-commenter
Copy link

codecov-commenter commented Oct 28, 2022

Codecov Report

Merging #1102 (81c2580) into authentication (35a71d0) will decrease coverage by 0.27%.
The diff coverage is 81.41%.

@@                Coverage Diff                 @@
##           authentication    #1102      +/-   ##
==================================================
- Coverage           80.80%   80.52%   -0.28%     
==================================================
  Files                  26       32       +6     
  Lines                1870     2193     +323     
==================================================
+ Hits                 1511     1766     +255     
- Misses                279      342      +63     
- Partials               80       85       +5     
Impacted Files Coverage Δ
internal/storage/sql/migrator.go 23.45% <5.88%> (-3.69%) ⬇️
internal/storage/sql/fields.go 18.42% <18.42%> (ø)
internal/storage/auth/auth.go 41.66% <41.66%> (ø)
internal/storage/sql/db.go 90.69% <68.42%> (-4.09%) ⬇️
internal/server/evaluator.go 93.60% <85.71%> (+1.90%) ⬆️
internal/config/config.go 80.00% <92.30%> (+3.28%) ⬆️
internal/storage/auth/sql/store.go 94.59% <94.59%> (ø)
internal/storage/sql/errors.go 98.24% <98.24%> (ø)
internal/config/authentication.go 100.00% <100.00%> (ø)
internal/config/cache.go 100.00% <100.00%> (ø)
... and 9 more

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

@GeorgeMac GeorgeMac changed the title WIP: gRPC service definition for AuthenticationMethodTokenService feat(authentication): gRPC service definition for AuthenticationMethodTokenService Oct 28, 2022
@GeorgeMac GeorgeMac marked this pull request as ready for review October 28, 2022 16:53
Copy link
Collaborator

@markphelps markphelps left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

looking good! just some minor typos. I think we just need to make the changes we talked about today with enabled -> required so that auth is always 'enabled' but not 'enforced' if off

I am exploring the approach of not using mocking, rather, using in-memory representations of dependencies.
Would like to hear your thoughts. I had a colleague previously who was passionately anti-mock.
That just testing against an in-memory implementation that was maintained to be functionality equivalent, gave a more representative test suite.
I am unsure, but I thought I could give it a go here.

I totally agree in re: to mocking. I usually don't mock at all in the storage layer and below, and prefer to use either sqlite or in-memory stores. As far as the server layer, i've been more relaxed and use mocking as I can more easily test error conditions that way. But im open to whatever

internal/server/auth/method/token/server.go Outdated Show resolved Hide resolved
internal/server/auth/method/token/server.go Outdated Show resolved Hide resolved
internal/storage/auth/memory/store.go Outdated Show resolved Hide resolved
@GeorgeMac
Copy link
Contributor Author

Cheers for the review @markphelps

I actually have the config changes lined up in here #1114
Mind if I forgo it here for now and solve it there when I un-draft?

Copy link
Collaborator

@markphelps markphelps left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm! one question

swagger/auth/auth.swagger.json Outdated Show resolved Hide resolved
@markphelps
Copy link
Collaborator

🚀 🌕

@GeorgeMac GeorgeMac merged commit 2487ffb into authentication Nov 3, 2022
@GeorgeMac GeorgeMac deleted the gm/authentication-grpc branch November 3, 2022 09:49
GeorgeMac added a commit that referenced this pull request Nov 8, 2022
…dTokenService (#1102)

feat(auth/method/token): initial gRPC server implementation

test(server/auth/method/token): assert token creation via API

chore(server/auth): synchronize server stop and fatal on error

fix(storage): use flipt/errors package

chore(server/auth): validate invalid error adapts appropriately

feat(authentication): wire up grpc service and gateway

feat(server/auth): define unary server interceptor

feat(storage/auth): implement list authentications

feat(storage/auth): list with method predicate

feat(auth): configure initial token bootstrap process

chore(auth): correct documentation typos

fix(proto): change create token http method PUT to POST
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants