[comm-lib][reports] Extract 'authenticated' service trait
ClosedPublic
Actions

Authored by bartek on Jun 20 2024, 2:32 AM.

Details

Reviewers

kamil
varun
will

Commits

rCOMM141c4d1346de: [comm-lib][reports] Extract 'authenticated' service trait

Summary

This diff is large, but smaller parts make no sense without each other.

Introduced two entities:

HttpAuthenticatedService trait - to be implemented by services like ReportsService or BlobServiceClient. They do require either CSAM (UserIdentity) or a service-to-service token to work (e.g. make blob calls). But they can still be used in unauthenticated endpoints.
Authenticated<S> extractor. This is to be used in HTTP handler functions to get an instance of an aforementioned service - see D12508 for example usage.

The business logic code (FromRequest impl) was already implemented for Reports service. I extracted it into
the above trait and made more flexible. Added lots of in-code comments to better understand.

In the next diff, this will be implemented for Backup service - D12508.

Depends on D12506

Test Plan

Local reports service:

Upload report with valid CSAT - use it to get User ID and for blob service requests
Upload report with invalid CSAT - fallback to services-token
Upload report with no CSAT - service-to-service token was retrieved.

All reports were large enough to trigger Blob service uploads.

Diff Detail

Repository

rCOMM Comm

Lint

Lint Not Applicable

Unit

Tests Not Applicable

Event Timeline

bartek created this revision.Jun 20 2024, 2:32 AM

bartek held this revision as a draft.

Herald added subscribers: tomek, ashoat. · View Herald TranscriptJun 20 2024, 2:32 AM

bartek added a child revision: D12508: [backup] Use authenticated service trait.Jun 20 2024, 2:35 AM

bartek edited the summary of this revision. (Show Details)Jun 20 2024, 2:36 AM

Harbormaster completed remote builds in B29812: Diff 41545.Jun 20 2024, 2:48 AM

bartek published this revision for review.Jun 20 2024, 2:51 AM

bartek added inline comments.

services/reports/src/http/service.rs
19–21 ↗	(On Diff #41545)	For Reports service, we use: #[post("/")] async fn post_reports( payload: web::Json<PostReportsPayload>, service: ReportsService, ) -> actix_web::Result<HttpResponse> { // ... } Left this trait here, to be able to use directly `service: ReportsService` instead of having to replace it with `service: Authenticated<ReportsService>` But wondering if replacing it won't make the code simpler and more obvious than keeping this trait
shared/comm-lib/src/http/auth_service.rs
24 ↗	(On Diff #41545)	This should be implemented by `ReportsService`, `BlobServiceClient` etc - see diff description
51 ↗	(On Diff #41545)
94 ↗	(On Diff #41545)	This is basically moved from `impl FromRequest for ReportsService`. Just made it more flexible