Amplify-cli: Specify claims group field in auth directive

Created on 25 Jul 2019  路  3Comments  路  Source: aws-amplify/amplify-cli

Is your feature request related to a problem? Please describe.
I need to use claims information in addition to the cognito groups to apply authentication rules. I would like to specify the identifyField for groups in a similar manner as exists for owners.
While I could overload the owner rule to have many owners, it produces ugly code that is hard to audit.

Describe the solution you'd like

graphql-auth-transformer/src/resources.ts has a TODO comment:
// TODO: Enhance cognito:groups to work with non cognito based auth.
There may be a straightforward way to allow this by overloading setUserGroups to take an optional "identityField" string.

I would like the setUserGroups function to take an optional "identityField" string to override "cognito:groups" as the claim to check against.

Describe alternatives you've considered
I could use the preToken Lambda to overload "cognito:groups" claim information, but I'd rather keep these systems separate and be clear what claims come from cognito user pools and what comes from other sources.

Additional context
Attached is a patch file based on 54a8d that accomplishes the basics with ~12 lines changed. You may want to consider a name other than identityField if you don't want to overload terminology
patch.txt

enhancement graphql-transformer pending-review

Most helpful comment

I don鈥檛 have the information to do a PR efficiently, AWS staff could be 10x faster.
I am not familiar with the e2e tests in this project, the roadmap, decisions on naming and name overloading, and the documentation update process. I鈥檝e tried briefly to identify the auth tests, but they seem very sparse.

While I have your attention about PRs, it would be helpful if the bug in #1581 could be looked at, I鈥檝e provided the exact line number and the commits where the bug was introduced. The fix is a single line. A bit concerning that a bug breaking updates has existed for so long, and I think others have filed additional bug reports. Again, it would be interesting if the e2e tests were more accessible so that PRs were easy to submit. Would additionally be helpful if commit and PR comments were more descriptive if there is an intention of making this a community project. It took too long to deal with this issue, mainly due to low public communication.

All 3 comments

@RossWilliams is this something you might be able to submit a PR for?

I don鈥檛 have the information to do a PR efficiently, AWS staff could be 10x faster.
I am not familiar with the e2e tests in this project, the roadmap, decisions on naming and name overloading, and the documentation update process. I鈥檝e tried briefly to identify the auth tests, but they seem very sparse.

While I have your attention about PRs, it would be helpful if the bug in #1581 could be looked at, I鈥檝e provided the exact line number and the commits where the bug was introduced. The fix is a single line. A bit concerning that a bug breaking updates has existed for so long, and I think others have filed additional bug reports. Again, it would be interesting if the e2e tests were more accessible so that PRs were easy to submit. Would additionally be helpful if commit and PR comments were more descriptive if there is an intention of making this a community project. It took too long to deal with this issue, mainly due to low public communication.

@RossWilliams we've added this functionality in https://github.com/aws-amplify/amplify-cli/pull/2068

You can read the pre-release docs here: https://github.com/aws-amplify/docs/pull/868

Was this page helpful?
0 / 5 - 0 ratings

Related issues

rygo6 picture rygo6  路  93Comments

kaustavghosh06 picture kaustavghosh06  路  51Comments

lennybr picture lennybr  路  46Comments

simon-lanf picture simon-lanf  路  42Comments

varmab picture varmab  路  41Comments