[nosajthenitram]

Add the option to configure the isAdmin flag on a user.

An admin user cannot edit this property on their own user.

[bo0tzz]

I believe currently with this it would be possible for the last admin to remove their own admin permissions, right?

[danieldietzler]

I believe currently with this it would be possible for the last admin to remove their own admin permissions, right?

Yup. And your diff of the generated files looks pretty broken, please fix that and your tests @nosajthenitram

[nosajthenitram]

I believe currently with this it would be possible for the last admin to remove their own admin permissions, right?

Yup. And your diff of the generated files looks pretty broken, please fix that and your tests @nosajthenitram

It shouldn't be possible for the last admin to remove admin rights for themselves, as there's a disable switch condition based on whether or not the current user is equal to the selected user. Would need to think through the race condition of two admins removing one another's rights at the same time.

Will clean up the generated file as well as look at some test augmentation.

[alextran1502]

Would need to think through the race condition of two admins removing one another's rights at the same time.

You can probably put this in a queue and check for the database before making the changes in the database

[bo0tzz]

A database lock could be appropriate, I think doing it through a queue is unnecessarily complex. What I think would be a better/more useful safety measure is to add an immich-admin subcommand to promote a user to admin.

[alextran1502]

What I think would be a better/more useful safety measure is to add an immich-admin subcommand to promote a user to admin.

I like this idea, @nosajthenitram. Can we go with this instead?

[nosajthenitram]

A database lock could be appropriate, I think doing it through a queue is unnecessarily complex. What I think would be a better/more useful safety measure is to add an immich-admin subcommand to promote a user to admin.

The immich-admin commands utilizes the same underlying psql queries, which doesn't offer any additional atomic database access. Why is this the preferred method? Seems as though the same race condition would exist, just moving it from the web to the server?

@alextran1502 Can you elaborate on the immich-admin as the preferred method?

[bo0tzz]

The idea is to not try all too hard to prevent the problem (disabling the toggle like you already do should make it exceedingly rare already), but make it really easy to fix instead with an admin command.

[nosajthenitram]

The idea is to not try all too hard to prevent the problem (disabling the toggle like you already do should make it exceedingly rare already), but make it really easy to fix instead with an admin command.

Gotcha, in addition to the Web control a command to grant admin access to a user. Not necessarily the primary use case, but a CYA.

[nosajthenitram]

The idea is to not try all too hard to prevent the problem (disabling the toggle like you already do should make it exceedingly rare already), but make it really easy to fix instead with an admin command.

Gotcha, in addition to the Web control a command to grant admin access to a user. Not necessarily the primary use case, but a CYA.

@danieldietzler prefer a single branch with the immich-admin command and web interface updates? Or multi-branch since someone else is responsible for server features?

[danieldietzler]

@nosajthenitram if you're happy to do both, feel free to put it all in this PR. It shouldn't be too much in total.

[nosajthenitram]

@nosajthenitram if you're happy to do both, feel free to put it all in this PR. It shouldn't be too much in total.

@danieldietzler I've added commands for grant and remove admin (figured why bother with a one way street, someone may find the remove command useful in automation), and it's a not a huge changeset

[bo0tzz]

I think you may have forgotten to push your changes?

[nosajthenitram]

I think you may have forgotten to push your changes?

No, I'm trying to figure out why server tests are failing (storage related, nothing I've changed) and adding some tests as recommended above. I'm completely new to this code base, so it's taking some time to understand how it's all thrown together.

[bo0tzz]

All good, no rush!

[nosajthenitram]

ok, tests added and files cleaned up. let's see how this goes :D

[danieldietzler]

Looks very good overall, just some minor nits :)