-
Notifications
You must be signed in to change notification settings - Fork 936
feat: add user/settings page for managing external auth #10945
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
Conversation
Current dependencies on/for this PR:
This stack of pull requests is managed by Graphite. |
21dcf27
to
6111d53
Compare
a9b2119
to
c02c754
Compare
6a98155
to
b9217dc
Compare
site/src/pages/UserExternalAuthSettingsPage/UserExternalAuthSettingsPage.tsx
Outdated
Show resolved
Hide resolved
site/src/pages/UserExternalAuthSettingsPage/UserExternalAuthSettingsPage.tsx
Outdated
Show resolved
Hide resolved
site/src/pages/UserExternalAuthSettingsPage/UserExternalAuthSettingsPage.tsx
Show resolved
Hide resolved
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.
About code: I left a few considerations about mutation usage
About design:
- The table should be wider
- I feel the button is taller than it should be
I think the whole table kind of feels "zoomed in" compared to the chrome around it. I guess mainly the icon and button are both way too big. Might be worth referencing the licenses page, it has kind of a similar vibe to this one. (although it actually kind of feels too big too...hmm) |
Yea, the look is off for sure. I spent some time trying to fix this, but at some point I figured someone else can do a much better job in much less time than myself 😄 So I just focused on the functionality. |
This is a huge improvement. Thanks @Emyrk 😊. |
One benefit to the confirm page is it educates the user that the "unlink" is only on our side. They need to revoke from the application to truly invalidate the token. |
I agree. Are there ways we can invalidate the token on our end? By sending a request to the provider? |
Also add support for unlinking on the coder side to allow reflow.
f6d8c53
to
cc7cd52
Compare
There is an RFC for an oauth2 extension: https://datatracker.ietf.org/doc/html/rfc7009 It is not part of the main spec though so, "your mileage may vary" based on the provider. Since we have provider
Fair. I think this ui is very first draft. |
Is this something we could fix in a follow up? @BrunoQuaresma |
is this behind an experiment? if so fixing later is fine, if not I think I'd rather land it in better shape since the changes would be very minor |
It's not behind an experiment. @aslilac do you or @BrunoQuaresma have some time to clean it up in this PR? |
@steve-Coder-Ent yeap, I can do it tomorrow morning. In case I miss that, can you ping me on Slack to remind me pls 🙏 ? |
Perfect thanks! 🌮 |
@steve-Coder-Ent I think it is just easier to merge it and I work on top of it so I can use the BE changes directly from dev.coder.com. Sounds good for you? |
FE team, if you have comments/suggestions that are quick, feel free to submit to this PR. This FE took me quite some time to get right. There is still the issue of a redundant fetch, and it could be styled more.
Screencast.from.2023-12-05.10-22-07.webm