Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions packages/amplify-graphql-auth-transformer/API.md
Original file line number Diff line number Diff line change
Expand Up @@ -117,6 +117,8 @@ export class AuthTransformer extends TransformerAuthBase implements TransformerA
// (undocumented)
addFieldsToObject: (ctx: TransformerTransformSchemaStepContextProvider, modelName: string, ownerFields: Array<string>) => void;
// (undocumented)
addIamAuthToCustomTypes: (ctx: TransformerTransformSchemaStepContextProvider) => void;
// (undocumented)
Comment on lines +120 to +121
Copy link
Contributor

Choose a reason for hiding this comment

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

It seems this could be private.
(on the other hand, we never paid a lot of attention to transformers public apis...).

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah. I haven't formed a strong opinion here yet. I'm just following the established pattern for now. 🤷

Copy link
Contributor

Choose a reason for hiding this comment

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

I would recommend adopting "if something can be private it should be". that creates less liability going forward. (we've been doing this for a while on backend side).
For transformers it would be more to form a habit (or maybe stop the bleeding) rather than fix them. Their apis are already overexposed.

Copy link
Contributor

Choose a reason for hiding this comment

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

This might not be customer facing API. But once exposed and called by some other package on it becomes liability.
An example where versioning "internal apis" matter is here aws-amplify/amplify-backend#2717 (comment) .
(this might not be that important for transformers because api construct locks/bundles them afaik - yet another assumption in the system to excuse ourselves from api versioning...).

Copy link
Contributor

Choose a reason for hiding this comment

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

Their apis are already overexposed

This might be a sign that all these transformers packages could have been directories in single package.

Copy link
Member Author

@svidgen svidgen May 1, 2025

Choose a reason for hiding this comment

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

I might agree. And, it all seems pretty chaotic to me at the moment. But, there's also a ton of intrinsic complexity in the problem space. So, I'm attempting to respect what came before and keep as many existing fences erected until I understand them better and have more solid opinions.

after: (context: TransformerContextProvider) => void;
// (undocumented)
before: (context: TransformerBeforeStepContextProvider) => void;
Expand Down
Original file line number Diff line number Diff line change
@@ -1,3 +1,4 @@
import { FunctionTransformer } from '@aws-amplify/graphql-function-transformer';
import { ModelTransformer } from '@aws-amplify/graphql-model-transformer';
import { mockSqlDataSourceStrategy, testTransform } from '@aws-amplify/graphql-transformer-test-utils';
import { PrimaryKeyTransformer } from '@aws-amplify/graphql-index-transformer';
Expand Down Expand Up @@ -40,6 +41,7 @@ const makeTransformers = (): TransformerPluginProvider[] => [
new AuthTransformer(),
new PrimaryKeyTransformer(),
new SqlTransformer(),
new FunctionTransformer(),
];

const makeSqlDirectiveDataSourceStrategies = (schema: string, strategy: ModelDataSourceStrategy): SqlDirectiveDataSourceStrategy[] =>
Expand Down Expand Up @@ -367,8 +369,7 @@ describe('Custom operations have @aws_iam directives when enableIamAuthorization
expect(out.schema).not.toMatch(/onUpdateFooCustom: String.*@aws_iam/);
});

// TODO: Enable this test once we fix https://github.com/aws-amplify/amplify-category-api/issues/2929
test.skip('Adds @aws_iam to non-model custom types when there is no model', () => {
test('Adds @aws_iam to non-model custom types when there is no model', () => {
const strategy = makeStrategy(strategyType);
const schema = /* GraphQL */ `
type Foo {
Expand Down Expand Up @@ -403,8 +404,7 @@ describe('Custom operations have @aws_iam directives when enableIamAuthorization
expect(out.schema).toMatch(/type Foo.*@aws_iam/);
});

// TODO: Enable this test once we fix https://github.com/aws-amplify/amplify-category-api/issues/2929
test.skip('Adds @aws_iam to non-model custom types when there is a model', () => {
test('Adds @aws_iam to non-model custom types when there is a model', () => {
const strategy = makeStrategy(strategyType);
const schema = /* GraphQL */ `
type Todo @model {
Expand Down Expand Up @@ -478,6 +478,42 @@ describe('Custom operations have @aws_iam directives when enableIamAuthorization
expect(out.schema).toMatch(/description: String.*@aws_iam/);
});

test('Adds @aws_iam to async function EventInvocationResponse type', () => {
const strategy = makeStrategy(strategyType);
const schema = /* GraphQL */ `
type Foo {
description: String
}
type EventInvocationResponse @aws_api_key {
success: Boolean!
}
type Query {
getFooCustom: Foo
}
type Mutation {
updateFooCustom: Foo
doSomethingAsync(body: String!): EventInvocationResponse
@function(name: "FnDoSomethingAsync", invocationType: Event)
@auth(rules: [{ allow: public, provider: apiKey }])
}
type Subscription {
onUpdateFooCustom: Foo @aws_subscribe(mutations: ["updateFooCustom"])
}
`;

const out = testTransform({
schema,
dataSourceStrategies: constructDataSourceStrategies(schema, strategy),
authConfig: makeAuthConfig(),
synthParameters: makeSynthParameters(),
transformers: makeTransformers(),
sqlDirectiveDataSourceStrategies: makeSqlDirectiveDataSourceStrategies(schema, strategy),
});

// Also expect the custom type referenced by the custom operation to be authorized
expect(out.schema).toMatch(/type EventInvocationResponse.*@aws_iam/);
});

test('Does not add duplicate @aws_iam directive to custom type if already present', () => {
const strategy = makeStrategy(strategyType);
const schema = /* GraphQL */ `
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,7 @@ import {
isBuiltInGraphqlNode,
isDynamoDbModel,
isModelType,
isObjectTypeDefinitionNode,
isSqlModel,
MappingTemplate,
TransformerAuthBase,
Expand Down Expand Up @@ -377,8 +378,32 @@ export class AuthTransformer extends TransformerAuthBase implements TransformerA
});
};

/**
* Adds the `aws_iam` directive to all custom/non-model types when IAM access is enabled.
*/
addIamAuthToCustomTypes = (ctx: TransformerTransformSchemaStepContextProvider): void => {
if (!ctx.transformParameters.sandboxModeEnabled && !ctx.synthParameters.enableIamAccess) {
return;
}

const needsAwsIamDirective = (type: TypeDefinitionNode): boolean => {
return !type.directives?.some((dir) => dir.name.value === 'aws_iam');
};

const isNonModelType = (type: TypeDefinitionNode): boolean => {
return !type.directives?.some((dir) => dir.name.value === 'model');
};

ctx.inputDocument.definitions
.filter(isObjectTypeDefinitionNode)
.filter(isNonModelType)
.filter(needsAwsIamDirective)
.forEach((def) => extendTypeWithDirectives(ctx, def.name.value, [makeDirective('aws_iam', [])]));
};

transformSchema = (context: TransformerTransformSchemaStepContextProvider): void => {
this.addCustomOperationFieldsToAuthNonModelConfig(context);
this.addIamAuthToCustomTypes(context);

const searchableAggregateServiceDirectives = new Set<AuthProvider>();

Expand Down
Loading