Remove confusing top-level attrs of Token Response #162
Conversation
OAuthTokenResponse top-level properties for accessing the top-level token aren't the desired/canonical/safe way to access tokens. We're telling people to always use `by_resource_server`, so we should cut down the interface for this class to that and only that. Minor change included: make the dependent token response a subclass of the typical token response, only overriding a helper in __init__. Also explicitly raises NotImplementedError if someone tries to decode an ID token with it. Closes globus#158
|
We've always said the "top-levelness" is a meaningless artifact of wedging in extra tokens while still complying with the spec. 👍 for this change. The "top level" tokens are not special or different in any way. |
|
I accidentally closed this by cleaning up all of the branches on my fork which were merged with it -- obviously including the branch itself. Whoops. Sorry for the noise. |
| The array of tokens other than the top-level one. | ||
| """ | ||
| return self['other_tokens'] | ||
|
|
There was a problem hiding this comment.
We definitely have some internal usage of these properties that will need to be refactored if they get dropped.
With no more top-level properties to address the top token in a response, the RT Authorizer neesds to do more "generic" handling of responses via the by_resource_server attribute. Tests need to be updated -- many of them removed -- to comply with the reduced interface to OAuthTokenResponse objects.
|
The update should pass tests. The internal usage seems to be isolated to the Outside of that, all of the changes necessary were made in the testsuite. Unfortunately, several tests simply had to be dropped because they were inspecting the exact attributes being removed here. As an unfortunate side-effect, the |
Trying to index into dict.values() doesn't work on py3, where that is a dict view. Instead, call next(iter(x)) on it, to get the first and only element. len(x) works when x is a dict view, so that's okay.
After some thought about #158, I've become convinced that these attributes should be removed.
The silent "promotion" of scopes by Auth, the lack of any public specification as to the method of determination for that top-level token, and the inability of users to change what that token is by any means mean that these properties offer very little value and a lot of confusion.
OAuthTokenResponsetop-level properties for accessing the top-level token aren't the desired/canonical/safe way to access tokens. We're telling people to always useby_resource_server, so we should cut down the interface for this class to that and only that.Minor change included: make the dependent token response a subclass of the typical token response, only overriding a helper used by
__init__.This will require that we bump to
0.7.0in the next release.