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

Add JWT auth via headers and RS256 signing option #146

Merged
merged 2 commits into from
Jul 23, 2018

Conversation

vayan
Copy link
Contributor

@vayan vayan commented Jul 20, 2018

Description

It's more common to use JWT tokens via headers. (e.g: Authorization Bearer token11)

Also added the RS256 signing method which is used a lot too.

By default everything works like before 👍

How Has This Been Tested?

Manual testing + lot of new UT

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)

Checklist:

  • My code follows the code style of this project.
  • My change requires a change to the documentation.
  • I have updated the documentation accordingly.
  • I have added tests to cover my changes.
  • All new and existing tests passed.

@vayan vayan force-pushed the add-header-jwt-and-rs256-token branch 3 times, most recently from c544821 to 03aa433 Compare July 20, 2018 15:49
@codecov-io
Copy link

codecov-io commented Jul 20, 2018

Codecov Report

Merging #146 into master will decrease coverage by 0.78%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #146      +/-   ##
==========================================
- Coverage   94.22%   93.43%   -0.79%     
==========================================
  Files          29       23       -6     
  Lines        1333     1173     -160     
==========================================
- Hits         1256     1096     -160     
  Misses         54       54              
  Partials       23       23
Impacted Files Coverage Δ
pkg/config/middleware.go 100% <100%> (ø) ⬆️
pkg/util/util.go 100% <0%> (ø) ⬆️
pkg/entity/autogenerated_variant.go
pkg/entity/autogenerated_segment.go
pkg/entity/autogenerated_flag_snapshot.go
pkg/entity/autogenerated_flag.go
pkg/entity/autogenerated_constraint.go
pkg/entity/autogenerated_distribution.go

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 63e6952...f6e98bf. Read the comment docs.

It's more common to use JWT tokens via headers. (e.g: `Authorization Bearer token11`)

Also added the RS256 signing method which is used a lot too.
@vayan vayan force-pushed the add-header-jwt-and-rs256-token branch from 03aa433 to f624e4f Compare July 20, 2018 15:58
@@ -90,4 +90,5 @@ var Config = struct {
JWTAuthSecret string `env:"FLAGR_JWT_AUTH_SECRET" envDefault:""`
JWTAuthNoTokenRedirectURL string `env:"FLAGR_JWT_AUTH_NO_TOKEN_REDIRECT_URL" envDefault:""`
JWTAuthUserProperty string `env:"FLAGR_JWT_AUTH_USER_PROPERTY" envDefault:"flagr_user"`
JWTAuthSigningMethod string `env:"FLAGR_JWT_AUTH_SIGNING_METHOD" envDefault:"HS256"`
Copy link
Collaborator

Choose a reason for hiding this comment

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

Let's add a comment for possible values of signing methods.

The whole file will be served as the doc at https://checkr.github.io/flagr/#/flagr_env

Copy link
Collaborator

Choose a reason for hiding this comment

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

Also, can you help change the comments above of the pattern of how to use JWT auth?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sure! I missed this comment 👍

assert.Equal(t, http.StatusOK, res.Code)
})

t.Run("it will pass if jwt enabled with correct HS256 token cookie and signing method is wrong", func(t *testing.T) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

nit: change to "it will pass with a correct HS256 token cookie when signing method is wrong and it defaults to empty string secret"

@zhouzhuojie
Copy link
Collaborator

Thanks for the PR! Welcome to Flagr! Want to collect some feedback on how are you going to use Flagr, for example, are you building your own UI or simply run it through some generated clients?

Changing to RS256 and sending through headers also requires some changes to the Flagr UI, but I guess you can put it in another pr.

@zhouzhuojie
Copy link
Collaborator

Related to #121

@vayan
Copy link
Contributor Author

vayan commented Jul 23, 2018

Changing to RS256 and sending through headers also requires some changes to the Flagr UI, but I guess you can put it in another pr.

What changes need to be done?

how are you going to use Flagr

I'm going to use Flagr using https://github.com/checkr/jsflagr on our backend and manual changes via the UI.

@zhouzhuojie zhouzhuojie merged commit a37ab86 into openflagr:master Jul 23, 2018
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.

3 participants