Draft: OIDC native login by sandhose · Pull Request #660 · element-hq/hydrogen-web

sandhose · 2022-02-04T09:34:04Z

This is on top of #655, so ignore the first few commits.

There are some changes that could be extracted in separate PRs if needed, including:

the refresh token mechanism, which could be adapted to work for MSC2918 (WIP: MSC2918 initial implementation #235)
I added a ObservableValue#map method, which creates an observable from another one plus a mapper ; basically like #flatMap but not with the flat part
Make the accessToken an Observable in the HSAPI (needed for the refresh token)
The ability to override the access token for a single HSAPI call

There are still some stuff to do, especially around teardown, like pausing the token refresher when the session is not active

bwindels

Thank you for your PR! Great start, while it still seems a bit WIP. I like the observableValue.map()! I had a first look and left some comments. In a few places it seems to deviate a bit from how things are doing for SSO, which is probably not intentional, unless I'm missing something?

bwindels · 2022-02-11T10:45:25Z

+        this._authorizationEndpoint = null;
+        this._api = new OidcApi({
+            clientId: "hydrogen-web",
+            issuer: options.loginOptions.oidc.issuer,


consider only passing in the oidc login method here, as you don't need the other ones?

I did it this way so it is consistent with StartSSOLoginViewModel

bwindels · 2022-02-11T10:50:22Z

+    ): Promise<string> {
+        const encoder = new TextEncoder();
+        const data = encoder.encode(codeVerifier);
+        const digest = await window.crypto.subtle.digest("SHA-256", data);


you can use platform.crypto.digest("SHA-256", data) here, which deals better with cross-browser differences and maybe one day different platforms.

Done in ea1bcd1

bwindels · 2022-02-11T10:51:47Z

+    async _generateCodeChallenge(
+        codeVerifier: string
+    ): Promise<string> {
+        const encoder = new TextEncoder();


You can use platform.encoding.utf8.encode() here, which deals better with cross-browser differences and maybe one day different platforms.

Done in ea1bcd1

bwindels · 2022-02-11T10:59:52Z

+    }
+
+    get redirectUri() {
+        return window.location.origin;


We generally don't use DOM apis like window in any code not in platform/web, see startSSOLogin in StartSSOLoginViewModel how to deal with this.

Fixed in 69a3404

I think this property can be removed now?

bwindels · 2022-02-11T15:14:38Z

        case undefined:
            // allowed root segments
-            return type === "login"  || type === "session" || type === "sso" || type === "logout";
+            return type === "login" || type === "session" || type === "sso" || type === "logout" || type === "oidc-callback" || type === "oidc-error";


Why not just go with one oidc segment for both an error and callback, with different values? Would reduce the amount of boilerplate I think.

bwindels · 2022-02-11T15:17:29Z

+        return t.div({ className: "StartOIDCLoginView" },
+            t.a({
+                className: "StartOIDCLoginView_button button-action secondary",
+                href: vm => (vm.isBusy ? "#" : vm.authorizationEndpoint),


As mentioned with the view model, I'd only calculate the redirect url once we've clicked this button/link. Once it's clicked, we should show a spinner so people don't think it didn't work. Once the redirect url is known, we programatically redirect.

Done in 6205478

bwindels · 2022-02-11T15:19:24Z

+            this.platform.settingsStorage.setString(`oidc_${p.state}_issuer`, this._api.issuer),
+        ]);
+
+        this._authorizationEndpoint = await this._api.authorizationEndpoint(p);


Could we postpone this until the button is clicked? It could be confusing that the button doesn't work as long as this request is running. If we do it afterwards, we can show a spinner until we can redirect.

Done in 6205478

bwindels · 2022-02-11T15:34:10Z

+                accessToken: sessionInfo.accessToken,
+                accessTokenExpiresAt: sessionInfo.accessTokenExpiresAt,
+                refreshToken: sessionInfo.refreshToken,
+                anticipation: 30 * 1000,


What is this? A margin to make sure we're never too late?

Yep, we refresh the token 30s before it expires to avoid doing requests with a just-expired token

Can you add a comment to the token refresher please?

bwindels · 2022-02-11T15:39:38Z

+            const { homeserver, issuer } = await lookupHomeserver(initialHomeserver, (url, options) => {
                return setAbortable(this._platform.request(url, options));
            });
+            if (issuer) {


This should follow the same pattern as the other login options (e.g. set all the applicable ones in _parseLoginOptions, and put the code below in a separate class, see SSOLoginHelper), unless there's a reason that wouldn't work?

The logic behind this is that if the sever advertises OIDC, we most probably want to use it. There will be a time where m.org will support both OIDC-based login and using the legacy endpoints, and we don't want to have a confusing UI during that time?

Right, I'd prefer that decision to be taken higher up the stack though, in LoginViewModel, and not in SDK-level code. So I'd not prevent other login options from being advertised here in case OIDC support is found, and implement OIDC it just like the other options. And then choose to ignore the other options in the LoginViewModel.

bwindels

Thanks for the changes, I've added some more comments.

bwindels · 2022-03-25T09:30:41Z

+        } = options;
+        this._request = options.platform.request;
+        this._encoding = options.platform.encoding;
+        this._crypto = options.platform.crypto;


Nit: In view models, there no need to store the properties of the options in member variables as the options are stored in the ViewModel base class. In this case, you can just do this.platform.crypto/encoding/request at any point in a view model.

bwindels · 2022-03-25T09:35:32Z

+        this._code = code;
+        this._attemptLogin = attemptLogin;
+        this._errorMessage = "";
+        this.performOIDCLoginCompletion();


Generally we try (there is some places where we sin though, like LoginViewModel) to not call async methods from the constructor (which can't be async itself), unless there is really no other way and we can be 100% sure the method wont throw.

Usually, we deal with this by adding an async start or init method that is called from the parent view model after creating the child view model.

bwindels · 2022-03-25T09:41:01Z

+                        oidc: { issuer },
+                    };
+                } catch (e) {
+                    console.log(e);


This code might change wrt to the comment above, but wrt to console logging, it is usually only used during development. For production code we use the structured logging api from src/logging/

You could log an operation here by wrapping it in

return this._platform.logger.run("queryLogin", async log => { // log in here is an ILogItem, happy to explain the API if needed // pass it through the call stack to log create a tree of log items });

bwindels · 2022-03-25T09:45:11Z

        return window.location.origin;
    }

+    createOIDCRedirectURL() {


Just realized this is also not in /src/platform/web, but we can clean that up with the method above some other time, so fine for now 👍

bwindels · 2022-03-25T09:46:11Z

        <meta name="apple-mobile-web-app-status-bar-style" content="black">
        <meta name="apple-mobile-web-app-title" content="Hydrogen Chat">
        <meta name="description" content="A matrix chat application">
+        <script src="config.js"></script>


Can we take this out of this PR though?

bwindels · 2022-03-25T11:18:30Z

    // substr(1) to take of initial /
    const parts = urlPath.substr(1).split("/");
    const iterator = parts[Symbol.iterator]();
-    const segments = [];


why is this removed?

bwindels · 2022-03-25T11:18:52Z

    const iterator = parts[Symbol.iterator]();
-    const segments = [];
-    let next; 
+    let next;


What is going on here?

bwindels · 2022-03-25T11:20:18Z

            case "right-panel":
            case "sso":
+            case "oidc-callback":
+            case "oidc-error":


should both of these be just "oidc"?

bwindels · 2022-03-25T11:25:31Z

+
+    get(): C {
+        const sourceValue = this.source.get();
+        return this.mapper(sourceValue);


if the mapper creates a new object, the result of multiple get calls won't be equal to each other as the mapper is run each time. Probably better to run the mapper on update once and store the result in a member variable.

bwindels · 2022-03-25T11:27:01Z

    uploadProgress?: (loadedBytes: number) => void;
    timeout?: number;
-    body?: EncodedBody;
+    body?: EncodedBody["body"];


what is this about?

This also saves the redirectUri during the flow

Design matches matrix-org/matrix-react-sdk#8681

sandhose marked this pull request as draft February 4, 2022 09:34

sandhose requested a review from bwindels February 4, 2022 09:34

bwindels reviewed Feb 11, 2022

View reviewed changes

bwindels mentioned this pull request Feb 16, 2022

Docker image improvements #655

Closed

richvdh mentioned this pull request Mar 2, 2022

Consider replacing our auth system with OAuth2 matrix-org/matrix-spec#636

Closed

sandhose force-pushed the sandhose/oidc-login branch from 4fe85d2 to 6205478 Compare March 3, 2022 12:43

sandhose requested a review from bwindels March 14, 2022 08:16

sandhose force-pushed the sandhose/oidc-login branch from 9cc6f4b to eaf876a Compare March 18, 2022 08:41

bwindels marked this pull request as ready for review March 25, 2022 09:13

bwindels reviewed Mar 25, 2022

View reviewed changes

sandhose force-pushed the sandhose/oidc-login branch 2 times, most recently from de8c561 to 2ad96c7 Compare April 25, 2022 07:32

sandhose force-pushed the sandhose/oidc-login branch from 7b0e045 to 12d1760 Compare July 4, 2022 16:44

sandhose force-pushed the sandhose/oidc-login branch 2 times, most recently from e23a06b to f7ffae4 Compare August 1, 2022 14:55

bwindels mentioned this pull request Jan 3, 2023

OIDC / Registration #829

Open

1 task

hughns self-requested a review as a code owner January 20, 2023 11:49

sandhose and others added 13 commits February 17, 2023 10:17

Multi-arch capable Dockerfile

a59e4bd

Build and push multi-arch Docker images in CI

5379b83

Make the Docker image configurable at runtime

24c5879

Native OIDC login

2604e6a

Only generate the auth URL and start the login flow on click

b9bca9d

Generate the OIDC redirect URI from the URLRouter

83684c8

This also saves the redirectUri during the flow

Simplify OIDC callback navigation handling

2ba8bc3

Use platform APIs for text encoding and hashing

68daf51

Stop the token refresher when disposing the client

dd8cd31

Typo.

21cf845

OIDC dynamic client registration

46e884b

Add client_uri, tos_uri and policy_uri client metadata

4644004

Make hydrogen generate the device scope

8da49df

sandhose and others added 23 commits February 17, 2023 11:00

Only generate the auth URL and start the login flow on click

37e9727

Improve error handling for OIDC discovery and registration

1ead9bc

Actually make SessionLoadViewModel.logout do something

f177a94

Fix typing and tests

a4c16e5

Fix the runtime config template to include the default theme

896f2b7

Add static client for thirdroom

9ce9e2d

Also build Docker images for the OIDC-login branch

a2370da

Also publish sha-* tags to GHCR

7c40c7c

Manual revert of docker related changes

b4ff736

Fix up merge

13a4299

Never attempt to encode OIDC segments

9c52fb9

FIx regression bug

317d97c

Put static OIDC client config into config file

1716a30

Remove some debug logging

94352da

Reinstate building of OIDC branch docker images

f4b1d99

Fix incorrect reference to OIDC segment type

59b06a0

Offer guest mode login if advertised by OIDC Provider

2739572

Show OIDC sign in errors more sensibly

da86db3

Support sync without filters for guest access

0aafd55

Fix test cases

6580fcf

Store id_token and fix up logout implementation

7b7557a

Account management section design changes

97b8861

Design matches matrix-org/matrix-react-sdk#8681

Add missing param

d6dff1d

hughns force-pushed the sandhose/oidc-login branch from 416ba09 to d6dff1d Compare February 17, 2023 11:14

hughns added 2 commits February 17, 2023 11:28

Revert docker changes to those in master

6acc0ea

Remove unused code from rebase

bde85c9

This was referenced May 29, 2023

OIDC: Attempt dynamic registration with OP element-hq/element-web#25468

Closed

OIDC: Client registration with OP element-hq/element-web#25467

Closed

kerryarchibald mentioned this pull request Jun 13, 2023

OIDC: Make authorization request element-hq/element-web#25574

Closed

kerryarchibald mentioned this pull request Jun 23, 2023

OIDC: Get accessToken and complete login element-hq/element-web#25657

Closed

Conversation

sandhose commented Feb 4, 2022

Uh oh!

bwindels left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bwindels Mar 25, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bwindels Mar 25, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bwindels left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

bwindels left a comment •

edited

Loading

bwindels Mar 25, 2022 •

edited

Loading

bwindels Mar 25, 2022 •

edited

Loading