Videopress: Request upload token with correct user

[vykes-mac]

Fixes Automattic/wp-calypso#72900

Proposed changes:

Currently the generated token use for uploading videos does not take the user uploading the video into account but always use the blog owner as the user. This causes the blog owner to be attached as the author for every video that's uploaded. This affects audits as the activity log shows incorrect author on the video upload. See the issue for more information

• Send the user requesting the upload token so that the upload token endpoint returns the correct user token.

Other information:

• Have you written new tests for your changes, if applicable?
• Have you checked the E2E test CI results, and verified that your changes do not break them?
• Have you tested your changes on WordPress.com, if applicable (if so, you'll see a generated comment below with a script to run)?

Jetpack product discussion

Does this pull request change what data or activity we track or use?

Testing instructions:

• You will need to apply this diff D165318-code to your sandbox.
• On your Atomic site add another user B who is not the blog owner
• User Jetpack Beta tester to apply this branch to your site.
• Upload a video with User B and verify that the correct author attached to the video is User B
• Do the same with User A or blog owner and verify the same
• Also verify that the activity log is showing the correct user that uploads the video

[github-actions]

Thank you for your PR!

When contributing to Jetpack, we have a few suggestions that can help us test and review your patch:

• ✅ Include a description of your PR changes.
  
• ✅ Add a "[Status]" label (In Progress, Needs Team Review, ...).
  
• ✅ Add a "[Type]" label (Bug, Enhancement, Janitorial, Task).
  
• ✅ Add testing instructions.
  
• ✅ Specify whether this PR includes any changes to data or privacy.
  
• ✅ Add changelog entries to affected projects
  

This comment will be updated as you work on your PR and make changes. If you think that some of those checks are not needed for your PR, please explain why you think so. Thanks for cooperation 🤖

---

The e2e test report can be found here. Please note that it can take a few minutes after the e2e tests checks are complete for the report to be available.

---

Follow this PR Review Process:

1. Ensure all required checks appearing at the bottom of this PR are passing.
2. Choose a review path based on your changes:
   • A. Team Review: add the "[Status] Needs Team Review" label
     • For most changes, including minor cross-team impacts.
     • Example: Updating a team-specific component or a small change to a shared library.
   • B. Crew Review: add the "[Status] Needs Review" label
     • For significant changes to core functionality.
     • Example: Major updates to a shared library or complex features.
   • C. Both: Start with Team, then request Crew
     • For complex changes or when you need extra confidence.
     • Example: Refactor affecting multiple systems.
3. Get at least one approval before merging.

Still unsure? Reach out in #jetpack-developers for guidance!

[github-actions]

Are you an Automattician? Please test your changes on all WordPress.com environments to help mitigate accidental explosions.

• To test on WoA, go to the Plugins menu on a WordPress.com Simple site. Click on the "Upload" button and follow the upgrade flow to be able to upload, install, and activate the Jetpack Beta plugin. Once the plugin is active, go to Jetpack > Jetpack Beta, select your plugin, and enable the update/get-upload-token-for-user branch.

• To test on Simple, run the following command on your sandbox:

  bin/jetpack-downloader test jetpack update/get-upload-token-for-user
  

Interested in more tips and information?

• In your local development environment, use the jetpack rsync command to sync your changes to a WoA dev blog.
• Read more about our development workflow here: PCYsg-eg0-p2
• Figure out when your changes will be shipped to customers here: PCYsg-eg5-p2

[p-jackson]

I'm not super familiar with VideoPress so I've been playing around with it and reading the docs to figure out how it's supposed to work.

Upload a video with User B and verify that the correct author attached to the video is User B

VideoPress seems to change the concept of an uploaded media file belonging to a particular user. The docs mention special considerations about roles (only admins can properly manage videos), and if you look at the VideoPress dashboard at /wp-admin/admin.php?page=jetpack-videopress it doesn't mention anything about the "owner" of a piece of media—unlike the usual media gallery.

No owner info here

And no owner info here

Unlike videos uploaded without VideoPress

That's because I believe VideoPress keeps your media files elsewhere; off your site. You can confirm this with a call to wp post list --post_type=attachment . The CDN where we keep videos mustn't have finegrained ownership metadata.

So I think the idea from #72900 of trying to correct the metadata so it has the "correct" owner isn't the right approach. It doesn't seem compatible with VideoPress.
We can of course still have the activity log record the user who does the uploading correctly. But in that case maybe we should keep using the JETPACK__ANY_USER_TOKEN to do the upload and then find a way to record the activity log correctly as an independent thing.

[p-jackson]

I left a comment on D165318-code for why this external_user_id isn't always going to be enough to generate a user-specific token.

[zaguiini]

Do we need to pass that external_user_id property? There's an already existing Client::wpcom_json_api_request_as_user method we can use to pass the user id.

[vykes-mac]

Do we need to pass that external_user_id property? There's an already existing Client::wpcom_json_api_request_as_user method we can use to pass the user id.

For some reason when this function is used it fails with error You must be logged-in to get an upload token , I suspect because the upload endpoint is an older endpoint it might need some additional configuration for it to work. wanted to get eyes on the PR for discussion so didn't look into it too much. can revisit it.

[vykes-mac]

That's because I believe VideoPress keeps your media files elsewhere; off your site. You can confirm this with a call to wp post list --post_type=attachment . The CDN where we keep videos mustn't have finegrained ownership metadata.

So I think the idea from #72900 of trying to correct the metadata so it has the "correct" owner isn't the right approach. It doesn't seem compatible with VideoPress.
We can of course still have the activity log record the user who does the uploading correctly. But in that case maybe we should keep using the JETPACK__ANY_USER_TOKEN to do the upload and then find a way to record the activity log correctly as an independent thing.

If I'm understanding you correctly the metadata we are referencing here is not necessarily the video metadata but the post meta data.

When a video is uploaded a post type attachment is created on the jetpack site using Jetpack_Media_Sync::upload_media That is the information displayed here

The user token is used for that xml_rpc call to create the attachment on the jetpack blog. This way the correct author is associated with the creation of the attachment post_type and not so much the videopress video metadata.

Hope we are on the same page 😅

[jeherve]

Related: #39034

[CGastrell]

Thanks for taking the time to dig into this!

At this point you know better what the flows are and if you feel this is the right approach, I'll trust you with it. That said, I can only think about what this means in terms of licensing: a user paying for VP immediately attaches the license to a blog and allows all (capable) users to upload videos. I do like this, just noting that we need to be able to track down users (or uploads) by a given token that, in turn, can be tracked down to the license owner. Not a problem, just something to keep in mind.

@vykes-mac if you update (merge or rebase) I'll give it one last spin, but I also encourage you to go forth and try to super test this with as many edge cases as you can think of :) Ping me!

[vykes-mac]

@CGastrell I did a rebase, you can perform some test!

[CGastrell]

@CGastrell I did a rebase, you can perform some test!

Testing from an AT site running this build, I'm getting very mixed results. Simply editing a post and uploading a video produces the wrong activity author and then some bunch of follow up activities, also with mixed authors:

This was a single video upload with user B (later seen as yabranh), but the activity claims I did it with user A (cgastrell):

When that "multiple users" thing is uncollapsed, I see this:

Anything I might have done wrong? Is this expected @vykes-mac ?

[vykes-mac]

@CGastrell I did a rebase, you can perform some test!

Testing from an AT site running this build, I'm getting very mixed results. Simply editing a post and uploading a video produces the wrong activity author and then some bunch of follow up activities, also with mixed authors:

This was a single video upload with user B (later seen as yabranh), but the activity claims I did it with user A (cgastrell):

When that "multiple users" thing is uncollapsed, I see this:

Anything I might have done wrong? Is this expected @vykes-mac ?

Interesting 🤔, I know there are some thumbnails generated when uploading a video that will be logged as the primary owner but the video upload itself should be logged as uploaded by user B. The accompanied diff is applied to your sandbox ?

[CGastrell]

The accompanied diff is applied to your sandbox ?

Yes.

[kraftbj]

Removing this out of the review queue. @vykes-mac - I've updated the branch to trunk. Can you get this back around to the team for review (if ready) etc? Thank you!