Skip to content

Commit 0d28304

Browse files
reivilibrereivilibre
authored
Revoke personal sessions when users are deactivated (#5181)
Revoke both personal sessions that are owned by, and acting as, the deactivated user. Owned by because: it doesn't make sense for a deactivated user to be able to control themselves or other users, so them having active personal sessions is just confusing. Acting as because: current precedent is that deactivated users are not controllable, even by admins. To uphold this, the admin API is also fixed to stop allowing the creation of personal sessions for deactivated users.
2 parents 8359f8b + 409f354 commit 0d28304

File tree

5 files changed

+232
-0
lines changed

5 files changed

+232
-0
lines changed

‎crates/handlers/src/admin/v1/personal_sessions/add.rs‎

Lines changed: 8 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -37,6 +37,9 @@ pub enum RouteError {
3737
#[error("User not found")]
3838
UserNotFound,
3939

40+
#[error("User is not active")]
41+
UserDeactivated,
42+
4043
#[error("Invalid scope")]
4144
InvalidScope,
4245
}
@@ -51,6 +54,7 @@ impl IntoResponse for RouteError {
5154
let status = match self {
5255
Self::Internal(_) => StatusCode::INTERNAL_SERVER_ERROR,
5356
Self::UserNotFound => StatusCode::NOT_FOUND,
57+
Self::UserDeactivated => StatusCode::GONE,
5458
Self::InvalidScope => StatusCode::BAD_REQUEST,
5559
};
5660
(status, sentry_event_id, Json(error)).into_response()
@@ -114,6 +118,10 @@ pub async fn handler(
114118
.await?
115119
.ok_or(RouteError::UserNotFound)?;
116120

121+
if !actor_user.is_valid_actor() {
122+
return Err(RouteError::UserDeactivated);
123+
}
124+
117125
let scope: Scope = params.scope.parse().map_err(|_| RouteError::InvalidScope)?;
118126

119127
// Create the personal session

‎crates/storage-pg/src/personal/mod.rs‎

Lines changed: 98 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -181,6 +181,104 @@ mod tests {
181181
assert!(session_lookup.is_revoked());
182182
}
183183

184+
#[sqlx::test(migrator = "crate::MIGRATOR")]
185+
async fn test_session_revoke_bulk(pool: PgPool) {
186+
let mut rng = ChaChaRng::seed_from_u64(42);
187+
let clock = MockClock::default();
188+
let mut repo = PgRepository::from_pool(&pool).await.unwrap();
189+
190+
let alice_user = repo
191+
.user()
192+
.add(&mut rng, &clock, "alice".to_owned())
193+
.await
194+
.unwrap();
195+
let bob_user = repo
196+
.user()
197+
.add(&mut rng, &clock, "bob".to_owned())
198+
.await
199+
.unwrap();
200+
201+
let session1 = repo
202+
.personal_session()
203+
.add(
204+
&mut rng,
205+
&clock,
206+
(&alice_user).into(),
207+
&bob_user,
208+
"Test Personal Session".to_owned(),
209+
"openid".parse().unwrap(),
210+
)
211+
.await
212+
.unwrap();
213+
repo.personal_access_token()
214+
.add(
215+
&mut rng,
216+
&clock,
217+
&session1,
218+
"mpt_hiss",
219+
Some(Duration::days(42)),
220+
)
221+
.await
222+
.unwrap();
223+
224+
let session2 = repo
225+
.personal_session()
226+
.add(
227+
&mut rng,
228+
&clock,
229+
(&bob_user).into(),
230+
&bob_user,
231+
"Test Personal Session".to_owned(),
232+
"openid".parse().unwrap(),
233+
)
234+
.await
235+
.unwrap();
236+
repo.personal_access_token()
237+
.add(
238+
&mut rng, &clock, &session2, "mpt_meow", // No expiry
239+
None,
240+
)
241+
.await
242+
.unwrap();
243+
244+
// Just one session without a token expiry time
245+
assert_eq!(
246+
repo.personal_session()
247+
.revoke_bulk(
248+
&clock,
249+
PersonalSessionFilter::new()
250+
.active_only()
251+
.with_expires(false)
252+
)
253+
.await
254+
.unwrap(),
255+
1
256+
);
257+
258+
// Just one session with a token expiry time
259+
assert_eq!(
260+
repo.personal_session()
261+
.revoke_bulk(
262+
&clock,
263+
PersonalSessionFilter::new()
264+
.active_only()
265+
.with_expires(true)
266+
)
267+
.await
268+
.unwrap(),
269+
1
270+
);
271+
272+
// No active sessions left
273+
assert_eq!(
274+
repo.personal_session()
275+
.revoke_bulk(&clock, PersonalSessionFilter::new().active_only())
276+
.await
277+
.unwrap(),
278+
0
279+
);
280+
}
281+
184282
#[sqlx::test(migrator = "crate::MIGRATOR")]
185283
async fn test_access_token_repository(pool: PgPool) {
186284
const FIRST_TOKEN: &str = "first_access_token";

‎crates/storage-pg/src/personal/session.rs‎

Lines changed: 71 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -357,6 +357,73 @@ impl PersonalSessionRepository for PgPersonalSessionRepository<'_> {
357357
.map_err(DatabaseError::to_invalid_operation)
358358
}
359359

360+
#[tracing::instrument(
361+
name = "db.personal_session.revoke_bulk",
362+
skip_all,
363+
fields(
364+
db.query.text,
365+
),
366+
err,
367+
)]
368+
async fn revoke_bulk(
369+
&mut self,
370+
clock: &dyn Clock,
371+
filter: PersonalSessionFilter<'_>,
372+
) -> Result<usize, Self::Error> {
373+
let revoked_at = clock.now();
374+
375+
let (sql, arguments) = Query::update()
376+
.table(PersonalSessions::Table)
377+
.value(PersonalSessions::RevokedAt, revoked_at)
378+
.and_where(
379+
Expr::col((PersonalSessions::Table, PersonalSessions::PersonalSessionId))
380+
// Because filters apply to both the session and access token tables,
381+
// Use a subquery to make it possible to use a JOIN
382+
// onto the personal access token table.
383+
.in_subquery(
384+
Query::select()
385+
.expr(Expr::col((
386+
PersonalSessions::Table,
387+
PersonalSessions::PersonalSessionId,
388+
)))
389+
.from(PersonalSessions::Table)
390+
.left_join(
391+
PersonalAccessTokens::Table,
392+
Cond::all()
393+
// Match session ID
394+
.add(
395+
Expr::col((
396+
PersonalSessions::Table,
397+
PersonalSessions::PersonalSessionId,
398+
))
399+
.eq(Expr::col((
400+
PersonalAccessTokens::Table,
401+
PersonalAccessTokens::PersonalSessionId,
402+
))),
403+
)
404+
// Only choose the active access token for each session
405+
.add(
406+
Expr::col((
407+
PersonalAccessTokens::Table,
408+
PersonalAccessTokens::RevokedAt,
409+
))
410+
.is_null(),
411+
),
412+
)
413+
.apply_filter(filter)
414+
.take(),
415+
),
416+
)
417+
.build_sqlx(PostgresQueryBuilder);
418+
419+
let res = sqlx::query_with(&sql, arguments)
420+
.traced()
421+
.execute(&mut *self.conn)
422+
.await?;
423+
424+
Ok(res.rows_affected().try_into().unwrap_or(usize::MAX))
425+
}
426+
360427
#[tracing::instrument(
361428
name = "db.personal_session.list",
362429
skip_all,
@@ -433,13 +500,15 @@ impl PersonalSessionRepository for PgPersonalSessionRepository<'_> {
433500
.left_join(
434501
PersonalAccessTokens::Table,
435502
Cond::all()
503+
// Match session ID
436504
.add(
437505
Expr::col((PersonalSessions::Table, PersonalSessions::PersonalSessionId))
438506
.eq(Expr::col((
439507
PersonalAccessTokens::Table,
440508
PersonalAccessTokens::PersonalSessionId,
441509
))),
442510
)
511+
// Only choose the active access token for each session
443512
.add(
444513
Expr::col((PersonalAccessTokens::Table, PersonalAccessTokens::RevokedAt))
445514
.is_null(),
@@ -477,13 +546,15 @@ impl PersonalSessionRepository for PgPersonalSessionRepository<'_> {
477546
.left_join(
478547
PersonalAccessTokens::Table,
479548
Cond::all()
549+
// Match session ID
480550
.add(
481551
Expr::col((PersonalSessions::Table, PersonalSessions::PersonalSessionId))
482552
.eq(Expr::col((
483553
PersonalAccessTokens::Table,
484554
PersonalAccessTokens::PersonalSessionId,
485555
))),
486556
)
557+
// Only choose the active access token for each session
487558
.add(
488559
Expr::col((PersonalAccessTokens::Table, PersonalAccessTokens::RevokedAt))
489560
.is_null(),

‎crates/storage/src/personal/session.rs‎

Lines changed: 24 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -87,6 +87,24 @@ pub trait PersonalSessionRepository: Send + Sync {
8787
personal_session: PersonalSession,
8888
) -> Result<PersonalSession, Self::Error>;
8989

90+
/// Revoke all the [`PersonalSession`]s matching the given filter.
91+
///
92+
/// Returns the number of sessions affected
93+
///
94+
/// # Parameters
95+
///
96+
/// * `clock`: The clock used to generate timestamps
97+
/// * `filter`: The filter to apply
98+
///
99+
/// # Errors
100+
///
101+
/// Returns [`Self::Error`] if the underlying repository fails
102+
async fn revoke_bulk(
103+
&mut self,
104+
clock: &dyn Clock,
105+
filter: PersonalSessionFilter<'_>,
106+
) -> Result<usize, Self::Error>;
107+
90108
/// List [`PersonalSession`]s matching the given filter and pagination
91109
/// parameters
92110
///
@@ -150,6 +168,12 @@ repository_impl!(PersonalSessionRepository:
150168
personal_session: PersonalSession,
151169
) -> Result<PersonalSession, Self::Error>;
152170

171+
async fn revoke_bulk(
172+
&mut self,
173+
clock: &dyn Clock,
174+
filter: PersonalSessionFilter<'_>,
175+
) -> Result<usize, Self::Error>;
176+
153177
async fn list(
154178
&mut self,
155179
filter: PersonalSessionFilter<'_>,

‎crates/tasks/src/user.rs‎

Lines changed: 31 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -10,6 +10,7 @@ use mas_storage::{
1010
RepositoryAccess,
1111
compat::CompatSessionFilter,
1212
oauth2::OAuth2SessionFilter,
13+
personal::PersonalSessionFilter,
1314
queue::{DeactivateUserJob, ReactivateUserJob},
1415
user::{BrowserSessionFilter, UserEmailFilter, UserRepository},
1516
};
@@ -80,6 +81,36 @@ impl RunnableJob for DeactivateUserJob {
8081
.map_err(JobError::retry)?;
8182
info!(affected = n, "Killed all compatibility sessions for user");
8283

84+
let n = repo
85+
.personal_session()
86+
.revoke_bulk(
87+
clock,
88+
PersonalSessionFilter::new()
89+
.for_actor_user(&user)
90+
.active_only(),
91+
)
92+
.await
93+
.map_err(JobError::retry)?;
94+
info!(
95+
affected = n,
96+
"Killed all compatibility sessions acting as user"
97+
);
98+
99+
let n = repo
100+
.personal_session()
101+
.revoke_bulk(
102+
clock,
103+
PersonalSessionFilter::new()
104+
.for_owner_user(&user)
105+
.active_only(),
106+
)
107+
.await
108+
.map_err(JobError::retry)?;
109+
info!(
110+
affected = n,
111+
"Killed all compatibility sessions owned by user"
112+
);
113+
83114
// Delete all the email addresses for the user
84115
let n = repo
85116
.user_email()

0 commit comments

Comments
 (0)