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

fix: add aws_iam to custom operations when enableIamAuthorization is enabled #2921

Merged
merged 1 commit into from
Oct 10, 2024

Conversation

palpatim
Copy link
Member

@palpatim palpatim commented Oct 3, 2024

Description of changes

  • Add aws_iam to custom operations when enableIamAuthorization is enabled. Fixes Custom mutation not accessible with IAM #2837
  • Fix graphql type utils -- previously the return types of some type predicates were incorrectly grouped

Merge conflicts

  • packages/amplify-graphql-transformer-core/API.md
    • Regenerated

Issue #, if available

#2837

Description of how you validated changes

Checklist

  • PR description included
  • yarn test passes
  • E2E test run linked
    • Passes
    • Test schema_auth_15 fails because the prompt handling for that test hasn't been updated to reflect the new CLI Gen 1 prompts as in test: fix gen 1 init #2924. Fixed in 5965fab
    • Test RelationalWithAuthV2Redacted.e2e.test.ts fails b/c of known bucket naming conflict. Passes locally
  • Tests are changed or added

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

@palpatim palpatim requested a review from a team as a code owner October 3, 2024 19:54
@palpatim palpatim force-pushed the palpatim.fix.iam-auth-on-custom-mutation branch from ca67e6f to 8ca4ab2 Compare October 4, 2024 15:26
@palpatim palpatim requested a review from a team as a code owner October 4, 2024 15:26
@palpatim palpatim requested a review from atierian October 4, 2024 16:06
Copy link
Member

@atierian atierian left a comment

Choose a reason for hiding this comment

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

From what I can tell this is adding @aws_iam to operations, but not to any types those operations may return. In Gen2 world, I'm thinking about this scenario:

const schema = a.schema({
  Foo: a.customType({
    bar: a.string()
  }),
  myFooQuery: a.query()
    // ..
    .returns(a.ref(Foo)),
});
type Foo { 
  bar: String
} # does this have @aws_iam?

type Query {
  myFooQuery: Foo @aws_iam
}

Do those types already have @aws_iam / am I missing something in this PR / is it planned in a follow up?

Comment on lines 360 to 368
const makeAwsIamDirective = (): DirectiveNode => {
return {
kind: 'Directive',
name: {
kind: 'Name',
value: 'aws_iam',
},
};
};
Copy link
Member

Choose a reason for hiding this comment

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

nit

Suggested change
const makeAwsIamDirective = (): DirectiveNode => {
return {
kind: 'Directive',
name: {
kind: 'Name',
value: 'aws_iam',
},
};
};
const makeAwsIamDirective = () => makeDirective('aws_iam', []);

Copy link
Member Author

Choose a reason for hiding this comment

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

TIL! Will fix

Copy link
Member Author

Choose a reason for hiding this comment

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

@atierian - Decided to promote the existing fix for scalar return values, and opened #2929 to track the complex custom type issue. Thanks for pointing that out!

sundersc
sundersc previously approved these changes Oct 4, 2024
@palpatim
Copy link
Member Author

palpatim commented Oct 7, 2024

Converting this to draft while we figure out the right way of implementing automatically decorating custom return types, as in:

type Foo {
  description: String
}
type Query {
  getFooCustom: Foo
}

@palpatim palpatim marked this pull request as draft October 7, 2024 17:31
@palpatim
Copy link
Member Author

palpatim commented Oct 7, 2024

Will also discuss whether we want to just release this fix as-is, to support custom operations that return scalars. We can then take on custom operations that return non-model types.

@palpatim
Copy link
Member Author

palpatim commented Oct 7, 2024

Decided to release this fix for scalar return values, and opened #2929 to track the complex custom type issue.

@palpatim palpatim force-pushed the palpatim.fix.iam-auth-on-custom-mutation branch from 2119df6 to 4fa000b Compare October 9, 2024 14:54
…enabled; fix graphql type utils

- test: Add additional tests to fix coverage metrics for unchanged files
- test: Add implicit IAM auth support tests
  - Added a skipped test for custom type support, to be re-enabled once we
    figure out the right strategy for this.
@palpatim palpatim force-pushed the palpatim.fix.iam-auth-on-custom-mutation branch from 4fa000b to b9ff3f9 Compare October 9, 2024 15:30
@palpatim palpatim marked this pull request as ready for review October 9, 2024 15:45

const hasAwsIamDirective = (field: FieldDefinitionNode): boolean => {
return field.directives?.some((dir) => dir.name.value === 'aws_iam');
};
Copy link
Contributor

@phani-srikar phani-srikar Oct 9, 2024

Choose a reason for hiding this comment

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

nit: We already have a centralized directive existence checker on Object types defined in the common package, we could add one for field types so it is re-usable across transformers

Copy link
Member Author

Choose a reason for hiding this comment

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

Thanks, will address in a followup PR!

@@ -110,6 +110,8 @@ export class AuthTransformer extends TransformerAuthBase implements TransformerA
// (undocumented)
addAutoGeneratedRelationalFields: (ctx: TransformerContextProvider, def: ObjectTypeDefinitionNode, allowedFields: Set<string>, fields: readonly string[]) => void;
// (undocumented)
addCustomOperationFieldsToAuthNonModelConfig: (ctx: TransformerTransformSchemaStepContextProvider) => void;
Copy link
Member Author

Choose a reason for hiding this comment

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

note: this should be private. Will make it so in a future PR, which will remove it from the published API. Since the transformer interface isn't actually used by customers, this won't be a breaking change

@palpatim palpatim merged commit 5cb5a2b into main Oct 10, 2024
7 of 8 checks passed
@palpatim palpatim deleted the palpatim.fix.iam-auth-on-custom-mutation branch October 10, 2024 14:51
palpatim added a commit that referenced this pull request Oct 10, 2024
* chore: update .jsii assembly

* chore: update .jsii assembly

* chore: migrate pg array objects e2e test in gen2 cdk (#2906)

* chore: graphql prep for test migration

* refactor: generic graphql field selection string with fieldmap

* feat: add postgres array objects e2e test

* test: remove bootstrap in test code

* chore: schema cleanup

* chore: final cleanup

* chore: add explanation on FieldMap ans examples

* chore: remove dup test

---------

Signed-off-by: Kevin Shan <siqishan@amazon.com>
Co-authored-by: Tim Schmelter <schmelte@amazon.com>

* fix(model-transformer) IndexName -> index in query list resolver (#2912)

* chore: upgrade cdk library dependency to 2.158.0 (#2876)

* chore: upgrade cdk dependency to 2.158.0

* chore: install and use nvm

* chore: use full version for nvm

* chore: testing linux build with nvm

* chore: fix version in cdk tests

* chore: update jsii files

* update: increase memory size

* add: debug statement

* update: mem size back to 8096, use ps1 file for shell script

* fix: path to Setup-NodeVersion.ps1

* fix: path to codebuild_specs/Setup-NodeVersion.ps1

* add: set runtime version

* update: image

* add: debug statement

* update: use earlier code

* add: debug statements

* update: clean up code

* update: use the correct image

* add: list installed node versions and used nodejs.install

* restart: install nvm using choco

* add: back mem size variable

* add: nvm install and use 18.20.4

* add: env var NVM_HOME and NVM_SYMLINK

* add: spawn powershell as admin

* update: remove all other builds

* add: debug statement

* add: env var path

* update: print env var

* add: commands

* update: env var set up

* add: refresh env var

* update: more debug statement

* update

* revamp: find nvm.exe

* update: install nvm windows directly

* update: launch new shell if current shell does not recognize nvm

* update: install node in buildspec

* add: install and use node in build spec

* update: use single quote to prevent interpreting \

* add: 2 scripts, one for installing nvm, another for using nvm

* fix: path error

* test: which way set env var

* update: set up env var in pre_build

* update: use choco in pre-build

* fix: syntax error

* update: build_windows working, running all tests

* test: remove bootstrap in test code

* debug: _runGqlE2ETests

* update: debug_workflow

* update: debug_workflow

* update: debug_workflow

* update: debug_workflow

* add: debug statement

* add: debug test

* add: debug

* update: use uuid for bucket name

* remove: use of uuid

* add: debug statement

* update: use differrent bucket name

* add: mili second timestamp

* add: debug statement

* remove: debug statement

* remove: redundant code

---------

Co-authored-by: Bobby Yu <bobbyu@amazon.com>
Co-authored-by: Tim Schmelter <schmelte@amazon.com>

* test: fix gen 1 init (#2924)

* fix(conversation): allow changes to systemPrompt, inferenceConfig, aiModel to be hotswapped (#2923)

* feat: auto increment support (#2883)

* chore(graphql-default-value-transformer): tidy tests

* test(graphql-default-value-transformer): add unit tests for auto increment support

* feat: 🎸 utils to detect Postgres datasource

* feat: 🎸 support auto increment

Implements support for auto increment (serial) fields from Postgres
datasources. Such fields are denoted by an empty `@default` applied to
an `Int` field.

* test(graphql-default-value-transformer): pk can be auto increment

* test(graphql-default-value-transformer): auto-increment crud e2e

* chore: describe test purpose

* chore: removing logging

* chore: describe why invalid cases are invalid

* chore: remove unecessary e2e test case

* chore: test messaging clarity

* chore: type safety

* chore: alphabetize list

* chore: type of return value asserts against string

Co-authored-by: Tim Schmelter <schmelte+github@amazon.com>

* chore: test ensures customers can insert to serial fields with custom values

* chore: verify that @default(value) works on mysql

* chore: remove unecessary ssm test case

* chore: update branch from main

* test: value cannot be null on ddb

---------

Co-authored-by: Tim Schmelter <schmelte+github@amazon.com>

* fix(conversation): use functionMap for custom handler IFunction reference (#2922)

* fix(generation): gracefully handle stringified tool_use responses (#2919)

* feat(conversation): per message items and lambda history retrieval pattern (#2914)

* fix: sql default value e2e failures (#2932)

* fix(generation): remove trailing comma in inferenceConfig resolver code (#2933)

* fix: add aws_iam to custom operations when enableIamAuthorization is enabled; fix graphql type utils (#2921)

- test: Add additional tests to fix coverage metrics for unchanged files
- test: Add implicit IAM auth support tests
  - Added a skipped test for custom type support, to be re-enabled once we
    figure out the right strategy for this.

* fix: appsync ttl correct duration time unit in ms (#2928)

Signed-off-by: Kevin Shan <siqishan@amazon.com>

---------

Signed-off-by: Kevin Shan <siqishan@amazon.com>
Co-authored-by: amplify-data-ci <amplify-data-dev+github@amazon.com>
Co-authored-by: Kevin Shan <siqishan@amazon.com>
Co-authored-by: Ian Saultz <52051793+atierian@users.noreply.github.com>
Co-authored-by: Phani Srikar Edupuganti <55896475+phani-srikar@users.noreply.github.com>
Co-authored-by: Bobby Yu <bobbyu@amazon.com>
Co-authored-by: Dane Pilcher <dppilche@amazon.com>
Co-authored-by: Peter V. <98245483+p5quared@users.noreply.github.com>
tejas2008 pushed a commit that referenced this pull request Oct 29, 2024
…enabled; fix graphql type utils (#2921)

- test: Add additional tests to fix coverage metrics for unchanged files
- test: Add implicit IAM auth support tests
  - Added a skipped test for custom type support, to be re-enabled once we
    figure out the right strategy for this.
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.

Custom mutation not accessible with IAM
7 participants