Skip to content

Add support for Authorization request header#11

Merged
weavejester merged 1 commit intoring-clojure:masterfrom
devurandom:ds/authorization
Mar 1, 2024
Merged

Add support for Authorization request header#11
weavejester merged 1 commit intoring-clojure:masterfrom
devurandom:ds/authorization

Conversation

@devurandom
Copy link
Copy Markdown
Contributor

@devurandom devurandom commented Jan 28, 2024

No complete authentication scheme, but the "Access Authentication Framework"
defined by RFC 7235, providing the building blocks to implement auth schemes.

Raises dependency on ring-core to 1.8.1. ring.util.parsing/re-value was
added in 1.3, but until 1.8.1 it did not capture the content of quoted values
without the quotes. This drops support for Clojure 1.5 and 1.6, since 1.8.1
requires Clojure 1.7.

@devurandom devurandom marked this pull request as ready for review January 28, 2024 12:52
@devurandom devurandom changed the title Add support for Authorization header Add support for Authorization request header Jan 28, 2024
Copy link
Copy Markdown
Member

@weavejester weavejester left a comment

Choose a reason for hiding this comment

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

Thank you very much for the PR. I've reviewed the code, and it looks good overall, but I have some suggestions/alterations (mostly to do with the coding style guidelines Ring uses).

Comment thread README.md Outdated
Comment thread project.clj Outdated
Comment thread src/ring/middleware/authorization.clj Outdated
Comment thread src/ring/middleware/authorization.clj Outdated
Comment thread src/ring/middleware/authorization.clj Outdated
Comment thread src/ring/middleware/authorization.clj Outdated
Comment thread src/ring/middleware/authorization.clj Outdated
Comment thread src/ring/middleware/authorization.clj Outdated
Comment thread test/ring/middleware/authorization_test.clj Outdated
Comment thread test/ring/middleware/authorization_test.clj Outdated
Copy link
Copy Markdown
Member

@weavejester weavejester left a comment

Choose a reason for hiding this comment

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

Thanks very much for the updates. I have a few smaller changes/suggestions, but the only major thing left is to make the tests a little more robust.

Comment thread src/ring/middleware/authorization.clj Outdated
Comment thread src/ring/middleware/authorization.clj Outdated
Comment thread src/ring/middleware/authorization.clj Outdated
Comment thread src/ring/middleware/authorization.clj Outdated
Comment thread test/ring/middleware/authorization_test.clj Outdated
Comment thread test/ring/middleware/authorization_test.clj
Copy link
Copy Markdown
Member

@weavejester weavejester left a comment

Choose a reason for hiding this comment

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

Thanks for all your work! This all looks good aside from a couple of very minor formatting comments. Can you squash down your commits and give the result an appropriate commit message? Then I'll merge it in and cut a release.

Comment thread test/ring/middleware/authorization_test.clj Outdated
Comment thread src/ring/middleware/authorization.clj Outdated
@devurandom
Copy link
Copy Markdown
Contributor Author

Thanks for all your work!

You are welcome! Thanks for taking the time to review this!

Can you squash down your commits and give the result an appropriate commit message? Then I'll merge it in and cut a release.

Does fb74d88 work?

@weavejester
Copy link
Copy Markdown
Member

Almost! Git commit messages are typically wrapped at 72 characters, and at least in this repository they're plaintext rather than markdown, so backticks are unnecessary.

I also thought the first paragraph was a little hard to understand, so I rewrote it:

Add support for Authorization header

Add the wrap-authorization middleware for parsing the Authorization
header on the request map. This is not a complete authentication system;
it only parses the auth token and parameters. See RFC 7235 and RFC 9110:

* https://datatracker.ietf.org/doc/html/rfc7235
* https://datatracker.ietf.org/doc/html/rfc9110#section-11

This change raises the dependency on ring-core to 1.8.1, and drops
support for Clojure 1.5 and 1.6.

I also don't think we need to go into too much detail about why we're updating the Ring dependency. Updating dependencies is a natural part of updating a library.

Add the wrap-authorization middleware for parsing the Authorization
header on the request map. This is not a complete authentication system;
it only parses the auth token and parameters. See RFC 7235 and RFC 9110:

* https://datatracker.ietf.org/doc/html/rfc7235
* https://datatracker.ietf.org/doc/html/rfc9110#section-11

This change raises the dependency on ring-core to 1.8.1, and drops
support for Clojure 1.5 and 1.6.
@devurandom
Copy link
Copy Markdown
Contributor Author

I changed the commit message in 41af011.

@weavejester weavejester merged commit 050edcb into ring-clojure:master Mar 1, 2024
@devurandom devurandom deleted the ds/authorization branch March 1, 2024 21:22
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.

2 participants