wordpress-mobile / WordPress-iOS

WordPress for iOS - Official repository
http://ios.wordpress.org/
GNU General Public License v2.0
3.69k stars 1.12k forks source link

Handle OAuth2 token storage properly #3964

Closed astralbodies closed 7 years ago

astralbodies commented 9 years ago

Related: #3950

When storing the OAuth2 token of a WPAccount instance make sure to use a key for the iOS Keychain that isn't the username. The username is a volatile piece of data and changing that will cause the token to be lost in the keychain with no way to fetch it. #3956 fixes this with a workaround but it's not the final solution.

  1. Instead of using WPAccount.username, use WPAccount.uuid. Verify that the UUID is generated consistently upon creation of a new WPAccount.
  2. Also verify that existing WPAccount instances have logic in place to get a UUID if one wasn't set. This could happen in the case of a Core Data migration from an older version. I think we accounted for this in a heavy migration.
  3. If someone's app is in a junked state (authToken is nil) detect this soon and show the NUX. Do NOT delete the data on disk. Verify that logging back in with the same .com username does not wipe the existing data (importance here is on potential local-only drafts or unsynced changes).
  4. If the app is in the state from step 3 but they are logged in with additional self-hosted accounts (meaning the NUX isn't totally appropriate) show them a login screen that they can cancel out of. Canceling from logging in should most likely wipe the data. Tell the user before this happens.
  5. Users should get an explanation as to why they're being presented the NUX or default .com login screen. This could come in the form of a UIAlertView (which would make me cry) or some sort of information label above the credentials box.

This issue will require fairly extensive testing. Two-factor auth, restore from backup, revoking tokens from server side, etc.

cc: @sendhil @koke @diegoreymendez @oguzkocer @SergioEstevao @aerych

koke commented 9 years ago

I don't think we should use a UUID. Some thoughts:

astralbodies commented 9 years ago

@diegoreymendez is this resolved by #3967 do you think?

diegoreymendez commented 9 years ago

My PR only solves points 3, 4 and 5. The logic we use for storing the username still needs to be reviewed.

I agree that we should never store the email of our users as the username. We should let them log in and call /me to retrieve the proper username as @koke pointed out. The username's case sensitivity should be taken into account, too.

We also need to make sure we have the logic in place to handle the token being invalidated, and other scenarios that could require re-authentication.

diegoreymendez commented 9 years ago

Also: re-authentication currently doesn't care if you log back in with the same account or a different one. This is something to fix, although it probably deserves a different ticket.

diegoreymendez commented 8 years ago

I'm dissociating myself from this issue for the time being, since this is not the current priority.

catehstn commented 7 years ago

This hasn't been looked at in a year, and points 3, 4 and 5 were addressed - @astralbodies what do you think? Can we close it?

astralbodies commented 7 years ago

We can close it - we'll eventually want to address it again when supporting multiple accounts.