-
Notifications
You must be signed in to change notification settings - Fork 2k
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
Revoke provider access #1843
Revoke provider access #1843
Conversation
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.
Nice! works well and is nicely backwards-compatible (it even works if you're using an older Uppy client, just without the info bubble)
@@ -49,6 +49,7 @@ class Uppy { | |||
youCanOnlyUploadFileTypes: 'You can only upload: %{types}', | |||
companionError: 'Connection with Companion failed', | |||
companionAuthError: 'Authorization required', | |||
companionUnauthorizeHint: 'To unauthorize to your %{provider} account, please go to %{url}', |
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.
The link is not clickable and users won't be able to copy it before the bubble disappears, so I'm not sure if this is super useful. Maybe we could make it say something like "You can now remove the authorization in %{provider}'s settings panel"? Would be nice to actually link to the page but that would require some changes in the info bubbles...
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.
We could make the link clickable (by setting content of the informer with __dangerouslySetInnerHTML
) and set the bubble to hold for 10 seconds? Until we add support for alert modals with “ok” (or we could use actual alert? I think it would be too intrusive and ugly in this case).
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.
my thought is that the URL being displayed now at least helps users find the URL where to revoke this access. Otherwise, some of these settings can be hard to find. Is there someway to make informer stay longer maybe?
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.
3rd argument to uppy.info()
is duration in milliseconds
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.
thank you! I have extended the duration of the message to last for 7 seconds, and it seemed long enough for me to highlight and copy the message.
fixes #1563
This PR extends the logout behaviour of companion so that when users logout from a Provider via Companion, it will attempt to revoke the access token of the corresponding Provider. In the event that the Provider doesn't support
access revoke
via it's API, Uppy will display a helpful message about how users can revoke the permissions manually.