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

Add option to disable local auth form #2752

Merged
merged 2 commits into from
Aug 13, 2024

Conversation

thefinn93
Copy link
Contributor

@thefinn93 thefinn93 commented Jul 18, 2024

Do you follow the guidelines?

What: This adds a config option to indicate that user auth should only happen via oauth2. Setting DISABLE_LOCAL_AUTH=1 will remove the username/password form from the login screen, and remove the password change/oauth link/unlink options from the user settings screen. It also makes the login form target refuse to validate credentials, and makes the oauth2 unlink endpoint refuse to unlink, instead redirecting back to the login screen.

Why: I'd like my users not to be confused about how to login. The (non-functional) form asking for a username and password is very confusing. My users don't know how the login system works and they shouldn't need to. #2649 discusses some other reasons this may be a desirable configuration. I decided to hide the password change and oauth account unlink settings to prevent users from thinking they changed their oauth password or accidentally breaking their miniflux account by unlinking it.

Other things to note:

  • I added a check at startup to refuse to run if DISABLE_LOCAL_AUTH is enabled and no oauth provider is configured.
  • I'm not sure I like the name DISABLE_LOCAL_AUTH. Maybe DISABLE_PASSWORD_AUTH or something? I'm also not a huge fan of the double negative, maybe it should be something like ENABLE_PASSWORD_AUTH with a default value of true.
  • Setting DISABLE_LOCAL_AUTH hides the UI elements to set/change user password, but does not disable the endpoint/internal logic to do it.
  • This seems to have been requested a few times, so shout out to Add flag to disable local auth #2649 and Disable built-in login form #2220.
  • I didn't add any automated tests, since this is largely UI changes. I'm sure I can come up with a test or two if you want. I did manually test this, including deploying it to my own infrastructure.

Copy link
Member

@fguillot fguillot left a comment

Choose a reason for hiding this comment

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

Can you please update the man page with the new config option.

@thefinn93
Copy link
Contributor Author

Done

@57194
Copy link

57194 commented Aug 1, 2024

This only checks against if an OAuth provider is configured…what about AUTH_PROXY_HEADER?

Edit: Probably out of scope of this PR, but it would be nice if there was a way to clear a password if one had been set in the past, that way only API keys can work.

Edit 2: Never run random SQL you find online, but if you're brave, password-clearing isn't too bad:

UPDATE users SET password='' WHERE username='theUsernameToClearPasswordFor';

@thefinn93
Copy link
Contributor Author

I've never used miniflux with AUTH_PROXY_HEADER, but I'd imagine adding support for that would just mean updating the safety check in internal/cli/cli.go that currently ensures that an oauth provider is configured. Are there other considerations? That check is easy enough to update.

Clearing a previously set password is definitely out of scope for this PR.

@57194
Copy link

57194 commented Aug 1, 2024

I've never used miniflux with AUTH_PROXY_HEADER, […]. Are there other considerations?

There's nothing I can think of off the top of my head (but I am by no means an expert).

The only two keys for proxy-based authentication are:

  • AUTH_PROXY_HEADER: string
  • AUTH_PROXY_USER_CREATION: bool

Much like OIDC/OAuth…if you don't have the authentication configured right elsewhere, you're not going to be able to get in anyway. (Or you'll let anyone in if you didn't configure your reverse proxy to require authentication for Miniflux. :-P )


Edit: If you like reading too much, here are some pages that provide some idea on how ForwardAuth/proxy-based auth would be setup:

@thefinn93
Copy link
Contributor Author

I definitely understand how AUTH_PROXY_HEADER works broadly, just not sure if there are any quirks of how Miniflux handles it. Maybe @fguillot can chime in? Happy to update the safety check to add that support.

@fguillot
Copy link
Member

fguillot commented Aug 2, 2024

This is a good point. At least one external authentication must be configured when DISABLE_LOCAL_AUTH=1, either OIDC/OAuth2 or the proxy auth.

I'm also assuming that OAUTH2_USER_CREATION/AUTH_PROXY_USER_CREATION must be set to true when DISABLE_LOCAL_AUTH=1 because the entire authentication section is removed on the settings page, which prevent people to associate their local account to a third-party provider.

image

@thefinn93
Copy link
Contributor Author

I have expanded the check to allow HTTP_PROXY_HEADER but require user creation to also be enabled.

@fguillot fguillot merged commit 770cc1d into miniflux:main Aug 13, 2024
16 checks passed
@thefinn93 thefinn93 deleted the disable-local-auth branch August 13, 2024 05:15
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

3 participants