Skip to content

Commit c8da9cb

Browse files
zecakehstefanceriu
authored andcommitted
refactor(oauth): Remove the issuer from OAuthAuthData
It is actually unused, and now that we only need homeserver URLs for static registrations, users don't need to access it easily. Signed-off-by: Kévin Commaille <zecakeh@tedomum.fr>
1 parent 6789389 commit c8da9cb

File tree

10 files changed

+59
-152
lines changed

10 files changed

+59
-152
lines changed

bindings/matrix-sdk-ffi/src/client.rs

Lines changed: 2 additions & 30 deletions
Original file line numberDiff line numberDiff line change
@@ -43,10 +43,7 @@ use matrix_sdk_ui::notification_client::{
4343
};
4444
use mime::Mime;
4545
use ruma::{
46-
api::client::{
47-
alias::get_alias, discovery::discover_homeserver::AuthenticationServerInfo,
48-
error::ErrorKind, uiaa::UserIdentifier,
49-
},
46+
api::client::{alias::get_alias, error::ErrorKind, uiaa::UserIdentifier},
5047
events::{
5148
ignored_user_list::IgnoredUserListEventContent,
5249
key::verification::request::ToDeviceKeyVerificationRequestEvent,
@@ -1659,10 +1656,9 @@ impl Session {
16591656
let matrix_sdk::authentication::oauth::UserSession {
16601657
meta: matrix_sdk::SessionMeta { user_id, device_id },
16611658
tokens: matrix_sdk::SessionTokens { access_token, refresh_token },
1662-
issuer,
16631659
} = api.user_session().context("Missing session")?;
16641660
let client_id = api.client_id().context("OIDC client ID is missing.")?.clone();
1665-
let oidc_data = OidcSessionData { client_id, issuer };
1661+
let oidc_data = OidcSessionData { client_id };
16661662

16671663
let oidc_data = serde_json::to_string(&oidc_data).ok();
16681664
Ok(Session {
@@ -1707,7 +1703,6 @@ impl TryFrom<Session> for AuthSession {
17071703
device_id: device_id.into(),
17081704
},
17091705
tokens: matrix_sdk::SessionTokens { access_token, refresh_token },
1710-
issuer: oidc_data.issuer,
17111706
};
17121707

17131708
let session = OAuthSession { client_id: oidc_data.client_id, user: user_session };
@@ -1731,31 +1726,8 @@ impl TryFrom<Session> for AuthSession {
17311726
/// Represents a client registration against an OpenID Connect authentication
17321727
/// issuer.
17331728
#[derive(Serialize, Deserialize)]
1734-
#[serde(try_from = "OidcSessionDataDeHelper")]
17351729
pub(crate) struct OidcSessionData {
17361730
client_id: ClientId,
1737-
issuer: Url,
1738-
}
1739-
1740-
#[derive(Deserialize)]
1741-
struct OidcSessionDataDeHelper {
1742-
client_id: ClientId,
1743-
issuer_info: Option<AuthenticationServerInfo>,
1744-
issuer: Option<Url>,
1745-
}
1746-
1747-
impl TryFrom<OidcSessionDataDeHelper> for OidcSessionData {
1748-
type Error = String;
1749-
1750-
fn try_from(value: OidcSessionDataDeHelper) -> Result<Self, Self::Error> {
1751-
let OidcSessionDataDeHelper { client_id, issuer_info, issuer } = value;
1752-
1753-
let issuer = issuer
1754-
.or(issuer_info.and_then(|info| Url::parse(&info.issuer).ok()))
1755-
.ok_or_else(|| "missing field `issuer`".to_owned())?;
1756-
1757-
Ok(Self { client_id, issuer })
1758-
}
17591731
}
17601732

17611733
#[derive(uniffi::Enum)]

crates/matrix-sdk/CHANGELOG.md

Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -241,6 +241,11 @@ simpler methods:
241241
- [**breaking**] `OidcRegistrations` was removed. Clients are supposed to
242242
re-register with the homeserver for every login.
243243
([#4879](https://github.com/matrix-org/matrix-rust-sdk/pull/4879))
244+
- [**breaking**] The `OAuth::restore_registered_client()` doesn't take an
245+
`issuer` anymore.
246+
([#4879](https://github.com/matrix-org/matrix-rust-sdk/pull/4879))
247+
- `OAuth::issuer()` was removed.
248+
- The `issuer` field of `UserSession` was removed.
244249

245250
## [0.10.0] - 2025-02-04
246251

crates/matrix-sdk/src/authentication/oauth/auth_code_builder.rs

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -102,7 +102,7 @@ impl OAuthAuthCodeUrlBuilder {
102102

103103
let data = oauth.data().expect("OAuth 2.0 data should be set after registration");
104104
info!(
105-
issuer = data.issuer.as_str(),
105+
issuer = server_metadata.issuer.as_str(),
106106
?scopes,
107107
"Authorizing scope via the OAuth 2.0 Authorization Code flow"
108108
);

crates/matrix-sdk/src/authentication/oauth/cross_process.rs

Lines changed: 8 additions & 30 deletions
Original file line numberDiff line numberDiff line change
@@ -293,10 +293,7 @@ mod tests {
293293
let session_hash = compute_session_hash(&tokens);
294294
client
295295
.oauth()
296-
.restore_session(
297-
mock_session(tokens.clone(), "https://oauth.example.com/issuer"),
298-
RoomLoadSettings::default(),
299-
)
296+
.restore_session(mock_session(tokens.clone()), RoomLoadSettings::default())
300297
.await?;
301298

302299
assert_eq!(client.session_tokens().unwrap(), tokens);
@@ -324,12 +321,8 @@ mod tests {
324321
server.mock_who_am_i().ok().expect(1).named("whoami").mount().await;
325322

326323
let tmp_dir = tempfile::tempdir()?;
327-
let client = server
328-
.client_builder()
329-
.sqlite_store(&tmp_dir)
330-
.registered_with_oauth(server.server().uri())
331-
.build()
332-
.await;
324+
let client =
325+
server.client_builder().sqlite_store(&tmp_dir).registered_with_oauth().build().await;
333326
let oauth = client.oauth();
334327

335328
// Enable cross-process lock.
@@ -389,7 +382,7 @@ mod tests {
389382
// Restore the session.
390383
oauth
391384
.restore_session(
392-
mock_session(mock_prev_session_tokens_with_refresh(), server.server().uri()),
385+
mock_session(mock_prev_session_tokens_with_refresh()),
393386
RoomLoadSettings::default(),
394387
)
395388
.await?;
@@ -417,7 +410,6 @@ mod tests {
417410
#[async_test]
418411
async fn test_cross_process_concurrent_refresh() -> anyhow::Result<()> {
419412
let server = MatrixMockServer::new().await;
420-
let issuer = server.server().uri();
421413

422414
let oauth_server = server.oauth();
423415
oauth_server.mock_server_metadata().ok().expect(1..).named("server_metadata").mount().await;
@@ -434,10 +426,7 @@ mod tests {
434426
oauth.enable_cross_process_refresh_lock("client1".to_owned()).await?;
435427

436428
oauth
437-
.restore_session(
438-
mock_session(prev_tokens.clone(), issuer.clone()),
439-
RoomLoadSettings::default(),
440-
)
429+
.restore_session(mock_session(prev_tokens.clone()), RoomLoadSettings::default())
441430
.await?;
442431

443432
// Create a second client, without restoring it, to test that a token update
@@ -455,10 +444,7 @@ mod tests {
455444
let oauth3 = client3.oauth();
456445
oauth3.enable_cross_process_refresh_lock("client3".to_owned()).await?;
457446
oauth3
458-
.restore_session(
459-
mock_session(prev_tokens.clone(), issuer.clone()),
460-
RoomLoadSettings::default(),
461-
)
447+
.restore_session(mock_session(prev_tokens.clone()), RoomLoadSettings::default())
462448
.await?;
463449

464450
// Run a refresh in the second client; this will invalidate the tokens from the
@@ -492,10 +478,7 @@ mod tests {
492478
)?;
493479

494480
oauth
495-
.restore_session(
496-
mock_session(prev_tokens.clone(), issuer),
497-
RoomLoadSettings::default(),
498-
)
481+
.restore_session(mock_session(prev_tokens.clone()), RoomLoadSettings::default())
499482
.await?;
500483

501484
// And this client is now aware of the latest tokens.
@@ -572,12 +555,7 @@ mod tests {
572555

573556
// Restore the session.
574557
let tokens = mock_session_tokens_with_refresh();
575-
oauth
576-
.restore_session(
577-
mock_session(tokens.clone(), server.server().uri()),
578-
RoomLoadSettings::default(),
579-
)
580-
.await?;
558+
oauth.restore_session(mock_session(tokens.clone()), RoomLoadSettings::default()).await?;
581559

582560
oauth.logout().await.unwrap();
583561

crates/matrix-sdk/src/authentication/oauth/mod.rs

Lines changed: 10 additions & 38 deletions
Original file line numberDiff line numberDiff line change
@@ -225,7 +225,6 @@ impl OAuthCtx {
225225
}
226226

227227
pub(crate) struct OAuthAuthData {
228-
pub(crate) issuer: Url,
229228
pub(crate) client_id: ClientId,
230229
/// The data necessary to validate authorization responses.
231230
authorization_data: Mutex<HashMap<CsrfToken, AuthorizationValidationData>>,
@@ -234,9 +233,7 @@ pub(crate) struct OAuthAuthData {
234233
#[cfg(not(tarpaulin_include))]
235234
impl fmt::Debug for OAuthAuthData {
236235
fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result {
237-
f.debug_struct("OAuthAuthData")
238-
.field("issuer", &self.issuer.as_str())
239-
.finish_non_exhaustive()
236+
f.debug_struct("OAuthAuthData").finish_non_exhaustive()
240237
}
241238
}
242239

@@ -457,7 +454,7 @@ impl OAuth {
457454
.or_else(|| static_registrations.get(&server_metadata.issuer));
458455

459456
if let Some(client_id) = client_id {
460-
self.restore_registered_client(server_metadata.issuer.clone(), client_id.clone());
457+
self.restore_registered_client(client_id.clone());
461458
return Ok(());
462459
}
463460
}
@@ -467,15 +464,6 @@ impl OAuth {
467464
Ok(())
468465
}
469466

470-
/// The OAuth 2.0 authorization server used for authorization.
471-
///
472-
/// Returns `None` if the client was not registered or if the registration
473-
/// was not restored with [`OAuth::restore_registered_client()`] or
474-
/// [`OAuth::restore_session()`].
475-
pub fn issuer(&self) -> Option<&Url> {
476-
self.data().map(|data| &data.issuer)
477-
}
478-
479467
/// The account management actions supported by the authorization server's
480468
/// account management URL.
481469
///
@@ -629,8 +617,7 @@ impl OAuth {
629617
pub fn user_session(&self) -> Option<UserSession> {
630618
let meta = self.client.session_meta()?.to_owned();
631619
let tokens = self.client.session_tokens()?;
632-
let issuer = self.data()?.issuer.clone();
633-
Some(UserSession { meta, tokens, issuer })
620+
Some(UserSession { meta, tokens })
634621
}
635622

636623
/// The full OAuth 2.0 session of this client.
@@ -655,11 +642,6 @@ impl OAuth {
655642
///
656643
/// * `client_metadata` - The serialized client metadata to register.
657644
///
658-
/// The client ID in the response should be persisted for future use and
659-
/// reused for the same authorization server, identified by the
660-
/// [`OAuth::issuer()`], along with the client metadata sent to the server,
661-
/// even for different sessions or user accounts.
662-
///
663645
/// # Panic
664646
///
665647
/// Panics if the authentication data was already set.
@@ -672,7 +654,7 @@ impl OAuth {
672654
/// # use matrix_sdk::authentication::oauth::registration::ClientMetadata;
673655
/// # use ruma::serde::Raw;
674656
/// # let client_metadata = unimplemented!();
675-
/// # fn persist_client_registration (_: &url::Url, _: &Raw<ClientMetadata>, _: &ClientId) {}
657+
/// # fn persist_client_registration (_: url::Url, _: &ClientId) {}
676658
/// # _ = async {
677659
/// let server_name = ServerName::parse("myhomeserver.org")?;
678660
/// let client = Client::builder().server_name(&server_name).build().await?;
@@ -697,9 +679,8 @@ impl OAuth {
697679
///
698680
/// // The API only supports clients without secrets.
699681
/// let client_id = response.client_id;
700-
/// let issuer = oauth.issuer().expect("issuer should be set after registration");
701682
///
702-
/// persist_client_registration(issuer, &client_metadata, &client_id);
683+
/// persist_client_registration(client.homeserver(), &client_id);
703684
/// # anyhow::Ok(()) };
704685
/// ```
705686
pub async fn register_client(
@@ -725,10 +706,7 @@ impl OAuth {
725706

726707
// The format of the credentials changes according to the client metadata that
727708
// was sent. Public clients only get a client ID.
728-
self.restore_registered_client(
729-
server_metadata.issuer.clone(),
730-
registration_response.client_id.clone(),
731-
);
709+
self.restore_registered_client(registration_response.client_id.clone());
732710

733711
Ok(registration_response)
734712
}
@@ -744,17 +722,14 @@ impl OAuth {
744722
///
745723
/// # Arguments
746724
///
747-
/// * `issuer` - The authorization server that was used to register the
748-
/// client.
749-
///
750725
/// * `client_id` - The unique identifier to authenticate the client with
751726
/// the server, obtained after registration.
752727
///
753728
/// # Panic
754729
///
755730
/// Panics if authentication data was already set.
756-
pub fn restore_registered_client(&self, issuer: Url, client_id: ClientId) {
757-
let data = OAuthAuthData { issuer, client_id, authorization_data: Default::default() };
731+
pub fn restore_registered_client(&self, client_id: ClientId) {
732+
let data = OAuthAuthData { client_id, authorization_data: Default::default() };
758733

759734
self.client
760735
.auth_ctx()
@@ -783,9 +758,9 @@ impl OAuth {
783758
session: OAuthSession,
784759
room_load_settings: RoomLoadSettings,
785760
) -> Result<()> {
786-
let OAuthSession { client_id, user: UserSession { meta, tokens, issuer } } = session;
761+
let OAuthSession { client_id, user: UserSession { meta, tokens } } = session;
787762

788-
let data = OAuthAuthData { issuer, client_id, authorization_data: Default::default() };
763+
let data = OAuthAuthData { client_id, authorization_data: Default::default() };
789764

790765
self.client.auth_ctx().set_session_tokens(tokens.clone());
791766
self.client
@@ -1434,9 +1409,6 @@ pub struct UserSession {
14341409
/// The tokens used for authentication.
14351410
#[serde(flatten)]
14361411
pub tokens: SessionTokens,
1437-
1438-
/// The OAuth 2.0 server used for this session.
1439-
pub issuer: Url,
14401412
}
14411413

14421414
/// The data necessary to validate a response from the Token endpoint in the

0 commit comments

Comments
 (0)