MbinOrg / mbin

Mbin: a federated content aggregator, voting, discussion and microblogging platform (By the community, for the community)
https://joinmbin.org
GNU Affero General Public License v3.0
236 stars 17 forks source link

Improve sso-only mode #576

Open WebVoid opened 7 months ago

WebVoid commented 7 months ago

Is your feature request related to a problem? Please describe. We are discussing of implementing an Mbin instance in our company to manage our communities and would like to disallow the use of user/password registration and authentication, using only an Azure OIDC provider.

Describe the solution you'd like A configuration + env variable allowing to not display the login and registration forms but only the configured OIDC buttons. Also I believe hiding the MFA and password change tabs of the profile would be a good idea

Describe alternatives you've considered Leaving the form available, but this would lead to errors from our users

Additional context We would be able to propose a PR if the feature request is accepted

e-five256 commented 7 months ago

We do have an SSO only mode env var here https://github.com/MbinOrg/mbin/blob/7707f2e2cd2901c2af5e0447b30485a67c38b877/.env.example#L73-L74

It hides the login/register forms, but does not disable and remove the buttons for password and 2fa as you point out. I don't think that should be too difficult, and makes sense, so should be able to get a PR together for it soon

WebVoid commented 7 months ago

Thanks, I will admit I checked out the .env file from Kbin but not the one from Mbin, my apologies. Still, it makes sense that this mode should indeed hide everything "basic-auth" related in the application.

I will not close the issue as part of it is still relevant, but will rename it to a more adequate caption.

e-five256 commented 7 months ago

No worries, it's not well documented, as https://github.com/MbinOrg/mbin/issues/570 which was made yesterday shows Actually we have a lot of SSO issues now, perhaps we should tag them all to make it easier to find issues people might be concerned about

BentiGorlich commented 7 months ago

So what has to be done here is only

or is there something else?

e-five256 commented 7 months ago

Potentially the API as well, I see a user update endpoint, or maybe that's covered by the controller/manager, but worth double checking

I wanted to check if email should also be disabled. I setup keycloak and saw it handled email, but not sure if mbin also needs it and it is managed separately? @thepaperpilot do you happen to know how SSO handles email and should also be removed?

So I started this but noticed quite quickly I can't run keycloak and mbin at the same time on my system, so currently looking for more lightweight alternatives to test dev with

BentiGorlich commented 7 months ago

Zitadel is relatively lightweight I'd sy. Maybe you can test with the PR code from #429

thepaperpilot commented 7 months ago

I wanted to check if email should also be disabled. I setup keycloak and saw it handled email, but not sure if mbin also needs it and it is managed separately? @thepaperpilot do you happen to know how SSO handles email and should also be removed?

The idp will probably have their own email, although I know GitHub at least will try to keep emails private unless a user has specifically allowed their email to be public. They should definitely be considered read-only though, and in fact when you authenticate it'd probably make sense to update the email if the idp sent you a new one (noting that we don't use the email to identify the user, but rather a separate I'd sent by the idp). So shutting down the endpoints to update (email and 2fa) settings makes sense to me.

When you mention "check if email should also be disabled", do you mean to suggest mbin not even store the email in the db? Because I think it'd still be useful to have that information for, say, notification emails.

e-five256 commented 7 months ago

I think you answered my exact question, thanks. It might be better if I leave the email field alone until we have a way to resync from the SSO authority, so maybe not part of this, hopefully doesn't cause too much issue if users can change it themselves

I realized last night I was a bit silly in what I said above :facepalm: The password/2fa should only be hidden if the user is SSO controlled, not if the site is in SSO only mode. Need to make sure it's hidden/inaccessible for those that don't use SSO only mode but still use SSO. Luckily don't think that's any harder, just checking if the user has some oauth setup

e-five256 commented 6 months ago

Whoops, in hindsight I should've delinked the PR or something to not close this issue.

It was pointed out in the PR we need to fix account deletion of SSO users as well. I wasn't quite sure how deletion of accounts is handled in SSO (well, it's possible every implementation handles it differently), so wasn't sure how to tackle it in that PR.

Accounts can still be deleted by admins, just like in the past, but it's missing the recent improvement native accounts have that require no admin actions.

For now will reopen this issue with the goal being of fixing account deletion of SSO accounts requiring admin action

thepaperpilot commented 6 months ago

I don't think you should have to do anything special to delete SSO users. You definitely don't want to be telling the identity provider to delete the account there - just clear the data about the user for mbin specifically.

github-actions[bot] commented 5 months ago

This issue is stale because it has been open 50 days with no activity. Remove stale label or comment or this will be closed in 6 days.

github-actions[bot] commented 3 months ago

This issue is stale because it has been open 50 days with no activity. Remove stale label or comment or this will be closed in 6 days.