-
Notifications
You must be signed in to change notification settings - Fork 68
Add experimental and preliminary policy-driven session limiting when logging in compatibility sessions. #5287
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Changes from 10 commits
9f36cfd
f670577
86d71de
86f0b27
3d50eae
04951d9
1ce2c39
959e383
68107af
0ff619f
9c7c157
d21922f
8f523e3
673cfa0
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -11,23 +11,27 @@ use axum::{ | |
| extract::{Form, Path, State}, | ||
| response::{Html, IntoResponse, Redirect, Response}, | ||
| }; | ||
| use axum_extra::extract::Query; | ||
| use axum_extra::{TypedHeader, extract::Query}; | ||
| use chrono::Duration; | ||
| use hyper::StatusCode; | ||
| use mas_axum_utils::{ | ||
| InternalError, | ||
| cookies::CookieJar, | ||
| csrf::{CsrfExt, ProtectedForm}, | ||
| }; | ||
| use mas_data_model::{BoxClock, BoxRng, Clock}; | ||
| use mas_policy::{Policy, model::CompatLoginType}; | ||
| use mas_router::{CompatLoginSsoAction, UrlBuilder}; | ||
| use mas_storage::{BoxRepository, RepositoryAccess, compat::CompatSsoLoginRepository}; | ||
| use mas_templates::{CompatSsoContext, ErrorContext, TemplateContext, Templates}; | ||
| use mas_templates::{ | ||
| CompatLoginPolicyViolationContext, CompatSsoContext, ErrorContext, TemplateContext, Templates, | ||
| }; | ||
| use serde::{Deserialize, Serialize}; | ||
| use ulid::Ulid; | ||
|
|
||
| use crate::{ | ||
| PreferredLanguage, | ||
| session::{SessionOrFallback, load_session_or_fallback}, | ||
| BoundActivityTracker, PreferredLanguage, | ||
| session::{SessionOrFallback, count_user_sessions_for_limiting, load_session_or_fallback}, | ||
| }; | ||
|
|
||
| #[derive(Serialize)] | ||
|
|
@@ -56,10 +60,15 @@ pub async fn get( | |
| mut repo: BoxRepository, | ||
| State(templates): State<Templates>, | ||
| State(url_builder): State<UrlBuilder>, | ||
| mut policy: Policy, | ||
| activity_tracker: BoundActivityTracker, | ||
| user_agent: Option<TypedHeader<headers::UserAgent>>, | ||
| cookie_jar: CookieJar, | ||
| Path(id): Path<Ulid>, | ||
| Query(params): Query<Params>, | ||
| ) -> Result<Response, InternalError> { | ||
| let user_agent = user_agent.map(|ua| ua.to_string()); | ||
|
|
||
| let (cookie_jar, maybe_session) = match load_session_or_fallback( | ||
| cookie_jar, &clock, &mut rng, &templates, &locale, &mut repo, | ||
| ) | ||
|
|
@@ -107,6 +116,39 @@ pub async fn get( | |
| return Ok((cookie_jar, Html(content)).into_response()); | ||
| } | ||
|
|
||
| let session_counts = count_user_sessions_for_limiting(&mut repo, &session.user).await?; | ||
|
|
||
| let res = policy | ||
| .evaluate_compat_login(mas_policy::CompatLoginInput { | ||
| user: &session.user, | ||
| is_interactive: true, | ||
| login_type: CompatLoginType::WebSso, | ||
| // We don't know if there's going to be a replacement until we received the device ID, | ||
| // which happens too late. | ||
| session_replaced: false, | ||
| session_counts, | ||
| requester: mas_policy::Requester { | ||
| ip_address: activity_tracker.ip(), | ||
| user_agent, | ||
| }, | ||
| }) | ||
| .await?; | ||
| if !res.valid() { | ||
| let ctx = CompatLoginPolicyViolationContext::for_violations( | ||
|
||
| res.violations | ||
| .into_iter() | ||
| .filter_map(|v| Some(v.code?.as_str())) | ||
| .collect(), | ||
| ) | ||
| .with_session(session) | ||
| .with_csrf(csrf_token.form_value()) | ||
| .with_language(locale); | ||
|
|
||
| let content = templates.render_compat_login_policy_violation(&ctx)?; | ||
|
|
||
| return Ok((StatusCode::FORBIDDEN, cookie_jar, Html(content)).into_response()); | ||
| } | ||
|
|
||
| let ctx = CompatSsoContext::new(login) | ||
| .with_session(session) | ||
| .with_csrf(csrf_token.form_value()) | ||
|
|
@@ -129,11 +171,16 @@ pub async fn post( | |
| PreferredLanguage(locale): PreferredLanguage, | ||
| State(templates): State<Templates>, | ||
| State(url_builder): State<UrlBuilder>, | ||
| mut policy: Policy, | ||
| activity_tracker: BoundActivityTracker, | ||
| user_agent: Option<TypedHeader<headers::UserAgent>>, | ||
| cookie_jar: CookieJar, | ||
| Path(id): Path<Ulid>, | ||
| Query(params): Query<Params>, | ||
| Form(form): Form<ProtectedForm<()>>, | ||
| ) -> Result<Response, InternalError> { | ||
| let user_agent = user_agent.map(|ua| ua.to_string()); | ||
|
|
||
| let (cookie_jar, maybe_session) = match load_session_or_fallback( | ||
| cookie_jar, &clock, &mut rng, &templates, &locale, &mut repo, | ||
| ) | ||
|
|
@@ -200,6 +247,41 @@ pub async fn post( | |
| redirect_uri | ||
| }; | ||
|
|
||
| let session_counts = count_user_sessions_for_limiting(&mut repo, &session.user).await?; | ||
|
|
||
| let res = policy | ||
| .evaluate_compat_login(mas_policy::CompatLoginInput { | ||
| user: &session.user, | ||
| is_interactive: true, | ||
| login_type: CompatLoginType::WebSso, | ||
| session_counts, | ||
| // We don't know if there's going to be a replacement until we received the device ID, | ||
| // which happens too late. | ||
| session_replaced: false, | ||
| requester: mas_policy::Requester { | ||
| ip_address: activity_tracker.ip(), | ||
| user_agent, | ||
| }, | ||
| }) | ||
| .await?; | ||
|
|
||
| if !res.valid() { | ||
| let (csrf_token, cookie_jar) = cookie_jar.csrf_token(&clock, &mut rng); | ||
| let ctx = CompatLoginPolicyViolationContext::for_violations( | ||
| res.violations | ||
| .into_iter() | ||
| .filter_map(|v| Some(v.code?.as_str())) | ||
| .collect(), | ||
| ) | ||
| .with_session(session) | ||
| .with_csrf(csrf_token.form_value()) | ||
| .with_language(locale); | ||
|
|
||
| let content = templates.render_compat_login_policy_violation(&ctx)?; | ||
|
|
||
| return Ok((StatusCode::FORBIDDEN, cookie_jar, Html(content)).into_response()); | ||
| } | ||
|
|
||
| // Note that if the login is not Pending, | ||
| // this fails and aborts the transaction. | ||
| repo.compat_sso_login() | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
what's the reasoning behind having a different message only if there is a single "too-many-sessions" violation vs. if it is one of them in the list?
In any case, I think this would benefit from a comment of what we're doing and why
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Definitely should comment, but I think the underlying thought was: If you have too many sessions but also your client 'Tnemele' is banned, isn't showing you the UI for 'pick a device to delete' missing the point?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
added this comment to 8f523e3