Add experimental and preliminary policy-driven session limiting when logging in compatibility sessions. #5287
Conversation
reivilibre
force-pushed
the
rei/policy_for_compat_login
branch
from
dd9fc94 to
5b7e908
Compare
Deploying matrix-authentication-service-docs with
|
| Latest commit: |
444e084
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://7011f741.matrix-authentication-service-docs.pages.dev |
| Branch Preview URL: | https://rei-policy-for-compat-login.matrix-authentication-service-docs.pages.dev |
reivilibre
force-pushed
the
rei/policy_for_compat_login
branch
from
5b7e908 to
1ce2c39
Compare
crates/policy/src/model.rs
Outdated
| // TODO I don't know if we should keep this anymore, not used by current policy | ||
| pub login_type: CompatLoginType, |
reivilibre
force-pushed
the
rei/policy_for_compat_login
branch
from
060253f to
959e383
Compare
reivilibre
force-pushed
the
rei/policy_for_compat_login
branch
from
cf82370 to
0ff619f
Compare
crates/policy/src/model.rs
Outdated
|
|
||
| // TODO is this actually what we care about? Don't we care a bit more about whether we're in an | ||
| // interactive context or a non-interactive context? (SSO type has both phases :() | ||
| pub login_type: CompatLoginType, |
There was a problem hiding this comment.
It would be nice if the context also included the SSO redirect URI on m.login.sso? Surely we'll want to limit those in some contexts
|
|
||
| # This is a web-based interactive login | ||
| # TODO not strictly correct... | ||
| input.login_type == "m.login.sso" |
There was a problem hiding this comment.
you could easily do a helper like
is_interactive if {
input.login_type == "m.login.sso"
}| }) | ||
| .await?; | ||
| if !res.valid() { | ||
| if res.violations.len() == 1 { |
There was a problem hiding this comment.
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.
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?
| }) | ||
| .await?; | ||
| if !res.valid() { | ||
| let ctx = CompatLoginPolicyViolationContext::for_violations( |
There was a problem hiding this comment.
I wonder if we should just pass the violation result? I assume this is a dependency problem, mas-templates would need to depend on mas-policy? Not a strong opinion if it's too annoying
crates/policy/src/model.rs
Outdated
| /// - true when asking for consent, | ||
| /// - false when actually performing the login (at which point we create the | ||
| /// compat session, but it's too late to show a web page) | ||
| pub is_interactive: bool, |
There was a problem hiding this comment.
what if the distinction was instead m.login.token on the login type since that's the actual login type that is going through at this stage?
There was a problem hiding this comment.
Did consider, no super strong opinion on that, but if you veer towards it let's do that.
Part of: #4339
Follows: #5221
This PR introduces a new policy for compatibility logins and starts off by adding a session limit.
Compatibility login is a bit troublesome in that the actual device creation is uninteractive, so we can't show an interactive screen to clean up devices at the time of actually creating the device!
Only for
m.login.sso-style logins can we do an optimistic 'please clean up some devices' screen (although this PR on its own just displays a generic policy violation screen),but this can only happen before the actual creation of the session.
For this reason we will make use of a soft and hard limit. The soft limit will display a policy violation screen in interactive contexts but not be enforced in uninteractive contexts.
Meanwhile the hard limit will be enforced in the uninteractive contexts, eventually (in the future) through automatic eviction of a session.