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

identity-service-api: Add unbind_3pid endpoint. #627

Merged
merged 5 commits into from Aug 15, 2021

Conversation

Frinksy
Copy link
Contributor

@Frinksy Frinksy commented Jun 11, 2021

Resolves: #331

I created the ThirdPartyId since ruma_common::thirdparty::ThirdPartyIdentifier has two extra fields that aren't needed in this case.

@Frinksy Frinksy requested a review from iinuwa as a code owner June 11, 2021 17:54
Copy link
Member

@iinuwa iinuwa left a comment

Choose a reason for hiding this comment

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

I created the ThirdPartyId since ruma_common::thirdparty::ThirdPartyIdentifier has two extra fields that aren't needed in this case.

Yeah, I think that's fine.

I left some questions I had about the spec concerning authentication. We'll need more clarification on those before moving forward with this one.

name: "unbind_3pid",
path: "/_matrix/identity/v2/3pid/unbind",
rate_limited: false,
authentication: AccessToken,
Copy link
Member

Choose a reason for hiding this comment

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

I have a couple questions based on what the spec says:

The identity server should authenticate the request in one of two ways:

  1. The request is signed by the homeserver which controls the user_id.
  2. The request includes the sid and client_secret parameters, as per /3pid/bind, which proves ownership of the 3PID.

Does this mean that you still need an Identity Service (IS) API access token, but you also need a signature or sid/client_secret pair? That's what I read this as at first. If not, I think this would need to be a MaybeAccessToken, perhaps. (You may need to file an issue or ask in the #matrix-spec channel to get answers to these.)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I assumed that an access token was necessary in both cases. I've asked about it (and about your other comment) to make sure in the #matrix-spec channel.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The current answer in the issue I opened isn't very conclusive about the need of a token. However, it seems that the swagger for the endpoint states an access token is needed.
Looking at other endpoints in the IS spec, endpoints do not require a token in their v1 variant, and do require one in the v2 variant. So to be consistent with the rest of the v2 endpoints currently implemented in Ruma, we may want to require an AccessToken.

Copy link
Member

Choose a reason for hiding this comment

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

Sure, it's probably better to default to being more secure (i.e. AccessToken instead of MaybeAccessToken). I wish that we could get some more concrete answers on this one, but this will have to do for now.

@Frinksy Frinksy changed the base branch from main to next August 5, 2021 13:38
@Frinksy Frinksy changed the base branch from next to main August 11, 2021 13:07
Copy link
Member

@iinuwa iinuwa left a comment

Choose a reason for hiding this comment

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

Hey, sorry for the long delay. One more nit to change and then we can merge this one.

/// The proof that the client owns the 3PID.
///
/// If this is not provided, the request must be signed by the homeserver which controls
/// the `user_id`.
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
/// the `user_id`.
/// the `mxid`.

user_id is mentioned in the docs, but it's not the name of any parameter on this endpoint. I think it refers to the mxid parameter, but I can't really think this morning. Does that make sense to you? (A git blame on this part of the spec might show if there was mass parameter rename or something but this was missed in the docs.)

Either way, we should modify this doc to refer to the parameter name.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Changed to mxid, I also don't see what else it could refer to and it does make sene. I did have a look at the history of the IS spec, and it seems that the inconsistency was introduced when v1 of this endpoint was specced, and copied word-for-word in the v2 endpoint.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Found the answer in the MSC which introduced the endpoint:

Add POST /_matrix/identity/api/v1/3pid/unbind with mxid and threepid fields.
The mxid is the user's user_id and threepid is a dict with the usual
medium and address fields.

If the server returns a 400, 404 or 501 HTTP error code then the homeserver
should assume that the identity server doesn't support the /3pid/unbind API, unless
it returns a specific matrix error response (i.e. the body is a JSON object with
error and errcode fields).

The identity server should authenticate the request in one of two ways:

  1. The request is signed by the homeserver which controls the user_id.
  2. The request includes the sid and client_secret params (as per /bind),
    which proves ownership of the given 3PID.

Or to be more concise:

The mxid is the user's user_id

name: "unbind_3pid",
path: "/_matrix/identity/v2/3pid/unbind",
rate_limited: false,
authentication: AccessToken,
Copy link
Member

Choose a reason for hiding this comment

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

Sure, it's probably better to default to being more secure (i.e. AccessToken instead of MaybeAccessToken). I wish that we could get some more concrete answers on this one, but this will have to do for now.

@iinuwa iinuwa merged commit 4a91f30 into ruma:main Aug 15, 2021
@iinuwa
Copy link
Member

iinuwa commented Aug 15, 2021

Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

identity-service: Add 3PID unbind endpoint
3 participants