Skip to content
This repository has been archived by the owner on Jan 11, 2023. It is now read-only.

Extract the keychain wrapper code and add tests for it #7

Merged
merged 5 commits into from Nov 26, 2017

Conversation

nataliq
Copy link
Contributor

@nataliq nataliq commented Oct 10, 2017

The problem: the current code that interacts with the Keychain fails to remove the stored session data. This is not a big issue because the session username is removed from UserDefaults thus the stored data can't be retrieved, however it is still not good.

How to know that the Keychain functions failed? - Currently those do not report success/failure and are not tested. The SpotifyLogin tests only check that the stored session username is saved/removed but not that the data associated with it is securely stored. We need to add tests that verify this in order to be sure that the app login flow works correctly.

Solution: I split the Keychain-related code from the session-specific methods so that those could be tested. The only change to that code is making it return boolean success/failure indicating flag.

The diff is a bit weird although I used git mv to rename the Keychain file..
Anyway, the first commit does not modify the SpotifyLogin functionality, it just extracts the Keychain-related code to its own class and renames the Keychain class to SessionLocalStorage which reflects better what it does (save / load / remove).

Then the next two commits setup the Sample project to include and actually execute a test which check if the keychain values are stored/removed properly.
I added these tests to the Sample project because I was running into problems to interact with the Keychain APIs from the test target of the SpotifyLogin framework (getting OSStatus -34018 all the time, looks like it is a known issue).

Lastly, I will push another commit that fixes the problem and the tests should pass 馃

@nataliq
Copy link
Contributor Author

nataliq commented Oct 11, 2017

Tests failed as expected which is good: https://travis-ci.org/spotify/SpotifyLogin/jobs/286226465#L2283
Pushing the fix now 馃檭

@@ -0,0 +1,39 @@
//
// KeychainWrapperTests.swift
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we replace this with the Spotify copyright notice?

@marmelroy
Copy link
Contributor

Thanks @nataliq - this is a significant improvement and an important bug fix. Can we just fix the copyright comment in the new test?

@marmelroy marmelroy merged commit 87af644 into spotify:master Nov 26, 2017
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants