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
Closed
1 change: 1 addition & 0 deletions .gitignore
Original file line number Diff line number Diff line change
@@ -1 +1,2 @@
target/
config.yaml
57 changes: 56 additions & 1 deletion crates/handlers/src/graphql/model/upstream_oauth.rs
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,11 @@
use anyhow::Context as _;
use async_graphql::{Context, Object, ID};
use chrono::{DateTime, Utc};
use mas_storage::{upstream_oauth2::UpstreamOAuthProviderRepository, user::UserRepository};
use mas_storage::{
upstream_oauth2::{UpstreamOAuthLinkFilter, UpstreamOAuthProviderRepository},
user::UserRepository,
Pagination,
};

use super::{NodeType, User};
use crate::graphql::state::ContextExt;
Expand Down Expand Up @@ -45,6 +49,57 @@ impl UpstreamOAuth2Provider {
pub async fn client_id(&self) -> &str {
&self.provider.client_id
}

/// The human-readable name of the provider.
pub async fn human_name(&self) -> Option<&str> {
self.provider.human_name.as_deref()
}

/// The brand name of the provider.
///
/// Values supported by the default template are:
///
/// - `apple`
/// - `google`
/// - `facebook`
/// - `github`
/// - `gitlab`
/// - `twitter`
///
/// Note that this is a free-form field and can be any string value.
pub async fn brand_name(&self) -> Option<&str> {
self.provider.brand_name.as_deref()
}

/// 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> {
Comment on lines +74 to +78
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?

let state = ctx.state();
let user = ctx
.requester()
.user()
.ok_or_else(|| async_graphql::Error::new("User ID not found in the request context"))?;

let mut repo = state.repository().await?;
let filter = UpstreamOAuthLinkFilter::new()
.for_provider(&self.provider)
.for_user(user);
let links = repo
.upstream_oauth_link()
// Hardcoded limit of 100 links. We do not expect reasonably more links
// See also https://github.com/element-hq/matrix-authentication-service/pull/3245#discussion_r1776850096
.list(filter, Pagination::first(100))
.await?;
repo.cancel().await?;

Ok(links
.edges
.into_iter()
.map(UpstreamOAuth2Link::new)
.collect())
}
}

impl UpstreamOAuth2Link {
Expand Down
2 changes: 2 additions & 0 deletions crates/handlers/src/graphql/mutations/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,7 @@ mod browser_session;
mod compat_session;
mod matrix;
mod oauth2_session;
mod upstream_oauth;
mod user;
mod user_email;

Expand All @@ -22,6 +23,7 @@ pub struct Mutation(
compat_session::CompatSessionMutations,
browser_session::BrowserSessionMutations,
matrix::MatrixMutations,
upstream_oauth::UpstreamOauthMutations,
);

impl Mutation {
Expand Down
151 changes: 151 additions & 0 deletions crates/handlers/src/graphql/mutations/upstream_oauth.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,151 @@
// Copyright 2024 New Vector Ltd.
//
// SPDX-License-Identifier: AGPL-3.0-only
// Please see LICENSE in the repository root for full details.

use anyhow::Context as _;
use async_graphql::{Context, Description, Enum, InputObject, Object, ID};
use mas_storage::{user::UserRepository, RepositoryAccess};

use crate::graphql::{
model::{NodeType, UpstreamOAuth2Link, UpstreamOAuth2Provider, User},
state::ContextExt,
};

#[derive(Default)]
pub struct UpstreamOauthMutations {
_private: (),
}

/// The input for the `removeEmail` mutation
#[derive(InputObject)]
struct RemoveUpstreamLinkInput {
/// The ID of the upstream link to remove
upstream_link_id: ID,
}

/// The status of the `removeEmail` mutation
#[derive(Enum, Copy, Clone, Eq, PartialEq)]
enum RemoveUpstreamLinkStatus {
/// The upstream link was removed
Removed,

/// The upstream link was not found
NotFound,
}

/// The payload of the `removeEmail` mutation
#[derive(Description)]
enum RemoveUpstreamLinkPayload {
Removed(mas_data_model::UpstreamOAuthLink),
NotFound,
}

#[Object(use_type_description)]
impl RemoveUpstreamLinkPayload {
/// Status of the operation
async fn status(&self) -> RemoveUpstreamLinkStatus {
match self {
RemoveUpstreamLinkPayload::Removed(_) => RemoveUpstreamLinkStatus::Removed,
RemoveUpstreamLinkPayload::NotFound => RemoveUpstreamLinkStatus::NotFound,
}
}

/// The upstream link that was removed
async fn upstream_link(&self) -> Option<UpstreamOAuth2Link> {
match self {
RemoveUpstreamLinkPayload::Removed(link) => Some(UpstreamOAuth2Link::new(link.clone())),
RemoveUpstreamLinkPayload::NotFound => None,
}
}

/// The provider to which the upstream link belonged
async fn provider(
&self,
ctx: &Context<'_>,
) -> Result<Option<UpstreamOAuth2Provider>, async_graphql::Error> {
Comment on lines +63 to +66
Copy link
Member

Choose a reason for hiding this comment

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

Aren't those properties already on the link?

let state = ctx.state();
let provider_id = match self {
RemoveUpstreamLinkPayload::Removed(link) => link.provider_id,
RemoveUpstreamLinkPayload::NotFound => return Ok(None),
};

let mut repo = state.repository().await?;
let provider = repo
.upstream_oauth_provider()
.lookup(provider_id)
.await?
.context("Upstream OAuth 2.0 provider not found")?;

Ok(Some(UpstreamOAuth2Provider::new(provider)))
}

/// The user to whom the upstream link belonged
async fn user(&self, ctx: &Context<'_>) -> Result<Option<User>, 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.

Same

let state = ctx.state();
let mut repo = state.repository().await?;

let user_id = match self {
RemoveUpstreamLinkPayload::Removed(link) => link.user_id,
RemoveUpstreamLinkPayload::NotFound => return Ok(None),
};

match user_id {
None => Ok(None),
Some(user_id) => {
let user = repo
.user()
.lookup(user_id)
.await?
.context("User not found")?;

Ok(Some(User(user)))
}
}
}
}

#[Object]
impl UpstreamOauthMutations {
/// Remove an upstream linked account
async fn remove_upstream_link(
&self,
ctx: &Context<'_>,
input: RemoveUpstreamLinkInput,
) -> Result<RemoveUpstreamLinkPayload, async_graphql::Error> {
let state = ctx.state();
let upstream_link_id =
NodeType::UpstreamOAuth2Link.extract_ulid(&input.upstream_link_id)?;
let requester = ctx.requester();

let mut repo = state.repository().await?;

let upstream_link = repo.upstream_oauth_link().lookup(upstream_link_id).await?;
let Some(upstream_link) = upstream_link else {
return Ok(RemoveUpstreamLinkPayload::NotFound);
};

if !requester.is_owner_or_admin(&upstream_link) {
return Ok(RemoveUpstreamLinkPayload::NotFound);
}

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

return Err(async_graphql::Error::new("Unauthorized"));
}

let upstream_link = repo
.upstream_oauth_link()
.lookup(upstream_link.id)
.await?
.context("Failed to load user")?;

repo.upstream_oauth_link()
.remove(upstream_link.clone())
.await?;

repo.save().await?;

Ok(RemoveUpstreamLinkPayload::Removed(upstream_link))
}
}

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

48 changes: 47 additions & 1 deletion crates/storage-pg/src/upstream_oauth2/link.rs
Original file line number Diff line number Diff line change
Expand Up @@ -14,7 +14,7 @@ use mas_storage::{
use rand::RngCore;
use sea_query::{enum_def, Expr, PostgresQueryBuilder, Query};
use sea_query_binder::SqlxBinder;
use sqlx::PgConnection;
use sqlx::{Connection, PgConnection};
use ulid::Ulid;
use uuid::Uuid;

Expand Down Expand Up @@ -355,4 +355,50 @@ impl<'c> UpstreamOAuthLinkRepository for PgUpstreamOAuthLinkRepository<'c> {
.try_into()
.map_err(DatabaseError::to_invalid_operation)
}

#[tracing::instrument(
name = "db.upstream_oauth_link.remove",
skip_all,
fields(
db.query.text,
upstream_oauth_link.id,
upstream_oauth_link.provider_id,
%upstream_oauth_link.subject,
),
err,
)]
async fn remove(&mut self, upstream_oauth_link: UpstreamOAuthLink) -> Result<(), Self::Error> {
let mut tx = self.conn.begin().await?;

// Unset the authorization sessions first, as they have a foreign key
// 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.

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
"#,
"#,

Uuid::from(upstream_oauth_link.id),
)
.traced()
.execute(&mut *tx)
.await?;

// Then delete the link itself
let res = sqlx::query!(
r#"
DELETE FROM upstream_oauth_links
WHERE upstream_oauth_link_id = $1
"#,
Uuid::from(upstream_oauth_link.id),
)
.traced()
.execute(&mut *tx)
.await?;

DatabaseError::ensure_affected_rows(&res, 1)?;

tx.commit().await?;

Ok(())
}
}
13 changes: 13 additions & 0 deletions crates/storage/src/upstream_oauth2/link.rs
Original file line number Diff line number Diff line change
Expand Up @@ -184,6 +184,17 @@ pub trait UpstreamOAuthLinkRepository: Send + Sync {
///
/// Returns [`Self::Error`] if the underlying repository fails
async fn count(&mut self, filter: UpstreamOAuthLinkFilter<'_>) -> Result<usize, Self::Error>;

/// Delete a [`UpstreamOAuthLink`]
///
/// # Parameters
///
/// * `upstream_oauth_link`: The [`UpstreamOAuthLink`] to delete
///
/// # Errors
///
/// Returns [`Self::Error`] if the underlying repository fails
async fn remove(&mut self, upstream_oauth_link: UpstreamOAuthLink) -> Result<(), Self::Error>;
}

repository_impl!(UpstreamOAuthLinkRepository:
Expand Down Expand Up @@ -216,4 +227,6 @@ repository_impl!(UpstreamOAuthLinkRepository:
) -> Result<Page<UpstreamOAuthLink>, Self::Error>;

async fn count(&mut self, filter: UpstreamOAuthLinkFilter<'_>) -> Result<usize, Self::Error>;

async fn remove(&mut self, upstream_oauth_link: UpstreamOAuthLink) -> Result<(), Self::Error>;
);
13 changes: 12 additions & 1 deletion frontend/locales/en.json
Original file line number Diff line number Diff line change
Expand Up @@ -35,6 +35,10 @@
"title": "Edit profile",
"username_label": "Username"
},
"linked_upstreams": {
"description": "Sign in with another account provider that supports Single Sign On (SSO).",
"heading": "Linked accounts"
},
"password": {
"change": "Change password",
"change_disabled": "Password changes are disabled by the administrator.",
Expand Down Expand Up @@ -91,6 +95,9 @@
"active_now": "Active now",
"inactive_90_days": "Inactive for 90+ days"
},
"link_upstream_button": {
"text": "Link to {{provider}}"
},
"nav": {
"devices": "Devices",
"settings": "Settings"
Expand Down Expand Up @@ -232,6 +239,10 @@
"title": "Cannot find session: {{deviceId}}"
}
},
"unlink_upstream_button": {
"confirmation_modal_title": "Unlink your {{provider}} account?",
"text": "Unlink {{provider}}"
},
"unverified_email_alert": {
"button": "Review and verify",
"text:one": "You have {{count}} unverified email address.",
Expand Down Expand Up @@ -289,4 +300,4 @@
"view_profile": "See your profile info and contact details"
}
}
}
}
Loading
Loading