-
Notifications
You must be signed in to change notification settings - Fork 4
Add cookie support #144
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add cookie support #144
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The simplicity of the cookie approach feels so much more sound than a proxy. Nicely done.
I performed the testing instructions with private image, video, and audio files. All functioned as expected on both iOS and Android.
The one media that failed was VideoPress URLs. First, it appears VideoPress relies upon a URL token parameter for authentication and the cookie has no impact. Second, the editor seemed to fail handling 206 responses when token authentication succeeded. That said, neither of these seem related to the proposed cookie authentication.
Noting this replaces #30 and supersedes #33, which can be removed and closed respectively. Also, we can close wordpress-mobile/WordPress-Android#21331 as its approach is no longer applicable. We will need to implement appropriate cookie configuration in WP-iOS and WP-Android after merging this work. |
@dcalhoun I'll leave this to you to merge given the breaking changes. |
d1de551
to
c9336a3
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I rebased this atop the latest trunk
branch and resolved merge conflicts. I preformed the Demo app testing instructions for both iOS and Android again—both succeeded.
I also smoke tested the WP-iOS and WP-Android apps with these changes. From reviewing the code and my testing, the proposed changes are backwards compatible and safe to merge as-is. We can work to integrate the cookie support in the host apps later.
What?
Adds the ability to set cookies that'll be used by the editor.
Why?
Ref CMM-235. Close CMM-450. This will make it easy to support private sites for WP.com where the assets require authentication.
How?
Properly routes cookies to where they need to go.
At the moment, there's nothing fancy here – the developer has to know how to properly construct the cookie. We can make this more ergonomic later, if we want.
Testing Instructions
For iOS:
ContentView.swift
'sinit
method to read:[redacted]
with the value of the cookie sent when requestinghttps://wpmobilep2.wordpress.com/wp-content/uploads/2025/05/screenshot-2025-05-20-at-3.35.11e280afpm.jpeg from your browser.
For Android:
MainActivity
'ssetCookies
call to:[redacted]
with the value of the cookie sent when requestinghttps://wpmobilep2.wordpress.com/wp-content/uploads/2025/05/screenshot-2025-05-20-at-3.35.11e280afpm.jpeg from your browser.