immich icon indicating copy to clipboard operation
immich copied to clipboard

refactor(server): update user vs public user vs auth endpoints

Open martabal opened this issue 1 year ago • 9 comments

Currently, all users have access to all information about other users such as the date they were created, quota usages, if memories is enabled...

This information should only be accessible by the administrators and the users themselves.

martabal avatar Apr 24 '24 17:04 martabal

Deploying immich with  Cloudflare Pages  Cloudflare Pages

Latest commit: 23784ab
Status: ✅  Deploy successful!
Preview URL: https://87dbde42.immich.pages.dev
Branch Preview URL: https://fix-restrict-userresponsedto.immich.pages.dev

View logs

In general I like the idea of differentiating between admin user endpoints and non admin ones, but I think the endpoints need to be adjusted. I was thinking all the existing user endpoints can eventually become admin only. Then we add a new endpoint for searching users that everyone else can use. We should migrate clients to this new one before changing anything about the old one.

The new one can maybe be something like GET /public-users. It would be best if it was in a separate controller.

Good idea! Where should be the getMyUserInfo endpoint? Does it make sense to move it to the auth controller ?

martabal avatar Apr 25 '24 06:04 martabal

Yes I think auth controller should make sense. Probably just /auth/user

jrasm91 avatar Apr 25 '24 11:04 jrasm91

A few notes:

  • user profile methods are used by manually constructing the endpoint, so these need to be updated
  • removing getMyUser will be a breaking change for the mobile app for essentially everyone
  • password updates should happen via auth/reset-password, not "update user" the mobile app needs to migrate already
  • there are enough endpoints changing that we might want to take this opportunity to make any other breaking changes as well

jrasm91 avatar Apr 26 '24 01:04 jrasm91

I would like to take this opportunity to start building up a system for managing breaking changes/backward compatibility in API for the mobile app as well, so the endpoint that got deleted and caused breaking changes should be put back and marked as deprecated and will be removed in the future date

alextran1502 avatar Apr 26 '24 05:04 alextran1502

This is going to be a bit complicated to do because there are probably 4-5 endpoints that fall into this category.

jrasm91 avatar Apr 26 '24 11:04 jrasm91

If we have to do this in a non-breaking manner, I think we'll need to add the new endpoints for public users without changing any of the old implementations (a bit annoying). Then, in the future, update the endpoints to be admin-only, and simplify the implementation.

jrasm91 avatar Apr 26 '24 12:04 jrasm91

Thinking about it, I don't feel good about admins being able to have access to all users' UserResponseDto, IMO they shouldn't know what preferences/features are enabled by other users, so I was thinking of having a new dto to represent users for administrators, something like UserAdminDto. So we have:

  • UserResponseDto which contains all information about the user (accessible only by the user himself)
  • UserAdminDto which is like UserResponseDto without user preferences (accessible only by admins)
  • UserDto which only has the user ID, name, email and avatar/profile photo (accessible to everyone)

martabal avatar Apr 27 '24 11:04 martabal

That sounds good @martabal! I was also thinking about it and we probably should add the new endpoints, then transition the web and mobile app to use them in the future and deprecate the old endpoints before finally removing the old ones.

Specifically we can add public users and new profile picture routes, get my auth user, but revert any changes that deleted old routes and revert the user core changes.

Once the mobile app stops using the old get my user and update user we can change those routes to return the admin user dto too

jrasm91 avatar Apr 27 '24 11:04 jrasm91

Closing in favor of #9730

jrasm91 avatar May 24 '24 17:05 jrasm91