RFC: 0095 cognito construct library

[nija-at]

#95

---

Ready for feedback.

Rendered version

---

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

[eladb]

Initial comments

[eladb]

Please follow the RFC README, create a tracking issues, reference it here in the title, etc.

[duarten]

Thanks for working on this! :)

[duarten]

Why not an array for custom attributes too?

custom: [new StringAttr('myappid', {minLen: 5, maxLen: 15 })]

[rix0rrr]

I also don't see from this example code how you make a custom attribute required or a standard attribute optional. The sample seems to conflate required == standard, custom == optional (although that might be a Cognito limitation that I'm unaware of).

Would expect to see this though:

new UserPool(this, 'myuserpool', {
  // ...
  requiredAttributes: [
    Attribute.NAME,
    Attribute.customString('myappid', { minLen: 5, maxLen: 15 })
  ],
  optionalAttributes: [
    Attribute.ADDRESS,
    Attribute.customNumber('callingcode', { min: 1, max: 3 })
  ]
});

[nija-at]

@rix0rrr -

The sample seems to conflate required == standard, custom == optional (although that might be a Cognito limitation that I'm unaware of).

That is exactly Cognito's modeling.

Quoting from the documentation for standard attributes -- "These attributes are available as optional attributes for all users. To make an attribute required, select the check box next to the attribute. ".

Quoting from the documentation for custom attributes -- "cannot be required"

Given this information, is my modeling ok?

[nija-at]

@duarten - switched this from an array to map based on another comment. I believe the map-style is a bit more ergonomic and human friendly.

[duarten]

Why isn't this going to be supported? Seems like it could be a simple switch.

[rix0rrr]

Yes if CFN can do it there shouldn't really be a blocker.

Or is it because SES doesn't have appropriate L2s yet?

[nija-at]

Or is it because SES doesn't have appropriate L2s yet?

Correct!

Seems like it could be a simple switch.

It's not a simple switch. A bunch of configurability comes along with it and without proper class modeling in SES, this will look weird.

At the same time, given the extended nature of Cognito's features, something had to be cut for the first round of implementation and this got cut. This is not saying we will not support it but just not yet. Users can always avail this configuration via escape hatches, albeit it won't be clean.

[rix0rrr]

May be worth making a class with factory functions for this?

new UserPool(this, 'myuserpool', {
  signUp: SignUp.byEmail()  // Has some nice defaults that can be customized
});

[nija-at]

I've flattened this mapping out by one level. What do you think about this now? Do you think it still need factory functions?

[rix0rrr]

I also don't see from this example code how you make a custom attribute required or a standard attribute optional. The sample seems to conflate required == standard, custom == optional (although that might be a Cognito limitation that I'm unaware of).

Would expect to see this though:

new UserPool(this, 'myuserpool', {
  // ...
  requiredAttributes: [
    Attribute.NAME,
    Attribute.customString('myappid', { minLen: 5, maxLen: 15 })
  ],
  optionalAttributes: [
    Attribute.ADDRESS,
    Attribute.customNumber('callingcode', { min: 1, max: 3 })
  ]
});

[rix0rrr]

Aha. Well I'd still expect to see the previous code with a runtime check or something.

[nija-at]

I don't follow this comment. Can you re-state?

[rix0rrr]

I think I'd like to encourage a factory function here again, and same for passwordPolicy. Or flatten out the properties into the top-level props object.

We have a construct library design guideline to flatten out the properties namespace.

[rix0rrr]

Yes if CFN can do it there shouldn't really be a blocker.

Or is it because SES doesn't have appropriate L2s yet?

[rix0rrr]

We will provide factory functions for the common use cases, right?

[nija-at]

The subsequent paragraphs and code snippet is how I propose to model this JSON. This should be sufficient, no?

[rix0rrr]

Does it make sense to autocreate these?

const pool = new IdentityPool(this, 'my-awesome-id-pool');

bucket.grantRead(pool.unauthenticatedRole);
bucket.grantReadWrite(pool.authenticatedRole);

Especially since I believe the assumedBy parameter for the roles is going to be quite tricky in practice, no?

[nija-at]

Does it make sense to autocreate these?

Yes, there should be a default role created for these with the authenticatedRole property to specify an existing role. I seem to have missed this and will add that in.

Especially since I believe the assumedBy parameter for the roles is going to be quite tricky in practice, no?

The doc states -- "The CDK, by default, adds a trust policy with these configured to all roles used by the IdentityPool. To disable this, set the configureRoleTrust to false."

Essentially I'm hoping to call role.assumeRolePolicy.addStatements(...) and add the correct trust parameters.

Does this satisfy?

[rix0rrr]

Can't the key be implicit in the mapping class?

[rix0rrr]

Also, doesn't it make sense to push these mappings into the Facebook provider?

new IdentityPool(this, 'myidentitypool', {
  // ...
  identityProviders: [
    IdentityProvider.Facebook('7346241598935555' , {
      roleMappingRules: [
        {
          // if the identifier of the user as returned by facebook is 'boris'
          claim: FacebookRoleMappingClaim.SUB,
          matchType: MatchType.EQUALS,
          value: 'boris',
          role: adminRole
        }
      ],
    })
 ]
});

[nija-at]

Nice! I like the idea of merging identityProviders and roleMappingRules.

[shivlaks]

would we have convenience methods to add/remove sign-in types?

[nija-at]

can you elaborate?

[shivlaks]

so the constructor is one way to initialize with supported sign-in types. would UserPool have methods to the UserPool class -> addSignInType

use case I'm thinking of is conditionally allowing different sign-in types based on stage in a CI/CD pipeline

I'm not sure how it's currently handled, does it replace the UserPool altogether or is this an attribute that can be updated?