Add support for OAuth login #1719
Labels
No labels
Abandoned
Blocked
Bug
Changelog
Added
Changelog
Missing
Changelog
None
Cherry-picking
Database
Dependencies
Dependencies/Renovate
Difficulty
Easy
Difficulty
Hard
Difficulty
Medium
Documentation
Enhancement
Good first issue
Help wanted
Inherited
Matrix/Administration
Matrix/Appservices
Matrix/Auth
Matrix/Client
Matrix/Core
Matrix/E2EE
Matrix/Federation
Matrix/Hydra
Matrix/MSC
Matrix/Media
Matrix/T&S
Merge
Merge/Manual
Merge/Squash
Meta
Meta/CI
Meta/Packaging
Priority
Blocking
Priority
High
Priority
Low
Security
Status
Confirmed
Status
Duplicate
Status
Invalid
Status
Needs Investigation
Support
bob the builder
No project
No assignees
5 participants
Notifications
Due date
No due date set.
Blocks
Depends on
#1810 WIP: Admin API
continuwuation/continuwuity
#1743 refactor: Move room joining logic into a new service
continuwuation/continuwuity
Reference
continuwuation/continuwuity!1719
Loading…
Add table
Add a link
Reference in a new issue
No description provided.
Delete branch "ginger/oauth"
Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?
This pull request adds support for authenticating clients using the new OAuth 2.0 login API. It does not add support for authenticating clients with an external identity provider.
Currently working clients:
Fixes !1211.
Pull request checklist:
mainbranch, and the branch is named something other thanmain.myself, if applicable. This includes ensuring code compiles.
4156d50b805fd3e166055fd3e16605a2bfe66e4ca2bfe66e4cad5c0d4b84WIP: Add support for OAuth loginto Add support for OAuth loginprompt=createin the authorization code flowb9d636014f49cf772b80@ -303,4 +192,2 @@}#[admin_command]pub(super) async fn issue_password_reset_link(&self, username: String) -> Result {RIP?
It wasn't compatible with oauth password resets😔
@ -67,6 +96,7 @@ impl IntoResponse for WebError {context: TemplateContext {// Statically set false to prevent error pages from being indexed.allow_indexing: false,csp_nonce: String::new(),🤨
Nothing in the error template needs a CSP nonce, and we can't access the regular template context inside the
IntoResponseimpl, so it's fine to just make one up here.Perhaps make it an option then?
That would require
unwrap()ping it in every other template which needs it, because for those templates there should always be a CSP nonce.@ -0,0 +23,4 @@<form method="get" action="validate"><input type="hidden" name="session_id" value="{{ session_id }}"><input type="hidden" name="client_secret" value="{{ client_secret }}"><button type="submit">Continue</button>Is it still possible to continue the flow if this window gets unloaded / backgrounded? Common issue on mobile where switching from the browser to the email client can kill the page
@ -0,0 +21,4 @@<form method="get" action="validate"><input type="hidden" name="session_id" value="{{ session_id }}"><input type="hidden" name="client_secret" value="{{ client_secret }}"><button type="submit">Continue</button>Same here
allow_deactivation1928e4d761a16840e32b83b753ec7e845f8c0484845f8c0484f5a38dd787f5a38dd7870afd791e79743447765a57109ff38557109ff3859776f1f1fc239455ea49to5e20f117d35e20f117d3afaa04b941afaa04b9413a1c287f173a1c287f17a316ebe734a316ebe73445e14bd1eda316ebe7346756bacfc86756bacfc8c663a0903e@ -2354,0 +2367,4 @@/// The language code to provide to clients along with the policy documents.////// default: "en"pub language: String,"default" my ass, this has no such thing
5e23545fb5476f5249ce3ca0b6ba5246e2607aeb@ -31,3 +31,3 @@self.write_str(&format!("New registration token issued: `{token}`. {}.","New registration token issued: `{token}` . {}.",unnecessary whitespace?
That's intentional, it allows you to copy the token out of a terminal emulator by double-clicking on it without selecting the period as well.
@ -90,0 +57,4 @@.await?;let password = HashedPassword::new(&password.unwrap_or_else(|| utils::random_string(AUTO_GEN_PASSWORD_LENGTH)),Why would we want to auto-generate passwords?
That's been the default behavior of this admin command since it was implemented
Also, generally generating a secure password by default is a better idea than leaving a human to come up with it, even if the admin room isn't really encrypted.
@ -288,0 +262,4 @@let sender_user = identity.expect_sender_user()?;if !services.config.allow_deactivation {Why would we ever want to forbid deactivating one's own account?
I don't recall exactly what I was thinking when I implemented this, but it could be useful in a corporate environment where you don't want your users to click buttons they shouldn't click
@ -288,0 +263,4 @@let sender_user = identity.expect_sender_user()?;if !services.config.allow_deactivation {return Err!(Request(Unauthorized(Should be Forbidden instead of Unauthorized
46e2607aeb3e8ec63e42@ -0,0 +145,4 @@})?;let client_id = base64::prelude::BASE64_STANDARD.encode(sha256::hash(serde_json::to_string(metadata).unwrap().as_bytes()));not even like a hash? you're just gonna stuff the entire client registration JSON as base64 into the column key? bold
Uh looks like a sha256 to me
That literally is a sha256 hash, but just encoded in base64, no?
oh yeah i cant read
@ -0,0 +384,4 @@.await// This can only panic if the authorizing user suffered a spontaneous existence// failure during authentication, which should(?) be impossible(?).expect("failed to create device");What if the rapture happens in the middle of authentication? this probably needs handling /j
OK I'm not seeing any major issues, this is probably pretty baked now
e1a4dae224fdc1b75897green star for good code
@ -437,0 +588,4 @@// We already checked that the stage type is one that exists in the flow,// so we can only get here if we ourselves served a flow with a stage that we// don't understand.panic!("tried to check an unsupported stage type: {unknown:?}");nit:
panic!->unreachable!panic!is appropriate here because this branch can be reached in the event of a logic error.unreachable!is for cases where a branch can categorically never be reached even though rustc isn't smart enough to understand what's going onI mean that is unreachable_unchecked, unreachable is just panic with a compiler hint.
@ -215,0 +412,4 @@if let Err(e) = user_id.validate_strict() {// Unless we are in emergency mode, we should follow synapse's behaviour// on not allowing things like spaces and UTF-8 characters in// usernamesWe should perform relaxed validation at the very least otherwise people will create dastardly usernames and wonder why their federation is permanently busted later
play stupid games win stupid prizes 👍 I'm not super concerned about this given that you can only do it if you turn emergency mode on (and, most likely, checked the source to discover that it's possible in the first place)
fdc1b7589752026bb0f1