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

Clean up and flesh out all three editions of the /requestToken API #1636

Merged
merged 11 commits into from Aug 31, 2018

Conversation

Projects
None yet
4 participants
@turt2live
Member

turt2live commented Aug 31, 2018

Rendered: see 'docs' status check


Fixes #1634

Addresses some of #1396

@turt2live turt2live requested a review from matrix-org/spec-core-team Aug 31, 2018

@uhoreg

I don't have time to fully review this, but a couple quick changes

send_attempt:
type: integer
description: |-
Optional. If specified, the server will only send an email if

This comment has been minimized.

@uhoreg

uhoreg Aug 31, 2018

Member

Should remove "Optional" if it's defined as required. (also in api/identity/definitions/request_msisdn_validation.yaml)

This comment has been minimized.

@turt2live

turt2live Aug 31, 2018

Member

good catch - will fix.

ftr it became required because synapse has an assert on it.

@turt2live turt2live requested review from uhoreg and matrix-org/spec-core-team and removed request for uhoreg Aug 31, 2018

description: |-
Proxies the identity server API ``validate/email/requestToken``, but
Proxies the identity service API ``validate/email/requestToken``, but
first checks that the given email address is **not** already associated
with an account on this Home Server. This API should be used to request

This comment has been minimized.

@richvdh

richvdh Aug 31, 2018

Member

homeserver

first checks that the given email address is **not** already associated
with an account on this Home Server. This API should be used to request
validation tokens when adding an email address to an account. This API's
parameters and response is identical to that of the HS API
parameters and response is identical to that of the client-server API

This comment has been minimized.

@richvdh

richvdh Aug 31, 2018

Member

are identical

This comment has been minimized.

@richvdh

richvdh Aug 31, 2018

Member

seems odd to specifically call out that it's the client-server API when we are in the CS spec?

This comment has been minimized.

@richvdh

richvdh Aug 31, 2018

Member

(indeed this may be redundant now that you've actually populated the params table.

}
400:
description: |-
The homeserver was unable to location a user with the third party identifier

This comment has been minimized.

@richvdh

richvdh Aug 31, 2018

Member

to location a user?

400:
description: |-
The homeserver was unable to location a user with the third party identifier
alrady bound, or the request was invalid.

This comment has been minimized.

@richvdh

richvdh Aug 31, 2018

Member

alrady

This comment has been minimized.

@ara4n

ara4n Aug 31, 2018

Member

as per above, this should be: "The third party identifier is already in use on the homeserver.", and the error should be M_THREEPID_IN_USE. (Or the request was invalid).

schema:
$ref: "../identity/definitions/sid.yaml"
403:
description: The homeserver does not permit the address to be bound.

This comment has been minimized.

@richvdh

richvdh Aug 31, 2018

Member

is this actually up to the HS, or might it be the IS?

This comment has been minimized.

@turt2live

turt2live Aug 31, 2018

Member

Synapse complaints loudly. The homeserver is able to deny particular identifiers from being bound in it's database, however there's nothing stopping the client from working around the homeserver and going straight to the IS.

This comment has been minimized.

@turt2live

turt2live Aug 31, 2018

Member

This should probably be clarified.

first checks that the given phone number is **not** already associated
with an account on this Home Server. This API should be used to request
validation tokens when adding a phone number to an account. This API's
parameters and response is identical to that of the HS API
parameters and response is identical to that of the client-server API

This comment has been minimized.

@richvdh

richvdh Aug 31, 2018

Member

as above

@turt2live turt2live requested a review from richvdh Aug 31, 2018

validation tokens when adding an email address to an account. This API's
parameters and response is identical to that of the HS API
|/register/email/requestToken|_ endpoint.
parameters and response is identical to that of the |/register/email/requestToken|_

This comment has been minimized.

@richvdh

richvdh Aug 31, 2018

Member

are identical

description: |-
The homeserver does not permit the user from having the third party
identifier as a contact option. This does not prevent the identity
service from binding the third party identifier, however.

This comment has been minimized.

@richvdh

richvdh Aug 31, 2018

Member

I'm now more confused, sorry. Presumably the IS won't have bound the 3pid, because the HS didn't proxy the request?

It might be clearer to say "This does not necessarily mean that the identity service will not permit the third party identifier to be bound". But then I'm not sure why we're bothering to say that: there are a lot of things that it doesn't necessarily mean.

@@ -226,8 +229,8 @@ paths:
}
400:
description: |-
The homeserver was unable to location a user with the third party identifier
alrady bound, or the request was invalid.
The homeserver was unable to locate a user with the third party identifier

This comment has been minimized.

@richvdh

richvdh Aug 31, 2018

Member

surely the point is that we don't want a user with the 3pid bound? why would that give a 400?

@@ -243,8 +246,8 @@ paths:
first checks that the given phone number is **not** already associated
with an account on this Home Server. This API should be used to request
validation tokens when adding a phone number to an account. This API's
parameters and response is identical to that of the client-server API
|/register/msisdn/requestToken|_ endpoint.
parameters and response is identical to that of the |/register/msisdn/requestToken|_

This comment has been minimized.

@richvdh

richvdh Aug 31, 2018

Member

are identical

}
400:
description: |-
The homeserver was unable to locate a user with the third party identifier

This comment has been minimized.

@ara4n

ara4n Aug 31, 2018

Member

This is wrong. It should be: "The third party identifier is already in use on the homeserver.", and the error should be M_THREEPID_IN_USE. (Or the request was invalid).

@@ -185,29 +185,111 @@ paths:
- User data
"/account/3pid/email/requestToken":

This comment has been minimized.

@ara4n

ara4n Aug 31, 2018

Member

Why is this file called administrative_contact.yaml and "Matrix Client-Server Account Administrative Contact API"?

This comment has been minimized.

@turt2live

turt2live Aug 31, 2018

Member

History says 2015 would have an answer, but unfortunately I don't: c6e0322

This comment has been minimized.

@ara4n

ara4n Aug 31, 2018

Member

looks like it was illicitonion being weird, presumably contorting to differentiate it from 3pid management on the IS. i'd call it homeserver_3pid_mappings.yaml or something, but it's not a blocker for the review.

This comment has been minimized.

@ara4n

ara4n Aug 31, 2018

Member

or just account_3pid.yaml, given it describes the /account/3pid endpoints.

turt2live added some commits Aug 31, 2018

Don't explain how the IS might accept a 3pid
The IS is bound to it's own set of specifications, and if the client chose this API then they should be aware of the risks but not necessarily arbitrary alternatives.

@turt2live turt2live requested review from richvdh and ara4n Aug 31, 2018

The hostname of the identity service to communicate with. May
optionally include a port.
example: "id.example.com"
required: ['id_server']
responses:

This comment has been minimized.

@ara4n

ara4n Aug 31, 2018

Member

We seem to be missing the:

  • 403, "Third party identifier is not allowed", Codes.THREEPID_DENIED,
  • 400, "MSISDN not found", Codes.THREEPID_NOT_FOUND
    errors.
The hostname of the identity service to communicate with. May
optionally include a port.
example: "id.example.com"
required: ['id_server']
responses:

This comment has been minimized.

@ara4n

ara4n Aug 31, 2018

Member

Missing the:

  • 403, "Third party identifier is not allowed", Codes.THREEPID_DENIED
  • 400, "Email not found", Codes.THREEPID_NOT_FOUND
    responses
@ara4n

lgtm other than the stuff with the response codes (and strongly suggesting we rename the file and the API to something which remotely resembles its contents)

@turt2live turt2live requested a review from ara4n Aug 31, 2018

@turt2live turt2live referenced this pull request Aug 31, 2018

Merged

Fix all naming of "homeserver" and "identity server" #1643

1 of 1 task complete
schema:
$ref: "../identity/definitions/sid.yaml"
403:
description: |-
The homeserver does not permit the user from having the third party

This comment has been minimized.

@richvdh

richvdh Aug 31, 2018

Member

does not permit from having?

comments resolved

}
400:
description: |-
The third party identifier is already in use on the homeserver, or

This comment has been minimized.

@ara4n

ara4n Aug 31, 2018

Member

This should be: "The referenced third party identifier is not recognised by the homeserver, or the request was invalid".

}
400:
description: |-
The third party identifier is already in use on the homeserver, or

This comment has been minimized.

@ara4n

ara4n Aug 31, 2018

Member

This should be: "The referenced third party identifier is not recognised by the homeserver, or the request was invalid".

@ara4n

This comment has been minimized.

Member

ara4n commented Aug 31, 2018

there are still thinkos on the response codes which i've pointed out.

@turt2live turt2live requested a review from ara4n Aug 31, 2018

@ara4n

ara4n approved these changes Aug 31, 2018

lgtm, thanks

@turt2live turt2live merged commit 683072e into matrix-org:master Aug 31, 2018

7 checks passed

ci/circleci: build-dev-scripts Your tests passed on CircleCI!
Details
ci/circleci: build-docs Your tests passed on CircleCI!
Details
ci/circleci: build-swagger Your tests passed on CircleCI!
Details
ci/circleci: check-docs Your tests passed on CircleCI!
Details
ci/circleci: validate-docs Your tests passed on CircleCI!
Details
docs Click details to preview the HTML documentation.
Details
swagger Click to preview the swagger build.
Details

@turt2live turt2live deleted the turt2live:travis/c2s/id-server branch Aug 31, 2018

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