Automattic
Videopress: Request upload token with correct user
Fixes https://github.com/Automattic/wp-calypso/issues/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
Thank you for your PR!
When contributing to Jetpack, we have a few suggestions that can help us test and review your patch:
- :white_check_mark: Include a description of your PR changes.
- :white_check_mark: Add a "[Status]" label (In Progress, Needs Review, ...).
- :white_check_mark: Add a "[Type]" label (Bug, Enhancement, Janitorial, Task).
- :white_check_mark: Add testing instructions.
- :white_check_mark: Specify whether this PR includes any changes to data or privacy.
- :white_check_mark: 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 :robot:
Follow this PR Review Process:
- Ensure all required checks appearing at the bottom of this PR are passing.
- Make sure to test your changes on all platforms that it applies to. You're responsible for the quality of the code you ship.
- You can use GitHub's Reviewers functionality to request a review.
- When it's reviewed and merged, you will be pinged in Slack to deploy the changes to WordPress.com simple once the build is done.
If you have questions about anything, reach out in #jetpack-developers for guidance!
![github-actions[bot] avatar](https://avatars.githubusercontent.com/in/15368?v=4 "Published by a pub.dev verified publisher"){kind=small}
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-userbranch. -
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 rsynccommand 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
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 😅
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!
@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 ?
@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 ?
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!
This PR has been marked as stale. This happened because:
- It has been inactive for the past 3 months.
- It hasn’t been labeled `[Pri] BLOCKER`, `[Pri] High`, etc.
If this PR is still useful, please do a [trunk merge or rebase](https://github.com/Automattic/jetpack/blob/trunk/docs/git-workflow.md#keeping-your-branch-up-to-date) and otherwise make sure it's up to date and has clear testing instructions. You may also want to ping possible reviewers in case they've forgotten about it. Please close this PR if you think it's not valid anymore — if you do, please add a brief explanation.
If the PR is not updated (or at least commented on) in another month, it will be automatically closed.
This PR has been marked as stale. This happened because:
- It has been inactive for the past 3 months.
- It hasn’t been labeled `[Pri] BLOCKER`, `[Pri] High`, etc.
If this PR is still useful, please do a [trunk merge or rebase](https://github.com/Automattic/jetpack/blob/trunk/docs/git-workflow.md#keeping-your-branch-up-to-date) and otherwise make sure it's up to date and has clear testing instructions. You may also want to ping possible reviewers in case they've forgotten about it. Please close this PR if you think it's not valid anymore — if you do, please add a brief explanation.
If the PR is not updated (or at least commented on) in another month, it will be automatically closed.
This PR is being closed due to concerns about potential impacts on other parts of the system. A Linear issue has been created, and this PR can be revisited once work on the issue resumes.
https://linear.app/a8c/issue/VIDP-24/video-uploads-on-at-sites-shows-site-owner-username-in-wp-admin-meta
