fix: redact relational field in mutation & subscription based on model auth rules #2536
Conversation
AaronZyLee
changed the title
| @@ -185,7 +185,7 @@ export class DDBRelationalResolverGenerator extends RelationalResolverGenerator | |||
| MappingTemplate.s3MappingTemplateFromString( | |||
| print( | |||
| compoundExpression([ | |||
| iff(ref('ctx.stash.deniedField'), raw('#return($util.toJson(null))')), | |||
| iff(ref('ctx.stash.deniedField'), compoundExpression([set(ref('result'), obj({ items: list([]) })), raw('#return($result)')])), | |||
There was a problem hiding this comment.
For a hasMany relation, when the field is redacted, the { item: [] } should be returned instead of a null value. Otherwise the following error will be thrown by AppSync
There was a problem hiding this comment.
Is it because the related field is not null. I believe we don't this today if the auth rules on the relational field is restrictive than model rules.
| /** | ||
| * Primary auth rules - Related auth rules - Redact relational field | ||
| */ | ||
| const testCases: [string[], string[], boolean][] = [ |
There was a problem hiding this comment.
These test cases are the main part of this unit test, which defines the different auth rules on both relation models and whether to redact the relational field. These cases are tested below in the dimension of data source types and relation types(field or reference)
| * @param role2 auth role 2 | ||
| * @returns boolean for two roles are identical | ||
| */ | ||
| const isIdenticalAuthRole = (role1: RoleDefinition, role2: RoleDefinition): boolean => |
There was a problem hiding this comment.
nit: can you add this as a utliity near the RoleDefinition type itself, and add some unit tests to catch edge cases. Especially to make sure we handle undefined elements in one or both operands.
| (relatedRole) => | ||
| isIdenticalAuthRole(relatedRole, fieldRole) || | ||
| (relatedRole.provider === fieldRole.provider && | ||
| (relatedRole.provider === 'userPools' || relatedRole.provider === 'oidc') && |
There was a problem hiding this comment.
- Wouldn't
identityPoolsbe a valid provider for this case? - As above, put this utility in RoleDefinition & make sure we have plenty of tests for it
| * @param role auth role definition | ||
| * @returns boolean for the auth role is a dynamic or custom auth role | ||
| */ | ||
| const isDynamicAuthOrCustomAuth = (role: RoleDefinition): boolean => |
There was a problem hiding this comment.
as above, put this utility in RoleDefinition & make sure we have plenty of tests for it
| if (isFieldRoleHavingAccessToBothSide(fieldRole, filteredRelatedModelReadRoleDefinitions)) { | ||
| // Check if two role definitions are identical without dynamic auth role or custom auth role | ||
| // If not, redact the relational field | ||
| redactRelationalField = !( |
There was a problem hiding this comment.
This filter is pretty hard to parse (a negation of a multi-layer boolean test that includes 2 loops). Can you separate it out for readibility?
| } | ||
| `; | ||
|
|
||
| const referenceSchemaTempalte = ` |
There was a problem hiding this comment.
nit
| const referenceSchemaTempalte = ` | |
| const referenceSchemaTemplate = ` |
| } | ||
| test.each(testTable)('%s - Primary %s - Related %s should redact relational field - %s', expectation); | ||
| }); | ||
| describe('RDS datasources - reference relation', () => { |
There was a problem hiding this comment.
It "shouldn't" make a difference, but we should test heterogeneous relationships as well (DDB Primary, SQL Related; and SQL Primary, DDB Related)
dpilch
force-pushed
the
fix-sub-owner
branch
2 times, most recently
from
28c90f2
to
1a6beac
Compare
Description of changes
The relational field is redacted in mutation result and subsequent subscription from another client based on the comparison result of
The comparison will occur if the relational field is not protected by any field level auth rules (as the field is redacted when field level auth is added currently). The field redaction will only happen when the model subscription level is on in either case
CDK / CloudFormation Parameters Changed
Issue #, if available
N/A
Description of how you validated changes
Checklist
yarn testpassesBy submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.