NotificationsYou must be signed in to change notification settings
Fork928
Star10.1k

feat: Allow hiding password auth, changing OpenID Connect text and OpenID Connect icon#5101

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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.

Already on GitHub?Sign in to your account

Jump to bottom

Merged

kylecarbs merged 24 commits intocoder:mainfromnormana10:configurable-openid-connect-text

Jan 31, 2023

Merged

feat: Allow hiding password auth, changing OpenID Connect text and OpenID Connect icon#5101

kylecarbs merged 24 commits intocoder:mainfromnormana10:configurable-openid-connect-text

Jan 31, 2023

Conversation

Copy link

Contributor

normana10 commentedNov 16, 2022•
edited
Loading

Just some ideas to help decrease friction in onboarding developers to my coder deployment

For context, my users willnever use password auth, only Oauth

Examples:

Default coder login (nothing new here, just for reference)
With the following env vars set (also nothing new here, just for reference):

CODER_OIDC_ISSUER_URL
CODER_OIDC_EMAIL_DOMAIN
CODER_OIDC_CLIENT_ID
CODER_OIDC_CLIENT_SECRET

With the following env vars set:

CODER_OIDC_ISSUER_URL
CODER_OIDC_EMAIL_DOMAIN
CODER_OIDC_CLIENT_ID
CODER_OIDC_CLIENT_SECRET
CODER_PASSWORD_AUTH_HIDDEN=true
CODER_OIDC_SIGN_IN_TEXT="Sign in with Gitea"
CODER_OIDC_ICON_URL=https://gitea.io/images/gitea.png

I only reference Gitea because that's my OpenID Connect provider at home, feel free to replace that with anything you like 👍

I also acknowledge that I'm changing the shape of the return body ofapi/v2/users/authmethods. Not sure if that's a deal breaker?

Also let me know if Coderintentionally didn't want to do any of these. Don't want my random PRs to steamroll any actual conversations that have taken place

normana10 added3 commits

November 15, 2022 21:13

Allow hiding password entry, changing OpenID Connect text and OpenID …

3addcaf

…Connect icon

Docs

40bc156

Cleaning

0f4a40e

normana10 requested a review froma team as acode owner

November 16, 2022 02:48

normana10 requested review frompresleyp and removed request fora team

November 16, 2022 02:48

normana10 added4 commits

November 15, 2022 22:05

Fix Prettier and Go test and TS compile error

90d900c

Fix LoginPage test

97b5019

Prettier

8019ec7

Fix storybook

97d9d46

presleyp requested a review fromkylecarbs

November 16, 2022 14:46

kylecarbs reviewed

Nov 16, 2022

View reviewed changes

Copy link

Member

kylecarbs left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

Hey 👋 I appreciate the contribution!

I think we'll want to end up disabling password authentication instead of hiding it for security reasons. I'll review this later today!

Copy link

Member

bpmct commentedNov 22, 2022

@kylecarbs and I just had a chat and decided we shouldn't completely disable the password login so the defaultOwner account can log in.

However, we can put it behind a backdoor (e.g.?auth=password) so that users don't see an option to log in with a user/pass. Also, OIDC users wouldn't be able to log in with a password anyways

Add query param to un-hide password auth

0858ad0

Copy link

ContributorAuthor

normana10 commentedNov 24, 2022

Implemented

With the following env vars set:

CODER_OIDC_ISSUER_URL
CODER_OIDC_EMAIL_DOMAIN
CODER_OIDC_CLIENT_ID
CODER_OIDC_CLIENT_SECRET
CODER_PASSWORD_AUTH_HIDDEN=true
CODER_OIDC_SIGN_IN_TEXT="Sign in with Gitea"
CODER_OIDC_ICON_URL=https://gitea.io/images/gitea.png

presleyp removed their request for review

November 28, 2022 15:39

Copy link

github-actionsbot commentedDec 6, 2022

This Pull Request is becoming stale. In order to minimize WIP, prevent merge conflicts and keep the tracker readable, I'm going close to this PR in 3 days if there isn't more activity.

github-actionsbot added the staleThis issue is like stale bread. label

Dec 6, 2022

normana10 added2 commits

December 5, 2022 22:05

Merge branch 'main' into configurable-openid-connect-text

f13df97

# Conflicts:#cli/deployment/config.go#cli/server.go#cli/testdata/coder_server_--help.golden#coderd/userauth.go#codersdk/deploymentconfig.go#docs/admin/auth.md#site/src/api/typesGenerated.ts#site/src/components/SignInForm/SignInForm.tsx

Cleaning

6afca64

normana10 requested a review fromkylecarbs

December 6, 2022 04:36

github-actionsbot removed the staleThis issue is like stale bread. label

Dec 7, 2022

Copy link

github-actionsbot commentedDec 14, 2022

This Pull Request is becoming stale. In order to minimize WIP, prevent merge conflicts and keep the tracker readable, I'm going close to this PR in 3 days if there isn't more activity.

github-actionsbot added the staleThis issue is like stale bread. label

Dec 14, 2022

Merge branch 'main' into configurable-openid-connect-text

410fc98

Copy link

Member

mafredri commentedDec 14, 2022

I think we'll want to end up disabling password authentication instead of hiding it for security reasons. I'll review this later today!

@kylecarbs how would an owner/admin sign in if we disable password auth? As@bpmct proposed, we could put it behind?password=password, or maybe better/login/admin. But ultimately that seems like an inconvenience. Maybe it's negligible though.

mafredri reviewed

Dec 14, 2022

View reviewed changes

Copy link

Member

mafredri left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

Backend changes look good to me with one minor request if we keep the flag.

cli/deployment/config.go OutdatedShow resolvedHide resolved

Copy link

Member

kylecarbs commentedDec 14, 2022

I think I'd prefer if we did a small "Login with Password" link at the bottom that would permit authenticating with a password. It's awkward to not allow it, since in emergency situations it would be required.

An alternative is to take the stance that owners/admins should be able to modify the deployment, so they could always disable this feature.

Copy link

Member

bpmct commentedDec 14, 2022

I think I'd prefer if we did a small "Login with Password" link at the bottom that would permit authenticating with a password. It's awkward to not allow it, since in emergency situations it would be required.
An alternative is to take the stance that owners/admins should be able to modify the deployment, so they could always disable this feature.

I can get behind a small link as long as it's clear that SSO is preferred. Many login forms tend to have both even if 99% of an organization's users use SSO

kylecarbs mentioned this pull request

Dec 14, 2022

Option to disable login form when oidc is enabled#4433

Closed

bpmct approved these changes

Jan 19, 2023

View reviewed changes

Copy link

Member

bpmct left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

This UX LGTM! Thanks for the contribution, let's ship it 🚢

Kira-Pilot reviewed

Jan 19, 2023

View reviewed changes

site/src/components/SignInForm/SignInForm.tsx Outdated

		<div>
		<LoadingButton
		loading={isLoading}
		{showPasswordAuth && (

Copy link

Member

Kira-PilotJan 19, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

We have some new conditionals in this file introduced by the new booleans:nonPasswordAuthEnabled andshowPasswordAuth. Considering the file is getting fairly large, it might be nice to break the JSX inside these conditionals into separate components in separate files. This will make everything a bit more readable. Not a blocker though!

Copy link

ContributorAuthor

normana10Jan 20, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

I agree

I took a hack at it (might have made a few stylistic choices, open to feedback)

Also turned thesevar && into<Maybes as I saw that used all over the codebase (and might have to steal that for myself)

Copy link

Member

Kira-PilotJan 20, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

Looks beautiful! Thanks for leaving things better than you found them :)

Kira-Pilot reviewed

Jan 19, 2023

View reviewed changes

site/src/pages/LoginPage/LoginPage.test.tsxShow resolvedHide resolved

Kira-Pilot approved these changes

Jan 19, 2023

View reviewed changes

PR Feedback and split up SignInForm.tsx

77f0fb2

Copy link

github-actionsbot commentedJan 20, 2023•
edited
Loading

CLA Assistant Lite bot All contributors have signed the CLA ✍️ ✅

Updated golden files

1ed7911

normana10 force-pushed theconfigurable-openid-connect-text branch from6e20df2 to1ed7911Compare

January 20, 2023 04:33

Copy link

ContributorAuthor

normana10 commentedJan 20, 2023•
edited
Loading

Oops sorry pushed the golden files update from a different computer

Fix auto-genned-files

e31a37e

Copy link

Member

kylecarbs commentedJan 24, 2023

@normana10 seems likemake gen isn't quite producing the same for you... want me to post a patch you can apply?

Copy link

ContributorAuthor

normana10 commentedJan 25, 2023

@kylecarbs

Feel free

I keep trying to runmake gen and it does nothing

Then I do amake gen -B

And I get a:

$ make gen -Bpanic: could not start resource: : dial unix /var/run/docker.sock: connect: connection refusedgoroutine 1 [running]:main.main()/Users/normana10/git/coder/coderd/database/gen/dump/main.go:23 +0xb25exit status 2make: *** [Makefile:461: coderd/database/dump.sql] Error 1

(On my Mac)

Which.... totally makes sense, I don't have Docker installed on my Mac

Buuuut, when I try the same on my Fedora computer it ends up erroring out with a:

...../docs/api/workspaces.md 99ms../docs/manifest.json 16ms../coderd/apidoc/swagger.json 351msDone in 4.37s.ERROR: The 'yq' dependency is required, but is not available.ERROR: One or more dependencies are not available, check above log output for more details.make: *** [Makefile:509: site/.prettierrc.yaml] Error 1

But...yq is definitely installed

Copy link

Member

mafredri commentedJan 25, 2023

@normana10 sorry the error is so unhelpful, you'll need to haveyq v4 installed, either asyq4 oryq --version >= v4.

Copy link

ContributorAuthor

normana10 commentedJan 26, 2023

$ yq --versionyq version 4.2.0

🤷

Copy link

ContributorAuthor

normana10 commentedJan 26, 2023

Wait I just noticed thatyqoutside of nix-shell is v4 butinside it's v3 o.O

Just symlinked my install ofyq toyq4 andmake gen -B ran just fine

Pushing up now

normana10 added2 commits

January 25, 2023 23:01

make gen -B

9207175

Revert provisioner files?

201d424

Copy link

ContributorAuthor

normana10 commentedJan 26, 2023

That was a roller coaster but I think it's all good now? 👍

Thanks!

Copy link

Member

kylecarbs commentedJan 26, 2023

Woot woot. Thanks for pushing through@normana10 🥳

Copy link

Member

kylecarbs commentedJan 30, 2023

@normana10 can you merge inmain then I'll merge this in ASAP! Thank you again for being patient, this was a controversial one for us.

Merge branch 'main' into configurable-openid-connect-text

8333256

Copy link

ContributorAuthor

normana10 commentedJan 30, 2023

@kylecarbs

Done

Sorry if I caused any issues 😬

normana10 changed the title~~Allow hiding password auth, changing OpenID Connect text and OpenID Connect icon~~feat: Allow hiding password auth, changing OpenID Connect text and OpenID Connect icon

Jan 30, 2023

normana10and others added2 commits

January 30, 2023 00:39

Fix lint error

7ce525e