User authentication should be done in middleware #369
Conversation
Codecov ReportAttention:
Additional details and impacted files@@ Coverage Diff @@
## develop #369 +/- ##
===========================================
+ Coverage 41.35% 41.42% +0.07%
===========================================
Files 80 81 +1
Lines 4996 5002 +6
===========================================
+ Hits 2066 2072 +6
Misses 2930 2930 ☔ View full report in Codecov by Sentry. |
mario-nt
force-pushed
the
39-user-authentication-should-be-done-in-middleware
branch
from
3d94826
to
2c74ebe
Compare
mario-nt
force-pushed
the
39-user-authentication-should-be-done-in-middleware
branch
from
2c74ebe
to
8d19a66
Compare
mario-nt
force-pushed
the
39-user-authentication-should-be-done-in-middleware
branch
from
8d19a66
to
84dd507
Compare
mario-nt
force-pushed
the
39-user-authentication-should-be-done-in-middleware
branch
from
84dd507
to
349b438
Compare
mario-nt
force-pushed
the
39-user-authentication-should-be-done-in-middleware
branch
from
ba0c25d
to
4696653
Compare
mario-nt
force-pushed
the
39-user-authentication-should-be-done-in-middleware
branch
from
4696653
to
52be4b0
Compare
mario-nt
force-pushed
the
39-user-authentication-should-be-done-in-middleware
branch
from
52be4b0
to
c384e58
Compare
mario-nt
force-pushed
the
39-user-authentication-should-be-done-in-middleware
branch
from
c384e58
to
684a922
Compare
mario-nt
force-pushed
the
39-user-authentication-should-be-done-in-middleware
branch
from
684a922
to
6d772dd
Compare
mario-nt
force-pushed
the
39-user-authentication-should-be-done-in-middleware
branch
from
6d772dd
to
c5b956e
Compare
mario-nt
force-pushed
the
39-user-authentication-should-be-done-in-middleware
branch
from
c5b956e
to
a6ad8cc
Compare
mario-nt
force-pushed
the
39-user-authentication-should-be-done-in-middleware
branch
from
a6ad8cc
to
30b646a
Compare
mario-nt
force-pushed
the
39-user-authentication-should-be-done-in-middleware
branch
from
7710f19
to
acb8610
Compare
|
@josecelano Could you please review the final refactor of the extractor and the upload torrent handler before I continue with the remaining handlers? |
There was a problem hiding this comment.
Hi @mario-nt it looks good to me. I've just added some comments regarding the other extractor when the logged-in user is optional.
See #39 (comment)
| @@ -37,20 +38,19 @@ use crate::web::api::v1::routes::API_VERSION_URL_PREFIX; | |||
| #[allow(clippy::unused_async)] | |||
| pub async fn upload_torrent_handler( | |||
| State(app_data): State<Arc<AppData>>, | |||
| Extract(maybe_bearer_token): Extract, | |||
| ExtractLoggedInUser(user_id): ExtractLoggedInUser, | |||
There was a problem hiding this comment.
Hi @mario-nt I think this is non-optional Acum extractor that returns an optional value. That's not the same as an optional extractor. See #39 (comment)
| match app_data | ||
| .torrent_service | ||
| .add_torrent(add_torrent_form, user_id.unwrap().value()) | ||
| .await | ||
| { |
There was a problem hiding this comment.
Hi @mario-nt you can't just unwrap because that would make the API panic. We need to return the unauthorized response. This endpoint requires a logged-in user. The whole point of using the extractor is to remove the logic from the handler. The extractor should return the unauthorized response if there is no logged-in user. So it user_id should be an UserId not an Option<UserId>.
This extractor would be used when you need a logged-in user. The Optional Extractor could be used in an endpoint that optionally can allow a logged-in user.
You can create two different extractors. See #39 (comment).
|
Parent issue #39 has been split into three new issues with their own PRs. Closing this PR. |
Resolves #39.