fix(amplify): add the default amplify policy to the auto-generated role #29076

johnf · 2024-02-12T00:57:16Z

Issue # (if applicable)

Reason for this change

The default created role for the Amplify App has no managed policy applied. So it can't do things like pull details of the backend config.

Description of changes

I've attached the Managed policy AdministratorAccess-Amplify which is what happens when you create an Amplify application in the console.

Description of how you validated changes

I've added unit tests.

Checklist

My code adheres to the CONTRIBUTING GUIDE and DESIGN GUIDELINES

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

aws-cdk-automation

The pull request linter has failed. See the aws-cdk-automation comment below for failure reasons. If you believe this pull request should receive an exemption, please comment and provide a justification.

A comment requesting an exemption should contain the text Exemption Request. Additionally, if clarification is needed add Clarification Request to a comment.

johnf · 2024-02-12T04:43:36Z

I've also updated the customRule integ to set a status.
Even though according to the docs status isn't required, cloudformation errors if it isn't provided

✅ Updated pull request passes all PRLinter validations. Dismissing previous PRLinter review.

aws-cdk-automation · 2024-02-12T05:09:25Z

AWS CodeBuild CI Report

CodeBuild project: AutoBuildv2Project1C6BFA3F-wQm2hXv2jqQv
Commit ID: c6db37b
Result: SUCCEEDED
Build Logs (available for 30 days)

Powered by github-codebuild-logs, available on the AWS Serverless Application Repository

kaizencc

Hi @johnf, thanks for the contribution. I'm not sure if we want to add this set of permissions to the role. I came to this conclusion independent of @pahud, so that is a pretty strong signal for me. I'm going to close this PR as something we are not willing to accept in its current iteration. Lets discuss with @pahud and myself on the issue if you want to figure something out.

kaizencc · 2024-02-12T19:27:38Z

packages/@aws-cdk/aws-amplify-alpha/lib/app.ts

@@ -224,6 +224,7 @@ export class App extends Resource implements IApp, iam.IGrantable {

    const role = props.role || new iam.Role(this, 'Role', {
      assumedBy: new iam.ServicePrincipal('amplify.amazonaws.com'),
+      managedPolicies: [iam.ManagedPolicy.fromAwsManagedPolicyName('AdministratorAccess-Amplify')],


I'm down to add common sense permissions to this role, but I balk at the idea of adding this managed policy. We typically don't like adding managed policies to our roles in CDK, and instead opt for fine grained controls (so our users know exactly what policy they are adding.

As always, if you want to use AdministratorAccess-Amplify, you can easily add it in as your role. But I don't want to push that on to everyone.

That's precisely the conversation I wanted to have in the issue before creating a PR :)
I can try to develop a smaller set of permissions, i.e., the minimum required to deploy a basic app. But I might miss some key things I'm not using myself.
The main thing missing for me was permission to talk to the backend to grab the amplify configuration.

That makes sense and I'm wary of my initial disdain towards the black box of the managed policy. Just need someone to confirm w me.

kaizencc · 2024-02-12T19:32:37Z

Hmm I've had an instant change of heart and will source another opinion :). Stay with me @johnf

johnf · 2024-02-12T20:59:08Z

I'm not sure if we want to add this set of permissions to the role.

I think it's key that something is added to support amplify apps. Even if we define some prebaked permissions that someone can use or at a minimum add something to the documentation.

aws-cdk-automation · 2024-02-27T00:09:23Z