Page MenuHomePhabricator

[reports-service] Obtain service-to-service token for anonymous requests
ClosedPublic

Authored by bartek on Sep 25 2023, 5:54 AM.
Tags
None
Referenced Files
F3553702: D9282.id31597.diff
Thu, Dec 26, 8:58 PM
Unknown Object (File)
Sat, Dec 21, 2:09 PM
Unknown Object (File)
Sat, Dec 21, 2:09 PM
Unknown Object (File)
Sat, Dec 21, 2:09 PM
Unknown Object (File)
Sat, Dec 21, 2:08 PM
Unknown Object (File)
Sat, Dec 21, 2:00 PM
Unknown Object (File)
Sat, Dec 7, 9:46 PM
Unknown Object (File)
Fri, Nov 29, 7:26 AM
Subscribers

Details

Summary

For reports service, we need to support both authenticated and anonymous requests. For authenticated requests, we can obtain the client token from the request context. For anonymous requests, we need to obtain the service-to-service token from the AuthService.

Depends on D9281

Test Plan

Used Postman to call reports service. When provided UserIdentity token, it passed it through to Blob service. When not provided, it obtained the service-to-service token and passed it through to Blob service.
When called with service-to-service token, it returned HTTP 403 - we don't expect reports service to be called by other services.

Diff Detail

Repository
rCOMM Comm
Lint
No Lint Coverage
Unit
No Test Coverage

Event Timeline

bartek held this revision as a draft.
bartek published this revision for review.Sep 25 2023, 6:40 AM
services/reports/src/service.rs
237–261 ↗(On Diff #31395)

Do you think it would make sense to move this into authorization middleware? So you can specify something like

.wrap(get_comm_authentication_middleware(AllowedAuth::user()))
// or
.wrap(get_comm_authentication_middleware(AllowedAuth::unauthorized())) // -> this would insert `AuthorizationCredential` with service token automatically
// or
.wrap(get_comm_authentication_middleware(AllowedAuth::services().user()))

I don't remember exactly what the API for the reports service is, but right now this diff would allow everyone to "authenticate" to the reports service by just not providing credentials (and get the service token by default). This should probably be scoped per (group of) endpoints.

services/reports/src/service.rs
237–261 ↗(On Diff #31395)

I played with various solutions a bit (spent a little too long on this).
Your suggestion makes sense in case of API, and looks like it is doable.

However, regarding having protected and unprotected endpoints:

right now this diff would allow everyone to "authenticate" to the reports service by just not providing credentials (and get the service token by default). This should probably be scoped per (group of) endpoints.

This is expected, we don't authenticate endpoints yet because we don't have client token.
However, this code works well with your existing auth middleware - endpoints wrapped with it will return 401 even before this code is executed.

so in the future, when we have client token, doing the following will work:

let auth_middleware = get_comm_authentication_middleware();

// this will require client token and fail if not provided
  web::scope("/authenticated")
    .wrap(auth_middleware)
    .service(...)

// this will accept both authenticated and anonymous requests.
// for the latter, service-to-service token will be obtained
  web::scope("/anonymous")
    // .wrap(auth_middleware) // no wrap
    .service(...)

To conclude, I might refactor to look more as you suggested, but it makes more sense to do it later when we have client token available (2nd part of my task that is currently blocked). I can create a follow up task for this if you wish

michal added inline comments.
services/reports/src/service.rs
237–261 ↗(On Diff #31395)

Makes sense, I got confused a bit

This revision is now accepted and ready to land.Oct 2 2023, 6:43 AM