immich-app/immich
1
0
Fork 0
mirror of https://github.com/immich-app/immich.git synced 2026-02-05 00:30:57 +03:00
Code Issues 11 Packages Projects Releases 10 Wiki Activity

[BUG] If the OAuth user signs in before the admin user is created, the web interface doesn't go through the admin user creation proces #433

New Issue
Closed
opened 2026-02-04 20:27:15 +03:00 by OVERLORD · 11 comments
OVERLORD commented 2026-02-04 20:27:15 +03:00
Owner
Copy Link

Originally created by @alextran1502 on GitHub (Nov 21, 2022).

Originally assigned to: @jrasm91 on GitHub.

Describe the bug
If the OAuth user signs in before the admin user is created, the web interface doesn't go through the admin user creation process.

To Reproduce
Steps to reproduce the behavior:

  1. Create a new instance with the OAuth feature.
  2. Login with OAuth user.
  3. Navigate to the web and click on getting started.
  4. The redirect will be going to the login form instead of the admin user creation form.

Expected behavior
This is debatable:
Scenario 1: The first OAuth user that sign in will be made admin
Scenario 2: Correctly determined there is no admin user yet to show the admin creation form.

System

  • Server Version: 1.36.1
  • Mobile App Version: 1.36.1
Originally created by @alextran1502 on GitHub (Nov 21, 2022). Originally assigned to: @jrasm91 on GitHub. <!-- Note: Please search to see if an issue already exists for the bug you encountered. --> **Describe the bug** If the OAuth user signs in before the admin user is created, the web interface doesn't go through the admin user creation process. **To Reproduce** Steps to reproduce the behavior: 1. Create a new instance with the OAuth feature. 2. Login with OAuth user. 3. Navigate to the web and click on getting started. 4. The redirect will be going to the login form instead of the admin user creation form. **Expected behavior** This is debatable: Scenario 1: The first OAuth user that sign in will be made admin Scenario 2: Correctly determined there is no admin user yet to show the admin creation form. **System** - Server Version: 1.36.1 - Mobile App Version: 1.36.1
OVERLORD commented 2026-02-04 20:27:23 +03:00
Author
Owner
Copy Link

@alextran1502 commented on GitHub (Nov 22, 2022):

Also, a good chance to work on #351 as well.

@alextran1502 commented on GitHub (Nov 22, 2022): Also, a good chance to work on #351 as well.
OVERLORD commented 2026-02-04 20:27:28 +03:00
Author
Owner
Copy Link

@Mortein commented on GitHub (Nov 27, 2022):

Scenario 3...?

If using OAuth, a user's current group membership should be used to grant the Admin role on login (every login).
In some instances, there could be more than one administrator, there may be no administrators, or a user may have the admin group temporarily added.

@Mortein commented on GitHub (Nov 27, 2022): Scenario 3...? If using OAuth, a user's current group membership should be used to grant the Admin role on login (every login). In some instances, there could be more than one administrator, there may be no administrators, or a user may have the admin group temporarily added.
OVERLORD commented 2026-02-04 20:27:31 +03:00
Author
Owner
Copy Link

@bo0tzz commented on GitHub (Nov 28, 2022):

I think we should always have a local admin user first, to prevent lockouts. That would also be a necessity if we want to move the oauth configuration into the web interface instead of using env vars.

@bo0tzz commented on GitHub (Nov 28, 2022): I think we should always have a local admin user first, to prevent lockouts. That would also be a necessity if we want to move the oauth configuration into the web interface instead of using env vars.
OVERLORD commented 2026-02-04 20:27:34 +03:00
Author
Owner
Copy Link

@alextran1502 commented on GitHub (Nov 29, 2022):

I agree with @bo0tzz here

@alextran1502 commented on GitHub (Nov 29, 2022): I agree with @bo0tzz here
OVERLORD commented 2026-02-04 20:27:41 +03:00
Author
Owner
Copy Link

@jrasm91 commented on GitHub (Dec 4, 2022):

Unless there are strong opinions otherwise, I think the first user to login should be made an admin. I can add that logic to the oauth login flow.

@jrasm91 commented on GitHub (Dec 4, 2022): Unless there are strong opinions otherwise, I think the first user to login should be made an admin. I can add that logic to the oauth login flow.
OVERLORD commented 2026-02-04 20:27:48 +03:00
Author
Owner
Copy Link

@alextran1502 commented on GitHub (Dec 4, 2022):

@jrasm91 I would like to keep the first login flow on the web intact regardless of whether OAuth is enabled. The user can link the OAuth account to the admin account later after creating the first admin account on the web. My reason is that I want to keep the diversification of creating an admin account as small as possible.

@alextran1502 commented on GitHub (Dec 4, 2022): @jrasm91 I would like to keep the first login flow on the web intact regardless of whether OAuth is enabled. The user can link the OAuth account to the admin account later after creating the first admin account on the web. My reason is that I want to keep the diversification of creating an admin account as small as possible.
OVERLORD commented 2026-02-04 20:27:55 +03:00
Author
Owner
Copy Link

@jrasm91 commented on GitHub (Dec 4, 2022):

OK, so the fix would instead be changing the web "isAdminUserExist" logic to work as expected? Right now it assumes if user count > 0 there is an admin.

@jrasm91 commented on GitHub (Dec 4, 2022): OK, so the fix would instead be changing the web "isAdminUserExist" logic to work as expected? Right now it assumes if user count > 0 there is an admin.
OVERLORD commented 2026-02-04 20:27:59 +03:00
Author
Owner
Copy Link

@bo0tzz commented on GitHub (Dec 4, 2022):

Fixing that logic to actually check for an admin user is definitely a good idea. I think we should also stick with the requirement of creating an admin account before the app can be used. That was enforced previously, but the oauth setup 'circumvents' it. That can be resolved just by moving the oauth config into the web config panel instead of using env vars.

@bo0tzz commented on GitHub (Dec 4, 2022): Fixing that logic to actually check for an admin user is definitely a good idea. I think we should also stick with the requirement of creating an admin account before the app can be used. That was enforced previously, but the oauth setup 'circumvents' it. That can be resolved just by moving the oauth config into the web config panel instead of using env vars.
OVERLORD commented 2026-02-04 20:28:06 +03:00
Author
Owner
Copy Link

@jrasm91 commented on GitHub (Dec 4, 2022):

I don't think admin setup logic needs to be coupled with how we store configuration.

The requirement you're describing is to prevent (non-admin) user from signing up until after there's a local admin. I think we could do that just by adding an is local admin check before registering a new user (via oauth).

@jrasm91 commented on GitHub (Dec 4, 2022): I don't think admin setup logic needs to be coupled with how we store configuration. The requirement you're describing is to prevent (non-admin) user from signing up until after there's a local admin. I think we could do that just by adding an is local admin check before registering a new user (via oauth).
OVERLORD commented 2026-02-04 20:28:12 +03:00
Author
Owner
Copy Link

@bo0tzz commented on GitHub (Dec 4, 2022):

I'm not saying we need to hard-couple them, but just that if oauth config happens via the admin panel then there will necessarily need to be an admin user before oauth can be set up. That's not a specific enforcement in code, but just something that happens 'naturally'. Adding an extra check to only allow oauth login if there is already an admin can't hurt, of course.

@bo0tzz commented on GitHub (Dec 4, 2022): I'm not saying we need to hard-couple them, but just that if oauth config happens via the admin panel then there will necessarily need to be an admin user before oauth can be set up. That's not a specific enforcement in code, but just something that happens 'naturally'. Adding an extra check to only allow oauth login if there is already an admin can't hurt, of course.
OVERLORD commented 2026-02-04 20:28:17 +03:00
Author
Owner
Copy Link

@jrasm91 commented on GitHub (Dec 4, 2022):

Yeah, that makes sense. IMO, having a specific check for this would be less likely to lead to related bugs in the future.

We could add the check to the user repository create method that should be used anytime a new user is created. This would also take care of any new user creation processes that get added in the future.

@jrasm91 commented on GitHub (Dec 4, 2022): Yeah, that makes sense. IMO, having a specific check for this would be less likely to lead to related bugs in the future. We could add the check to the user repository create method that should be used anytime a new user is created. This would also take care of any new user creation processes that get added in the future.
Sign in to join this conversation.
No Branch/Tag Specified
Branches Tags
Labels
Clear labels
accessibility
changelog:enhancement
changelog:security
changelog:translation
cli
date-time
documentation
external-library
format
good first issue
needs-answer
nice to have
sharing
tech-debt
📱mobile
🖥️web
🗄️server
🧠machine-learning
No Label
Milestone
No items
No Milestone
Projects
Clear projects
No project
Assignees
Clear assignees
OVERLORD
No Assignees
1 Participants
Notifications
Due Date
No due date set.
Dependencies

No dependencies set.

Reference: immich-app/immich#433
Delete Branch "%!s()"

Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?