Skip to content

Commit

Permalink
refactor(security): use operator configuration to protect administrat…
Browse files Browse the repository at this point in the history
…ive endpoints
  • Loading branch information
azasypkin committed May 12, 2024
1 parent 88e4cfc commit d618f26
Show file tree
Hide file tree
Showing 18 changed files with 93 additions and 136 deletions.
7 changes: 4 additions & 3 deletions src/main.rs
Original file line number Diff line number Diff line change
Expand Up @@ -120,13 +120,14 @@ mod tests {
trial_started_at: None,
trial_ends_at: None,
},
activated: false,
is_activated: false,
is_operator: false,
},
}
}

pub fn set_activated(mut self) -> Self {
self.user.activated = true;
pub fn set_is_activated(mut self) -> Self {
self.user.is_activated = true;

self
}
Expand Down
4 changes: 3 additions & 1 deletion src/security/api_ext.rs
Original file line number Diff line number Diff line change
Expand Up @@ -84,9 +84,11 @@ where
return Ok(None);
};

let operators = self.api.config.security.operators.as_ref();
Ok(Some(User {
created_at: identity.created_at,
activated: identity.activated(),
is_activated: identity.is_activated(),
is_operator: operators.map_or(false, |operators| operators.contains(&user.email)),
..user
}))
}
Expand Down
6 changes: 3 additions & 3 deletions src/security/kratos/identity.rs
Original file line number Diff line number Diff line change
Expand Up @@ -19,7 +19,7 @@ pub struct Identity {

impl Identity {
/// Determines if the identity is activated.
pub fn activated(&self) -> bool {
pub fn is_activated(&self) -> bool {
self.verifiable_addresses
.iter()
.any(|address| address.value == self.traits.email && address.verified)
Expand Down Expand Up @@ -84,7 +84,7 @@ mod tests {
],
created_at: OffsetDateTime::from_unix_timestamp(946720800)?,
}
.activated());
.is_activated());

assert!(!Identity {
id: "f7f3b3b4-3b0b-4b3b-8b3b-3b3b3b3b3b3b".parse()?,
Expand All @@ -107,7 +107,7 @@ mod tests {
],
created_at: OffsetDateTime::from_unix_timestamp(946720800)?,
}
.activated());
.is_activated());

Ok(())
}
Expand Down
56 changes: 1 addition & 55 deletions src/server/app_state.rs
Original file line number Diff line number Diff line change
Expand Up @@ -3,10 +3,7 @@ use crate::{
config::Config,
network::{DnsResolver, EmailTransport, TokioDnsResolver},
server::{Status, StatusLevel},
users::User,
};
use actix_web::{error::ErrorForbidden, Error};
use anyhow::anyhow;
use lettre::{AsyncSmtpTransport, Tokio1Executor};
use std::sync::{Arc, RwLock};

Expand All @@ -30,15 +27,6 @@ impl<DR: DnsResolver, ET: EmailTransport> AppState<DR, ET> {
api,
}
}

/// Ensures that the user is an admin, otherwise returns an error (`403`).
pub fn ensure_admin(&self, user: &User) -> Result<(), Error> {
if user.subscription.get_features(&self.config).admin {
Ok(())
} else {
Err(ErrorForbidden(anyhow!("Forbidden")))
}
}
}

#[cfg(test)]
Expand All @@ -49,14 +37,11 @@ pub mod tests {
network::{Network, TokioDnsResolver},
server::AppState,
templates::create_templates,
tests::{mock_config, mock_network, mock_search_index, mock_user, MockUserBuilder},
users::{SubscriptionTier, UserSubscription},
tests::{mock_config, mock_search_index},
};
use insta::assert_debug_snapshot;
use lettre::{AsyncSmtpTransport, Tokio1Executor};
use sqlx::PgPool;
use std::sync::Arc;
use time::OffsetDateTime;

pub async fn mock_app_state(pool: PgPool) -> anyhow::Result<AppState> {
let config = mock_config()?;
Expand All @@ -75,43 +60,4 @@ pub mod tests {

Ok(AppState::new(api.config.clone(), api))
}

#[sqlx::test]
async fn can_detect_admin(pool: PgPool) -> anyhow::Result<()> {
let config = mock_config()?;
let api = Arc::new(Api::new(
config,
Database::create(pool).await?,
mock_search_index()?,
mock_network(),
create_templates()?,
));

let state = AppState::new(api.config.clone(), api);

let user = mock_user()?;
assert!(state.ensure_admin(&user).is_ok());

let user = MockUserBuilder::new(
user.id,
&format!("dev-{}@secutils.dev", *user.id),
&format!("dev-handle-{}", *user.id),
OffsetDateTime::now_utc(),
)
.set_subscription(UserSubscription {
tier: SubscriptionTier::Professional,
started_at: OffsetDateTime::now_utc(),
ends_at: None,
trial_started_at: Some(OffsetDateTime::now_utc()),
trial_ends_at: None,
})
.build();

assert_debug_snapshot!(
state.ensure_admin(&user).unwrap_err(),
@"Forbidden"
);

Ok(())
}
}
7 changes: 5 additions & 2 deletions src/server/extractors/operator.rs
Original file line number Diff line number Diff line change
Expand Up @@ -21,9 +21,12 @@ impl FromRequest for Operator {
let credentials = Credentials::extract(&req).await?;
match state.api.security().get_operator(credentials).await {
Ok(Some(user)) => Ok(user),
Ok(None) => Err(ErrorUnauthorized(anyhow!("Unauthorized"))),
Ok(None) => {
log::warn!(request_path:serde = req.path(); "Non-operator tried to access protected endpoint.");
Err(ErrorUnauthorized(anyhow!("Unauthorized")))
}
Err(err) => {
log::error!("Failed to extract operator information due to: {err:?}");
log::error!(request_path:serde = req.path(); "Failed to extract operator information due to: {err:?}");
Err(ErrorInternalServerError(anyhow!("Internal server error")))
}
}
Expand Down
17 changes: 10 additions & 7 deletions src/server/handlers/security_subscription_update.rs
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
use crate::{
security::Operator,
server::{app_state::AppState, http_errors::generic_internal_server_error},
users::{User, UserSubscription},
users::UserSubscription,
};
use actix_web::{web, Error, HttpResponse, Responder};
use serde::Deserialize;
Expand All @@ -16,10 +17,8 @@ pub struct UpdateSubscriptionParams {
pub async fn security_subscription_update(
state: web::Data<AppState>,
body_params: web::Json<UpdateSubscriptionParams>,
user: User,
operator: Operator,
) -> impl Responder {
state.ensure_admin(&user)?;

let UpdateSubscriptionParams {
user_email,
subscription,
Expand All @@ -30,15 +29,19 @@ pub async fn security_subscription_update(
.await
{
Ok(Some(updated_user)) => {
log::info!(user:serde = updated_user.log_context(); "Successfully updated user subscription.");
log::info!(
operator:serde = operator.id(),
user:serde = updated_user.log_context();
"Successfully updated user subscription."
);
Ok::<HttpResponse, Error>(HttpResponse::NoContent().finish())
}
Ok(None) => {
log::error!("Failed to find user by email (`{user_email}`).");
log::error!(operator:serde = operator.id(); "Failed to find user by email (`{user_email}`).");
Ok(HttpResponse::NotFound().finish())
}
Err(err) => {
log::error!("Failed to update user's tier by email (`{user_email}`): {err:?}");
log::error!(operator:serde = operator.id(); "Failed to update user's tier by email (`{user_email}`): {err:?}");
Ok(generic_internal_server_error())
}
}
Expand Down
13 changes: 8 additions & 5 deletions src/server/handlers/security_users_get.rs
Original file line number Diff line number Diff line change
@@ -1,22 +1,25 @@
use crate::{
logging::UserLogContext,
security::Operator,
server::{http_errors::generic_internal_server_error, AppState},
users::{User, UserId},
users::UserId,
};
use actix_web::{web, Error, HttpResponse, Responder};

pub async fn security_users_get(
state: web::Data<AppState>,
user: User,
operator: Operator,
user_id: web::Path<UserId>,
) -> impl Responder {
state.ensure_admin(&user)?;

Ok::<HttpResponse, Error>(match state.api.users().get(*user_id).await {
Ok(Some(user_to_retrieve)) => HttpResponse::Ok().json(user_to_retrieve),
Ok(None) => HttpResponse::NotFound().finish(),
Err(err) => {
log::error!(user:serde = UserLogContext::new(*user_id); "Failed to retrieve user by ID: {err:?}");
log::error!(
operator:serde = operator.id(),
user:serde = UserLogContext::new(*user_id);
"Failed to retrieve user by ID: {err:?}"
);
generic_internal_server_error()
}
})
Expand Down
8 changes: 3 additions & 5 deletions src/server/handlers/security_users_get_by_email.rs
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
use crate::{
security::Operator,
server::{http_errors::generic_internal_server_error, AppState},
users::User,
};
use actix_web::{web, Error, HttpResponse, Responder};
use serde::Deserialize;
Expand All @@ -12,16 +12,14 @@ pub struct Query {

pub async fn security_users_get_by_email(
state: web::Data<AppState>,
user: User,
operator: Operator,
query: web::Query<Query>,
) -> impl Responder {
state.ensure_admin(&user)?;

Ok::<HttpResponse, Error>(match state.api.users().get_by_email(&query.email).await {
Ok(Some(user_to_retrieve)) => HttpResponse::Ok().json(user_to_retrieve),
Ok(None) => HttpResponse::NotFound().finish(),
Err(err) => {
log::error!("Failed to retrieve user by email: {err:?}");
log::error!(operator:serde = operator.id(); "Failed to retrieve user by email: {err:?}");
generic_internal_server_error()
}
})
Expand Down
11 changes: 5 additions & 6 deletions src/server/handlers/security_users_remove.rs
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
use crate::{
logging::UserLogContext,
security::Operator,
server::{app_state::AppState, http_errors::generic_internal_server_error},
users::User,
};
use actix_web::{web, Error, HttpResponse, Responder};
use serde::Deserialize;
Expand All @@ -15,10 +15,8 @@ pub struct RemoveParams {
pub async fn security_users_remove(
state: web::Data<AppState>,
body_params: web::Json<RemoveParams>,
user: User,
operator: Operator,
) -> impl Responder {
state.ensure_admin(&user)?;

let body_params = body_params.into_inner();
if body_params.email.is_empty() {
return Ok::<HttpResponse, Error>(
Expand All @@ -30,15 +28,16 @@ pub async fn security_users_remove(
match users_api.remove_by_email(&body_params.email).await {
Ok(Some(user_id)) => {
log::info!(
operator:serde = operator.id(),
user:serde = UserLogContext::new(user_id);
"Successfully removed user.",
);
}
Ok(None) => {
log::warn!("Cannot remove non-existent user.");
log::warn!(operator:serde = operator.id(); "Cannot remove non-existent user.");
}
Err(err) => {
log::error!("Failed to remove user: {err:?}");
log::error!(operator:serde = operator.id(); "Failed to remove user: {err:?}");
return Ok(generic_internal_server_error());
}
}
Expand Down
3 changes: 2 additions & 1 deletion src/server/handlers/security_users_signup.rs
Original file line number Diff line number Diff line change
Expand Up @@ -63,7 +63,8 @@ pub async fn security_users_signup(
email,
handle,
created_at: body_params.identity.created_at,
activated: body_params.identity.activated(),
is_activated: body_params.identity.is_activated(),
is_operator: false,
subscription,
};

Expand Down
11 changes: 6 additions & 5 deletions src/server/handlers/status_set.rs
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
use crate::{
security::Operator,
server::{app_state::AppState, StatusLevel},
users::User,
};
use actix_web::{error::ErrorInternalServerError, web, HttpResponse, Responder};
use anyhow::anyhow;
Expand All @@ -14,16 +14,17 @@ pub struct SetStatusAPIParams {
pub async fn status_set(
state: web::Data<AppState>,
body_params: web::Json<SetStatusAPIParams>,
user: User,
operator: Operator,
) -> impl Responder {
state.ensure_admin(&user)?;

state
.status
.write()
.map(|mut status| {
status.level = body_params.level;
HttpResponse::NoContent().finish()
})
.map_err(|err| ErrorInternalServerError(anyhow!("Failed to set server status: {:?}.", err)))
.map_err(|err| {
log::error!(operator:serde = operator.id(); "Failed to set server status: {err:?}.");
ErrorInternalServerError(anyhow!("Failed to set server status: {:?}.", err))
})
}
5 changes: 2 additions & 3 deletions src/server/ui_state.rs
Original file line number Diff line number Diff line change
Expand Up @@ -99,16 +99,15 @@ mod tests {
"user": {
"email": "[email protected]",
"handle": "dev-handle-00000000-0000-0000-0000-000000000001",
"created_at": 1262340000,
"activated": false,
"createdAt": 1262340000,
"isActivated": false,
"subscription": {
"tier": "ultimate",
"startedAt": 1262340001
}
},
"subscription": {
"features": {
"admin": true,
"certificates": {},
"webhooks": {
"responderRequests": 30
Expand Down
1 change: 0 additions & 1 deletion src/server/ui_state/subscription_state.rs
Original file line number Diff line number Diff line change
Expand Up @@ -40,7 +40,6 @@ mod tests {
}, @r###"
{
"features": {
"admin": true,
"certificates": {},
"webhooks": {
"responderRequests": 30
Expand Down
Loading

0 comments on commit d618f26

Please sign in to comment.