Admin API for adding and removing upstream oauth links #4255
Conversation
From element-hq#3245 with changes from review
|
|
| let filter = UpstreamOAuthLinkFilter::new() | ||
| .for_user(&user) | ||
| .for_provider(&provider); | ||
| let count = repo.upstream_oauth_link().count(filter).await?; | ||
|
|
||
| if count > 0 { | ||
| return Err(RouteError::LinkAlreadyExists( | ||
| params.user_id, | ||
| params.provider_id, | ||
| )); | ||
| } | ||
|
|
||
| let mut link = repo | ||
| .upstream_oauth_link() | ||
| .add( | ||
| &mut rng, | ||
| &clock, | ||
| &provider, | ||
| params.subject, | ||
| params.human_account_name, | ||
| ) | ||
| .await?; |
There was a problem hiding this comment.
I tried locally, and one thing this doesn't check is the (provider, subject) unique constraint. The filter+count up there checks if the user already have a link with the provider, even though it's (relatively) valid to have multiple links with the same provider
There is also the question of what to do if there is an existing link for a given subject, but with no user associated. I think in this case we should use the existing link and associate it to the user?
So, I would change this whole block to:
- not check for existing links on the user+provider
- use
UpstreamOAuthLinkRepository::find_by_subjectto find an existing link for the given subject- if it doesn't exist, create it
- if it exists but with no existing user associated, use the existing one and associate the user
- if it exists but with the same user associated, use the existing one
- if it exists but with another user associated, error out
- if we created the link, return a
201 CREATED, else a200 OKif we reused one
There was a problem hiding this comment.
I have no idea how I missed the unassociated link case despite it staring at me right in the face. I'll fix that asap tomorrow
There was a problem hiding this comment.
Done in 7c4a9bf
if it exists but with the same user associated, use the existing one
I didn't think this made sense though. In that case the endpoint wouldn't do anything and I think that should be considered a failure case.
tonkku107
force-pushed
the
admin-api-upstream-oauth
branch
from
0282578 to
bb0e355
Compare
tonkku107
force-pushed
the
admin-api-upstream-oauth
branch
from
bb0e355 to
7c4a9bf
Compare
sandhose
added
A-Admin-API
4 participants
Includes link removal storage API from #3245 with the review comment addressed by me