Page MenuHomePhabricator
Differential D11008

[web] Replace ashoatKeyserverID with authoritativeKeyserverID
ClosedPublic
Actions

Authored by inka on Feb 9 2024, 2:16 AM.
Tags
None
Referenced Files
F6270711: D11008.id37349.diff
Fri, Apr 25, 1:16 PM
Unknown Object (File)
Fri, Apr 18, 6:55 AM
Unknown Object (File)
Thu, Apr 17, 11:18 AM
Unknown Object (File)
Thu, Apr 17, 8:01 AM
Unknown Object (File)
Thu, Apr 17, 6:44 AM
Unknown Object (File)
Thu, Apr 17, 4:33 AM
Unknown Object (File)
Thu, Apr 17, 2:09 AM
Unknown Object (File)
Thu, Apr 17, 2:09 AM
View All Files
Subscribers
ashoat

Details

Reviewers
michal
tomek
Commits
rCOMMa71b22170f0a: [web] Replace ashoatKeyserverID with authoritativeKeyserverID
Summary

issue: ENG-6635

Test Plan

Tested that it is possible to send messages, recieve them, create entries, log out, log in, sent media and no errors show up

Diff Detail

Repository
rCOMM Comm
Lint
No Lint Coverage
Unit
No Test Coverage

Event Timeline

inka created this revision.Feb 9 2024, 2:16 AM
Herald added a subscriber: ashoat. · View Herald TranscriptFeb 9 2024, 2:16 AM
inka added a parent revision: D11007: [native] Replace ashoatKeyserverID with authoritativeKeyserverID.Feb 9 2024, 2:17 AM
Harbormaster returned this revision to the author for changes because remote builds failed.Feb 9 2024, 2:26 AM
Harbormaster failed remote builds in B26686: Diff 36900!
inka updated this revision to Diff 36931.Feb 9 2024, 7:23 AM

Rebase

Harbormaster completed remote builds in B26708: Diff 36931.Feb 9 2024, 7:38 AM
inka requested review of this revision.Feb 9 2024, 7:38 AM
inka added a child revision: D11011: [lib] Update genesis.id to use authoritativeKeyserverID.Feb 9 2024, 7:52 AM
tomek accepted this revision.Feb 12 2024, 1:18 AM
This revision is now accepted and ready to land.Feb 12 2024, 1:18 AM
inka updated this revision to Diff 36985.Feb 12 2024, 1:54 AM

Rebase - remove changes from Socket, because D11013 replaced ashoatKeyserverID with props.keyserverID

Harbormaster failed remote builds in B26747: Diff 36985!Feb 12 2024, 2:01 AM
Harbormaster completed remote builds in B26747: Diff 36985.Feb 12 2024, 2:29 AM
inka updated this revision to Diff 37349.Feb 20 2024, 4:28 AM

Use value from web - we dno't need to get the value from config, we can use the value from the env variable. This will let us use authoritativeKeyserverID toplevel on web

inka requested review of this revision.Feb 20 2024, 4:28 AM
Harbormaster completed remote builds in B26989: Diff 37349.Feb 20 2024, 4:43 AM
tomek accepted this revision.Feb 21 2024, 1:34 AM
This revision is now accepted and ready to land.Feb 21 2024, 1:34 AM
Closed by commit rCOMMa71b22170f0a: [web] Replace ashoatKeyserverID with authoritativeKeyserverID (authored by inka). · Explain WhyFeb 23 2024, 4:35 AM
This revision was automatically updated to reflect the committed changes.
inka added a commit: rCOMMa71b22170f0a: [web] Replace ashoatKeyserverID with authoritativeKeyserverID.

Revision Contents
Changeset List

PathSize
web/
avatars/
4 lines
calendar/
6 lines
push-notif/
4 lines
4 lines
4 lines
redux/
6 lines
6 lines
7 lines
14 lines

Diff 37349

View Options

web/avatars/avatar-hooks.react.js

use actix_web::FromRequest; use actix_web::FromRequest;
use chrono::Utc; use chrono::Utc;
use comm_services_lib::{ use comm_services_lib::{
auth::UserIdentity, auth::{AuthService, AuthorizationCredential},
blob::client::{BlobServiceClient, BlobServiceError}, blob::client::{BlobServiceClient, BlobServiceError},
crypto::aes256, crypto::aes256,
database, database,
}; };
use derive_more::{Display, Error, From}; use derive_more::{Display, Error, From};
use std::{ use std::{
collections::HashMap, collections::HashMap,
future::{ready, Ready}, future::{ready, Future},
pin::Pin,
sync::Arc, sync::Arc,
}; };
use tracing::{error, trace}; use tracing::{error, trace, warn};
use crate::{ use crate::{
config::CONFIG, config::CONFIG,
database::{ database::{
client::{DatabaseClient, ReportsPage}, client::{DatabaseClient, ReportsPage},
item::{ReportContent, ReportItem}, item::{ReportContent, ReportItem},
}, },
email::{config::EmailConfig, ReportEmail}, email::{config::EmailConfig, ReportEmail},
▲ Show 20 Lines • Show All 42 Lines • ▼ Show 20 Lines ) -> Self {
Self { Self {
db, db,
blob_client, blob_client,
requesting_user_id: None, requesting_user_id: None,
email_config: email_config.map(Arc::new), email_config: email_config.map(Arc::new),
} }
} }
pub fn authenticated(&self, user: UserIdentity) -> Self { /// Clones the service with a new auth identity. When the credential is
let user_id = user.user_id.to_string(); /// a service-to-service, the `user_id` is None.
pub fn with_authentication(&self, token: AuthorizationCredential) -> Self {
let requesting_user_id = match &token {
AuthorizationCredential::ServicesToken(_) => None,
AuthorizationCredential::UserToken(user) => {
Some(user.user_id.to_string())
}
};
Self { Self {
db: self.db.clone(), db: self.db.clone(),
email_config: self.email_config.clone(), email_config: self.email_config.clone(),
blob_client: self.blob_client.with_user_identity(user), blob_client: self.blob_client.with_authentication(token),
requesting_user_id: Some(user_id), requesting_user_id,
} }
} }
pub async fn save_reports( pub async fn save_reports(
&self, &self,
inputs: Vec<ReportInput>, inputs: Vec<ReportInput>,
) -> ServiceResult<Vec<ReportID>> { ) -> ServiceResult<Vec<ReportID>> {
let mut reports = Vec::with_capacity(inputs.len()); let mut reports = Vec::with_capacity(inputs.len());
▲ Show 20 Lines • Show All 101 Lines • ▼ Show 20 Linesimpl ReportsService {
) -> ServiceResult<ReportsPage> { ) -> ServiceResult<ReportsPage> {
let page = self.db.scan_reports(cursor, page_size).await?; let page = self.db.scan_reports(cursor, page_size).await?;
Ok(page) Ok(page)
} }
} }
impl FromRequest for ReportsService { impl FromRequest for ReportsService {
type Error = actix_web::Error; type Error = actix_web::Error;
type Future = Ready<Result<Self, actix_web::Error>>; type Future = Pin<Box<dyn Future<Output = Result<Self, actix_web::Error>>>>;
#[inline] #[inline]
fn from_request( fn from_request(
req: &actix_web::HttpRequest, req: &actix_web::HttpRequest,
_payload: &mut actix_web::dev::Payload, _payload: &mut actix_web::dev::Payload,
) -> Self::Future { ) -> Self::Future {
use actix_web::error::{ErrorForbidden, ErrorInternalServerError};
use actix_web::HttpMessage; use actix_web::HttpMessage;
let Some(service) = req.app_data::<ReportsService>() else { let base_service =
req.app_data::<ReportsService>().cloned().ok_or_else(|| {
tracing::error!( tracing::error!(
"FATAL! Failed to extract ReportsService from actix app_data. \ "FATAL! Failed to extract ReportsService from actix app_data. \
Check HTTP server configuration" Check HTTP server configuration"
); );
return ready(Err(actix_web::error::ErrorInternalServerError("Internal server error"))); ErrorInternalServerError("Internal server error")
}; });
let auth_service = let auth_service =
if let Some(user_identity) = req.extensions().get::<UserIdentity>() { req.app_data::<AuthService>().cloned().ok_or_else(|| {
tracing::trace!("Found user identity. Creating authenticated service"); tracing::error!(
service.authenticated(user_identity.clone()) "FATAL! Failed to extract AuthService from actix app_data. \
} else { Check HTTP server configuration"
tracing::trace!(
"No user identity found. Leaving unauthenticated service"
); );
service.clone() ErrorInternalServerError("Internal server error")
}; });
let request_auth_value =
req.extensions().get::<AuthorizationCredential>().cloned();
ready(Ok(auth_service)) Box::pin(async move {
let auth_service = auth_service?;
let base_service = base_service?;
// This is Some for endpoints hidden behind auth validation middleware
let auth_token = match request_auth_value {
Some(token @ AuthorizationCredential::UserToken(_)) => token,
Some(_) => {
// Reports service shouldn't be called by other services
warn!("Reports service requires user authorization");
return Err(ErrorForbidden("Forbidden"));
}
None => {
// Unauthenticated requests get a service-to-service token
let services_token =
auth_service.get_services_token().await.map_err(|err| {
error!("Failed to get services token: {err}");
ErrorInternalServerError("Internal server error")
})?;
AuthorizationCredential::ServicesToken(services_token)
}
};
let service = base_service.with_authentication(auth_token);
Ok(service)
})
michalUnsubmitted
Not Done
Inline Actions

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.

michal: Do you think it would make sense to move this into authorization middleware? So you can specify…
bartekAuthorUnsubmitted
Done
Inline Actions

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

bartek: I played with various solutions a bit (spent a little too long on this). Your suggestion makes…
michalUnsubmitted
Not Done
Inline Actions

Makes sense, I got confused a bit

michal: Makes sense, I got confused a bit
} }
} }
struct ProcessedReport { struct ProcessedReport {
id: ReportID, id: ReportID,
db_item: ReportItem, db_item: ReportItem,
email: ReportEmail, email: ReportEmail,
} }
▲ Show 20 Lines • Show All 78 Lines • Show Last 20 Lines
View Options

web/calendar/entry.react.js

use actix_web::FromRequest; use actix_web::FromRequest;
use chrono::Utc; use chrono::Utc;
use comm_services_lib::{ use comm_services_lib::{
auth::UserIdentity, auth::{AuthService, AuthorizationCredential},
blob::client::{BlobServiceClient, BlobServiceError}, blob::client::{BlobServiceClient, BlobServiceError},
crypto::aes256, crypto::aes256,
database, database,
}; };
use derive_more::{Display, Error, From}; use derive_more::{Display, Error, From};
use std::{ use std::{
collections::HashMap, collections::HashMap,
future::{ready, Ready}, future::{ready, Future},
pin::Pin,
sync::Arc, sync::Arc,
}; };
use tracing::{error, trace}; use tracing::{error, trace, warn};
use crate::{ use crate::{
config::CONFIG, config::CONFIG,
database::{ database::{
client::{DatabaseClient, ReportsPage}, client::{DatabaseClient, ReportsPage},
item::{ReportContent, ReportItem}, item::{ReportContent, ReportItem},
}, },
email::{config::EmailConfig, ReportEmail}, email::{config::EmailConfig, ReportEmail},
▲ Show 20 Lines • Show All 42 Lines • ▼ Show 20 Lines ) -> Self {
Self { Self {
db, db,
blob_client, blob_client,
requesting_user_id: None, requesting_user_id: None,
email_config: email_config.map(Arc::new), email_config: email_config.map(Arc::new),
} }
} }
pub fn authenticated(&self, user: UserIdentity) -> Self { /// Clones the service with a new auth identity. When the credential is
let user_id = user.user_id.to_string(); /// a service-to-service, the `user_id` is None.
pub fn with_authentication(&self, token: AuthorizationCredential) -> Self {
let requesting_user_id = match &token {
AuthorizationCredential::ServicesToken(_) => None,
AuthorizationCredential::UserToken(user) => {
Some(user.user_id.to_string())
}
};
Self { Self {
db: self.db.clone(), db: self.db.clone(),
email_config: self.email_config.clone(), email_config: self.email_config.clone(),
blob_client: self.blob_client.with_user_identity(user), blob_client: self.blob_client.with_authentication(token),
requesting_user_id: Some(user_id), requesting_user_id,
} }
} }
pub async fn save_reports( pub async fn save_reports(
&self, &self,
inputs: Vec<ReportInput>, inputs: Vec<ReportInput>,
) -> ServiceResult<Vec<ReportID>> { ) -> ServiceResult<Vec<ReportID>> {
let mut reports = Vec::with_capacity(inputs.len()); let mut reports = Vec::with_capacity(inputs.len());
▲ Show 20 Lines • Show All 101 Lines • ▼ Show 20 Linesimpl ReportsService {
) -> ServiceResult<ReportsPage> { ) -> ServiceResult<ReportsPage> {
let page = self.db.scan_reports(cursor, page_size).await?; let page = self.db.scan_reports(cursor, page_size).await?;
Ok(page) Ok(page)
} }
} }
impl FromRequest for ReportsService { impl FromRequest for ReportsService {
type Error = actix_web::Error; type Error = actix_web::Error;
type Future = Ready<Result<Self, actix_web::Error>>; type Future = Pin<Box<dyn Future<Output = Result<Self, actix_web::Error>>>>;
#[inline] #[inline]
fn from_request( fn from_request(
req: &actix_web::HttpRequest, req: &actix_web::HttpRequest,
_payload: &mut actix_web::dev::Payload, _payload: &mut actix_web::dev::Payload,
) -> Self::Future { ) -> Self::Future {
use actix_web::error::{ErrorForbidden, ErrorInternalServerError};
use actix_web::HttpMessage; use actix_web::HttpMessage;
let Some(service) = req.app_data::<ReportsService>() else { let base_service =
req.app_data::<ReportsService>().cloned().ok_or_else(|| {
tracing::error!( tracing::error!(
"FATAL! Failed to extract ReportsService from actix app_data. \ "FATAL! Failed to extract ReportsService from actix app_data. \
Check HTTP server configuration" Check HTTP server configuration"
); );
return ready(Err(actix_web::error::ErrorInternalServerError("Internal server error"))); ErrorInternalServerError("Internal server error")
}; });
let auth_service = let auth_service =
if let Some(user_identity) = req.extensions().get::<UserIdentity>() { req.app_data::<AuthService>().cloned().ok_or_else(|| {
tracing::trace!("Found user identity. Creating authenticated service"); tracing::error!(
service.authenticated(user_identity.clone()) "FATAL! Failed to extract AuthService from actix app_data. \
} else { Check HTTP server configuration"
tracing::trace!(
"No user identity found. Leaving unauthenticated service"
); );
service.clone() ErrorInternalServerError("Internal server error")
}; });
let request_auth_value =
req.extensions().get::<AuthorizationCredential>().cloned();
ready(Ok(auth_service)) Box::pin(async move {
let auth_service = auth_service?;
let base_service = base_service?;
// This is Some for endpoints hidden behind auth validation middleware
let auth_token = match request_auth_value {
Some(token @ AuthorizationCredential::UserToken(_)) => token,
Some(_) => {
// Reports service shouldn't be called by other services
warn!("Reports service requires user authorization");
return Err(ErrorForbidden("Forbidden"));
}
None => {
// Unauthenticated requests get a service-to-service token
let services_token =
auth_service.get_services_token().await.map_err(|err| {
error!("Failed to get services token: {err}");
ErrorInternalServerError("Internal server error")
})?;
AuthorizationCredential::ServicesToken(services_token)
}
};
let service = base_service.with_authentication(auth_token);
Ok(service)
})
michalUnsubmitted
Not Done
Inline Actions

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.

michal: Do you think it would make sense to move this into authorization middleware? So you can specify…
bartekAuthorUnsubmitted
Done
Inline Actions

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

bartek: I played with various solutions a bit (spent a little too long on this). Your suggestion makes…
michalUnsubmitted
Not Done
Inline Actions

Makes sense, I got confused a bit

michal: Makes sense, I got confused a bit
} }
} }
struct ProcessedReport { struct ProcessedReport {
id: ReportID, id: ReportID,
db_item: ReportItem, db_item: ReportItem,
email: ReportEmail, email: ReportEmail,
} }
▲ Show 20 Lines • Show All 78 Lines • Show Last 20 Lines
View Options

web/push-notif/badge-handler.react.js

use actix_web::FromRequest; use actix_web::FromRequest;
use chrono::Utc; use chrono::Utc;
use comm_services_lib::{ use comm_services_lib::{
auth::UserIdentity, auth::{AuthService, AuthorizationCredential},
blob::client::{BlobServiceClient, BlobServiceError}, blob::client::{BlobServiceClient, BlobServiceError},
crypto::aes256, crypto::aes256,
database, database,
}; };
use derive_more::{Display, Error, From}; use derive_more::{Display, Error, From};
use std::{ use std::{
collections::HashMap, collections::HashMap,
future::{ready, Ready}, future::{ready, Future},
pin::Pin,
sync::Arc, sync::Arc,
}; };
use tracing::{error, trace}; use tracing::{error, trace, warn};
use crate::{ use crate::{
config::CONFIG, config::CONFIG,
database::{ database::{
client::{DatabaseClient, ReportsPage}, client::{DatabaseClient, ReportsPage},
item::{ReportContent, ReportItem}, item::{ReportContent, ReportItem},
}, },
email::{config::EmailConfig, ReportEmail}, email::{config::EmailConfig, ReportEmail},
▲ Show 20 Lines • Show All 42 Lines • ▼ Show 20 Lines ) -> Self {
Self { Self {
db, db,
blob_client, blob_client,
requesting_user_id: None, requesting_user_id: None,
email_config: email_config.map(Arc::new), email_config: email_config.map(Arc::new),
} }
} }
pub fn authenticated(&self, user: UserIdentity) -> Self { /// Clones the service with a new auth identity. When the credential is
let user_id = user.user_id.to_string(); /// a service-to-service, the `user_id` is None.
pub fn with_authentication(&self, token: AuthorizationCredential) -> Self {
let requesting_user_id = match &token {
AuthorizationCredential::ServicesToken(_) => None,
AuthorizationCredential::UserToken(user) => {
Some(user.user_id.to_string())
}
};
Self { Self {
db: self.db.clone(), db: self.db.clone(),
email_config: self.email_config.clone(), email_config: self.email_config.clone(),
blob_client: self.blob_client.with_user_identity(user), blob_client: self.blob_client.with_authentication(token),
requesting_user_id: Some(user_id), requesting_user_id,
} }
} }
pub async fn save_reports( pub async fn save_reports(
&self, &self,
inputs: Vec<ReportInput>, inputs: Vec<ReportInput>,
) -> ServiceResult<Vec<ReportID>> { ) -> ServiceResult<Vec<ReportID>> {
let mut reports = Vec::with_capacity(inputs.len()); let mut reports = Vec::with_capacity(inputs.len());
▲ Show 20 Lines • Show All 101 Lines • ▼ Show 20 Linesimpl ReportsService {
) -> ServiceResult<ReportsPage> { ) -> ServiceResult<ReportsPage> {
let page = self.db.scan_reports(cursor, page_size).await?; let page = self.db.scan_reports(cursor, page_size).await?;
Ok(page) Ok(page)
} }
} }
impl FromRequest for ReportsService { impl FromRequest for ReportsService {
type Error = actix_web::Error; type Error = actix_web::Error;
type Future = Ready<Result<Self, actix_web::Error>>; type Future = Pin<Box<dyn Future<Output = Result<Self, actix_web::Error>>>>;
#[inline] #[inline]
fn from_request( fn from_request(
req: &actix_web::HttpRequest, req: &actix_web::HttpRequest,
_payload: &mut actix_web::dev::Payload, _payload: &mut actix_web::dev::Payload,
) -> Self::Future { ) -> Self::Future {
use actix_web::error::{ErrorForbidden, ErrorInternalServerError};
use actix_web::HttpMessage; use actix_web::HttpMessage;
let Some(service) = req.app_data::<ReportsService>() else { let base_service =
req.app_data::<ReportsService>().cloned().ok_or_else(|| {
tracing::error!( tracing::error!(
"FATAL! Failed to extract ReportsService from actix app_data. \ "FATAL! Failed to extract ReportsService from actix app_data. \
Check HTTP server configuration" Check HTTP server configuration"
); );
return ready(Err(actix_web::error::ErrorInternalServerError("Internal server error"))); ErrorInternalServerError("Internal server error")
}; });
let auth_service = let auth_service =
if let Some(user_identity) = req.extensions().get::<UserIdentity>() { req.app_data::<AuthService>().cloned().ok_or_else(|| {
tracing::trace!("Found user identity. Creating authenticated service"); tracing::error!(
service.authenticated(user_identity.clone()) "FATAL! Failed to extract AuthService from actix app_data. \
} else { Check HTTP server configuration"
tracing::trace!(
"No user identity found. Leaving unauthenticated service"
); );
service.clone() ErrorInternalServerError("Internal server error")
}; });
let request_auth_value =
req.extensions().get::<AuthorizationCredential>().cloned();
ready(Ok(auth_service)) Box::pin(async move {
let auth_service = auth_service?;
let base_service = base_service?;
// This is Some for endpoints hidden behind auth validation middleware
let auth_token = match request_auth_value {
Some(token @ AuthorizationCredential::UserToken(_)) => token,
Some(_) => {
// Reports service shouldn't be called by other services
warn!("Reports service requires user authorization");
return Err(ErrorForbidden("Forbidden"));
}
None => {
// Unauthenticated requests get a service-to-service token
let services_token =
auth_service.get_services_token().await.map_err(|err| {
error!("Failed to get services token: {err}");
ErrorInternalServerError("Internal server error")
})?;
AuthorizationCredential::ServicesToken(services_token)
}
};
let service = base_service.with_authentication(auth_token);
Ok(service)
})
michalUnsubmitted
Not Done
Inline Actions

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.

michal: Do you think it would make sense to move this into authorization middleware? So you can specify…
bartekAuthorUnsubmitted
Done
Inline Actions

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

bartek: I played with various solutions a bit (spent a little too long on this). Your suggestion makes…
michalUnsubmitted
Not Done
Inline Actions

Makes sense, I got confused a bit

michal: Makes sense, I got confused a bit
} }
} }
struct ProcessedReport { struct ProcessedReport {
id: ReportID, id: ReportID,
db_item: ReportItem, db_item: ReportItem,
email: ReportEmail, email: ReportEmail,
} }
▲ Show 20 Lines • Show All 78 Lines • Show Last 20 Lines
View Options

web/push-notif/push-notifs-handler.js

use actix_web::FromRequest; use actix_web::FromRequest;
use chrono::Utc; use chrono::Utc;
use comm_services_lib::{ use comm_services_lib::{
auth::UserIdentity, auth::{AuthService, AuthorizationCredential},
blob::client::{BlobServiceClient, BlobServiceError}, blob::client::{BlobServiceClient, BlobServiceError},
crypto::aes256, crypto::aes256,
database, database,
}; };
use derive_more::{Display, Error, From}; use derive_more::{Display, Error, From};
use std::{ use std::{
collections::HashMap, collections::HashMap,
future::{ready, Ready}, future::{ready, Future},
pin::Pin,
sync::Arc, sync::Arc,
}; };
use tracing::{error, trace}; use tracing::{error, trace, warn};
use crate::{ use crate::{
config::CONFIG, config::CONFIG,
database::{ database::{
client::{DatabaseClient, ReportsPage}, client::{DatabaseClient, ReportsPage},
item::{ReportContent, ReportItem}, item::{ReportContent, ReportItem},
}, },
email::{config::EmailConfig, ReportEmail}, email::{config::EmailConfig, ReportEmail},
▲ Show 20 Lines • Show All 42 Lines • ▼ Show 20 Lines ) -> Self {
Self { Self {
db, db,
blob_client, blob_client,
requesting_user_id: None, requesting_user_id: None,
email_config: email_config.map(Arc::new), email_config: email_config.map(Arc::new),
} }
} }
pub fn authenticated(&self, user: UserIdentity) -> Self { /// Clones the service with a new auth identity. When the credential is
let user_id = user.user_id.to_string(); /// a service-to-service, the `user_id` is None.
pub fn with_authentication(&self, token: AuthorizationCredential) -> Self {
let requesting_user_id = match &token {
AuthorizationCredential::ServicesToken(_) => None,
AuthorizationCredential::UserToken(user) => {
Some(user.user_id.to_string())
}
};
Self { Self {
db: self.db.clone(), db: self.db.clone(),
email_config: self.email_config.clone(), email_config: self.email_config.clone(),
blob_client: self.blob_client.with_user_identity(user), blob_client: self.blob_client.with_authentication(token),
requesting_user_id: Some(user_id), requesting_user_id,
} }
} }
pub async fn save_reports( pub async fn save_reports(
&self, &self,
inputs: Vec<ReportInput>, inputs: Vec<ReportInput>,
) -> ServiceResult<Vec<ReportID>> { ) -> ServiceResult<Vec<ReportID>> {
let mut reports = Vec::with_capacity(inputs.len()); let mut reports = Vec::with_capacity(inputs.len());
▲ Show 20 Lines • Show All 101 Lines • ▼ Show 20 Linesimpl ReportsService {
) -> ServiceResult<ReportsPage> { ) -> ServiceResult<ReportsPage> {
let page = self.db.scan_reports(cursor, page_size).await?; let page = self.db.scan_reports(cursor, page_size).await?;
Ok(page) Ok(page)
} }
} }
impl FromRequest for ReportsService { impl FromRequest for ReportsService {
type Error = actix_web::Error; type Error = actix_web::Error;
type Future = Ready<Result<Self, actix_web::Error>>; type Future = Pin<Box<dyn Future<Output = Result<Self, actix_web::Error>>>>;
#[inline] #[inline]
fn from_request( fn from_request(
req: &actix_web::HttpRequest, req: &actix_web::HttpRequest,
_payload: &mut actix_web::dev::Payload, _payload: &mut actix_web::dev::Payload,
) -> Self::Future { ) -> Self::Future {
use actix_web::error::{ErrorForbidden, ErrorInternalServerError};
use actix_web::HttpMessage; use actix_web::HttpMessage;
let Some(service) = req.app_data::<ReportsService>() else { let base_service =
req.app_data::<ReportsService>().cloned().ok_or_else(|| {
tracing::error!( tracing::error!(
"FATAL! Failed to extract ReportsService from actix app_data. \ "FATAL! Failed to extract ReportsService from actix app_data. \
Check HTTP server configuration" Check HTTP server configuration"
); );
return ready(Err(actix_web::error::ErrorInternalServerError("Internal server error"))); ErrorInternalServerError("Internal server error")
}; });
let auth_service = let auth_service =
if let Some(user_identity) = req.extensions().get::<UserIdentity>() { req.app_data::<AuthService>().cloned().ok_or_else(|| {
tracing::trace!("Found user identity. Creating authenticated service"); tracing::error!(
service.authenticated(user_identity.clone()) "FATAL! Failed to extract AuthService from actix app_data. \
} else { Check HTTP server configuration"
tracing::trace!(
"No user identity found. Leaving unauthenticated service"
); );
service.clone() ErrorInternalServerError("Internal server error")
}; });
let request_auth_value =
req.extensions().get::<AuthorizationCredential>().cloned();
ready(Ok(auth_service)) Box::pin(async move {
let auth_service = auth_service?;
let base_service = base_service?;
// This is Some for endpoints hidden behind auth validation middleware
let auth_token = match request_auth_value {
Some(token @ AuthorizationCredential::UserToken(_)) => token,
Some(_) => {
// Reports service shouldn't be called by other services
warn!("Reports service requires user authorization");
return Err(ErrorForbidden("Forbidden"));
}
None => {
// Unauthenticated requests get a service-to-service token
let services_token =
auth_service.get_services_token().await.map_err(|err| {
error!("Failed to get services token: {err}");
ErrorInternalServerError("Internal server error")
})?;
AuthorizationCredential::ServicesToken(services_token)
}
};
let service = base_service.with_authentication(auth_token);
Ok(service)
})
michalUnsubmitted
Not Done
Inline Actions

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.

michal: Do you think it would make sense to move this into authorization middleware? So you can specify…
bartekAuthorUnsubmitted
Done
Inline Actions

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

bartek: I played with various solutions a bit (spent a little too long on this). Your suggestion makes…
michalUnsubmitted
Not Done
Inline Actions

Makes sense, I got confused a bit

michal: Makes sense, I got confused a bit
} }
} }
struct ProcessedReport { struct ProcessedReport {
id: ReportID, id: ReportID,
db_item: ReportItem, db_item: ReportItem,
email: ReportEmail, email: ReportEmail,
} }
▲ Show 20 Lines • Show All 78 Lines • Show Last 20 Lines
View Options

web/push-notif/service-worker.js

use actix_web::FromRequest; use actix_web::FromRequest;
use chrono::Utc; use chrono::Utc;
use comm_services_lib::{ use comm_services_lib::{
auth::UserIdentity, auth::{AuthService, AuthorizationCredential},
blob::client::{BlobServiceClient, BlobServiceError}, blob::client::{BlobServiceClient, BlobServiceError},
crypto::aes256, crypto::aes256,
database, database,
}; };
use derive_more::{Display, Error, From}; use derive_more::{Display, Error, From};
use std::{ use std::{
collections::HashMap, collections::HashMap,
future::{ready, Ready}, future::{ready, Future},
pin::Pin,
sync::Arc, sync::Arc,
}; };
use tracing::{error, trace}; use tracing::{error, trace, warn};
use crate::{ use crate::{
config::CONFIG, config::CONFIG,
database::{ database::{
client::{DatabaseClient, ReportsPage}, client::{DatabaseClient, ReportsPage},
item::{ReportContent, ReportItem}, item::{ReportContent, ReportItem},
}, },
email::{config::EmailConfig, ReportEmail}, email::{config::EmailConfig, ReportEmail},
▲ Show 20 Lines • Show All 42 Lines • ▼ Show 20 Lines ) -> Self {
Self { Self {
db, db,
blob_client, blob_client,
requesting_user_id: None, requesting_user_id: None,
email_config: email_config.map(Arc::new), email_config: email_config.map(Arc::new),
} }
} }
pub fn authenticated(&self, user: UserIdentity) -> Self { /// Clones the service with a new auth identity. When the credential is
let user_id = user.user_id.to_string(); /// a service-to-service, the `user_id` is None.
pub fn with_authentication(&self, token: AuthorizationCredential) -> Self {
let requesting_user_id = match &token {
AuthorizationCredential::ServicesToken(_) => None,
AuthorizationCredential::UserToken(user) => {
Some(user.user_id.to_string())
}
};
Self { Self {
db: self.db.clone(), db: self.db.clone(),
email_config: self.email_config.clone(), email_config: self.email_config.clone(),
blob_client: self.blob_client.with_user_identity(user), blob_client: self.blob_client.with_authentication(token),
requesting_user_id: Some(user_id), requesting_user_id,
} }
} }
pub async fn save_reports( pub async fn save_reports(
&self, &self,
inputs: Vec<ReportInput>, inputs: Vec<ReportInput>,
) -> ServiceResult<Vec<ReportID>> { ) -> ServiceResult<Vec<ReportID>> {
let mut reports = Vec::with_capacity(inputs.len()); let mut reports = Vec::with_capacity(inputs.len());
▲ Show 20 Lines • Show All 101 Lines • ▼ Show 20 Linesimpl ReportsService {
) -> ServiceResult<ReportsPage> { ) -> ServiceResult<ReportsPage> {
let page = self.db.scan_reports(cursor, page_size).await?; let page = self.db.scan_reports(cursor, page_size).await?;
Ok(page) Ok(page)
} }
} }
impl FromRequest for ReportsService { impl FromRequest for ReportsService {
type Error = actix_web::Error; type Error = actix_web::Error;
type Future = Ready<Result<Self, actix_web::Error>>; type Future = Pin<Box<dyn Future<Output = Result<Self, actix_web::Error>>>>;
#[inline] #[inline]
fn from_request( fn from_request(
req: &actix_web::HttpRequest, req: &actix_web::HttpRequest,
_payload: &mut actix_web::dev::Payload, _payload: &mut actix_web::dev::Payload,
) -> Self::Future { ) -> Self::Future {
use actix_web::error::{ErrorForbidden, ErrorInternalServerError};
use actix_web::HttpMessage; use actix_web::HttpMessage;
let Some(service) = req.app_data::<ReportsService>() else { let base_service =
req.app_data::<ReportsService>().cloned().ok_or_else(|| {
tracing::error!( tracing::error!(
"FATAL! Failed to extract ReportsService from actix app_data. \ "FATAL! Failed to extract ReportsService from actix app_data. \
Check HTTP server configuration" Check HTTP server configuration"
); );
return ready(Err(actix_web::error::ErrorInternalServerError("Internal server error"))); ErrorInternalServerError("Internal server error")
}; });
let auth_service = let auth_service =
if let Some(user_identity) = req.extensions().get::<UserIdentity>() { req.app_data::<AuthService>().cloned().ok_or_else(|| {
tracing::trace!("Found user identity. Creating authenticated service"); tracing::error!(
service.authenticated(user_identity.clone()) "FATAL! Failed to extract AuthService from actix app_data. \
} else { Check HTTP server configuration"
tracing::trace!(
"No user identity found. Leaving unauthenticated service"
); );
service.clone() ErrorInternalServerError("Internal server error")
}; });
let request_auth_value =
req.extensions().get::<AuthorizationCredential>().cloned();
ready(Ok(auth_service)) Box::pin(async move {
let auth_service = auth_service?;
let base_service = base_service?;
// This is Some for endpoints hidden behind auth validation middleware
let auth_token = match request_auth_value {
Some(token @ AuthorizationCredential::UserToken(_)) => token,
Some(_) => {
// Reports service shouldn't be called by other services
warn!("Reports service requires user authorization");
return Err(ErrorForbidden("Forbidden"));
}
None => {
// Unauthenticated requests get a service-to-service token
let services_token =
auth_service.get_services_token().await.map_err(|err| {
error!("Failed to get services token: {err}");
ErrorInternalServerError("Internal server error")
})?;
AuthorizationCredential::ServicesToken(services_token)
}
};
let service = base_service.with_authentication(auth_token);
Ok(service)
})
michalUnsubmitted
Not Done
Inline Actions

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.

michal: Do you think it would make sense to move this into authorization middleware? So you can specify…
bartekAuthorUnsubmitted
Done
Inline Actions

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

bartek: I played with various solutions a bit (spent a little too long on this). Your suggestion makes…
michalUnsubmitted
Not Done
Inline Actions

Makes sense, I got confused a bit

michal: Makes sense, I got confused a bit
} }
} }
struct ProcessedReport { struct ProcessedReport {
id: ReportID, id: ReportID,
db_item: ReportItem, db_item: ReportItem,
email: ReportEmail, email: ReportEmail,
} }
▲ Show 20 Lines • Show All 78 Lines • Show Last 20 Lines
View Options

web/redux/action-types.js

Loading...
View Options

web/redux/default-state.js

Loading...
View Options

web/redux/initial-state-gate.js

Loading...
View Options

web/redux/persist.js

Loading...