Skip to content
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

automate-dex: make local user signin disappear via config #4386

Merged
merged 4 commits into from
Oct 9, 2020

Conversation

srenatus
Copy link
Contributor

@srenatus srenatus commented Oct 1, 2020

This would be a noop if there always were local users -- regardless of
configuring LDAP or SAML, you'd always have more than one method.

If we want to allow disabling local users, we'll have to enable this, because
if you had ONLY SAML, you couldn't sign out -- you'd immediately be sent back
to your SAML IdP; and if your session was still valid, you'd even go straight
back to being signed into A2.

To not have users click "sign in with X" if they only have LDAP, or only local
users, enabled, but have them go straight to the username/password field, we
set alwaysShowLoginScreen to true only if SAML is in the mix.

If you have LDAP+SAML, or local+SAML, you'll see the selection anyways.

ℹ️ I've added the extra commits to

  • add a new configurable
  • use it to disable local users in dex if there's another connector set up. Otherwise, it would fail to start yelling about "no connectors set up" (rightly so!)

👟 How to Build and Test the Change

  • deployinate, rebuild components/automate-dex, sign in and sign out with different SAML/non-SAML combinations
  • you should see no "sign in with email" button if there's only local users, but should go straight to username/password
  • same for "only LDAP/MSAD"
  • as long as there's more than one connector; or ONLY SAML (not configurable right now), you'd see the selection.

ℹ️ To play with disabling the local users, use this config snippet,

[dex.v1.sys.connectors]
disable_local_users = true

and rebuild automate-dex, automate-cli and automate-deployment. Then try to sign in, and remove the SAML config to see that you can't do that. With this config applied, the the SAML config not removed, you should see this:

image

@srenatus srenatus added auth-team anything that needs to be on the auth team board emergent labels Oct 1, 2020
@srenatus srenatus self-assigned this Oct 1, 2020
@netlify
Copy link

netlify bot commented Oct 1, 2020

Deploy preview for chef-automate ready!

Built with commit e9a7be4

https://deploy-preview-4386--chef-automate.netlify.app

@susanev susanev added this to the Auth Sprint 22 milestone Oct 1, 2020
This would be a noop if there always were local users -- regardless of
configuring LDAP or SAML, you'd always have more than one method.

If we want to allow disabling local users, we'll have to enable this, because
if you had ONLY SAML, you couldn't sign out -- you'd immediately be sent back
to your SAML IdP; and if your session was still valid, you'd even go straight
back to being signed into A2.

To not have users click "sign in with X" if they only have LDAP, or only local
users, enabled, but have them go straight to the username/password field, we
set alwaysShowLoginScreen to true only if SAML is in the mix.

If you have LDAP+SAML, or local+SAML, you'll see the selection anyways.

Signed-off-by: Stephan Renatus <srenatus@chef.io>
Signed-off-by: Stephan Renatus <srenatus@chef.io>
Signed-off-by: Stephan Renatus <srenatus@chef.io>
…ctor set up

Signed-off-by: Stephan Renatus <srenatus@chef.io>
@srenatus srenatus force-pushed the sr/remove-local-users-step-0 branch from f6f75fa to e9a7be4 Compare October 5, 2020 12:21
@srenatus srenatus changed the title automate-dex: always show sign in if there's a SAML connector automate-dex: make local user signin disappear via config Oct 5, 2020
Comment on lines +227 to +230
skipApprovalScreen: true
{{- if cfg.connectors.saml}}
alwaysShowLoginScreen: true
{{- end}}
Copy link
Contributor

Choose a reason for hiding this comment

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

I am not clear on what is the approval screen vs the login screen, or the whole process flow here for that matter. 🤷 You state:

To not have users click "sign in with X" if they only have LDAP, or only local
users, enabled, but have them go straight to the username/password field, we
set alwaysShowLoginScreen to true only if SAML is in the mix.

Am I correct, for instance, that SAML is different than local or LDAP, in that we do not ask for the credentials, rather the SAML IdP does. So if we remove the "Sign in with SAML" (that, I guess, is on the login screen?) that is why one could never log out, yes?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Am I correct, for instance, that SAML is different than local or LDAP, in that we do not ask for the credentials, rather the SAML IdP does.

Yes, the "SAML sign in page" is a redirect to the IdP, no username/password prompt.

So if we remove the "Sign in with SAML" (that, I guess, is on the login screen?) that is why one could never log out, yes?

Signing out of A2 will get you back to dex. If dex decided that, since there's only one sign in method, that must be the one you want to use, it sends you to that method's sign in page. For SAML, that's a redirect to your IdP. If you still have an active session with the IdP (that's something we don't control), you'll be signed in automatically, and redirect to A2. So, from a user's perspective, there's no way to sign out. Clicking on "Sign out" gets you signed in again. (I'm just rephrasing, I think your understanding is correct.)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The approval screen is this:
image

We have never enabled that, since it doesn't make sense in our setup.

Copy link
Contributor

Choose a reason for hiding this comment

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

Thanks!

@srenatus srenatus merged commit b0097e3 into master Oct 9, 2020
@srenatus srenatus deleted the sr/remove-local-users-step-0 branch October 9, 2020 07:37
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
auth-team anything that needs to be on the auth team board emergent
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants