Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Draft: OIDC native login #660

Open
wants to merge 58 commits into
base: master
Choose a base branch
from

Conversation

sandhose
Copy link
Member

@sandhose sandhose commented Feb 4, 2022

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

@sandhose sandhose marked this pull request as draft February 4, 2022 09:34
Copy link
Contributor

@bwindels bwindels left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

src/domain/login/StartOIDCLoginViewModel.js Show resolved Hide resolved
this._authorizationEndpoint = null;
this._api = new OidcApi({
clientId: "hydrogen-web",
issuer: options.loginOptions.oidc.issuer,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I did it this way so it is consistent with StartSSOLoginViewModel

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done in ea1bcd1

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done in ea1bcd1

}

get redirectUri() {
return window.location.origin;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed in 69a3404

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this property can be removed now?

switch (parent?.type) {
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";
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done!

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done in 6205478

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

this._authorizationEndpoint = await this._api.authorizationEndpoint(p);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done in 6205478

accessToken: sessionInfo.accessToken,
accessTokenExpiresAt: sessionInfo.accessTokenExpiresAt,
refreshToken: sessionInfo.refreshToken,
anticipation: 30 * 1000,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

@bwindels bwindels Mar 25, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you add a comment to the token refresher please?

return setAbortable(this._platform.request(url, options));
});
if (issuer) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Contributor

@bwindels bwindels Mar 25, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

@bwindels bwindels left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

} = options;
this._request = options.platform.request;
this._encoding = options.platform.encoding;
this._crypto = options.platform.crypto;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

this._code = code;
this._attemptLogin = attemptLogin;
this._errorMessage = "";
this.performOIDCLoginCompletion();
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

oidc: { issuer },
};
} catch (e) {
console.log(e);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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
});

@@ -125,6 +125,10 @@ export class URLRouter {
return window.location.origin;
}

createOIDCRedirectURL() {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 👍

@@ -9,6 +9,7 @@
<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>
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we take this out of this PR though?

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why is this removed?

// substr(1) to take of initial /
const parts = urlPath.substr(1).split("/");
const iterator = parts[Symbol.iterator]();
const segments = [];
let next;
let next;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What is going on here?

@@ -191,6 +215,8 @@ export function stringifyPath(path) {
break;
case "right-panel":
case "sso":
case "oidc-callback":
case "oidc-error":
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

should both of these be just "oidc"?


get(): C {
const sourceValue = this.source.get();
return this.mapper(sourceValue);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@@ -21,11 +21,12 @@ import type {ILogItem} from "../../logging/types";
export interface IRequestOptions {
uploadProgress?: (loadedBytes: number) => void;
timeout?: number;
body?: EncodedBody;
body?: EncodedBody["body"];
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

what is this about?

@sandhose sandhose force-pushed the sandhose/oidc-login branch 2 times, most recently from de8c561 to 2ad96c7 Compare April 25, 2022 07:32
@sandhose sandhose force-pushed the sandhose/oidc-login branch 2 times, most recently from e23a06b to f7ffae4 Compare August 1, 2022 14:55
@bwindels bwindels mentioned this pull request Jan 3, 2023
1 task
@hughns hughns self-requested a review as a code owner January 20, 2023 11:49
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants