-
Notifications
You must be signed in to change notification settings - Fork 559
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
Fix and deprecate get_token_permission #2631
Conversation
The docs for this PR live here. All of your documentation changes will be reflected on that endpoint. The docs are available until 30 days after the last update. |
Aha, that explains it, thank you for picking this up! :) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM! thanks
@@ -1661,10 +1661,28 @@ def whoami(self, token: Union[bool, str, None] = None) -> Dict: | |||
) from e | |||
return r.json() | |||
|
|||
def get_token_permission(self, token: Union[bool, str, None] = None) -> Literal["read", "write", None]: | |||
@_deprecate_method( | |||
version="1.0", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
is there a particular reason why the deprecation runs until 1.0.0 and not in three minor versions? except maybe the fact that it may lead to some frictions if we remove it too soon 😅
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Got too shy to remove it in 3 minor versions 😄
TBH, also because it's 4 lines of code so not a big deal if kept for a longer time. Permissions is a quite sensitive topic.
As reported by @CISC in gradio-app/gradio#9820 (comment), using
get_token_permission
with an OAuth token is broken:This is because the whoami response for an OAuth token doesn't contain the detailed permission for that token.
This PR fixes
get_token_permission
to not raise an error when a OAuth token is passed but return None instead. This is a half-satisfying fix but at least it won't break things in the wild (same output as if no token is passed). What I also did is to fully deprecate this method as it doesn't make any sense to have it anymore. Deprecation cycle runs until v1.0.0. Tokens are not just "read" or "write". OAuth and fine-grained tokens have much more complex permissions depending on the user choices.In a follow-up PR, I'll make sure to remove all internal use of
get_token_permission
. This doesn't have to be done in the same PR.