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

Remove password and 2fa options for sso controlled users #603

Merged
merged 11 commits into from
Mar 29, 2024

Conversation

e-five256
Copy link
Member

@e-five256 e-five256 commented Mar 18, 2024

Remove the password and 2fa setting options for users that have oauth configured

If the user attempts to access the password / 2fa pages manually, gets a 403 access denied response

2fa remove/disable is still allowed in code, so admins should still be able to remove 2fa if for some reason it was or gets enabled, but enabling is not allowed


Fixes #576

@e-five256 e-five256 added the sso Single-sign-on issues or pull requests label Mar 18, 2024
@e-five256 e-five256 marked this pull request as ready for review March 20, 2024 23:05
@@ -763,6 +763,11 @@ public function removeOAuth2UserConsent(OAuth2UserConsent $oAuth2UserConsent): s
return $this;
}

public function isSsoControlled(): bool
{
return $this->oauthGithubId || $this->oauthGoogleId || $this->oauthFacebookId || $this->oauthKeycloakId || $this->oauthZitadelId;
Copy link
Member Author

Choose a reason for hiding this comment

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

this is obviously prone to getting out of date if new identity providers are added. Not sure if there's a better way, or maybe can think of one later. We could add a column on user that they are sso controlled, but that means we have to be sure to set it to true every time a new provider is added which is also possible to mess up, but perhaps less likely

@ghost
Copy link

ghost commented Mar 21, 2024

Does it also make sense to remove the email tab since it requires a password to change?

image

@e-five256
Copy link
Member Author

Dang, I clearly got it wrong in the issue. I assume that means it's impossible to update an SSO controlled users email, as obviously there's no current password for them to use.

I suppose I'll keep the page, but remove the form to update it, so you can see your current email value. If it ever changes upstream (downstream?) it won't update here, but at least users will be able to see that and potentially ask an admin to fix it until we have a way to resync with the SSO provider somehow

@e-five256 e-five256 marked this pull request as draft March 21, 2024 00:29
@e-five256 e-five256 marked this pull request as ready for review March 26, 2024 04:04
@e-five256
Copy link
Member Author

e-five256 commented Mar 26, 2024

It now looks like this when an SSO user

image

I may have gone overboard making a div to look like the input box... supposedly input can be used outside of forms but, didn't really seem like people thought it was a good idea. Maybe other styling would be more simple 🤔

@ghost
Copy link

ghost commented Mar 26, 2024

So...another problem (maybe), if an SSO user wants to delete their account, should it be allowed?

image

@e-five256
Copy link
Member Author

Is your comment about deleting / tombstoning the account, or the fact that it requires current password (and thus isn't currently possible)?

@ghost
Copy link

ghost commented Mar 26, 2024

Is your comment about deleting / tombstoning the account, or the fact that it requires current password (and thus isn't currently possible)?

The latter, without a password its not possible for an SSO user to delete their account now.

Copy link

@ghost ghost left a comment

Choose a reason for hiding this comment

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

SSO user delete seems like it's going to require a much bigger PR to address, so I'll approve this one as is since it's out of scope from the original intent.

@e-five256 e-five256 merged commit 789c1f7 into main Mar 29, 2024
7 checks passed
@e-five256 e-five256 deleted the e5/sso-controlled branch March 29, 2024 23:00
This pull request was closed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
sso Single-sign-on issues or pull requests
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Improve sso-only mode
1 participant