Fix Native OIDC for Element Desktop (#12253)

* Reuse exported common type Signed-off-by: Michael Telatynski <[email protected]> * Improve client metadata used for OIDC dynamic registration Signed-off-by: Michael Telatynski <[email protected]> * Fix Native OIDC for Element Desktop by including ssoid in the url_state of the /auth call Signed-off-by: Michael Telatynski <[email protected]> * Reuse exported common type Signed-off-by: Michael Telatynski <[email protected]> * Improve client metadata used for OIDC dynamic registration Signed-off-by: Michael Telatynski <[email protected]> * Fix typo Signed-off-by: Michael Telatynski <[email protected]> * Fix test Signed-off-by: Michael Telatynski <[email protected]> * Mock PlatformPeg Signed-off-by: Michael Telatynski <[email protected]> * Mock platform Signed-off-by: Michael Telatynski <[email protected]> * Add comment Signed-off-by: Michael Telatynski <[email protected]> * Improve comment Signed-off-by: Michael Telatynski <[email protected]> * Update src/BasePlatform.ts Co-authored-by: Richard van der Hoff <[email protected]> --------- Signed-off-by: Michael Telatynski <[email protected]> Co-authored-by: Richard van der Hoff <[email protected]>
matrix-org · Feb 19, 2024 · 618462b · 618462b
1 parent c71b8fd
commit 618462b
Show file tree

Hide file tree

Showing 6 changed files with 23 additions and 5 deletions.
diff --git a/src/BasePlatform.ts b/src/BasePlatform.ts
@@ -313,7 +313,11 @@ export default abstract class BasePlatform {
         return null;
     }
 
-    protected getSSOCallbackUrl(fragmentAfterLogin = ""): URL {
+    /**
+     * The URL to return to after a successful SSO/OIDC authentication
+     * @param fragmentAfterLogin optional fragment for specific view to return to
+     */
+    public getSSOCallbackUrl(fragmentAfterLogin = ""): URL {
         const url = new URL(window.location.href);
         url.hash = fragmentAfterLogin;
         return url;
@@ -478,4 +482,12 @@ export default abstract class BasePlatform {
             policyUri: config.privacy_policy_url,
         };
     }
+
+    /**
+     * Suffix to append to the `state` parameter of OIDC /auth calls. Will be round-tripped to the callback URI.
+     * Currently only required for ElectronPlatform for passing element-desktop-ssoid.
+     */
+    public getOidcClientState(): string {
+        return "";
+    }
 }
diff --git a/src/Lifecycle.ts b/src/Lifecycle.ts
@@ -777,7 +777,7 @@ async function createOidcTokenRefresher(credentials: IMatrixClientCreds): Promis
     try {
         const clientId = getStoredOidcClientId();
         const idTokenClaims = getStoredOidcIdTokenClaims();
-        const redirectUri = window.location.origin;
+        const redirectUri = PlatformPeg.get()!.getSSOCallbackUrl().href;
         const deviceId = credentials.deviceId;
         if (!deviceId) {
             throw new Error("Expected deviceId in user credentials.");

diff --git a/src/stores/oidc/OidcClientStore.ts b/src/stores/oidc/OidcClientStore.ts
@@ -21,6 +21,7 @@ import { OidcClient } from "oidc-client-ts";
 
 import { getStoredOidcTokenIssuer, getStoredOidcClientId } from "../../utils/oidc/persistOidcSettings";
 import { getDelegatedAuthAccountUrl } from "../../utils/oidc/getDelegatedAuthAccountUrl";
+import PlatformPeg from "../../PlatformPeg";
 
 /**
  * @experimental
@@ -139,7 +140,7 @@ export class OidcClientStore {
                 ...metadata,
                 authority: metadata.issuer,
                 signingKeys,
-                redirect_uri: window.location.origin,
+                redirect_uri: PlatformPeg.get()!.getSSOCallbackUrl().href,
                 client_id: clientId,
             });
         } catch (error) {

diff --git a/src/utils/oidc/authorize.ts b/src/utils/oidc/authorize.ts
@@ -21,6 +21,7 @@ import { randomString } from "matrix-js-sdk/src/randomstring";
 import { IdTokenClaims } from "oidc-client-ts";
 
 import { OidcClientError } from "./error";
+import PlatformPeg from "../../PlatformPeg";
 
 /**
  * Start OIDC authorization code flow
@@ -39,7 +40,7 @@ export const startOidcLogin = async (
     identityServerUrl?: string,
     isRegistration?: boolean,
 ): Promise<void> => {
-    const redirectUri = window.location.origin;
+    const redirectUri = PlatformPeg.get()!.getSSOCallbackUrl().href;
 
     const nonce = randomString(10);
 
@@ -53,6 +54,7 @@ export const startOidcLogin = async (
         identityServerUrl,
         nonce,
         prompt,
+        urlState: PlatformPeg.get()?.getOidcClientState(),
     });
 
     window.location.href = authorizationUrl;

diff --git a/test/stores/oidc/OidcClientStore-test.ts b/test/stores/oidc/OidcClientStore-test.ts
@@ -23,7 +23,7 @@ import { discoverAndValidateAuthenticationConfig } from "matrix-js-sdk/src/oidc/
 import { OidcError } from "matrix-js-sdk/src/oidc/error";
 
 import { OidcClientStore } from "../../../src/stores/oidc/OidcClientStore";
-import { flushPromises, getMockClientWithEventEmitter } from "../../test-utils";
+import { flushPromises, getMockClientWithEventEmitter, mockPlatformPeg } from "../../test-utils";
 import { mockOpenIdConfiguration } from "../../test-utils/oidc";
 
 jest.mock("matrix-js-sdk/src/oidc/discovery", () => ({
@@ -58,6 +58,7 @@ describe("OidcClientStore", () => {
         jest.spyOn(logger, "error").mockClear();
 
         fetchMock.get(`${metadata.issuer}.well-known/openid-configuration`, metadata);
+        mockPlatformPeg();
     });
 
     describe("isUserAuthenticatedWithOidc()", () => {

diff --git a/test/utils/oidc/authorize-test.ts b/test/utils/oidc/authorize-test.ts
@@ -23,6 +23,7 @@ import { mocked } from "jest-mock";
 import { completeOidcLogin, startOidcLogin } from "../../../src/utils/oidc/authorize";
 import { makeDelegatedAuthConfig } from "../../test-utils/oidc";
 import { OidcClientError } from "../../../src/utils/oidc/error";
+import { mockPlatformPeg } from "../../test-utils";
 
 jest.unmock("matrix-js-sdk/src/randomstring");
 
@@ -53,6 +54,7 @@ describe("OIDC authorization", () => {
         };
 
         jest.spyOn(randomStringUtils, "randomString").mockRestore();
+        mockPlatformPeg();
     });
 
     beforeAll(() => {