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

fix: Prevent Users From Being Created With Missing Group/Household #4500

Conversation

michael-genson
Copy link
Collaborator

@michael-genson michael-genson commented Nov 2, 2024

What type of PR is this?

(REQUIRED)

  • bug

What this PR does / why we need it:

(REQUIRED)

Currently there are a couple of ways of creating users with invalid groups/households. This puts the app into a broken state where it won't run (because Pydantic complains about the missing group/household). This PR catches this issue at the db level so at least users/developers are aware of why (and when) this happens.

Now upon user creation, if the group or household is invalid a ValueError is raised:

# Invalid Group
Group "GROUPNAME" does not exist; cannot create user

# Invalid Household on a valid Group
Household "HOUSEHOLDNAME" does not exist on group "GROUPNAME" (GROUPID); cannot create user

Which issue(s) this PR fixes:

(REQUIRED)

Fixes #4480

Special notes for your reviewer:

(fill-in or delete this section)

@cmintey as far as I know this mostly only impacts LDAP/OIDC user creation. If possible/reasonable it's probably worth addressing this in a more user-friendly way (unsure if it's possible to configure a group/household using the user's auth service?) but at least this will prevent bad things from happening. It also gives the user a reasonable log to work with.

Testing

(fill-in or delete this section)

Added tests to LDAP and OpenID which trigger this issue. Users are no longer created in a bad state.

@github-actions github-actions bot added the bugfix label Nov 2, 2024
Copy link
Contributor

@cmintey cmintey left a comment

Choose a reason for hiding this comment

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

LGTM. The ability to assign users to a group and household upon creation (and re-assign households upon login) is on my todo list. Just haven't had time to fully flesh out the details on it

@Kuchenpirat Kuchenpirat enabled auto-merge (squash) November 5, 2024 23:44
@Kuchenpirat Kuchenpirat merged commit 0fed5f5 into mealie-recipes:mealie-next Nov 5, 2024
13 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[BUG] - Failed to create user, now refusing to start
3 participants