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

UI for linking/unlinking the Upstream Providers #3245

Closed
wants to merge 11 commits into from

Conversation

MTRNord
Copy link

@MTRNord MTRNord commented Sep 25, 2024

Fixes #3244

grafik

Todos:

  • Allow linking the Provider
  • Allow unlinking the Provider
  • Improve the UI

@CLAassistant
Copy link

CLAassistant commented Sep 25, 2024

CLA assistant check
All committers have signed the CLA.

@MTRNord MTRNord marked this pull request as draft September 25, 2024 23:14
@MTRNord
Copy link
Author

MTRNord commented Sep 30, 2024

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
Copy link
Member

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

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
Copy link
Member

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

@MTRNord MTRNord marked this pull request as ready for review October 1, 2024 23:06
@MTRNord MTRNord requested a review from sandhose October 1, 2024 23:19
Copy link
Member

@sandhose sandhose left a 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:", "")}`;
Copy link
Member

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

Copy link
Author

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.

Copy link
Author

@MTRNord MTRNord Oct 2, 2024

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

Comment on lines 49 to 59
<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>
Copy link
Member

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?

Copy link
Author

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 {
Copy link
Member

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

Comment on lines +58 to +62
/// 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> {
Copy link
Member

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
"#,
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
"#,
"#,

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

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:

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.

…not end up in an in-between state in case of a database error

Signed-off-by: MTRNord <[email protected]>
@MTRNord
Copy link
Author

MTRNord commented Nov 8, 2024

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 :)

import { H5 } from "@vector-im/compound-web";
import { useCallback, useTransition } from "react";
import { useTranslation } from "react-i18next";
import { useQuery } from "urql";
Copy link
Member

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:

@MTRNord
Copy link
Author

MTRNord commented Dec 2, 2024

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 :/

@MTRNord MTRNord closed this Dec 2, 2024
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.

Users can link/unlink multiple IdPs to their account
3 participants