Allow existing assets for user photo

[timkelty]

Description

With the current controller, the only way to update a user photo is with an upload. This allows you to set it with an existing asset via the photoId body param.

[Mosnar]

Just skimming over this, it seems like there might be some security implications.

A bad actor could assign any arbitrary photo asset as their photo ID. Once they've taken ownership of the photo asset, they can delete that asset with the deletePhoto param. Further, once a photo is set, the asset is moved automatically to the user photos folder.

The extreme example here is that a ne'er-do-well could iterate over all elements, attempt to set them as a profile photo, and then delete the asset when they succeed. Eventually, they could delete all website imagery.

Here's another example - I could set my user photo to an asset ID used on the homepage and then replace my photo (without deleting) with a photo of my cat. Now the site homepage features a photo of my cat.

Sorry if I missed something that prevents this, but I felt like I should say something just in case!

[timkelty]

@Mosnar What a buzzkill! But really, good points all around. :)

Going to flip the PR to draft for now.

Here's another example - I could set my user photo to an asset ID used on the homepage and then replace my photo (without deleting) with a photo of my cat. Now the site homepage features a photo of my cat.

I'm pretty sure this is already somewhat possible prior to this PR. Once the photo is saved to the user, it's just an asset in the "User Photo Location" volume, so there's nothing preventing another field from selecting that asset, and then replacing it from the user, assets index, or the field.

A bad actor could assign any arbitrary photo asset as their photo ID.

For further restriction, we could:

• Require the asset be in the "User Photo Location" volume
• Require the asset was uploaded by the same user

Apart from the replaceability issue that already exists, it seems like these additional restrictions would nullify any bad-actor concerns, I think? Anything I'm missing?

[Mosnar]

I'm pretty sure this is already somewhat possible prior to this PR. Once the photo is saved to the user, it's just an asset in the "User Photo Location" volume, so there's nothing preventing another field from selecting that asset, and then replacing it from the user, assets index, or the field.

This is currently possible, but only if you configure Craft improperly by allowing content authors to select from volumes designated for untrusted user uploads. This PR would remove the opportunity to prevent it via configuration since it doesn't check which volume the user can select an asset from.

Personally, I don't like the idea of users being able to establish a relationship with existing assets, as it creates a lot of opportunities for problems. I think you'd want these restrictions:

1. (Definitely) Limit the asset selection to the user photos volume.
2. (Probably) Instead of using the existing asset ID, duplicate the asset & file to produce a new unique relationship. This would also enforce consistency with any object template variables in the "User Photo Location" setting.
3. (Maybe) Limit asset selection to only assets uploaded by the user.
4. (Maybe) Allow using an existing asset ID, but don't allow users to delete it or replace the file - only swap it out?

This is why I don't get invited to parties...

[timkelty]

Those sound reasonable!

FWIW – this change was to support a frontend where the user profile form is a more traditional "changes don't save until you click save" form, which becomes awkward when the photo field applies immediately, so I'm up for anything that achieves that.