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

test: add field auth e2e userpools provider #2156

Merged

Conversation

phani-srikar
Copy link
Contributor

@phani-srikar phani-srikar commented Dec 22, 2023

Description of changes

  • Remove the check in V2 Auth transformer to support field auth rules in SQL models.
  • Add E2Es for field auth tests using userpools provider. The tests cover combinations of private, default owner, custom owner field, custom array of owners. More complex cases like multiple auth rules and relationships are tracked separately.
  • Some tests are commented out since they are known issues that need to resolved in Auth utils. The group based tests are skipped until the known error is fixed in AppSync and we'll revisit those at that point.
CDK / CloudFormation Parameters Changed

Issue #, if available

Description of how you validated changes

Unit test for removed check.
Ran added E2Es on CI.

Checklist

  • PR description included
  • yarn test passes
  • Tests are changed or added
  • Relevant documentation is changed or added (and PR referenced)
  • New AWS SDK calls or CloudFormation actions have been added to relevant test and service IAM policies
  • Any CDK or CloudFormation parameter changes are called out explicitly

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

@phani-srikar phani-srikar changed the base branch from main to feature/sql-field-auth-support December 29, 2023 17:48
@phani-srikar phani-srikar marked this pull request as ready for review December 29, 2023 19:44
@phani-srikar phani-srikar requested a review from a team as a code owner December 29, 2023 19:44
import { ImportedRDSType } from '@aws-amplify/graphql-transformer-core';
import { convertToDBSpecificGraphQLString, generateDDL } from '../../rds-v2-test-utils';

export const schema = (engine: ImportedRDSType): string => `
Copy link
Member

Choose a reason for hiding this comment

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

Where is engine used?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It's certainly is not used here, removed.

input: any,
selectionSet?: string,
isCompleteQuery = true,
errorPolicy?: ErrorPolicy,
Copy link
Member

Choose a reason for hiding this comment

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

nit: why not assign a default here like you do for the preceding and following args?

selectionSet?: string,
operation?: string,
isCompleteQuery = true,
errorPolicy?: ErrorPolicy,
Copy link
Member

Choose a reason for hiding this comment

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

nit: as above, why not default it?

if (errors && errors.length > 0) {
expect(result.errors).toBeDefined();
expect(result.errors).toHaveLength(errors.length);
result.errors?.forEach((error: any, index: number) => {
Copy link
Member

Choose a reason for hiding this comment

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

If order doesn't matter, would it be less brittle to use arrayContaining matcher?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That's a good point. Order shouldn't matter here much. Updated.

async () => await user1TodoHelper.delete(`delete${modelName}`, { id: todo['id'] }, user1CreateAllowedSet),
).rejects.toThrowErrorMatchingInlineSnapshot(expectedOperationError(`delete${modelName}`, 'Mutation'));

// user2 can listen to updates on the non-protected fields
Copy link
Member

Choose a reason for hiding this comment

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

Do we test that user2 can also listen for updates to ownersContent?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes in the onUpdate subscription since that field only allows updates.

Copy link
Contributor

@AaronZyLee AaronZyLee left a comment

Choose a reason for hiding this comment

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

Overall LGTM. Reminder: the last few cases are skipped will be addressed later.

test('Private model auth and allowed field operations', async () => {
const modelName = 'TodoPrivateContentVarious';
const user1ModelOperationHelpers = createModelOperationHelpers(appSyncClients[userPoolProvider][userName1], schema);
const user2ModelOperationHelpers = createModelOperationHelpers(appSyncClients[userPoolProvider][userName2], schema);
Copy link
Contributor

Choose a reason for hiding this comment

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

Nit: the two user model operation helpers can be moved to the common part before the test suits as the outcome is the same

@phani-srikar phani-srikar merged commit 96bede4 into feature/sql-field-auth-support Jan 2, 2024
7 checks passed
@phani-srikar phani-srikar deleted the add-field-auth-e2e-private branch January 2, 2024 19:08
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