Don't Store WordPress.com Passwords#3308
Conversation
…tore-wpcom-passwords Conflicts: WordPress/WordPress.xcodeproj/project.pbxproj
Yah that was really all I was trying to say :) I used too many words. ;)
For sure! |
There was a problem hiding this comment.
Why is it necessary to copy the request object here?
There was a problem hiding this comment.
You're indeed right, i meant to change that down the road, but obviously forgot to address it. Will update soon!
WordPress/Classes/Models/WPAccount.m
Outdated
There was a problem hiding this comment.
I was trying to figure out why we need to return an empty NSString in this case, but couldn't really figure it out. Can you clarify?
My main concern is that internally we tend to use nil for unset properties - and this would go out of the norm a bit, which I would avoid if possible. If some interface needs a string instead of a nil value, I believe it should be that interface to make the appropriate conversion. Let me know.
There was a problem hiding this comment.
I took the empty string road, there, mostly, to avoid this kind of scenario (which would otherwise require us to check case by case).
However, on second thoughts, XMLRPC shouldn't be called for dotcom accounts. I'll make the arrangements so the password is nil (and will also add failsafes where needed).
Thank you Diego!
|
I might be missing it in all the changes but I haven't found where an existing wpcom is removed from the keychain. Is it in there? If not, should it be added? |
|
@Lantean - It seems oclint is failing. I'm copying the logs here, which show some possible improvements, for ease of reading: |
|
@diegoreymendez P2 error refers to cyclomatic complexity // code improvements. None of those errors apply to the changes introduced in this PR:
Do you mind if we file another issue to address those warnings? @aerych good catch Eric! i'll add a helper to nuke existing dotcom passwords. |
|
@aerych 'Nuke Dotcom Password' mechanism added, and ready for review. Thank you! |
|
I walked through the testing scenarios in the simulator except for D and E -- I'm not currently equipped to test those. |
|
I've gone through the code twice now. Apart from @diegoreymendez's comments (which you've already addressed) I have nothing to add. :) |
|
@aerych thank you very much!. This URI should do the trick:
I've just noticed that, as a side effect of one of the latest changes (stop returning nil passwords), the SSO helper breaks. Will get it fixed asap, thanks Eric! |
|
@jleandroperez - Opening another issue to review the logs I posted it totally acceptable - In fact I just opened a separate ticket and assigned it to myself here. |
|
@diegoreymendez thanks for adding that one!. I'm adding it to my list as well, whoever gets loose first picks it up!. @aerych WordPressApi dependency updated. Opening this link should help you test Scenario F:
If there's anything else we should update / fix / tweak, please, let me know =) Thank you all! |
|
Took Scenario F for another spin. Tested with the link you provided @jleandroperez and also with a link for one of my test apps. Both worked perfectly this time. |
|
I would like to confirm that Scenarios D and E have been thoroughly tested, but otherwise this patch has a 👍 from me. |
|
@aerych thank you very much sir! |
|
Took Scenarios D and E for a test drive (thanks for the test credentials @jleandroperez!) and everything worked as expected. |
from me :)
|
@aerych THANK YOU!!! |
Don't Store WordPress.com Passwords
4 participants
Description:
In this Pull Request, we enhance the ViewControllers outlined below, so that whenever a WordPress.com URL needs authentication, we send over the Bearer Token via the HTTP headers.
This enables us to stop storing the WordPress.com password in the keychain, and also allows us to prevent login errors due to Multifactor-Enabled accounts: the app might not know the latest MFA code, but the OAuth token is expected to be valid.
Plus, WPComOAuthController has been enhanced, so the Single Sign On mechanism also relies on the Bearer Token (scenario outlined in Testing Flow F).
Closes #1220
Enhanced viewControllers:
Testing Scenarios:
Below, we'll detail several testing flows that have been considered while implementing this enhancement.
Flow A: Private Blog
Expected Results:
#### Flow B: Web Stats 1. Open My Sites > 'Testing Blog' 2. Tap over 'Stats' 3. Scroll down, and tap over 'View Web Version'
Expected Results:
#### Flow C: Post Preview 1. Open My Sites > 'Testing Blog' 2. Tap over Blog Posts 3. Tap over any post 4. Press the right top eye button
Expected Results:
#### Flow D: Edit Site 1. Fresh Install 2. Log into a self hosted 'Blog A' 3. Install 'Disable XML-RPC' plugin 4. Open wp-admin in Safari, and update your dotcom password 5. Open 'My Sites' tab 6. Tap over 'Blog A' row 7. Tap 'Settings' 8. Update your password.
Expected Results:
#### Flow E: Install jetpack 1. Open wp-admin in Safari, and turn off Jetpack in your self hosted blog. 2. Log into your self hosted site in WPiOS 3. Tap over 'My Sites' > 'Self Hosted Blog' 4. Tap over 'Stats' 5. Tap over 'Install Jetpack'
Expected Results:
#### Flow F: Single Sign On 1. Install WPiOS and Sign Into a dotcom account. 2. Add a [new testing WordPress App in the Developer Portal](https://developer.wordpress.com/apps/) 3. Open this url in Safari.app: `wordpress-oauth-v2://authorize?client_id=[App ID]&redirect_url=[Callback URL]`
Expected Results: