Conversation
| h4. TokenRevocationRequest | ||
|
|
||
| * @(TF1)@ @TokenRevocationRequest@ is a type containing the token revocation request details sent to the "REST revokeTokens endpoint":/rest-api/#revokeTokens | ||
| * @(TF2)@ @TokenRevocationRequest#keys@ attribute is an array of strings containing the keys to revoke. For request to be valid, it must contain at least one key. A test must exist to ensure there is at least one key in the request before it is sent. |
There was a problem hiding this comment.
I think we should generally avoid adding statements that say "A test must exist ....".
The REST endpoint is POST /keys/appId.keyId/revokeTokens which revokes tokens associated with a specific key, whereas this request permits multiple keys. This introduces complexity because there is the possibility that the caller has rights to revoke tokens for one key but not others. I suggest we restrict this API to mirror the REST API, and only support token revocation for a single key.
There was a problem hiding this comment.
Thank you. I think reading spec and following and example I mistook the . with , and somehow thought we support multiple keys. I made some changes here 1c220a6
There was a problem hiding this comment.
@paddybyers If this conversation needs to continue, then that will need to happen against the new pull request that we will open against ably/specification#10. Please resolve this conversation here once you have re-raised your concern there, or if you are already happy that this matter is resolved. Thanks.
| ** @(RSA16b)@ If the library is provided with a token without the corresponding @TokenDetails@, then this holds a @TokenDetails@ instance in which only the @token@ attribute is populated with that token string | ||
| ** @(RSA16c)@ Is set with the current token (if applicable) on instantiation and each time it is replaced, whether the result of an explicit @Auth#authorize@ operation, or a library-initiated renewal resulting from expiry or a token error response | ||
| ** @(RSA16d)@ Is empty if there is no current token, including after a previous token has been determined to be invalid or expired, or if the library is using basic auth | ||
| * @(RSA17)@ @Auth#revokeToken@ function: |
There was a problem hiding this comment.
Why not revokeTokens to reflect (i) what it does, and (ii) the REST endpoint name ?
There was a problem hiding this comment.
@paddybyers If this conversation needs to continue, then that will need to happen against the new pull request that we will open against ably/specification#10. Please resolve this conversation here once you have re-raised your concern there, or if you are already happy that this matter is resolved. Thanks.
ikbalkaya
requested review from
KacperKluka,
Morganamilo,
Peter-Maguire,
QuintinWillison,
Rosalita,
mohyour,
owenpearson,
paddybyers and
stmoreau
and removed request for
paddybyers
|
From @ikbalkaya via Slack (internal):
So I am going to move this pull request back to the 'Draft' state and label it as |
QuintinWillison
removed request for
KacperKluka,
Morganamilo,
Peter-Maguire,
QuintinWillison,
Rosalita,
SimonWoolf,
lmars,
mattheworiordan,
mohyour,
owenpearson,
paddybyers,
stmoreau and
tomkirbygreen
|
This changes proposed in this pull request has been moved to the new home of the features spec, in the |
7 participants
Description
Client library spec for token revocation
Review
This feature spec is likely to change a bit after some reviews. I would like to highlight the areas that may need to be addressed better.
Exception messages in client side errors (invalid request etc)
Exception messages from REST: I saw there are some fixed error codes on other spec points but wasn't sure what the error codes would be for this spec.
Did I use spec numbering correctly? I just used arbitrary numbering that was similar to others and unique
Page to review