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

Allow RSA key used for JWT to be specified as a file path #6271

Merged
merged 1 commit into from
Jul 29, 2023

Conversation

eradman
Copy link
Collaborator

@eradman eradman commented Jul 24, 2023

  • auth_jwt_auth_public_certs_url may file:// in addition to http/https
  • Log an error if payload does not contain an email address

What type of PR is this?

  • Refactor
  • Feature

Description

How is this tested?

  • Unit tests (pytest, jest)

Background

This feature allows another application to craft a JWT token to automatically log into redash.

Formerly this was possible, but the RSA public key had to be on an HTTP server. A file path is less complex and easier to secure.

@codecov
Copy link

codecov bot commented Jul 24, 2023

Codecov Report

Merging #6271 (241ea41) into master (4a84738) will increase coverage by 0.28%.
The diff coverage is 75.86%.

@@            Coverage Diff             @@
##           master    #6271      +/-   ##
==========================================
+ Coverage   60.02%   60.31%   +0.28%     
==========================================
  Files         153      153              
  Lines       12494    12510      +16     
  Branches     1692     1694       +2     
==========================================
+ Hits         7500     7545      +45     
+ Misses       4781     4744      -37     
- Partials      213      221       +8     
Files Changed Coverage Δ
redash/authentication/__init__.py 81.34% <0.00%> (+5.03%) ⬆️
redash/authentication/jwt_auth.py 77.35% <84.61%> (+57.35%) ⬆️

@justinclift
Copy link
Member

The concept of the PR sounds useful.

Any idea how feasible it would be to get the Codecov test happy? 😄

@eradman
Copy link
Collaborator Author

eradman commented Jul 24, 2023

I'll see if I can contrive a test for http as well. This code does not have existing tests, but Codecov is complaining because I moved some of the logic around.

@justinclift
Copy link
Member

Cool, sounds like a plan. 😄

- auth_jwt_auth_public_certs_url may file:// in addition to http/https
- Log an error if payload does not contain an email address
@eradman
Copy link
Collaborator Author

eradman commented Jul 25, 2023

I didn't want to introduce another pip dev dependency, but JWK is difficult to craft by hand.

@eradman
Copy link
Collaborator Author

eradman commented Jul 25, 2023

@justinclift I'd say this change is ready to go

@justinclift
Copy link
Member

I didn't want to introduce another pip dev dependency, but JWK is difficult to craft by hand.

Sounds like a sensible compromise. 😄

@justinclift
Copy link
Member

@wlach @gaecoli @guidopetri Anyone interested in reviewing this PR? 😄

@gaecoli
Copy link
Member

gaecoli commented Jul 28, 2023

I have been using https, I hope to enable file and https/http options, otherwise there is no way to use it For old users.

@eradman eradman mentioned this pull request Jul 28, 2023
1 task
@eradman
Copy link
Collaborator Author

eradman commented Jul 28, 2023

I have been using https, I hope to enable file and https/http options, otherwise there is no way to use it For old users.

This PR should not change existing behavior, only add the ability to specify a private key from a file path

@gaecoli
Copy link
Member

gaecoli commented Jul 29, 2023

I have been using https, I hope to enable file and https/http options, otherwise there is no way to use it For old users.

This PR should not change existing behavior, only add the ability to specify a private key from a file path

Ok, LGTM!

@justinclift justinclift merged commit 0d69932 into getredash:master Jul 29, 2023
12 checks passed
@justinclift
Copy link
Member

Awesome. Just merged this, and the PR on the website repo which documents it a bit. 😄

@eradman eradman deleted the public-key-from-file branch July 29, 2023 11:12
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