fix(auth): handle missing or empty signUpOptions #627

Jordan-Nelson · 2021-06-10T16:08:42Z

Issue #, if available: #292

Description of changes:

Update FlutterSignUpRequest on iOS and Android to handle missing sign up options and missing user attributes
Update SignUpOptions and CognitoSignUpOptions to make userAttributes optional (was optional from perspective of the public API, but was treated as required)
Add integration tests for standard signUp w/ and w/o sign up options

For a standard username based auth (where email is a required attribute), if a consumer omits options, user attributes, or just the email attribute they will receive and InvalidParameterException.

For an email based auth (where email is the username) omitting any of these fields/values will not cause an error.

// currently throws unhelpful error in all scenarios
// will now throw a helpful error for username based auth
// will now work for email based auth
var res = await Amplify.Auth.signUp(
  username: <USERNAME/EMAIL>,
  password: <PASSWORD>,
);

// currently throws unhelpful error in all scenarios
// will now throw a helpful error for username based auth
// will now work for email based auth
var res = await Amplify.Auth.signUp(
  username: <USERNAME/EMAIL>,
  password: <PASSWORD>,
  options: CognitoSignUpOptions(),
);

// unchanged for username based auth (throws InvalidParamterException)
// unchaged for email based auth (works as expected)
Map<String, String> userAttributes = {};
var res = await Amplify.Auth.signUp(
  username: <USERNAME/EMAIL>,
  password: <PASSWORD>,
  options: CognitoSignUpOptions(userAttributes: userAttributes),
);

// unchaged in all scenarios (works as expected)
var res = await Amplify.Auth.signUp(
  username: <USERNAME/EMAIL>,
  password: <PASSWORD>,
  options: CognitoSignUpOptions(userAttributes: {'email': <EMAIL>}),
);

Test scenarios

Scenario	Auth Method	Platform	Expectation	Tested
sign up w/ email attribute	Username	iOS	Sign up works	✅
sign up w/ email attribute	Username	Android	Sign up works	✅
sign up w/ email attribute	Email	iOS	Sign up works	✅
sign up w/ email attribute	Email	Android	Sign up works	✅
sign up w/ no email attribute	Username	iOS	InvalidParamterException	✅
sign up w/ no email attribute	Username	Android	InvalidParamterException	✅
sign up w/ no email attribute	Email	iOS	Sign up works	✅
sign up w/ no email attribute	Email	Android	Sign up works	✅
sign up w/ no attributes	Username	iOS	InvalidParamterException	✅
sign up w/ no attributes	Username	Android	InvalidParamterException	✅
sign up w/ no attributes	Email	iOS	Sign up works	✅
sign up w/ no attributes	Email	Android	Sign up works	✅
sign up w/ no options	Username	iOS	InvalidParamterException	✅
sign up w/ no options	Username	Android	InvalidParamterException	✅
sign up w/ no options	Email	iOS	Sign up works	✅
sign up w/ no options	Email	Android	Sign up works	✅

By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.

haverchuck · 2021-06-10T21:41:57Z

@Jordan-Nelson Since the null safety branch will eventually be merged into master, I wonder if it would make sense to just fix THAT branch - that way we won't have to do duplicate work and release multiple times?

Jordan-Nelson · 2021-06-11T15:22:23Z

@haverchuck I was thinking that originally as well, but I don't think it will lead to much duplication or additional releases. The changes on the dart side were so minimal that I think bringing this into the NS branch should be pretty easy. And I don't think we have to do a release of the NS branch just for this since its an issue in the latest non NS release as well.

…fix/209-handle-missing-signUpOptions

Jordan-Nelson · 2021-06-14T14:31:42Z

Since integration tests are not running in CI yet, here are screenshots of the tests passing on iOS and Android. Note that these were run with #637 pulled in.

codecov-commenter · 2021-06-14T15:07:10Z

Codecov Report

Merging #627 (6d7ae24) into master (8d5b6df) will increase coverage by 0.04%.
The diff coverage is 80.88%.

@@            Coverage Diff             @@
##           master     #627      +/-   ##
==========================================
+ Coverage   69.36%   69.41%   +0.04%     
==========================================
  Files         262      262              
  Lines        8604     8631      +27     
  Branches      353      353              
==========================================
+ Hits         5968     5991      +23     
- Misses       2504     2508       +4     
  Partials      132      132

Flag	Coverage Δ
android-unit-tests	`63.95% <64.28%> (+0.08%)`	⬆️
flutter-unit-tests	`60.91% <ø> (ø)`
ios-unit-tests	`75.19% <85.18%> (+0.02%)`	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files	Coverage Δ
...y_auth_cognito/ios/Classes/AuthCognitoBridge.swift	`0.64% <0.00%> (+<0.01%)`	⬆️
..._flutter/lib/categories/amplify_auth_category.dart	`13.00% <ø> (ø)`
...amplify_auth_cognito/types/FlutterSignUpRequest.kt	`54.54% <64.28%> (+6.71%)`	⬆️
...uth_cognito/ios/Classes/FlutterSignUpRequest.swift	`75.00% <75.00%> (-25.00%)`	⬇️
...le/ios/unit_tests/amplify_auth_cognito_tests.swift	`96.06% <96.55%> (+<0.01%)`	⬆️

…fix/209-handle-missing-signUpOptions

dnys1 · 2021-07-07T15:57:44Z

packages/amplify_auth_plugin_interface/lib/src/SignUp/SignUpOptions.dart

@@ -13,13 +13,12 @@
 * permissions and limitations under the License.
 */

-
-
 class SignUpOptions {


Why not just make this abstract? https://github.com/aws-amplify/amplify-flutter/pull/697/files#diff-634553a25cf511114cd3409bd3324f52178d09bc6467715a52d9f78e8e1e4b8c

And set a default value on Cognito? https://github.com/aws-amplify/amplify-flutter/pull/697/files#diff-67c9b0e8dc3d1ab4a486c06096d4b78d5eaffb25ec1639a42b7f292dde25a6d2R58

The only actual changes in the file are lines 19 and 20. The rest are auto formatting.

(the change seems reasonable, it just isn't related to the PR)

Ah my bad! Sorry I misread the diff.

cshfang

Had some suggestions but

I don't see anything functionally wrong
Many suggestions are nitpicking on existing code

So lgtm overall 🚀

cshfang · 2021-07-07T16:17:10Z

...oid/src/main/kotlin/com/amazonaws/amplify/amplify_auth_cognito/types/FlutterSignUpRequest.kt

-        var authUserAttributes: MutableList<AuthUserAttribute> = mutableListOf();
-        var validationData = rawOptions["validationData"] as? MutableMap<String, String>;
+    private fun formatOptions(rawOptions: HashMap<String, *>?): AWSCognitoAuthSignUpOptions {
+        val options =  AWSCognitoAuthSignUpOptions.builder()


Nit: Is it convention to do this? Imo it's clearer to name this variable optionsBuilder unless the entire build chain is part of the assignment.

cshfang · 2021-07-07T16:39:12Z

...oid/src/main/kotlin/com/amazonaws/amplify/amplify_auth_cognito/types/FlutterSignUpRequest.kt

+    private fun formatOptions(rawOptions: HashMap<String, *>?): AWSCognitoAuthSignUpOptions {
+        val options =  AWSCognitoAuthSignUpOptions.builder()
+        if (rawOptions != null) {
+            val authUserAttributes: MutableList<AuthUserAttribute> = mutableListOf()


Suggested change

val authUserAttributes: MutableList<AuthUserAttribute> = mutableListOf()

cshfang · 2021-07-07T16:39:16Z

...oid/src/main/kotlin/com/amazonaws/amplify/amplify_auth_cognito/types/FlutterSignUpRequest.kt

+                attributeData.forEach { (key, value) ->
+                    val attribute = createAuthUserAttribute(key, value)
+                    authUserAttributes.add(attribute)
+                }
+                options.userAttributes(authUserAttributes)


Nit: More concise and arguably clearer with added benefit of not creating a list at all if attributeData is null

Suggested change

attributeData.forEach { (key, value) ->

val attribute = createAuthUserAttribute(key, value)

authUserAttributes.add(attribute)

}

options.userAttributes(authUserAttributes)

val authUserAttributes: List<AuthUserAttribute> = attributeData.map { (key, value) ->

createAuthUserAttribute(key, value)

}

options.userAttributes(authUserAttributes)

cshfang · 2021-07-07T16:44:40Z

packages/amplify_auth_cognito/example/integration_test/sign_in_sign_out_test.dart

+import 'utils/mock_data.dart';
+import 'utils/setup_utils.dart';


cshfang · 2021-07-07T16:49:55Z

packages/amplify_auth_plugin_interface/lib/src/SignUp/SignUpRequest.dart

+    if (options != null) {
+      pendingRequest['options'] = options!.serializeAsMap();
+    }


Since we remove nulls on line 32, could we just keep line 29 the way it was but will the null-aware access?

'options': options?.serializeAsMap()

cshfang · 2021-07-07T17:08:04Z

packages/amplify_auth_cognito/ios/Classes/FlutterSignUpRequest.swift

+    private func createOptions(options: Dictionary<String, Any>?) -> AuthSignUpRequest.Options? {
+        if (options == nil) {
+            return nil
+        }
+        var formattedAttributes: Array<AuthUserAttribute> = Array()
+        if (options!["userAttributes"] is Dictionary<String, String>) {
+            let rawAttributes: Dictionary<String, Any> = options!["userAttributes"] as! Dictionary<String, String>
+            for attr in rawAttributes {
+                formattedAttributes.append(createAuthUserAttribute(key: attr.key, value: attr.value as! String))
+            }
+        }
+        return AuthSignUpRequest.Options(userAttributes: formattedAttributes)
+
+    }


Is it possible for the initial guard clause to just be

if (!(options?["userAttributes"] is Dictionary<String, String>)) { return nil } // rest of function without if condition

It would reduce some of the nesting and improve readability if so. I think it'd also make it possible to just assign a mapped array directly to formattedAttributes similar to my kotlin suggestion above.

You are right. I am going to make this update. I think I was mimicking what I had done on the Android side. However, on Android the nested if statements has a purpose since options can contain userAttributes and/or validationData. On iOS we aren't doing anything with the validationData. That seems like it might be a bug.

@haverchuck It doesn't seem like there is any way to include "validationData" in the sign up request in amplify-ios. Amplify-flutter seems to just be ignoring that value on iOS. On Android validationData is part of the AWSCognitoAuthSignUpOptions that gets included in the request. Is that a known platform discrepancy?

@Jordan-Nelson Yeah - this is something we need to fix. It is related to the clientmetadata work that Hui and myself have implemented piecemeal. In this case, android and ios should be able to handle both validationData and clientMetadata attributes.

This doesn't need to be part of this PR.

cshfang · 2021-07-07T17:15:33Z

packages/amplify_auth_cognito/ios/Classes/FlutterSignUpRequest.swift

+        let rawAttributes: Dictionary<String, Any> = options["userAttributes"] as! Dictionary<String, String>
+        var formattedAttributes: Array<AuthUserAttribute> = Array()
+        for attr in rawAttributes {
+            formattedAttributes.append(createAuthUserAttribute(key: attr.key, value: attr.value as! String))
+        }
+        return formattedAttributes


Nit: Could simplify this I think

Suggested change

let rawAttributes: Dictionary<String, Any> = options["userAttributes"] as! Dictionary<String, String>

var formattedAttributes: Array<AuthUserAttribute> = Array()

for attr in rawAttributes {

formattedAttributes.append(createAuthUserAttribute(key: attr.key, value: attr.value as! String))

}

return formattedAttributes

let rawAttributes: Dictionary<String, Any> = options["userAttributes"] as! Dictionary<String, String>

return rawAttributes.map { attr in

createAuthUserAttribute(key: attr.key, value: attr.value as! String)

}

cshfang

Couple of cleanup nits but lgtm

packages/amplify_auth_cognito/ios/Classes/FlutterSignUpRequest.swift

packages/amplify_auth_plugin_interface/lib/src/SignUp/SignUpRequest.dart

handle missing or empty signUpOptions

2469c54

Jordan-Nelson and others added 4 commits June 11, 2021 11:50

add android unit test

d3dabd2

fix iOS tests

9235b89

Merge branch 'master' of github.com:aws-amplify/amplify-flutter into …

5601cf4

…fix/209-handle-missing-signUpOptions

Merge branch 'master' into fix/209-handle-missing-signUpOptions

6ff3e39

Jordan-Nelson marked this pull request as ready for review June 11, 2021 19:05

Jordan-Nelson requested a review from a team as a code owner June 11, 2021 19:05

add integration tests for auth.signUp

8e37160

remove unused imports, formatting

f3450e5

Jordan-Nelson mentioned this pull request Jun 14, 2021

chore(auth): integration tests for user attributes #640

Merged

Jordan-Nelson requested a review from haverchuck June 14, 2021 19:12

Jordan-Nelson force-pushed the fix/209-handle-missing-signUpOptions branch 2 times, most recently from 456990d to f3450e5 Compare July 6, 2021 20:47

Jordan-Nelson added 3 commits July 6, 2021 16:54

Merge branch 'master' of github.com:aws-amplify/amplify-flutter into …

72574b1

…fix/209-handle-missing-signUpOptions

update auth category after NS changes

5b0d9bd

update unit test for NS changes

6d7ae24

dnys1 reviewed Jul 7, 2021

View reviewed changes

cshfang approved these changes Jul 7, 2021

View reviewed changes

Jordan-Nelson and others added 2 commits July 8, 2021 11:19

address PR comments

9c344c4

Merge branch 'main' into fix/209-handle-missing-signUpOptions

7d7b848

cshfang approved these changes Jul 8, 2021

View reviewed changes

packages/amplify_auth_cognito/ios/Classes/FlutterSignUpRequest.swift Outdated Show resolved Hide resolved

packages/amplify_auth_plugin_interface/lib/src/SignUp/SignUpRequest.dart Outdated Show resolved Hide resolved

remove extra parens

eb4d8d0

Jordan-Nelson commented Jul 9, 2021

View reviewed changes

packages/amplify_auth_plugin_interface/lib/src/SignUp/SignUpRequest.dart Outdated Show resolved Hide resolved

Jordan-Nelson added 2 commits July 9, 2021 10:16

Apply SignUpRequest changes from code review

a40d5ad

Merge branch 'main' into fix/209-handle-missing-signUpOptions

2ad3e92

dnys1 approved these changes Jul 9, 2021

View reviewed changes

Jordan-Nelson merged commit abc22aa into aws-amplify:main Jul 9, 2021

Jordan-Nelson deleted the fix/209-handle-missing-signUpOptions branch November 11, 2021 19:12

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix(auth): handle missing or empty signUpOptions #627

fix(auth): handle missing or empty signUpOptions #627

Jordan-Nelson commented Jun 10, 2021 •

edited

Loading

haverchuck commented Jun 10, 2021 •

edited

Loading

Jordan-Nelson commented Jun 11, 2021

Jordan-Nelson commented Jun 14, 2021

codecov-commenter commented Jun 14, 2021 •

edited

Loading

dnys1 Jul 7, 2021

Jordan-Nelson Jul 7, 2021

Jordan-Nelson Jul 7, 2021

dnys1 Jul 7, 2021

cshfang left a comment

cshfang Jul 7, 2021

cshfang Jul 7, 2021

cshfang Jul 7, 2021

cshfang Jul 7, 2021

cshfang Jul 7, 2021

cshfang Jul 7, 2021

Jordan-Nelson Jul 8, 2021

haverchuck Jul 8, 2021 •

edited

Loading

cshfang Jul 7, 2021

cshfang left a comment

		import 'utils/mock_data.dart';
		import 'utils/setup_utils.dart';

fix(auth): handle missing or empty signUpOptions #627

fix(auth): handle missing or empty signUpOptions #627

Conversation

Jordan-Nelson commented Jun 10, 2021 • edited Loading

Test scenarios

haverchuck commented Jun 10, 2021 • edited Loading

Jordan-Nelson commented Jun 11, 2021

Jordan-Nelson commented Jun 14, 2021

codecov-commenter commented Jun 14, 2021 • edited Loading

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cshfang left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

haverchuck Jul 8, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cshfang left a comment

Choose a reason for hiding this comment

Jordan-Nelson commented Jun 10, 2021 •

edited

Loading

haverchuck commented Jun 10, 2021 •

edited

Loading

codecov-commenter commented Jun 14, 2021 •

edited

Loading

haverchuck Jul 8, 2021 •

edited

Loading