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 RequireAuthorizationAsync #118

Merged
merged 6 commits into from
Sep 18, 2021
Merged

Add RequireAuthorizationAsync #118

merged 6 commits into from
Sep 18, 2021

Conversation

ranile
Copy link
Contributor

@ranile ranile commented Aug 9, 2021

Fixes #116

Motivation

In it's current state, it is impossible to make async (database) calls in AuthorizeRequest::authorize. Such calls are are very important in a real-world application where user data is stored in a database.

From #116

Solution

Add a new struct RequireAuthorizationAsync which uses AuthorizeRequestAsync. AuthorizeRequestAsync allows the user to return a Future.

sidred
sidred reviewed Aug 17, 2021
View reviewed changes
tower-http/src/auth/require_authorization_async.rs Outdated
fn on_authorized<B>(&mut self, _request: &mut Request<B>, _output: Self::Output) {}

/// Create the response for an unauthorized request.
fn unauthorized_response<B>(&mut self, request: &Request<B>) -> Response<Self::ResponseBody>;
Copy link

Choose a reason for hiding this comment

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

Should there be a way to pass the error from authorize function to this? The use case for that is, if there is a db or redis call in authorize function, then on network error we should be able to send a 5xx error instead of a 401 error.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I agree with you but IMO that's outside the scope of this PR. This PR adds async version of RequireAuthorization. Such a change would also need to made to RequireAuthorization in addition to RequireAuthorizationAsync.

Copy link
Member

Choose a reason for hiding this comment

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

I think mapping errors to responses should be handled by a separate middleware. So I think this is fine. We can always consider adding error handling to these middleware for 0.2, if someone requests it.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

...and that person will be me. I was using it and ran into a case where it's impossible to return 401/403 depending upon the request. We need a better way here.

I was gonna make an issue but life got in the way...

Copy link
Member

Choose a reason for hiding this comment

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

Hehe alright. Do you wanna address it here or should we move forward with this as is, and fix it for 0.2? I'm fine with either actually.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Let's fix it in some other PR. It's a breaking change. I don't have the time for it right now anyway.

@davidpdrsn
Copy link
Member

Just a quick heads up. I haven't forgotten about this. Things have just been busy with work and axum.

davidpdrsn
davidpdrsn approved these changes Sep 18, 2021
View reviewed changes
Copy link
Member

@davidpdrsn davidpdrsn left a comment

Choose a reason for hiding this comment

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

Sorry the long delay 😞

I think this look great! Just a couple of small comments.

tower-http/src/auth/require_authorization_async.rs Outdated Show resolved Hide resolved
tower-http/src/auth/require_authorization_async.rs Outdated Show resolved Hide resolved
tower-http/src/auth/require_authorization_async.rs Outdated Show resolved Hide resolved
tower-http/src/auth/require_authorization_async.rs Outdated Show resolved Hide resolved
tower-http/src/auth/require_authorization_async.rs Outdated Show resolved Hide resolved
tower-http/src/auth/require_authorization_async.rs Outdated Show resolved Hide resolved
tower-http/src/auth/require_authorization_async.rs Outdated Show resolved Hide resolved
tower-http/src/auth/require_authorization_async.rs Outdated Show resolved Hide resolved
tower-http/src/auth/require_authorization_async.rs Outdated
fn on_authorized<B>(&mut self, _request: &mut Request<B>, _output: Self::Output) {}

/// Create the response for an unauthorized request.
fn unauthorized_response<B>(&mut self, request: &Request<B>) -> Response<Self::ResponseBody>;
Copy link
Member

Choose a reason for hiding this comment

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

I think mapping errors to responses should be handled by a separate middleware. So I think this is fine. We can always consider adding error handling to these middleware for 0.2, if someone requests it.

davidpdrsn
davidpdrsn requested changes Sep 18, 2021
View reviewed changes
Copy link
Member

@davidpdrsn davidpdrsn left a comment

Choose a reason for hiding this comment

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

Sorry didn't mean to approve quite yet.

@ranile
Copy link
Contributor Author

ranile commented Sep 18, 2021

Fixed the CI errors, should be good to go.

@davidpdrsn
Copy link
Member

Hm it seems CI doesn't actually check the docs for broken links 🤔 I'll fix that separately.

@ranile @davidpdrsn
Apply suggestions from code review (part 3)
e7e157f 
Co-authored-by: David Pedersen <david.pdrsn@gmail.com>
@ranile
Copy link
Contributor Author

ranile commented Sep 18, 2021

Hm it seems CI doesn't actually check the docs for broken links thinking I'll fix that separately.

Fixed the links

@davidpdrsn davidpdrsn merged commit 773a781 into tower-rs:master Sep 18, 2021
@davidpdrsn
Copy link
Member

Thanks for working on this!

davidpdrsn added a commit that referenced this pull request Nov 13, 2021
@davidpdrsn
Version 0.1.2
8a1627a 
- New middleware: Add `Cors` for setting [CORS] headers ([#112])
- New middleware: Add `AsyncRequireAuthorization` ([#118])
- `Compression`: Don't recompress HTTP responses ([#140])
- `Compression` and `Decompression`: Pass configuration from layer into middleware ([#132])
- `ServeDir` and `ServeFile`: Improve performance ([#137])
- `Compression`: Remove needless `ResBody::Error: Into<BoxError>` bounds ([#117])
- `ServeDir`: Percent decode path segments ([#129])
- `ServeDir`: Use correct redirection status ([#130])
- `ServeDir`: Return `404 Not Found` on requests to directories if
  `append_index_html_on_directories` is set to `false` ([#122])

[#112]: #112
[#118]: #118
[#140]: #140
[#132]: #132
[#137]: #137
[#117]: #117
[#129]: #129
[#130]: #130
[#122]: #122
@davidpdrsn davidpdrsn mentioned this pull request Nov 13, 2021
Merged
davidpdrsn added a commit that referenced this pull request Nov 13, 2021
@davidpdrsn
Version 0.1.2
c3530e1 
- New middleware: Add `Cors` for setting [CORS] headers ([#112])
- New middleware: Add `AsyncRequireAuthorization` ([#118])
- `Compression`: Don't recompress HTTP responses ([#140])
- `Compression` and `Decompression`: Pass configuration from layer into middleware ([#132])
- `ServeDir` and `ServeFile`: Improve performance ([#137])
- `Compression`: Remove needless `ResBody::Error: Into<BoxError>` bounds ([#117])
- `ServeDir`: Percent decode path segments ([#129])
- `ServeDir`: Use correct redirection status ([#130])
- `ServeDir`: Return `404 Not Found` on requests to directories if
  `append_index_html_on_directories` is set to `false` ([#122])

[#112]: #112
[#118]: #118
[#140]: #140
[#132]: #132
[#137]: #137
[#117]: #117
[#129]: #129
[#130]: #130
[#122]: #122
davidpdrsn added a commit that referenced this pull request Nov 13, 2021
@davidpdrsn
Version 0.1.2
470477a 
- New middleware: Add `Cors` for setting [CORS] headers ([#112])
- New middleware: Add `AsyncRequireAuthorization` ([#118])
- `Compression`: Don't recompress HTTP responses ([#140])
- `Compression` and `Decompression`: Pass configuration from layer into middleware ([#132])
- `ServeDir` and `ServeFile`: Improve performance ([#137])
- `Compression`: Remove needless `ResBody::Error: Into<BoxError>` bounds ([#117])
- `ServeDir`: Percent decode path segments ([#129])
- `ServeDir`: Use correct redirection status ([#130])
- `ServeDir`: Return `404 Not Found` on requests to directories if
  `append_index_html_on_directories` is set to `false` ([#122])

[#112]: #112
[#118]: #118
[#140]: #140
[#132]: #132
[#137]: #137
[#117]: #117
[#129]: #129
[#130]: #130
[#122]: #122
davidpdrsn added a commit that referenced this pull request Nov 13, 2021
@davidpdrsn
Version 0.1.2 (#162)
2c110d2 
- New middleware: Add `Cors` for setting [CORS] headers ([#112])
- New middleware: Add `AsyncRequireAuthorization` ([#118])
- `Compression`: Don't recompress HTTP responses ([#140])
- `Compression` and `Decompression`: Pass configuration from layer into middleware ([#132])
- `ServeDir` and `ServeFile`: Improve performance ([#137])
- `Compression`: Remove needless `ResBody::Error: Into<BoxError>` bounds ([#117])
- `ServeDir`: Percent decode path segments ([#129])
- `ServeDir`: Use correct redirection status ([#130])
- `ServeDir`: Return `404 Not Found` on requests to directories if
  `append_index_html_on_directories` is set to `false` ([#122])

[#112]: #112
[#118]: #118
[#140]: #140
[#132]: #132
[#137]: #137
[#117]: #117
[#129]: #129
[#130]: #130
[#122]: #122
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@sidred sidred sidred left review comments

@davidpdrsn davidpdrsn davidpdrsn 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.

Make AuthorizeRequest::authorize return a Future
3 participants
@ranile @davidpdrsn @sidred