Details

Reviewers

bartek
• jon
• patryk
kamil

Commits

rCOMM3bdda747deaf: [services-lib] Add authorization http middleware

Summary

ENG-4657

HTTP services need a way for clients to authenticate with them. This diff implements a middleware that will reject invalid requests and a UserIdentity extractor that is able to extract user identity from a correct request.

NOTE: actual checking if the access token is correct is skipped here, because the comm access token work isn't finished, so clients wouldn't be able to authorize anyway.

Test Plan

Created a resource:

web::resource("/hello")
  .route(
    web::get()
      .to(|user: UserIdentity| async move { format!("{user:?}") }),
  )
  .wrap(get_comm_authentication_middleware()),

Made a request without adding Authorization token -> got a 401 response with www-authenticate: Bearer header
Made a request with Authorization: Bearer eyJ1c2VySUQiOiAiMSIsICJhY2Nlc3NUb2tlbiI6ICIyIiwgInNpZ25pbmdQdWJsaWNLZXkiOiAiMyJ9 header
- got a 200 response with text: "UserIdentity { user_id: "1", access_token: "2", signing_public_key: "3" }"

Diff Detail

Repository

rCOMM Comm

Lint

No Lint Coverage

Unit

No Test Coverage

Event Timeline

michal created this revision.Aug 22 2023, 6:45 AM

Herald added subscribers: tomek, ashoat. · View Herald TranscriptAug 22 2023, 6:45 AM

michal added a parent revision: D8892: [services] Extract multipart handling logic to lib.Aug 22 2023, 6:46 AM

Harbormaster completed remote builds in B22008: Diff 30219.Aug 22 2023, 7:04 AM

michal requested review of this revision.Aug 22 2023, 7:04 AM

bartek added inline comments.Aug 22 2023, 12:08 PM

services/comm-services-lib/src/http/auth.rs
28–30 ↗	(On Diff #30219)	I don't really like that we discard error cause here - we should at least log it with `debug!`
43–49 ↗	(On Diff #30219)	Can we please save this parsed token into extensions? I'm relying on this in custom extractors. And repeating the code `UserIdentity::from_request()` doesn't really help here. This way, handlers and extractors can rely on existence of `Some(user_identity) = req.extensions().get::<UserIdentity>()` - if it's `Some` then it means authentication was successful. Alternatively, this can be done later after we implement the Identity Service call

bartek added inline comments.Aug 22 2023, 10:20 PM

services/comm-services-lib/src/http/auth.rs
22–25 ↗	(On Diff #30219)	Taking my below suggestion into account, this function could return already parsed `UserIdentity` from `extensions()` if exists and return early

michal added inline comments.Aug 23 2023, 12:23 AM

services/comm-services-lib/src/http/auth.rs
28–30 ↗	(On Diff #30219)	Good point, will fix
43–49 ↗	(On Diff #30219)	This way, handlers and extractors can rely on existence of Some(user_identity) = req.extensions().get::<UserIdentity>() - if it's Some then it means authentication was successful. Note that if authentication was unsuccessful the middleware is going to fail anyway and handlers and extractors won't be even run. So this: `Some(user_identity) = req.extensions().get::<UserIdentity>()` is equivalent to this: `Ok(user_identity) = UserIdentity::extract(req)` (or even `unwrap`ing it) If "optional authorization" is required for sth than sure I can update it. But then I would prefer to have two middlewares, one for optional and one for required so we don't need to repeat `if user.is_none() {return AuthorizationError}` at the beginning of every handler.

bartek added inline comments.Aug 23 2023, 12:49 AM

services/comm-services-lib/src/http/auth.rs
43–49 ↗	(On Diff #30219)	Yep, you're right, this will fail. But I'd still be happy to have this available in extension

bartek added inline comments.Aug 23 2023, 12:50 AM

services/comm-services-lib/src/auth.rs
16–24 ↗	(On Diff #30219)	up to you

• jon added inline comments.Aug 23 2023, 8:47 AM

services/comm-services-lib/src/auth.rs
22–23 ↗	(On Diff #30219)	for context on deviceID: https://linear.app/comm/issue/ENG-4072. `rename` should be used when directly mapping to another attribute. `alias` allows of the alias or the rust name to be used. Probably doesn't make a big difference if you're only deserializing, but if we do serialization, I think it will matter. https://serde.rs/field-attrs.html

Requesting changes to rename signingPublicKey to deviceID - see @jon's comment

services/comm-services-lib/src/auth.rs
22–23 ↗	(On Diff #30219)	Good to know, learned something new, thanks! Thanks for the context, I vote for `device_id` too

This revision now requires changes to proceed.Aug 23 2023, 12:31 PM

Respond to review

services/comm-services-lib/src/auth.rs
16–24 ↗	(On Diff #30219)	Now with `deviceID` all two fields need custom renames so I will leave it as is.
22–23 ↗	(On Diff #30219)	Thank you for linking ENG-4072, I wasn't aware of it! `device_id` is much clearer (at least for parts of the codebase that don't directly touch olm stuff). Agree with the `rename`, not sure where we would use serialization, but it's 99% going to be js so makes sense to also use camelCase