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

Make JWT authorization a middleware #1988

Closed
wants to merge 1 commit into from

Conversation

DavidLindbom
Copy link

I found myself switching the log level on my Postgres server a lot when needing to debug permissions because I wanted to see what role the query ran as. This PR moves the JWT parsing to a middleware such that it is now available for the logging middleware.

Example of new access log output (e.g the api_user value)

127.0.0.1 api_user - [23/Oct/2021:22:58:32 +0200] "GET /todos HTTP/1.1" 200 - "" "curl/7.64.1"

This follows the style of wai-middleware-auth package and makes the JWT claims available anywhere we have a Request value through the vault mechanism in Wai.

The existing JWT handling is not yet replaced as I wanted to get some feedback if this is interesting for anyone else.

@steve-chavez
Copy link
Member

@DavidLindbom Interesting! Seems vault could also help us in implementing #1578.

The existing JWT handling is not yet replaced as I wanted to get some feedback if this is interesting for anyone else.

Please do!

Once this PR is merged, we should update the log format in the docs.

@steve-chavez
Copy link
Member

127.0.0.1 api_user - [23/Oct/2021:22:58:32 +0200] "GET /todos HTTP/1.1" 200 - "" "curl/7.64.1"

To stick to the apache format, I think it should be like

127.0.0.1 - api_user [23/Oct/2021:22:58:32 +0200] "GET /todos HTTP/1.1" 200 - "" "curl/7.64.1"

This follows the style of wai-middleware-auth package and
makes the JWT parsing a middleware. With this we can add
the role into the access log for easier debugging and tracing.
@DavidLindbom
Copy link
Author

DavidLindbom commented Nov 14, 2021

Hello again,

I made some updates to remove the duplicated JWT parsing and now it should only use the new middleware.

There are a couple of not so nice parts you might want to look at

  1. The failure case in authMiddleware and how to propagate this to the logger middleware without running the full request function.
  2. Middleware.runPgLocals. I looked at it but quickly decided it was outside the scope of what I wanted to do. There are some simplifications that should be possible there.

@DavidLindbom DavidLindbom marked this pull request as ready for review November 14, 2021 16:42
@wolfgangwalther wolfgangwalther mentioned this pull request Dec 28, 2021
4 tasks
@wolfgangwalther
Copy link
Member

Now that #1724 is merged, the logger middleware has heavily changed. Therefore, I took this PR and rebased it on latest main. I can't figure out how to push to this branch, so I created #2115, instead. See my comment over there on what I did.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

3 participants