Page MenuHomePhabricator

[keyserver] Fix `updateUserAvatar` query
ClosedPublic

Authored by atul on Apr 13 2023, 3:22 PM.
Tags
None
Referenced Files
F3386611: D7437.id25184.diff
Fri, Nov 29, 5:19 AM
F3383032: D7437.diff
Thu, Nov 28, 1:09 PM
Unknown Object (File)
Wed, Nov 6, 5:17 PM
Unknown Object (File)
Tue, Nov 5, 4:24 AM
Unknown Object (File)
Fri, Nov 1, 1:57 AM
Unknown Object (File)
Fri, Nov 1, 1:57 AM
Unknown Object (File)
Fri, Nov 1, 1:57 AM
Unknown Object (File)
Fri, Nov 1, 1:57 AM
Subscribers

Details

Summary

We want to match the check introduced in D7430 (and fixed in D7435...) before setting container for any previous image avatar upload to NULL.


(Kind of) depends on D7435

Test Plan
  1. Set image avatar
  2. Set emoji avatar
  3. Set image avatar
  4. Set malformed image avatar (uploadID doesn't point to upload that can be set as image avatar... whether because it's nonsensical or belongs to another uploader or is already contained by eg a message)

Diff Detail

Repository
rCOMM Comm
Branch
master
Lint
No Lint Coverage
Unit
No Test Coverage

Event Timeline

keyserver/src/updaters/account-updaters.js
127–141

At a very high level this query is about unsetting container for upload belonging to previous image avatar.

One thing we want to be careful about is unsetting container and "breaking" the previous avatar state until we're sure we can get to the intended avatar state as described by the UpdateUserAvatarRequest.

The four possible new states are

  • no avatar (null)
  • emoji avatar
  • ens avatar
  • another image avatar

In the first three cases, we can safely unset container since "everything we need" (contents of newAvatarValue) for the new avatar state will be handled by the UPDATE users query and only affect the users table. For these cases the ${mediaID} IS NULL condition will be true and all upload rows with uploader and container set to viewer.userID will be cleared.

However, if the intended future avatar state is an image avatar... we want to make sure that container can be set for the upload corresponding to that future image avatar before we unlink the existing one.

If, for example, a malicious user or broken client sent a request with uploadID = 329058190 without this change:

  1. The first query would unlink the previous image avatar upload
  2. The second query would fail to set container for provided nonexistent uploadID
  3. The third query would fail to set the future image avatar contents in the users table because no upload with uploadID exists.

And we would be left with the old image avatar pointing to a now unlinked upload that will be swept into the void by the cron job that cleans up containerless uploads. This is a broken state.

However, with this change:

  1. The first query would NOT unlink the previous image avatar upload because there is no valid upload that corresponds with nonexistent uploadID
  2. The second query would not find any upload with nonexistent uploadID to set container for
  3. The third query would fail to find upload that matches uploadID (or meets any of the conditions such as uploader, etc) and make no updates to users table

And we would be where we were before the avatar change request was made. It might not be the intended state, but it's a consistent state that isn't broken.

atul requested review of this revision.Apr 13 2023, 3:40 PM
keyserver/src/updaters/account-updaters.js
148

I thought it didn't hurt to add this check. We definitely don't want to assign an upload to avatar that could also show up in fetchMediaForThread.

Whether this check is necessary or not is questionable... open to keeping or leaving out.

This revision is now accepted and ready to land.Apr 13 2023, 7:40 PM
This revision was automatically updated to reflect the committed changes.