Skip to content

Add RequireAuthorizationAsync #118

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
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
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
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
davidpdrsn reviewed Sep 18, 2021
View reviewed changes
@davidpdrsn
Copy link
Member

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

@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

@davidpdrsn davidpdrsn davidpdrsn approved these changes

+1 more reviewer

@sidred sidred sidred left review comments

Reviewers whose approvals may not affect merge requirements

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