Skip to content
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

[SIWA] Handle invalid Apple user #12549

Merged
merged 9 commits into from Sep 25, 2019

Conversation

ScoutHarris
Copy link
Contributor

@ScoutHarris ScoutHarris commented Sep 24, 2019

Fixes #12399
Related WPAuth PR: wordpress-mobile/WordPressAuthenticator-iOS#137

This uses the WPAuth changes to:

  • Save the Apple user id to the keychain.
  • On app launch, get the user id from the keychain and check it's state.
  • If the user id is no longer authorized, log out of the account (using the same logic as logging out from the Me tab).

To note, this only addresses the scenario in the testing steps, i.e. checking state on app launch. We're not yet actively listening for Apple ID disconnects (#12535). Stay tuned!

To test:

This should be tested on a real device for proper keychain handling.

  • Log in / sign up with Apple.
  • Disconnect in your Apple account on the device.
    • Go to iOS Settings.
    • Select your iCloud account at the top.
    • Go to Password & Security > Apps Using Your Apple ID.
    • Select WordPress > Stop Using Apple ID.
  • Rerun the app.
  • The app should log you out.
  • This message should appear in the logs: checkAppleIDCredentialState: Unauthorized Apple ID. User signed out.

Update release notes:

  • I have considered if this change warrants user-facing release notes and have added them to RELEASE-NOTES.txt if necessary.

Copy link
Contributor

@frosty frosty left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks good to me! One small comment about how we're logging an error, and then my only other question is whether we should do this every time the app returns to the foreground instead of on every launch – I don't know the correct answer to that though. @astralbodies, any thoughts?

try SFHFKeychainUtils.deleteItem(forUsername: WPAppleIDKeychainUsernameKey,
andServiceName: WPAppleIDKeychainUsernameKey)
} catch {
DDLogDebug("Error while removing Apple User ID: \(error)")
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we want to log this as an 'error'? If I'm reading correctly it could simply be that the user has never logged in using SIWA, so there just wouldn't be a keychain item for Apple so we'll be logging out "Error while removing..." on every launch.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hey @frosty .

user has never logged in using SIWA, so there just wouldn't be a keychain item for Apple so we'll be logging out "Error while removing..." on every launch.

That is correct. That's why I used DDLogDebug. But now I see that the actual message can be misleading. I can change it to be less alarming, maybe something like:
DDLogDebug("Unable to remove Apple User ID from keychain: \(error)")

I didn't want to not log it since there could be an actual error.

@ScoutHarris
Copy link
Contributor Author

Hey @frosty . I'm going to go ahead and merge these PRs to streamline my branchiness. My next task is working with the same code, so if the log message is still not acceptable I'd be more than happy to change it in the next PR.

@ScoutHarris ScoutHarris merged commit 1cac04f into develop Sep 25, 2019
@ScoutHarris ScoutHarris deleted the issue/12399-siwa_handle_invalid_user branch September 25, 2019 18:30
@frosty
Copy link
Contributor

frosty commented Sep 26, 2019

Thanks @ScoutHarris! The error message is better. As an additional change, we could not log the message if it's just that no item was found (which is the main non-error state):

} catch let error as NSError {
    if error.code != errSecItemNotFound {
        DDLogDebug("Unable to remove Apple User ID from keychain: \(error.localizedDescription)")
    }
}

We can roll this into a future PR, if you think it makes sense :)

@ScoutHarris
Copy link
Contributor Author

Heya @frosty .

Yep, yep that makes sense. 🤦‍♂ I'll roll it into #12552. Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Sign In With Apple Sign In With Apple
Projects
None yet
Development

Successfully merging this pull request may close these issues.

SIWA: Check for token validity on launch
2 participants