Issue #6 support revoke function

[judgej]

Provides a function to revoke a token completely, along with all granted scopes.

This endpoint uses Basic auth and requires the refresh token to identify the authorisation being revoked.

[judgej]

Best leave this for a day or so (at least) while I finish integrating this change with the Xero test app, and see if there is anything I've overlooked. But have a play in the meantime. The provider needs the client id and secret set, and revoking operates on the refresh token.

It will accept the token object and extract the refresh toen from it if avaiable. However, since this is not a part of the initial OAuth flow, the token object will unlikely to be available. It is a part of the OAuth 2.0, defined as an extension in the RFC so it does below here. Seems not to be handled by the League OAuth 2.0 package at all at present, unless I've simply missed it.

[calcinai]

My thinking would be that these properties would stay inside a jwt property of this object, or they're assigned to more meaningful properties of the XeroResourceOwner

Thoughts?

[judgej]

Not sure. I see the JWT is a signed delivery method, and once it's decoded, the JWT itself has served its purpose; we are now dealing with data, or properties. The resource owner class could provide more meaning to the properties, such as providing timestamps as DateTime classes. The JWT custom claims have little awareness of what they are. I can see how accessing additional custom claims that could potentially come along in the future becomes a little more difficult.

[judgej]

Just coming back to this, as I need to start using it in ernest. It still feels right moving the data out of the JWT and discarding the JWT at the earliest opportunity. The JWT is never used again once it has been used to deliver some data.

[calcinai]

@judgej agreed. I think my original thinking was "why would a Xero Resource Owner have an expiry time" etc. As in, if you think about the object itself those properties don't make sense. The problem here is probably because I created the fromJWT() method, which is really taking about a different thing.

Re your comment about the JWT being discarded, if we're taking that approach, then should we be using the attribute names from it? It feels a little bit contradictory for that to be the goal, yet still retaining sub,exp,aud etc, which are meaningless outside a JWT context.

[judgej]

Now I'm playing with it, I can see the expiry time makes sense here. The ID token is valid for only five minutes - it expires five minutes after it is issued. It's not used for access, so the expiry does not have any practical effect, but it just indicates that the identity should only be trust for five minutes. The names and email address could be changed at any time, so it is necessary to go back and fetch it again if you want to do anything serious with it.

Something else I have noticed, is that the identity token is also supplied with the new access token on a refresh. That is the only way of getting a fresh identity token - refresh the access token and a new identity token comes along with it (so long as you are asking for identity in the scopes).

[judgej]

Just a heads up: a fix for the updatedDateUtc property of the tenant model is included in this PR. The API always returns a timestamp for that property, but the model always parses it to null. Maybe I can put this into a separate PR?

[judgej]

I think on the discarding comment, my mind was still in OAuth 1.0a mode. The full JWT needs to be kept, as that is the token that is sent with each API request. Some values it is worth pulling out separately - exp, xero_userid, scopes, but otherwise the rest of the data can stay in the JWT. They can be unwrapped and inspected if need be, but most of the time they won't be needed as individual properties.

[calcinai]

Does a utility function really belong in this class?

[judgej]

I don't know. I wanted to deal with the claims as properties in cameCase to be a bit more PSR consistent. The properties were in snake case, as that is how the JWT presents it. I saw the utility function as a bridge between the JWT and PSR worlds.

They could be pulled in as traits or an abstract, and that can be done through refactoring. Or maybe you are suggesting this class should remain as a "JWT value object" that looks to be much closer to the JWT? Happy to go with whatever your line of thinking is.

[judgej]

Happy to remove this utility function (it's gone).

What are your thoughts on making the properties camelCase instead? That would be consistent with the xeroTenant class. The snake_case is in the OAuth world and rhe the camelCase is in the (PSR) PHP world, and this provider nicely transforms between the two.

[judgej]

Looking at the official generated PHP SDK supplied by Xero, they seem to have chosen snake_case for the model property names. We we the SDK supports something like this through the identity API:

$connection->tenant_name;
$connection->getTenantName();

Personally, I generate the API SDK code with camelCase properties, so get this:

$connection->tenantName;
$connection->getTenantName();

which is closer to the identity endpoint model in this provider:

$xeroTenant->tenantName;
// But no getter method.

It is strange that they have used snake_case in the generated code when even the API payload used camelCase. I'm just mentioning this because it can be a little confusing to peoeple when first using this provider along with the Xero SDK, as the (provider) XeroTenant model, and the (SDK) Connection model are essentially the same thing from the same API endpoint, but sue difference letter case properties. However, if xeroTenant had getter methods too, then that coud be an identical way to access the properties of both objects the same consistent way.

[StoyanPenev]

Hi guys, token revocation is just what I am looking for at the moment as we are going to apply for partner app.
One of the Xero compliance checkpoints is to revoke the active refresh token.
Not sure how 4a4ed25 is related to the revocation feature, maybe we can exclude this commit but merge the other two?

@calcinai Is there any chance we can merge this anytime soon.

[judgej]

I've been working on other things the last month, so have not been able to follow this up. The 4a4ed25 change was to help get to the information needed to help revoke the token, basically so the application knows who's token it is. That information can be presented to an admin, and can also be used to locate existing authorisations for udating when the user reauthorises with additional permissions. IIRC those details can only be captured once during the authorisarion process, so the model just makes it a little easier to capture.

I've got a revoke branch on the demo app that I have not yet sent a PR for, that may be of interest. I'll see if I can tidy it up this weekend, and catch up with the master commits (it's a little behind).