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

ability to set a default bearer token #556

Merged
merged 8 commits into from
Aug 1, 2023
Merged

ability to set a default bearer token #556

merged 8 commits into from
Aug 1, 2023

Conversation

michaelneale
Copy link
Contributor

Overview

Allow ability to have Bearer token optionally.

Description

By setting an AUTH_TOKEN api calls can be checked against a bearer token.

How Has This Been Tested?

A new test case for default behavior has been added.

References

https://github.com/zalando/gin-oauth2

@codecov-commenter
Copy link

codecov-commenter commented Jun 22, 2023

Codecov Report

Merging #556 (a0d8b46) into main (d9327f3) will decrease coverage by 0.02%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##             main     #556      +/-   ##
==========================================
- Coverage   27.02%   27.00%   -0.02%     
==========================================
  Files          48       53       +5     
  Lines        5832     5928      +96     
==========================================
+ Hits         1576     1601      +25     
- Misses       3973     4044      +71     
  Partials      283      283              
Files Changed Coverage Δ
pkg/server/middleware/Authentication.go 100.00% <100.00%> (ø)
pkg/server/server.go 67.24% <100.00%> (+0.14%) ⬆️

... and 4 files with indirect coverage changes

Copy link
Contributor

@andresuribe87 andresuribe87 left a comment

Choose a reason for hiding this comment

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

I think we should do the introspection approach that was introduced in https://github.com/TBD54566975/ssi-service/pull/369/files (see the introspect middleware). We can separate that out into a single PR.

Integrating with an existing authorization server is how I expect production deployments to work.

Copy link
Member

@decentralgabe decentralgabe left a comment

Choose a reason for hiding this comment

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

I like this approach but would suggest that we pause on auth until we have a (light) SIP in place that goes over options and examples.

@michaelneale
Copy link
Contributor Author

@andresuribe87 yeah I like that - what we want to be careful of is shipping without something default on (as it will come back to bite) and default means assuming something pretty basic (but yeah a rel prod will integrate with many things).

@michaelneale
Copy link
Contributor Author

cc @mistermoe @bradleydwyer if needed can easily add in

@decentralgabe
Copy link
Member

I believe this should be merged, but since docs have shifted, @michaelneale can you move the README additions to a new piece of service documentation here on auth?

@michaelneale
Copy link
Contributor Author

@decentralgabe yep working on it...

@@ -118,6 +118,7 @@ func setUpEngine(cfg config.ServerConfig, shutdown chan os.Signal) *gin.Engine {
gin.Recovery(),
gin.Logger(),
middleware.Errors(shutdown),
middleware.AuthMiddleware(),
Copy link
Member

Choose a reason for hiding this comment

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

I think we want to comment this out

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Copy link
Member

@decentralgabe decentralgabe left a comment

Choose a reason for hiding this comment

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

nice

@michaelneale
Copy link
Contributor Author

@decentralgabe ok to merge?

@decentralgabe decentralgabe merged commit 02d6f8f into main Aug 1, 2023
6 checks passed
@decentralgabe decentralgabe deleted the default_auth branch August 1, 2023 15:56
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.

4 participants