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

JWT package "interface" #77

Merged
merged 1 commit into from
Feb 26, 2021
Merged

JWT package "interface" #77

merged 1 commit into from
Feb 26, 2021

Conversation

grounded042
Copy link
Contributor

@grounded042 grounded042 commented Feb 19, 2021

Over in #73 (comment) I mentioned breaking JWT package functionality out into an interface. This is a first pass at that which ended up putting things into a type function instead of an interface. I'm suggesting we merge this into v2 as it's pre-release and if we need to iterate on the design we can.

@grounded042 grounded042 changed the title Spike on JWT "interface" Spike on JWT package "interface" Feb 19, 2021
Copy link

@cyx cyx left a comment

Choose a reason for hiding this comment

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

Seems good. Can we put the relevant "adapters" in a separate package? that way folks can have a drop in solution.

(Or a different repo, TL;DR just somewhere)

The only thing I'd consider is looking at what gets imported with/without the separate package. I think it's fine adding more deps, just something to consider.

@grounded042
Copy link
Contributor Author

Totally - separate package would be great.

// Default value: nil
ValidationKeyGetter jwt.Keyfunc
// Validate handles validating a token.
Validate ValidateToken

Choose a reason for hiding this comment

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

Awesome! So an example of an impl where jwx was the package choose would roughly look like:

func ValidateWithJwx(token string) (interface{}, error) {
	parsedToken, err := jwt.ParseString(token, jwt.WithVerify(m.Options.SigningMethod, []byte(m.Options.Key)))
	if err != nil {
		return nil, err
	}

	msg, err := jws.ParseString(token)

	if err != nil {
		return nil, err
	}

	signatures := msg.Signatures()

	if len(signatures) == 0 {
		return nil, errors.New("No signatures where found with the token")
	}

	algorithm := signatures[0].ProtectedHeaders().Algorithm()

	if m.Options.SigningMethod != algorithm {
		return nil, errors.New("Problems found with signing alg")
	}

	valid := m.Options.ValidatorFunc(parsedToken)

	// Check if the parsed token is valid...
	if !valid {
		return nil, errors.New("Token is invalid")
	}

	return parsedToken, nil
}

Itd definitely be better than that as the default, more like what is over on my PoC branch, but just throwing out an example for understanding sake.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yup, and we could even add in some options to that setup.

Choose a reason for hiding this comment

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

Nice! I like it.

// Default value:
// The function that will be called when there are errors in the
// middleware.
// Default value: OnError
ErrorHandler errorHandler

Choose a reason for hiding this comment

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

with this abstraction, does this mean the only thing this errorHandler would really get called for is if the token cant be found on the header, or if the token is an empty string?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It would be called if the Validate function errors.

@grounded042 grounded042 changed the title Spike on JWT package "interface" JWT package "interface" Feb 26, 2021
@grounded042 grounded042 changed the base branch from master to v2 February 26, 2021 22:56
@grounded042 grounded042 marked this pull request as ready for review February 26, 2021 22: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.

None yet

3 participants