UI for linking/unlinking the Upstream Providers #3245

MTRNord · 2024-09-25T23:14:49Z

Todos:

Allow linking the Provider
Allow unlinking the Provider
Improve the UI

CLAassistant · 2024-09-25T23:14:56Z

Thank you for your submission! We really appreciate it. Like many open source projects, we ask that you sign our Contributor License Agreement before we can accept your contribution.
_{You have signed the CLA already but the status is still pending? Let us recheck it.}

crates/handlers/src/graphql/model/upstream_oauth.rs

...storage-pg/.sqlx/query-100c6cfa46133f3b196b00ccf8f33ba0bcb4bb1afc1985092dd8ec51b40f631a.json

MTRNord · 2024-09-30T20:23:07Z

Just to keep you updated on whats missing from my POV:

The unlink button should have a way to display the error. I dont think it makes sense to actually show the error but it should show that something went wrong. I will have a look how that works in other places :)

One thing I have to check is actually the pagination. I currently think that the email and the upstream list will clash as they both use the same hook which (I think?) uses query parameters. I am not sure how to solve this issue though. Maybe thats beyond scope of this PR? Maybe not?

sandhose · 2024-09-30T20:28:49Z

One thing I have to check is actually the pagination. I currently think that the email and the upstream list will clash as they both use the same hook which (I think?) uses query parameters. I am not sure how to solve this issue though. Maybe thats beyond scope of this PR? Maybe not?

It's a local useState so no need to worry about them

matrix-authentication-service/frontend/src/pagination.ts

Lines 51 to 76 in 87f3452

    
           export const usePagination = ( 
        
             pageSize = 6, 
        
           ): [Pagination, (action: Action) => void] => { 
        
             const [pagination, setPagination] = useState<Pagination>({ 
        
               first: pageSize, 
        
               after: undefined, 
        
             }); 
        
             const handlePagination = (action: Action): void => { 
        
               if (action === FIRST_PAGE) { 
        
                 setPagination({ 
        
                   first: pageSize, 
        
                   after: undefined, 
        
                 }); 
        
               } else if (action === LAST_PAGE) { 
        
                 setPagination({ 
        
                   last: pageSize, 
        
                   before: undefined, 
        
                 }); 
        
               } else { 
        
                 setPagination(action); 
        
               } 
        
             }; 
        
             return [pagination, handlePagination]; 
        
           };

I try to keep state in the URL as much as possible, but sometimes in 'embedded' lists that rarely paginate, it's fine to have local state like that

sandhose · 2024-09-30T20:30:35Z

Just to keep you updated on whats missing from my POV:

And just to keep a note before I forget: it would be nice to be able to set in the config whether it's possible for user to link or unlink accounts themselves, as the administrator might not want that.

You would do that by adding it to the account config, then propagate it in the SiteConfig and in the GraphQL API

…showing existing linked providers Signed-off-by: MTRNord <[email protected]>

Signed-off-by: MTRNord <[email protected]>

…tom function Signed-off-by: MTRNord <[email protected]>

Signed-off-by: MTRNord <[email protected]>

…not end up in an in-between state in case of a database error Signed-off-by: MTRNord <[email protected]>

…on if no providers exist Signed-off-by: MTRNord <[email protected]>

sandhose

I've skimmed through it, thanks a lot for working on this.

Sorry if the first round of review is a bit rough, but I think I would like to see this change split in 4:

changes on the storage layer for the unlink functionality
add a config flag to allow linking/unlinking of new accounts. This should gate existing linking flow, probably default to false
all the new GraphQL API you need
then the actual frontend

This would make it easier to review and get all the bricks together

Also I've asked our design team to come up with actual designs for these screens, I'll share that once I have them

sandhose · 2024-10-02T09:30:26Z

frontend/src/components/UpstreamProvider/LinkUpstreamProvider.tsx

+    e.preventDefault();
+
+    setInProgress(true);
+    const upstreamURL = `/upstream/authorize/${data.id.replace("upstream_oauth2_provider:", "")}`;


This is very brittle and relies on an implementation detail of the GraphQL API.

It also doesn't work if MAS is mounted on a subpath.

I would rather add a authorizeUrl or something to the GraphQL API on the provider

I ideally wanted to use the Router here but i wasnt able to figure out how to make it be on the root of the page. I guess a better solution would be to provide the correct path from the server right? That way it Accounts for settings too.

Oops did not read far enough. Graphql it is. :D

sandhose · 2024-10-02T09:31:29Z

frontend/src/components/UpstreamProvider/LinkUpstreamProvider.tsx

+      <Button
+        type="button"
+        kind="primary"
+        onClick={onConfirm}
+        disabled={disabled ?? inProgress}
+        Icon={disabled ?? inProgress ? undefined : IconUserAdd}
+      >
+        {inProgress && <LoadingSpinner inline />}
+        {t("frontend.link_upstream_button.text", {
+          provider: data?.humanName ?? "Unknown",
+        })}
+      </Button>


Couldn't that just be an actual link?

It probably could. I just went for the Design here in the end. A link is probably more semantically correct though. 🤔

sandhose · 2024-10-02T09:32:44Z

frontend/src/components/UserProfile/UpstreamProviderList.tsx

@@ -0,0 +1,129 @@
+// Copyright 2024 New Vector Ltd.
+// Copyright 2023, 2024 The Matrix.org Foundation C.I.C.


Suggested change

// Copyright 2023, 2024 The Matrix.org Foundation C.I.C.

sandhose · 2024-10-02T13:57:43Z

crates/handlers/src/graphql/mutations/upstream_oauth.rs

+        }
+
+        // Allow non-admins to remove their email address if the site config allows it
+        if !requester.is_admin() && !state.site_config().email_change_allowed {


That should be another site config flag

sandhose · 2024-10-02T14:01:36Z

crates/handlers/src/graphql/model/upstream_oauth.rs

+    /// UpstreamOAuth2Links associated with this provider for the current user.
+    pub async fn upstream_oauth2_links_for_user(
+        &self,
+        ctx: &Context<'_>,
+    ) -> Result<Vec<UpstreamOAuth2Link>, async_graphql::Error> {


Not convinced it is good to have it at this level. I would rather have a list on the User to fetch links, and have the frontend do the stitching together of both lists?

sandhose · 2024-10-02T14:02:45Z

crates/storage-pg/src/upstream_oauth2/link.rs

+            r#"
+                    UPDATE upstream_oauth_authorization_sessions SET upstream_oauth_link_id = NULL
+                    WHERE upstream_oauth_link_id = $1
+                "#,


Suggested change

"#,

"#,

sandhose · 2024-10-02T14:13:24Z

crates/storage-pg/src/upstream_oauth2/link.rs

+        // constraint on the links.
+        sqlx::query!(
+            r#"
+                    UPDATE upstream_oauth_authorization_sessions SET upstream_oauth_link_id = NULL


It looks like I modelled the link on the rust side to not be possible to be in this state:

matrix-authentication-service/crates/data-model/src/upstream_oauth2/session.rs

Lines 23 to 28 in 44c31ed

Consumed {

completed_at: DateTime<Utc>,

consumed_at: DateTime<Utc>,

link_id: Ulid,

id_token: Option<String>,

},

I think we should:

add an 'unlinked_at' timestamp

add a variant to this enum like
Unlinked { competed_at: DateTime<Utc>, consumed_at: Option<DateTime<Utc>>, unlinked_at: DateTime<Utc>, id_token: Option<String> }

set this timestamp when we null this. This also means passing the clock to this remove function

Could you split out the 'link removal' API in a separate PR? That would make it easier to iterate on it, add tests, etc.

sandhose · 2024-10-02T14:13:42Z

frontend/src/components/UpstreamProvider/LinkUpstreamProvider.tsx

@@ -0,0 +1,65 @@
+// Copyright 2024 New Vector Ltd.
+// Copyright 2023, 2024 The Matrix.org Foundation C.I.C.


sandhose · 2024-10-02T14:14:42Z

frontend/src/components/UpstreamProvider/LinkUpstreamProvider.tsx

+      provider {
+        id
+      }


Why are we selecting this?

sandhose · 2024-10-02T14:14:50Z

frontend/src/components/UpstreamProvider/UnlinkUpstreamProvider.tsx

@@ -0,0 +1,111 @@
+// Copyright 2024 New Vector Ltd.
+// Copyright 2023, 2024 The Matrix.org Foundation C.I.C.


MTRNord marked this pull request as draft September 25, 2024 23:14

sandhose reviewed Sep 26, 2024

View reviewed changes

crates/handlers/src/graphql/model/upstream_oauth.rs Outdated Show resolved Hide resolved

MTRNord commented Sep 26, 2024

View reviewed changes

...storage-pg/.sqlx/query-100c6cfa46133f3b196b00ccf8f33ba0bcb4bb1afc1985092dd8ec51b40f631a.json Outdated Show resolved Hide resolved

MTRNord force-pushed the MTRNord/link-upstream-ui branch from bc74e13 to 08ab2dc Compare September 30, 2024 20:18

MTRNord added 8 commits October 2, 2024 01:00

Initial implementation of a UI for linking to upstream providers and …

3dc2b41

…showing existing linked providers Signed-off-by: MTRNord <[email protected]>

Fix the type errors

3590574

Signed-off-by: MTRNord <[email protected]>

Use the list function with a fixed amount of results instead of a cus…

a6c914b

…tom function Signed-off-by: MTRNord <[email protected]>

Run sqlx prepare

51e2b21

Signed-off-by: MTRNord <[email protected]>

Prepare the Unlink UI and add translations to the new UI

66c1158

Signed-off-by: MTRNord <[email protected]>

Add mutation and UI to delete an upstream link

37d346d

Signed-off-by: MTRNord <[email protected]>

Ensure the unlinking happens inside of a transaction. This way we do …

648d69e

…not end up in an in-between state in case of a database error Signed-off-by: MTRNord <[email protected]>

Hide the linked section if no links exist. Also hide the entire secti…

0b2d359

…on if no providers exist Signed-off-by: MTRNord <[email protected]>

MTRNord force-pushed the MTRNord/link-upstream-ui branch from f7b99c0 to 0b2d359 Compare October 1, 2024 23:06

MTRNord marked this pull request as ready for review October 1, 2024 23:06

MTRNord requested a review from sandhose October 1, 2024 23:19

sandhose requested changes Oct 2, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

UI for linking/unlinking the Upstream Providers #3245

UI for linking/unlinking the Upstream Providers #3245

MTRNord commented Sep 25, 2024 •

edited

Loading

CLAassistant commented Sep 25, 2024 •

edited

Loading

MTRNord commented Sep 30, 2024

sandhose commented Sep 30, 2024

sandhose commented Sep 30, 2024

sandhose left a comment

sandhose Oct 2, 2024

MTRNord Oct 2, 2024

MTRNord Oct 2, 2024 •

edited

Loading

sandhose Oct 2, 2024

MTRNord Oct 2, 2024

sandhose Oct 2, 2024

sandhose Oct 2, 2024

sandhose Oct 2, 2024

sandhose Oct 2, 2024

sandhose Oct 2, 2024

sandhose Oct 2, 2024

sandhose Oct 2, 2024

sandhose Oct 2, 2024

		@@ -0,0 +1,129 @@
		// Copyright 2024 New Vector Ltd.
		// Copyright 2023, 2024 The Matrix.org Foundation C.I.C.

	Consumed {
	completed_at: DateTime<Utc>,
	consumed_at: DateTime<Utc>,
	link_id: Ulid,
	id_token: Option<String>,
	},

		@@ -0,0 +1,65 @@
		// Copyright 2024 New Vector Ltd.
		// Copyright 2023, 2024 The Matrix.org Foundation C.I.C.

		@@ -0,0 +1,111 @@
		// Copyright 2024 New Vector Ltd.
		// Copyright 2023, 2024 The Matrix.org Foundation C.I.C.

UI for linking/unlinking the Upstream Providers #3245

Are you sure you want to change the base?

UI for linking/unlinking the Upstream Providers #3245

Conversation

MTRNord commented Sep 25, 2024 • edited Loading

Todos:

CLAassistant commented Sep 25, 2024 • edited Loading

MTRNord commented Sep 30, 2024

sandhose commented Sep 30, 2024

sandhose commented Sep 30, 2024

sandhose left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

MTRNord Oct 2, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

MTRNord commented Sep 25, 2024 •

edited

Loading

CLAassistant commented Sep 25, 2024 •

edited

Loading

MTRNord Oct 2, 2024 •

edited

Loading