Skip to content
This repository has been archived by the owner on Mar 18, 2022. It is now read-only.

add RequireClaim middleware for usage in consumer projects. #3

Merged
merged 3 commits into from
Sep 6, 2018

Conversation

trusch
Copy link
Contributor

@trusch trusch commented Sep 4, 2018

No description provided.

@coveralls
Copy link

coveralls commented Sep 4, 2018

Pull Request Test Coverage Report for Build 39

  • 40 of 40 (100.0%) changed or added relevant lines in 2 files are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage increased (+2.3%) to 100.0%

Totals Coverage Status
Change from base Build 31: 2.3%
Covered Lines: 166
Relevant Lines: 166

💛 - Coveralls

middleware.go Outdated

func RequireClaim(handler http.Handler, idpKey interface{}, header, claimKey, expectedClaimValue string) http.Handler {
return http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
_, claims, err := GetClaimsFromRequestWithValidation(r, idpKey)
Copy link
Member

Choose a reason for hiding this comment

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

Seems like this shouldn't validate the claims here, validation should happen in a separate middleware

Copy link
Contributor Author

Choose a reason for hiding this comment

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

A jwt is a signed json blob, so why shouldn't we validate the signature and expiry in the auth middleware, especially when it's just a 10 line middleware?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I refactored that so that we now extract and validate the token in a seperate middleware and store the claims in the requests context. With that in place it will be easy to continue working on the claims in http handlers further down the handler tree.

@LucasRoesler Ping :) This can be reviewed again.

@trusch trusch merged commit f5b3077 into master Sep 6, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants