Skip to content

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

Merged
merged 9 commits into from
Dec 6, 2023

Conversation

Emyrk
Copy link
Member

@Emyrk Emyrk commented Nov 29, 2023

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

Copy link
Member Author

Emyrk commented Nov 29, 2023

@Emyrk Emyrk force-pushed the external_auth_be_updates branch 3 times, most recently from 21dcf27 to 6111d53 Compare November 30, 2023 18:26
@Emyrk Emyrk force-pushed the external_auth_be_updates branch 8 times, most recently from a9b2119 to c02c754 Compare December 1, 2023 15:46
@Emyrk Emyrk force-pushed the external_auth_fe_updates branch from 6a98155 to b9217dc Compare December 1, 2023 15:58
@Emyrk Emyrk marked this pull request as ready for review December 5, 2023 16:23
Copy link
Collaborator

@BrunoQuaresma BrunoQuaresma left a 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

@aslilac
Copy link
Member

aslilac commented Dec 5, 2023

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)

@Emyrk
Copy link
Member Author

Emyrk commented Dec 5, 2023

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.

Base automatically changed from external_auth_be_updates to main December 5, 2023 20:03
@matifali
Copy link
Member

matifali commented Dec 5, 2023

This is a huge improvement. Thanks @Emyrk 😊.
I think we should not have the confirmation dialogue as a user can authenticator again very easily. It's not something very dangerous like Deleting a workplace or template.

@Emyrk
Copy link
Member Author

Emyrk commented Dec 5, 2023

This is a huge improvement. Thanks @Emyrk 😊. I think we should not have the confirmation dialogue as a user can authenticator again very easily. It's not something very dangerous like Deleting a workplace or template.

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.

@matifali
Copy link
Member

matifali commented Dec 5, 2023

I agree. Are there ways we can invalidate the token on our end? By sending a request to the provider?
Also this warning could also be moved as info text somewhere on the revoke button.
I feel its uncecarsy to type the name of the provider Integration.
Moreover, a simple reuthenticate button without unlinking would help if for some reason a user needs a new token.

@Emyrk Emyrk force-pushed the external_auth_fe_updates branch from f6d8c53 to cc7cd52 Compare December 5, 2023 21:01
@Emyrk
Copy link
Member Author

Emyrk commented Dec 5, 2023

I agree. Are there ways we can invalidate the token on our end? By sending a request to the provider?

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 types, we can implement them based on the provider type. I think for a first draft this is ok.

Also this warning could also be moved as info text somewhere on the revoke button. I feel its uncecarsy to type the name of the provider Integration. Moreover, a simple reuthenticate button without unlinking would help if for some reason a user needs a new token.

Fair. I think this ui is very first draft.

@Emyrk
Copy link
Member Author

Emyrk commented Dec 5, 2023

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

Is this something we could fix in a follow up? @BrunoQuaresma

@aslilac
Copy link
Member

aslilac commented Dec 5, 2023

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

@Emyrk
Copy link
Member Author

Emyrk commented Dec 5, 2023

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?

@BrunoQuaresma
Copy link
Collaborator

@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 🙏 ?

@Emyrk
Copy link
Member Author

Emyrk commented Dec 5, 2023

@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! 🌮

@BrunoQuaresma
Copy link
Collaborator

@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?

@Emyrk Emyrk merged commit b376b2c into main Dec 6, 2023
@Emyrk Emyrk deleted the external_auth_fe_updates branch December 6, 2023 14:41
@github-actions github-actions bot locked and limited conversation to collaborators Dec 6, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants