-
Notifications
You must be signed in to change notification settings - Fork 10
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
UI for linking/unlinking the Upstream Providers #3245
Conversation
...storage-pg/.sqlx/query-100c6cfa46133f3b196b00ccf8f33ba0bcb4bb1afc1985092dd8ec51b40f631a.json
Outdated
Show resolved
Hide resolved
bc74e13
to
08ab2dc
Compare
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? |
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
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 |
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 |
f7b99c0
to
0b2d359
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
e.preventDefault(); | ||
|
||
setInProgress(true); | ||
const upstreamURL = `/upstream/authorize/${data.id.replace("upstream_oauth2_provider:", "")}`; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oops did not read far enough. Graphql it is. :D
<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> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Couldn't that just be an actual link?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It probably could. I just went for the Design here in the end. A link is probably more semantically correct though. 🤔
} | ||
|
||
// Allow non-admins to remove their email address if the site config allows it | ||
if !requester.is_admin() && !state.site_config().email_change_allowed { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That should be another site config flag
/// 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> { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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?
r#" | ||
UPDATE upstream_oauth_authorization_sessions SET upstream_oauth_link_id = NULL | ||
WHERE upstream_oauth_link_id = $1 | ||
"#, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
"#, | |
"#, |
// constraint on the links. | ||
sqlx::query!( | ||
r#" | ||
UPDATE upstream_oauth_authorization_sessions SET upstream_oauth_link_id = NULL |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
frontend/src/components/UpstreamProvider/LinkUpstreamProvider.tsx
Outdated
Show resolved
Hide resolved
frontend/src/components/UpstreamProvider/LinkUpstreamProvider.tsx
Outdated
Show resolved
Hide resolved
frontend/src/components/UpstreamProvider/UnlinkUpstreamProvider.tsx
Outdated
Show resolved
Hide resolved
…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]>
Signed-off-by: MTRNord <[email protected]>
0b2d359
to
d2a3df9
Compare
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]>
d2a3df9
to
d32ab17
Compare
Signed-off-by: MTRNord <[email protected]>
Just to avoid some confusion: I will iterate on it here, then do my git magic to split this up into the 4 chunks and then see if I can split those into different PRs for review. Makes it a little less bad in terms of actual development initially :) |
Signed-off-by: MTRNord <[email protected]>
Signed-off-by: MTRNord <[email protected]>
import { H5 } from "@vector-im/compound-web"; | ||
import { useCallback, useTransition } from "react"; | ||
import { useTranslation } from "react-i18next"; | ||
import { useQuery } from "urql"; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
fyi, I'm in the process of moving off urql to @tanstack/react-query for querying data. This shouldn't make a massive difference, you'll have basically three things to change:
- useQuery calls have a slightly different pattern (see https://github.com/element-hq/matrix-authentication-service/pull/3504/files#diff-824d80f55dbfb304574729bfc83deb51ec11af2ed3e3467cbd132c10ffaafffd )
- useMutation calls are also slightly different (see https://github.com/element-hq/matrix-authentication-service/pull/3504/files#diff-79ea95d633c486de54ab8a248faa3ba49b217bd739d5a22644b982743dd0544e )
- there are now 'render' snapshot tests, so you'll need to add the additional fragments to the mocked GraphQL response here: https://github.com/element-hq/matrix-authentication-service/pull/3504/files#diff-74973935dbe4236af3173d6b7989acbb33a1318c44a5175ac9d573b825b9afa8R99
So sorry but I think I just dont have the time to finish this :/ CLA is signed so feel free to reuse the code within if it is any use in this state. It turns just out that this exceeds my available time currently and I cant estimate when I get to this :/ |
Fixes #3244
Todos: