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

fix: Clears old keychain if the configuration changes #3853

Merged
merged 2 commits into from Feb 28, 2022

Conversation

royjit
Copy link
Member

@royjit royjit commented Oct 30, 2021

Issue #, if available:

Description of changes:

Check points:

  • Added new tests to cover change, if needed
  • All unit tests pass
  • All integration tests pass
  • Updated CHANGELOG.md
  • [] Documentation update for the change if required
  • PR title conforms to conventional commit style

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

harsh62
harsh62 previously approved these changes Nov 12, 2021
@palpatim
Copy link
Member

Could not add test since this require re-configuring AWSMobileClient. This is not possible at runtime

I'm not comfortable with leaving this unautomated. Let's have an out-of-band process populate they keychain with various states so we can test the new code behavior. Off the top of my head:

  • Empty: no creds
  • Populated: same namespace
  • Populated: different namespace

Plus we need to make sure we test different configurations:

  • User Pool: yes, Identity Pool: yes
  • User Pool: yes, Identity Pool: no
  • User Pool: no, Identity Pool: yes (e.g., social federation only)
  • User Pool: no, Identity Pool: no (If valid--I'm thinking like an API key configuration--do we support that with AWSMC?)

It means a bit of extra work to write some utility methods, and it'll be slightly brittle since it it requires the test to know the internal storage details of the keychain. But I'd rather have a test that isn't quite as resilient to changes than have this important feature not have sufficient automated tests.

@royjit
Copy link
Member Author

royjit commented Dec 14, 2021

Testing update 1

1. App without the fix , signIn then install app with the fix

Expected behavior: The app should work as before, the effect will take place only on the next configuration change of the app.

1.1 Instal with same configuration
Expected: User should be still be signed In
Testing: Success

1.2 Instal with different configuration for CUP and CIDP
Expected: User should be still be signed In
Testing: Success

1.3 Instal with same configuration for CUP and no CIDP
Expected: User should be still be signed In
Testing: Success

1.4 Instal with same configuration for CIDP and no CUP
Expected: User should be still be signed In
Testing: Crashes. This behavior is same as the existing implementation. Not fixed with this bug, refer this issue to track.

1.5 no configuration for CIDP and CUP
Expected: User should be still be signed In
Testing: Crashes. This behavior is same as the existing implementation. Not fixed with this bug, refer this issue to track.

@royjit
Copy link
Member Author

royjit commented Dec 17, 2021

Testing 2

2. App with the fix, signIn then

2.1 Instal with same configuration
Expected: User should be still be signed In
Testing: Success

2.2 Instal with different configuration for CUP and CIDP
Expected: User should be signed out
Testing: Success

2.3 Instal with same configuration for CUP and no CIDP
Expected: User should be signed out
Testing: Success

2.4 Instal with same configuration for CIDP and no CUP
Expected: User should be signed out
Testing: Success

2.5 Instal with no configuration for CIDP and CUP
Expected: User should be signed out
Testing: Success

@@ -0,0 +1,33 @@
//
Copy link
Member

Choose a reason for hiding this comment

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

This file has not tests.. Remove it?

@@ -0,0 +1,23 @@
//
Copy link
Member

Choose a reason for hiding this comment

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

Is this the format of the file headers we are going with? Or should it be

//
// Copyright Amazon.com Inc. or its affiliates.
// All Rights Reserved.
//
// SPDX-License-Identifier: Apache-2.0
//

@royjit
Copy link
Member Author

royjit commented Jan 21, 2022

The following scenario needs to be addressed:

  1. Start an app with only CIDP configured
  2. Check the guest identity ID, let it be ID_1
  3. Now add Cognito User Pool
  4. Register a new user and sign In
  5. Check the identity ID
  6. The id should still be ID_1

@royjit royjit force-pushed the royjit.authClearKeychain branch 2 times, most recently from 56ae725 to 73fe601 Compare February 4, 2022 15:59
harsh62
harsh62 previously approved these changes Feb 18, 2022
@royjit
Copy link
Member Author

royjit commented Feb 28, 2022

Tested again all the above test cases and working fine.

@royjit royjit merged commit 8273eac into main Feb 28, 2022
@royjit royjit deleted the royjit.authClearKeychain branch February 28, 2022 22:28
samkudr pushed a commit to samkudr/aws-sdk-ios that referenced this pull request Sep 26, 2022
gabek pushed a commit to KeepSafe/aws-sdk-ios that referenced this pull request Aug 31, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants