Skip to content

Commit d618f26

Browse files
azasypkinazasypkin
committed
refactor(security): use operator configuration to protect administrative endpoints
1 parent 88e4cfc commit d618f26

18 files changed

+93
-136
lines changed

‎src/main.rs

+4-3
Original file line numberDiff line numberDiff line change
@@ -120,13 +120,14 @@ mod tests {
120120
trial_started_at: None,
121121
trial_ends_at: None,
122122
},
123-
activated: false,
123+
is_activated: false,
124+
is_operator: false,
124125
},
125126
}
126127
}
127128

128-
pub fn set_activated(mut self) -> Self {
129-
self.user.activated = true;
129+
pub fn set_is_activated(mut self) -> Self {
130+
self.user.is_activated = true;
130131

131132
self
132133
}

‎src/security/api_ext.rs

+3-1
Original file line numberDiff line numberDiff line change
@@ -84,9 +84,11 @@ where
8484
return Ok(None);
8585
};
8686

87+
let operators = self.api.config.security.operators.as_ref();
8788
Ok(Some(User {
8889
created_at: identity.created_at,
89-
activated: identity.activated(),
90+
is_activated: identity.is_activated(),
91+
is_operator: operators.map_or(false, |operators| operators.contains(&user.email)),
9092
..user
9193
}))
9294
}

‎src/security/kratos/identity.rs

+3-3
Original file line numberDiff line numberDiff line change
@@ -19,7 +19,7 @@ pub struct Identity {
1919

2020
impl Identity {
2121
/// Determines if the identity is activated.
22-
pub fn activated(&self) -> bool {
22+
pub fn is_activated(&self) -> bool {
2323
self.verifiable_addresses
2424
.iter()
2525
.any(|address| address.value == self.traits.email && address.verified)
@@ -84,7 +84,7 @@ mod tests {
8484
],
8585
created_at: OffsetDateTime::from_unix_timestamp(946720800)?,
8686
}
87-
.activated());
87+
.is_activated());
8888

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

112112
Ok(())
113113
}

‎src/server/app_state.rs

+1-55
Original file line numberDiff line numberDiff line change
@@ -3,10 +3,7 @@ use crate::{
33
config::Config,
44
network::{DnsResolver, EmailTransport, TokioDnsResolver},
55
server::{Status, StatusLevel},
6-
users::User,
76
};
8-
use actix_web::{error::ErrorForbidden, Error};
9-
use anyhow::anyhow;
107
use lettre::{AsyncSmtpTransport, Tokio1Executor};
118
use std::sync::{Arc, RwLock};
129

@@ -30,15 +27,6 @@ impl<DR: DnsResolver, ET: EmailTransport> AppState<DR, ET> {
3027
api,
3128
}
3229
}
33-
34-
/// Ensures that the user is an admin, otherwise returns an error (`403`).
35-
pub fn ensure_admin(&self, user: &User) -> Result<(), Error> {
36-
if user.subscription.get_features(&self.config).admin {
37-
Ok(())
38-
} else {
39-
Err(ErrorForbidden(anyhow!("Forbidden")))
40-
}
41-
}
4230
}
4331

4432
#[cfg(test)]
@@ -49,14 +37,11 @@ pub mod tests {
4937
network::{Network, TokioDnsResolver},
5038
server::AppState,
5139
templates::create_templates,
52-
tests::{mock_config, mock_network, mock_search_index, mock_user, MockUserBuilder},
53-
users::{SubscriptionTier, UserSubscription},
40+
tests::{mock_config, mock_search_index},
5441
};
55-
use insta::assert_debug_snapshot;
5642
use lettre::{AsyncSmtpTransport, Tokio1Executor};
5743
use sqlx::PgPool;
5844
use std::sync::Arc;
59-
use time::OffsetDateTime;
6045

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

7661
Ok(AppState::new(api.config.clone(), api))
7762
}
78-
79-
#[sqlx::test]
80-
async fn can_detect_admin(pool: PgPool) -> anyhow::Result<()> {
81-
let config = mock_config()?;
82-
let api = Arc::new(Api::new(
83-
config,
84-
Database::create(pool).await?,
85-
mock_search_index()?,
86-
mock_network(),
87-
create_templates()?,
88-
));
89-
90-
let state = AppState::new(api.config.clone(), api);
91-
92-
let user = mock_user()?;
93-
assert!(state.ensure_admin(&user).is_ok());
94-
95-
let user = MockUserBuilder::new(
96-
user.id,
97-
&format!("dev-{}@secutils.dev", *user.id),
98-
&format!("dev-handle-{}", *user.id),
99-
OffsetDateTime::now_utc(),
100-
)
101-
.set_subscription(UserSubscription {
102-
tier: SubscriptionTier::Professional,
103-
started_at: OffsetDateTime::now_utc(),
104-
ends_at: None,
105-
trial_started_at: Some(OffsetDateTime::now_utc()),
106-
trial_ends_at: None,
107-
})
108-
.build();
109-
110-
assert_debug_snapshot!(
111-
state.ensure_admin(&user).unwrap_err(),
112-
@"Forbidden"
113-
);
114-
115-
Ok(())
116-
}
11763
}

‎src/server/extractors/operator.rs

+5-2
Original file line numberDiff line numberDiff line change
@@ -21,9 +21,12 @@ impl FromRequest for Operator {
2121
let credentials = Credentials::extract(&req).await?;
2222
match state.api.security().get_operator(credentials).await {
2323
Ok(Some(user)) => Ok(user),
24-
Ok(None) => Err(ErrorUnauthorized(anyhow!("Unauthorized"))),
24+
Ok(None) => {
25+
log::warn!(request_path:serde = req.path(); "Non-operator tried to access protected endpoint.");
26+
Err(ErrorUnauthorized(anyhow!("Unauthorized")))
27+
}
2528
Err(err) => {
26-
log::error!("Failed to extract operator information due to: {err:?}");
29+
log::error!(request_path:serde = req.path(); "Failed to extract operator information due to: {err:?}");
2730
Err(ErrorInternalServerError(anyhow!("Internal server error")))
2831
}
2932
}

‎src/server/handlers/security_subscription_update.rs

+10-7
Original file line numberDiff line numberDiff line change
@@ -1,6 +1,7 @@
11
use crate::{
2+
security::Operator,
23
server::{app_state::AppState, http_errors::generic_internal_server_error},
3-
users::{User, UserSubscription},
4+
users::UserSubscription,
45
};
56
use actix_web::{web, Error, HttpResponse, Responder};
67
use serde::Deserialize;
@@ -16,10 +17,8 @@ pub struct UpdateSubscriptionParams {
1617
pub async fn security_subscription_update(
1718
state: web::Data<AppState>,
1819
body_params: web::Json<UpdateSubscriptionParams>,
19-
user: User,
20+
operator: Operator,
2021
) -> impl Responder {
21-
state.ensure_admin(&user)?;
22-
2322
let UpdateSubscriptionParams {
2423
user_email,
2524
subscription,
@@ -30,15 +29,19 @@ pub async fn security_subscription_update(
3029
.await
3130
{
3231
Ok(Some(updated_user)) => {
33-
log::info!(user:serde = updated_user.log_context(); "Successfully updated user subscription.");
32+
log::info!(
33+
operator:serde = operator.id(),
34+
user:serde = updated_user.log_context();
35+
"Successfully updated user subscription."
36+
);
3437
Ok::<HttpResponse, Error>(HttpResponse::NoContent().finish())
3538
}
3639
Ok(None) => {
37-
log::error!("Failed to find user by email (`{user_email}`).");
40+
log::error!(operator:serde = operator.id(); "Failed to find user by email (`{user_email}`).");
3841
Ok(HttpResponse::NotFound().finish())
3942
}
4043
Err(err) => {
41-
log::error!("Failed to update user's tier by email (`{user_email}`): {err:?}");
44+
log::error!(operator:serde = operator.id(); "Failed to update user's tier by email (`{user_email}`): {err:?}");
4245
Ok(generic_internal_server_error())
4346
}
4447
}

‎src/server/handlers/security_users_get.rs

+8-5
Original file line numberDiff line numberDiff line change
@@ -1,22 +1,25 @@
11
use crate::{
22
logging::UserLogContext,
3+
security::Operator,
34
server::{http_errors::generic_internal_server_error, AppState},
4-
users::{User, UserId},
5+
users::UserId,
56
};
67
use actix_web::{web, Error, HttpResponse, Responder};
78

89
pub async fn security_users_get(
910
state: web::Data<AppState>,
10-
user: User,
11+
operator: Operator,
1112
user_id: web::Path<UserId>,
1213
) -> impl Responder {
13-
state.ensure_admin(&user)?;
14-
1514
Ok::<HttpResponse, Error>(match state.api.users().get(*user_id).await {
1615
Ok(Some(user_to_retrieve)) => HttpResponse::Ok().json(user_to_retrieve),
1716
Ok(None) => HttpResponse::NotFound().finish(),
1817
Err(err) => {
19-
log::error!(user:serde = UserLogContext::new(*user_id); "Failed to retrieve user by ID: {err:?}");
18+
log::error!(
19+
operator:serde = operator.id(),
20+
user:serde = UserLogContext::new(*user_id);
21+
"Failed to retrieve user by ID: {err:?}"
22+
);
2023
generic_internal_server_error()
2124
}
2225
})

‎src/server/handlers/security_users_get_by_email.rs

+3-5
Original file line numberDiff line numberDiff line change
@@ -1,6 +1,6 @@
11
use crate::{
2+
security::Operator,
23
server::{http_errors::generic_internal_server_error, AppState},
3-
users::User,
44
};
55
use actix_web::{web, Error, HttpResponse, Responder};
66
use serde::Deserialize;
@@ -12,16 +12,14 @@ pub struct Query {
1212

1313
pub async fn security_users_get_by_email(
1414
state: web::Data<AppState>,
15-
user: User,
15+
operator: Operator,
1616
query: web::Query<Query>,
1717
) -> impl Responder {
18-
state.ensure_admin(&user)?;
19-
2018
Ok::<HttpResponse, Error>(match state.api.users().get_by_email(&query.email).await {
2119
Ok(Some(user_to_retrieve)) => HttpResponse::Ok().json(user_to_retrieve),
2220
Ok(None) => HttpResponse::NotFound().finish(),
2321
Err(err) => {
24-
log::error!("Failed to retrieve user by email: {err:?}");
22+
log::error!(operator:serde = operator.id(); "Failed to retrieve user by email: {err:?}");
2523
generic_internal_server_error()
2624
}
2725
})

‎src/server/handlers/security_users_remove.rs

+5-6
Original file line numberDiff line numberDiff line change
@@ -1,7 +1,7 @@
11
use crate::{
22
logging::UserLogContext,
3+
security::Operator,
34
server::{app_state::AppState, http_errors::generic_internal_server_error},
4-
users::User,
55
};
66
use actix_web::{web, Error, HttpResponse, Responder};
77
use serde::Deserialize;
@@ -15,10 +15,8 @@ pub struct RemoveParams {
1515
pub async fn security_users_remove(
1616
state: web::Data<AppState>,
1717
body_params: web::Json<RemoveParams>,
18-
user: User,
18+
operator: Operator,
1919
) -> impl Responder {
20-
state.ensure_admin(&user)?;
21-
2220
let body_params = body_params.into_inner();
2321
if body_params.email.is_empty() {
2422
return Ok::<HttpResponse, Error>(
@@ -30,15 +28,16 @@ pub async fn security_users_remove(
3028
match users_api.remove_by_email(&body_params.email).await {
3129
Ok(Some(user_id)) => {
3230
log::info!(
31+
operator:serde = operator.id(),
3332
user:serde = UserLogContext::new(user_id);
3433
"Successfully removed user.",
3534
);
3635
}
3736
Ok(None) => {
38-
log::warn!("Cannot remove non-existent user.");
37+
log::warn!(operator:serde = operator.id(); "Cannot remove non-existent user.");
3938
}
4039
Err(err) => {
41-
log::error!("Failed to remove user: {err:?}");
40+
log::error!(operator:serde = operator.id(); "Failed to remove user: {err:?}");
4241
return Ok(generic_internal_server_error());
4342
}
4443
}

‎src/server/handlers/security_users_signup.rs

+2-1
Original file line numberDiff line numberDiff line change
@@ -63,7 +63,8 @@ pub async fn security_users_signup(
6363
email,
6464
handle,
6565
created_at: body_params.identity.created_at,
66-
activated: body_params.identity.activated(),
66+
is_activated: body_params.identity.is_activated(),
67+
is_operator: false,
6768
subscription,
6869
};
6970

‎src/server/handlers/status_set.rs

+6-5
Original file line numberDiff line numberDiff line change
@@ -1,6 +1,6 @@
11
use crate::{
2+
security::Operator,
23
server::{app_state::AppState, StatusLevel},
3-
users::User,
44
};
55
use actix_web::{error::ErrorInternalServerError, web, HttpResponse, Responder};
66
use anyhow::anyhow;
@@ -14,16 +14,17 @@ pub struct SetStatusAPIParams {
1414
pub async fn status_set(
1515
state: web::Data<AppState>,
1616
body_params: web::Json<SetStatusAPIParams>,
17-
user: User,
17+
operator: Operator,
1818
) -> impl Responder {
19-
state.ensure_admin(&user)?;
20-
2119
state
2220
.status
2321
.write()
2422
.map(|mut status| {
2523
status.level = body_params.level;
2624
HttpResponse::NoContent().finish()
2725
})
28-
.map_err(|err| ErrorInternalServerError(anyhow!("Failed to set server status: {:?}.", err)))
26+
.map_err(|err| {
27+
log::error!(operator:serde = operator.id(); "Failed to set server status: {err:?}.");
28+
ErrorInternalServerError(anyhow!("Failed to set server status: {:?}.", err))
29+
})
2930
}

‎src/server/ui_state.rs

+2-3
Original file line numberDiff line numberDiff line change
@@ -99,16 +99,15 @@ mod tests {
9999
"user": {
100100
"email": "[email protected]",
101101
"handle": "dev-handle-00000000-0000-0000-0000-000000000001",
102-
"created_at": 1262340000,
103-
"activated": false,
102+
"createdAt": 1262340000,
103+
"isActivated": false,
104104
"subscription": {
105105
"tier": "ultimate",
106106
"startedAt": 1262340001
107107
}
108108
},
109109
"subscription": {
110110
"features": {
111-
"admin": true,
112111
"certificates": {},
113112
"webhooks": {
114113
"responderRequests": 30

‎src/server/ui_state/subscription_state.rs

-1
Original file line numberDiff line numberDiff line change
@@ -40,7 +40,6 @@ mod tests {
4040
}, @r###"
4141
{
4242
"features": {
43-
"admin": true,
4443
"certificates": {},
4544
"webhooks": {
4645
"responderRequests": 30

0 commit comments

Comments
 (0)