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

Allow verification of JWT with JWKS key. #3

Merged
merged 1 commit into from Sep 8, 2020
Merged

Allow verification of JWT with JWKS key. #3

merged 1 commit into from Sep 8, 2020

Conversation

marcinkoziej
Copy link
Contributor

Hi, I have implemented an option to fetch JWKS public key to verify JWT tokens.

@marcinkoziej
Copy link
Contributor Author

This patch needs this: discourse/discourse-omniauth-jwt#3 to be merged

@eviltrout
Copy link
Contributor

This looks good, thanks.

@eviltrout eviltrout merged commit f74afcc into discourse:master Sep 8, 2020
davidtaylorhq added a commit that referenced this pull request Sep 16, 2020
@davidtaylorhq
Revert "Merge pull request #3 from fixthestatusquo/master"
0981204 
This reverts commit f74afcc, reversing
changes made to 4e790a5.

This change had some breaking changes
 - It changes the required_claims in the JWT
 - The `if SiteSetting.jwt_jwks_url` check should be `if SiteSetting.jwt_jwks_url.present?`, to account for empty strings (the default)
@davidtaylorhq
Copy link
Member

davidtaylorhq commented Sep 16, 2020 •
edited

@marcinkoziej I'm afraid I had to revert this change in 0981204 for a few reasons:

  • It changed the required_claims in the JWT

  • The if SiteSetting.jwt_jwks_url check should be if SiteSetting.jwt_jwks_url.present?, to account for empty strings (the default)

  • We need to cut a new version of discourse-omniauth-jwt, and bump the required version in this plugin

Ideally these things would have been picked up in automated regression testing. I realise this plugin doesn't have any tests at the moment, but it would be good to add some to ensure this added complexity doesn't cause any more regressions.

Please do feel free to re-open a PR with the changes again @marcinkoziej. We are happy to add the feature, but need to ensure the existing behaviour is unchanged

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@eviltrout eviltrout eviltrout approved these changes

Assignees
No one assigned
Labels
None yet
Milestone
No milestone
3 participants
@marcinkoziej @eviltrout @davidtaylorhq