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

Improve documentation around /account/whoami #1152

Merged
merged 5 commits into from Mar 6, 2018

Conversation

Projects
None yet
4 participants
@turt2live
Member

turt2live commented Mar 6, 2018

Clarifies: #1135

  • Add a blurb about appservices as a reminder to read the appservice spec
  • The request can be ratelimited
  • Document what the server should do when an invalid access token is given
  • Document explicitly how the server should react to appservices
  • Formatting of some examples

turt2live added some commits Mar 6, 2018

Improve documentation around /account/whoami
Clarifies: #1135

Signed-off-by: Travis Ralston <travpc@gmail.com>
Add clarifications of /account/whoami to changelog
Signed-off-by: Travis Ralston <travpc@gmail.com>
"$ref": "definitions/error.yaml"
403:
description:
The appservice cannot masquerade the user or has not registered them.

This comment has been minimized.

@richvdh

richvdh Mar 6, 2018

Member

masquerade as

Gets information about the owner of a given access token.
If the owner of the access token is an application service,
the server should return the user ID making the request. The

This comment has been minimized.

@richvdh

richvdh Mar 6, 2018

Member

I'm not sure "the user ID making the request" is clear.

Presumably this refers to the user_id specified as a query param? Could you say that?

This comment has been minimized.

@turt2live

turt2live Mar 6, 2018

Member

yes, although that parameter is optional (which therefore makes the request by the bridge bot)

This comment has been minimized.

@richvdh

richvdh Mar 6, 2018

Member

Ok, sorry, but I'm afraid this isn't any clearer to me at all :/.

To me, in the AS model, "the user ID making the request" is the AS user; the AS user can masquerade as a user within its namespace by providing a user_id parameter.

So the server should not return the user ID making the request.

Rather than all this blurb, which is common to most of the C-S API, I would say something like:

Note that, as with the rest of the Client-Server API, Application Services may masquerade as users within their namespace by giving a user_id query parameter. In this situation, the server should verify that the given user_id is registered, and return it in the response body.

This comment has been minimized.

@turt2live

turt2live Mar 6, 2018

Member

Yours is a lot clearer and what I was trying to say. I'm just bad with words this time around it seems.

Taken nearly verbatim (minor edits).

the server should return the user ID making the request. The
server should respect the application service client/server API
extensions during this request. If the request is made for a
virtual user, the server should verify that it is registered.

This comment has been minimized.

@richvdh

richvdh Mar 6, 2018

Member

should I know what a virtual user is?

This comment has been minimized.

@turt2live

turt2live Mar 6, 2018

Member

Readers should be, or can at least become aware of what this means by reading the appservice spec.

@turt2live

This comment has been minimized.

Member

turt2live commented Mar 6, 2018

Annoyingly the examples are unhappy with my attempt to break out the user_id query param to a template. I'll pull that out and try to do it in a dedicated PR.

It does however generate, assuming you don't check the examples.

@anoadragon453

This comment has been minimized.

Member

anoadragon453 commented Mar 6, 2018

Very clear, thanks for adding the error codes as well.

Reword appservice requirements for /account/whoami
Signed-off-by: Travis Ralston <travpc@gmail.com>

@turt2live turt2live force-pushed the turt2live:travis/clarify-whoami branch from ccca213 to 6ba5d7c Mar 6, 2018

@turt2live

This comment has been minimized.

Member

turt2live commented Mar 6, 2018

@richvdh updated wording - please take another look when you get a chance.

edit: and @anoadragon453 if you haven't seen the reword I did moments before your comment

operationId: getTokenOwner
security:
- accessToken: []
parameters: []
parameters:
# TODO: Break this out to a template or something (and apply it everywhere)

This comment has been minimized.

@richvdh

richvdh Mar 6, 2018

Member

honestly I would leave this off. We're adding more confusion by including it here but not in any of the other APIs that honour it than we are by leaving it out and just referring to it in the description and the AS spec.

Gets information about the owner of a given access token.
If the owner of the access token is an application service,
the server should return the user ID making the request. The

This comment has been minimized.

@richvdh

richvdh Mar 6, 2018

Member

Ok, sorry, but I'm afraid this isn't any clearer to me at all :/.

To me, in the AS model, "the user ID making the request" is the AS user; the AS user can masquerade as a user within its namespace by providing a user_id parameter.

So the server should not return the user ID making the request.

Rather than all this blurb, which is common to most of the C-S API, I would say something like:

Note that, as with the rest of the Client-Server API, Application Services may masquerade as users within their namespace by giving a user_id query parameter. In this situation, the server should verify that the given user_id is registered, and return it in the response body.

Reword the appservice portion of /account/whoami
Credit goes to richvdh - suggestions taken with edits.

Signed-off-by: Travis Ralston <travpc@gmail.com>
The token is not recongized
examples:
application/json: {
"errcode": "M_UNKNOWN_TOKEN",

This comment has been minimized.

@krombel

krombel Mar 6, 2018

Contributor

Typo: you want to use "Unrecognized"

Fix typos in whoami.yaml
Signed-off-by: Travis Ralston <travpc@gmail.com>
@richvdh

richvdh approved these changes Mar 6, 2018

\o/

@richvdh richvdh merged commit 2644e56 into matrix-org:master Mar 6, 2018

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details

@turt2live turt2live deleted the turt2live:travis/clarify-whoami branch Mar 6, 2018

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