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

iOS Keychain conformance #257

Merged
merged 6 commits into from May 1, 2018

Conversation

@heyzooi
Copy link
Contributor

commented Feb 26, 2018

Description

JS SDK using the same params used by the Swift SDK

Changes

Using the same query params used by our Swift SDK

@heyzooi heyzooi self-assigned this Feb 26, 2018

@heyzooi

This comment has been minimized.

Copy link
Contributor Author

commented Feb 26, 2018

this PR is not required to fix the keychain issue, but conforms to our Swift SDK by using the same params.

@georgiwe

This comment has been minimized.

Copy link
Contributor

commented Feb 27, 2018

I don't fully understand keychain and iOS in general, but I'm thinking "will there be a backwards compatibility issue"? Users would have to log in again? Is there any necessary clean up of the old stuff in the keychain or something? Can't we "migrate" those?

@thomasconner
Copy link
Contributor

left a comment

@heyzooi @georgiwe Georgi is right. These changes will require a user to login again. I think these changes make sense so that we use the same params as our Swift SDK but we need to think of how we will migrate existing active users.

We should add some backwards compatibility code to prevent users from having to login again. I will help out with this.

@heyzooi

This comment has been minimized.

Copy link
Contributor Author

commented Feb 27, 2018

right, let me refactor that, that's really no need to keep the same keys, but the security changes are more important to be in place

@@ -158,7 +158,11 @@ export class Client {
* Set the active user
*/
setActiveUser(activeUser) {
return this.activeUserStorage.set(`${this.appKey}.${ACTIVE_USER_KEY}`, activeUser);
if (activeUser != null) {

This comment has been minimized.

Copy link
@heyzooi

heyzooi Feb 27, 2018

Author Contributor

@georgiwe and @thomasconner question that I had was if should use != or !== in this case

This comment has been minimized.

Copy link
@thomasconner

thomasconner Feb 27, 2018

Contributor

you can just do if (activeUser) {

This comment has been minimized.

Copy link
@heyzooi

heyzooi Feb 27, 2018

Author Contributor

done! thanks @thomasconner

@georgiwe

This comment has been minimized.

Copy link
Contributor

commented Mar 13, 2018

I see this is approved - shouldn't we fix the unit tests first?

@tejasranade

This comment has been minimized.

Copy link
Member

commented Mar 28, 2018

@thomasconner @heyzooi Who has the next steps on this PR? The unit tests are currently failing. We can't merge until they are fixed. Maybe this needs a JIRA?

@thomasconner

This comment has been minimized.

Copy link
Contributor

commented Mar 28, 2018

We need to look into why the unit tests are failing and make any changes that are needed. I don't know what changes are needed if any at the moment.

@heyzooi

This comment has been minimized.

Copy link
Contributor Author

commented Apr 24, 2018

@thomasconner should I rebase to see if the unit tests are not working?

@heyzooi heyzooi force-pushed the ios-keychain-conformance branch from 14feb94 to 9ea064a Apr 24, 2018

@heyzooi heyzooi force-pushed the ios-keychain-conformance branch from 2328855 to dd1e75f Apr 25, 2018

@heyzooi

This comment has been minimized.

Copy link
Contributor Author

commented Apr 26, 2018

unit test are running again, @thomasconner i think you can now approve

@thomasconner thomasconner merged commit 537ce45 into master May 1, 2018

2 checks passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
continuous-integration/travis-ci/push The Travis CI build passed
Details

@thomasconner thomasconner deleted the ios-keychain-conformance branch May 1, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants
You can’t perform that action at this time.