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

feat(server,web): add force delete to immediately remove user #7681

Merged
merged 15 commits into from Mar 8, 2024

Conversation

samholton
Copy link
Contributor

@samholton samholton commented Mar 6, 2024

Addresses #1616

regular delete
force delete confirmation
force delete ready

server/src/domain/user/user.service.ts Outdated Show resolved Hide resolved
server/src/immich/controllers/user.controller.ts Outdated Show resolved Hide resolved
server/src/immich/controllers/user.controller.ts Outdated Show resolved Hide resolved
@samholton samholton force-pushed the feature/add-user-force-delete branch from 85bf4d3 to 37474fb Compare March 7, 2024 03:35
Copy link
Contributor

@jrasm91 jrasm91 left a comment

Choose a reason for hiding this comment

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

LGTM but I agree we can clean up the delete experience. Id recommend sending an event when a user is deleted over a web socket

server/src/domain/user/user.service.ts Outdated Show resolved Hide resolved
@samholton samholton marked this pull request as draft March 8, 2024 04:01
@samholton samholton marked this pull request as ready for review March 8, 2024 05:37
@jrasm91
Copy link
Contributor

jrasm91 commented Mar 8, 2024

I added some websocket stuff, and cleaned up a few things. Let me know what you think and I'll probably merge it tomorrow if it looks good to you.

Copy link
Contributor

@michelheusschen michelheusschen left a comment

Choose a reason for hiding this comment

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

No major issues, just some smaller things that could also be addressed later on

Queue user and assets for immediate deletion
</label>

<input id="forceDelete" type="checkbox" class="form-checkbox h-5 w-5 color" bind:checked={forceDelete} />
Copy link
Contributor

Choose a reason for hiding this comment

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

Accidentally deleting a user is still a little too easy in my opinion, even with this extra checkbox. How about requiring an additional email input for permanent deletions, kinda like github does when deleting a repository?

Also the color class doesn't exist

Copy link
Contributor

Choose a reason for hiding this comment

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

Alex had this same idea and I think it is a good one. I don't care if it is done now or in a separate pull request.

Copy link
Contributor Author

@samholton samholton Mar 8, 2024

Choose a reason for hiding this comment

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

I should have time this weekend to work on that - can do it here or in a new PR, doesn't matter to me either. You thinking an input box that appears when you check the box that disables the button until the user id is entered? Or a separate screen like github that shows up after you click delete that then asks you input?

Removed the color class, not sure where I copied that from

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Added the extra input logic for force delete

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Updated screenshots, the force checkbox jumping when enabling it may be a bit disorienting

Copy link
Contributor

Choose a reason for hiding this comment

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

Perfect. Looks great

@@ -211,11 +218,11 @@
</button>
{/if}
{/if}
{#if isDeleted(immichUser)}
{#if immichUser.deletedAt && immichUser.status === UserStatus.Deleted}
Copy link
Contributor

Choose a reason for hiding this comment

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

Deleting a user can fail and will forever be in the removing state with no available actions from the UI. A cancel button would be a solution, but is probably too complex to implement. Maybe we can leave the restore option available?

Copy link
Contributor

Choose a reason for hiding this comment

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

It is a little tricky because your can't actually restore the user besides in that error situation. Showing the button right away might be confusing.

await this.albumRepository.restoreAll(id);
return mapUser(user);
return this.userRepository.update(id, { deletedAt: null, status: UserStatus.ACTIVE }).then(mapUser);
Copy link
Contributor

Choose a reason for hiding this comment

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

userRepository.restore becomes unused

Copy link
Contributor

Choose a reason for hiding this comment

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

Sweet. We should delete that code then.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

removed and pushed

@jrasm91 jrasm91 merged commit 7a4ae7d into immich-app:main Mar 8, 2024
23 of 24 checks passed
@aviv926
Copy link
Contributor

aviv926 commented Mar 9, 2024

Maybe a admin password for confirmation could be a more secure idea?

@samholton samholton deleted the feature/add-user-force-delete branch March 10, 2024 04:21
@samholton
Copy link
Contributor Author

Maybe a admin password for confirmation could be a more secure idea?

The original intention was more about confirmation than authorization. I believe the GitHub workflow has the confirmation followed by MFA.

I haven't actually tried disabling password with my oauth enabled, but could potentially be an issue if you have passwords disabled

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants