OIDC auth flow: correct device registration

src/api/client/oidc/authorize.rs

@@ -1,5 +1,5 @@
 use axum::extract::{Query, State};
-use conduwuit::{Result, err};
+use conduwuit::{Result, err, utils::ReadyExt};
 use conduwuit_web::oidc::{
 	oidc_consent_form, oidc_login_form, AuthorizationQuery, OidcRequest, OidcResponse,
 };
@@ -8,6 +8,7 @@ use oxide_auth::{
 	frontends::simple::endpoint::FnSolicitor,
 };
 use percent_encoding::percent_decode_str;
+use ruma::UserId;
 use service::oidc::{SCOPE_PREFIX_API, SCOPE_PREFIX_DEVICE};
 
 /// # `GET /_matrix/client/unstable/org.matrix.msc2964/authorize`
@@ -67,15 +68,52 @@ pub(crate) struct Allowance {
 
 /// # `POST /_matrix/client/unstable/org.matrix.msc2964/authorize?allow=[Option<String>]`
 ///
-/// Authorize the device based on the user's consent. If the user allows
+/// Authorize the device based on the owner's consent. If the owner allows
 /// it to access their data, the client may request a token at the
 /// [super::token::token] endpoint.
+///
+/// On the owner's consent, if their specific device is unregistered it will be
+/// registered in their device list (not to be confused with the OIDC client
+/// registration).
 pub(crate) async fn authorize_consent(
 	Query(Allowance { allow }): Query<Allowance>,
 	State(services): State<crate::State>,
+	Query(query): Query<AuthorizationQuery>,
 	oauth: OidcRequest,
 ) -> Result<OidcResponse> {
-	tracing::debug!("processing user's consent: {:?} - {:?}", allow, oauth);
+	tracing::debug!("processing owner's consent: {:?}", allow);
+	tracing::trace!("owner's consent request: {:#?}", query);
+	let Some(owner_id) = allow.clone() else {
+		return Err(err!(Request(Unknown("the owner did not consent to the client's access"))));
+	};
+	let server_name = services.globals.server_name();
+	let owner_id = UserId::parse_with_server_name(owner_id.clone(), server_name)
+		.map_err(|err|
+			err!(Request(InvalidUsername("invalid username {owner_id:?}: {err}")))
+		)?;
+	let Some(mc) = services.oidc.client_from_client_id(&query.client_id) else {
+		return Err(err!(Request(Unknown("no client has registered client_id {:?}", query.client_id))));
+	};
+	let scope = query.scope.parse().map_err(|err|
+		err!(Request(Unknown("could not parse scope {:?}: {}", query.scope, err))))?;
+	let device_id = services.oidc.device_id_from_scope(scope)?;
+	// Check that the device is registered in the owner devices list.
+	// Note that this is _not_ the OIDC client registration.
+	let device_is_registered_with_owner = services
+		.users
+		.all_device_ids(&owner_id)
+		.ready_any(|v| v == device_id)
+		.await; 
+	if ! device_is_registered_with_owner {
+		// TODO get the client's IP from the request.
+		let client_ip = None;
+		services.oidc.register_device(
+			&query.client_id,
+			(&owner_id, &device_id),
+			mc.name.as_deref(),
+			client_ip,
+		)?;
+	}
 
 	services
 		.oidc

src/api/client/oidc/login.rs

@@ -1,5 +1,5 @@
 use axum::extract::State;
-use conduwuit::{Result, err, utils::{hash::verify_password, ReadyExt}};
+use conduwuit::{Result, err, utils::hash::verify_password};
 use conduwuit_web::oidc::{
 	LoginError, LoginQuery, OidcRequest, OidcResponse, oidc_consent_form,
 };
@@ -37,25 +37,8 @@ pub(crate) async fn oidc_login(
 		return Err(err!(Request(InvalidParam("password does not match"))));
 	}
 	// TODO check if user disabled, etc. See /src/api/client/session.rs
-
 	let hostname = services.config.server_name.host();
 	tracing::info!("logging in {user_id:?}");
-	let Some(mc) = services.oidc.client_from_client_id(&query.client_id) else {
-		return Err(err!(Request(Unknown("no client has registered client_id {:?}", query.client_id))));
-	};
-	let device_id = services.oidc.device_id_from_scope(mc.client.default_scope)?;
-	let device_exists = services
-		.users
-		.all_device_ids(&user_id)
-		.ready_any(|v| v == device_id)
-		.await; 
-	// TODO register the client's name at registration time.
-	//let name = services.oidc.endpoint().registrar.lock().unwrap();
-	if ! device_exists {
-		// TODO get the client's IP from the request.
-		let client_ip = None;
-		services.oidc.register_device((&user_id, &device_id), mc.name.as_deref(), client_ip)?;
-	}
 
 	services
 		.oidc

src/service/oidc/mod.rs

@@ -78,10 +78,11 @@ impl Service {
 		Ok(())
 	}
 
-	/// Register a device in the main continuwuity database. Should only be called once the client
-	/// and the user both have authenticated.
+	/// Register a device in the main continuwuity database. This should only happen on successful
+	/// authentication and consent, and will register the client's device_id.
 	pub fn register_device(
 		&self,
+		client_id: &str,
 		(user_id, device_id): (&OwnedUserId, &OwnedDeviceId),
 		display_name: Option<&str>,
 		client_ip: Option<String>,
@@ -95,6 +96,11 @@ impl Service {
 		};
 		increment(&self.db.userid_devicelistversion, user_id.as_bytes());
 		self.db.userdeviceid_metadata.put(key, Json(val));
+		self
+			.registrar
+			.lock()
+			.expect("lockable registrar")
+			.set_client_device_id(client_id, &device_id.to_string())?;
 
 		Ok(())
 	}
@@ -167,7 +173,7 @@ impl Service {
 	}
 
 	pub fn user_and_device_from_token(&self, token: &str) -> Result<(OwnedUserId, OwnedDeviceId)> {
-		let Some(Grant { owner_id, scope, .. }) = self.grant_from_token(token) else {
+		let Some(Grant { owner_id, client_id, .. }) = self.grant_from_token(token) else {
 			return Err(err!(Request(MissingToken("unknown token: {token:?}"))));
 		};
 		let server_name = self.services.globals.server_name();
@@ -175,7 +181,11 @@ impl Service {
 			.map_err(|err|
 				err!(Request(InvalidUsername("invalid username {owner_id:?}: {err}")))
 			)?;
-		let device_id = self.device_id_from_scope(scope)?;
+		let client = self.client_from_client_id(&client_id).expect("validated client_id");
+		let Some(device_id) = client.device_id else {
+			return Err(err!(Request(Unknown("this client has no device_id yet"))));
+		};
+		let device_id = OwnedDeviceId::from(device_id.to_string());
 
 		Ok((owner_id, device_id))
 	}

src/service/oidc/registrar.rs

@@ -3,7 +3,7 @@ use url::Url;
 use std::collections::HashMap;
 use std::iter::{Extend, FromIterator};
 
-use crate::oidc::SCOPE_PREFIX_DEVICE;
+use conduwuit::{Result, err};
 use oxide_auth::endpoint::{PreGrant, Scope};
 use oxide_auth::primitives::prelude::{Client, ClientUrl};
 use oxide_auth::primitives::registrar::{Argon2, BoundClient, EncodedClient, PasswordPolicy, RegisteredClient, RegisteredUrl, Registrar, RegistrarError};
@@ -39,7 +39,7 @@ static DEFAULT_PASSWORD_POLICY: Lazy<Argon2> = Lazy::new(Argon2::default);
 #[derive(Clone)]
 pub struct MatrixClient {
 	pub name: Option<String>,
-	// scope option ?,
+	pub device_id: Option<String>,
 	pub client: EncodedClient,
 }
 
@@ -56,15 +56,25 @@ impl MatrixClientMap {
         MatrixClientMap::default()
     }
 
-    /// Insert or update the client record with an oxide-auth Client. It will be wrapped in a
-	/// MatrixClient.
+    /// Insert or update the client record with an oxide-auth OIDC Client.
+	/// This should only be called from the OIDC register endpoint.
     pub fn register_client(&mut self, name: Option<String>, client: Client) {
         let password_policy = Self::current_policy(&self.password_policy);
 		let client = client.encode(password_policy);
-		let matrix_client = MatrixClient { name, client: client.clone() };
+		// Matrix clients have no device_id at registration time.
+		let matrix_client = MatrixClient { name, device_id: None, client: client.clone() };
 		self.clients.insert(client.client_id.clone(), matrix_client);
     }
 
+	/// Add `device_id` to client stored at `client_id`.
+	pub fn set_client_device_id(&mut self, client_id: &str, device_id: &str) -> Result<()> {
+		let client = self.clients.get_mut(client_id).ok_or_else(||
+			err!(Request(Unknown("a client under client_id"))))?;
+		client.device_id = Some(device_id.to_string());
+
+		Ok(())
+	}
+
 	/// Returns a MatrixClient, containing an oxide-auth EncodedClient, and some metadata,
 	/// like a public name.
 	pub fn get_client(&self, client_id: &str) -> Option<&MatrixClient> {
@@ -72,11 +82,9 @@ impl MatrixClientMap {
 	}
 
 	pub fn find_device(&self, device_id: &str) -> Option<&EncodedClient> {
-		let scope = format!("{SCOPE_PREFIX_DEVICE}:{device_id}");
-
 		self.clients
 			.values()
-			.find(|c| c.client.default_scope.iter().any(|s| s == scope))
+			.find(|c| c.device_id.as_deref().is_some_and(|d| d == device_id))
 			.map(|c| &c.client)
 	}