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

Feature: Add "Authentication Method" to allow existing users to sign in with LDAP #2143

Merged

Conversation

cmintey
Copy link
Contributor

@cmintey cmintey commented Feb 17, 2023

What type of PR is this?

  • documentation
  • feature

What this PR does / why we need it:

Overview

This PR introduces a new auth_method to the users table so that an administrator can set the method of authentication for their users. The current authentication methods are

  • AuthMethod.MEALIE (Mealie, default)
  • AuthMethod.LDAP (LDAP)

Features

  • When a new user is created via LDAP, the auth_method is set to LDAP.
  • Existing users can be updated in the admin panel to use LDAP as their authentication.
  • Users with LDAP authentication method will not be able to update or reset their password as that should be handled via the LDAP provider itself

Migration

  • adds auth_method column to the user table
  • default auth_method is Mealie
  • users with a password of "LDAP" will be automatically set up with auth_method as LDAP

Open Items

  • If a user created by LDAP is changed over to use Mealie auth, their password will be "LDAP" (plaintext). How should we handle that?
  • Should users be able to be created in the admin panel with LDAP as auth method? Doesn't make a lot of sense since they can just log in and be created automatically.

Screenshots

User Management Screen User management screen
User Edit Screen User management screen
User Creation Screen User management screen

Testing

  • Test authenticate_user method with the new auth_method set for a test user
  • Test create and update user actions
  • Test the password reset service

Release Notes

Adds authentication method field to allow existing users to be switched over to LDAP auth

@cmintey cmintey changed the title adds authentication method for users [Feature] Add "Authentication Method" to allow existing users to sign in with LDAP Feb 17, 2023
@hay-kot
Copy link
Collaborator

hay-kot commented Feb 20, 2023

If a user created by LDAP is changed over to use Mealie auth, their password will be "LDAP" (plaintext). How should we handle that?

The "correct" thing to do would be to prompt them on first login to reset their password. We don't currently have any infrastructure setup for anything that, so probably not the best. I don't have any good ideas... I'll have to think on this for a bit

Should users be able to be created in the admin panel with LDAP as auth method? Doesn't make a lot of sense since they can just log in and be created automatically.

I would say let's not. Simplifies the implementation.

@cmintey
Copy link
Contributor Author

cmintey commented Feb 20, 2023

If a user created by LDAP is changed over to use Mealie auth, their password will be "LDAP" (plaintext). How should we handle that?

The "correct" thing to do would be to prompt them on first login to reset their password. We don't currently have any infrastructure setup for anything that, so probably not the best. I don't have any good ideas... I'll have to think on this for a bit

One solution (maybe the easiest) is just to have it documented that if this happens, the user will need to go through the reset password flow.

We could also include an option on the admin panel to generate a password reset link for a user. This could be useful for folks who don't have SMTP set up on their server, which makes the password reset functionality useless, currently.

I do think that long-term, it would be better to have that prompt-on-initial-login flow.

@cmintey cmintey force-pushed the feat/select-authentication-provider branch from 353cdf8 to 830281e Compare February 22, 2023 21:49
@cmintey cmintey marked this pull request as ready for review February 22, 2023 21:54
@hay-kot
Copy link
Collaborator

hay-kot commented Feb 23, 2023

One solution (maybe the easiest) is just to have it documented that if this happens, the user will need to go through the reset password flow.

I agree. If we can document that somewhere and mark it as a temporary solution until we do proper password reset on login, that would be sufficient.

We could also include an option on the admin panel to generate a password reset link for a user. This could be useful for folks who don't have SMTP set up on their server, which makes the password reset functionality useless, currently.

I think that would be ideal, I would say most users don't have SMTP enabled. I think that would be a fairly easy endpoint to implement, let me know if you want to do that as apart of this PR or in the future

I do think that long-term, it would be better to have that prompt-on-initial-login flow.

Tracking in

@cmintey
Copy link
Contributor Author

cmintey commented Feb 23, 2023

If we can document that somewhere and mark it as a temporary solution until we do proper password reset on login, that would be sufficient.

I've added the documentation in a new LDAP page that you'll need to reset your password.

I think that would be ideal, I would say most users don't have SMTP enabled. I think that would be a fairly easy endpoint to implement, let me know if you want to do that as apart of this PR or in the future

I'll look into this soon. I think it should probably be in a different PR, that way we can get this main functionality merged in now.

@cmintey cmintey force-pushed the feat/select-authentication-provider branch from 830281e to 6b29fd6 Compare February 24, 2023 15:01
@cmintey
Copy link
Contributor Author

cmintey commented Feb 24, 2023

Changed the create user screen to be more like the password field on the update user screen.

The field now shows up, but is readonly and disabled.

image

@cmintey cmintey marked this pull request as ready for review February 24, 2023 15:02
@cmintey cmintey changed the title [Feature] Add "Authentication Method" to allow existing users to sign in with LDAP Feature: Add "Authentication Method" to allow existing users to sign in with LDAP Feb 24, 2023
@hay-kot hay-kot merged commit 2e6ad5d into mealie-recipes:mealie-next Feb 26, 2023
@cmintey cmintey deleted the feat/select-authentication-provider branch March 24, 2023 17:18
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants