-
Notifications
You must be signed in to change notification settings - Fork 9
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
base: main
Are you sure you want to change the base?
Changes from all commits
0839b70
3ab5c8a
f496b4e
3eb40bf
729dccc
a0ec7c1
407a721
d32ab17
c980ee4
1897270
b417428
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1 +1,2 @@ | ||
target/ | ||
config.yaml |
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
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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> { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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.
Original file line number | Diff line number | Diff line change | ||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
@@ -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; | ||||||||||||||
|
||||||||||||||
|
@@ -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 | ||||||||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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
I think we should:
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 | ||||||||||||||
"#, | ||||||||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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(()) | ||||||||||||||
} | ||||||||||||||
} |
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?