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.

Sign up for GitHub

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

Jump to bottom

Add JWKS support #43

Merged
merged 2 commits into from
May 9, 2019
Merged

Add JWKS support #43

merged 2 commits into from
May 9, 2019

Conversation

kalafut
Copy link
Contributor

@kalafut kalafut commented Apr 24, 2019

Closes #27

tyrannosaurus-becks
tyrannosaurus-becks reviewed Apr 26, 2019
View reviewed changes
Copy link
Contributor

@tyrannosaurus-becks tyrannosaurus-becks left a comment

Choose a reason for hiding this comment

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

This looks really good! Just a couple minor questions.

I do have one other question. I noticed that the RWMutex is protecting a lot of variables on the backend, minus the oidcStates because they're a thread-safe cache. And I bet this could be a pretty racy backend IRL, in that it could receive quite a lot of requests in parallel because of its nature.

Do we have any tests explicitly for detecting races? Maybe something like spinning up one backend, then doing a bunch of requests on it in parallel reading and mutating objects, that we could run go's -race flag on? Would it be worthwhile to do that for this one? Not necessarily in this iteration.

backend.go Outdated Show resolved Hide resolved
path_config.go Show resolved Hide resolved
path_oidc.go Show resolved Hide resolved
@kalafut
Copy link
Contributor Author

kalafut commented Apr 30, 2019

Running concurrency tests is a good idea and something I'm going to put in.

@kalafut
Copy link
Contributor Author

kalafut commented May 1, 2019

All comments should be addressed.

briankassouf
briankassouf approved these changes May 9, 2019
View reviewed changes
Copy link
Contributor

@briankassouf briankassouf left a comment

Choose a reason for hiding this comment

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

Looks great, just one small optional comment

path_login.go Show resolved Hide resolved
@kalafut kalafut merged commit f7b66ba into master May 9, 2019
@kalafut kalafut deleted the jwks branch May 9, 2019 20:30
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@briankassouf briankassouf briankassouf approved these changes

@tyrannosaurus-becks tyrannosaurus-becks tyrannosaurus-becks approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Dynamically update verification key with plain JWT
3 participants
@kalafut @tyrannosaurus-becks @briankassouf