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

staticsite: add additional_redirect_domains option #457

Merged
merged 4 commits into from
Sep 22, 2020

Conversation

troyready
Copy link
Contributor

@troyready troyready commented Sep 18, 2020

WIP - there's still some DRYing and cleanup to be done, but the broad elements are ready for discussion before buttoning it up.

The main change at play here is solving the local development issue when using Auth@Edge, by way of conditionally allowing localhost callback URLs to be used with Cognito in development environments. Preliminary name for this new option is staticsite_additional_redirect_domains.

In adding the option, I've made one other change that's technically breaking, but I believe it's more intuitive & solves a timing issue for the majority of uses: now, if an alias for the Auth@Edge domain is specified (e.g. myapp.example.com, instead of just relying on accessing the site via https://dkldhy3d8df.cloudfront.net), then the CloudFront domain will not be included in the callback URLs. I don't believe there's a single existing user that will be negatively affected by this change, and if anyone desires the old behavior it can be achieved via the new staticsite_additional_redirect_domains option.

Example of using the new option to define a serverless app that only allows localhost redirects in dev:

variables:
  qa: &variables
    namespace: internalapptest-qa
    cert_domain: internalapptest-qa.example.com
    user_pool_arn: arn:aws:cognito-idp:us-east-1:123456789012:userpool/us-east-1_kodf83dfD
    user_pool_supported_identity_providers: Auth0
    user_pool_additional_redirect_domains: ""
  prod:
    <<: *variables
    namespace: internalapptest-prod
    cert_domain: internalapptest-prod.example.com
  dev:
    <<: *variables
    namespace: internalapptest-dev
    cert_domain: internalapptest.example.com
    user_pool_additional_redirect_domains: http://localhost:3000

deployments:
  - regions:
      - us-east-1
    parameters:
      namespace: ${var ${env DEPLOY_ENVIRONMENT}.namespace}
      region: ${env AWS_REGION}
      cert_domain: ${var ${env DEPLOY_ENVIRONMENT}.cert_domain}
    modules:
      - path: backend.sls
      - name: frontend
        path: frontend
        type: static
        parameters:
          staticsite_acmcert_arn: ${ssm /${var ${env DEPLOY_ENVIRONMENT}.namespace}/acm_cert_arn}
          staticsite_aliases: ${var ${env DEPLOY_ENVIRONMENT}.cert_domain}
          staticsite_auth_at_edge: true
          staticsite_user_pool_arn: ${var ${env DEPLOY_ENVIRONMENT}.user_pool_arn}
          staticsite_supported_identity_providers: ${var ${env DEPLOY_ENVIRONMENT}.user_pool_supported_identity_providers}
          staticsite_additional_redirect_domains: ${var ${env DEPLOY_ENVIRONMENT}.user_pool_additional_redirect_domains}
        options:
          build_output: build
          pre_build_steps:
            - command: npm ci
              cwd: ../backend.sls
            - command: npx sls exportEndpoints --stage ${env DEPLOY_ENVIRONMENT} --region ${env AWS_REGION}
              cwd: ../backend.sls
          build_steps:
            - npm run build
          source_hashing:
            enabled: true
            directories:
              - path: src
              - path: public

Copy link
Collaborator

@ITProKyle ITProKyle left a comment

Choose a reason for hiding this comment

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

I've made one other change that's technically breaking ...

Honestly, it sounds more like a bug fix. The only remaining issue is that if you supply aliases it won't work OOTB (before it kinda would, if you used the CF domain name). The user would be required to supply their own IaC to update DNS to point to CF (which is a slight complaint I have had in my usage of static site).

Not for this PR but a potential future enhancement would be to add the ability to update r53 hosted zones to add a record for the CF distribution.

@@ -366,6 +366,19 @@ Parameters
parameters
staticsite_user_pool_arn: arn:aws:cognito-idp:<region>:<account-id>:userpool/<pool>

**staticsite_additional_redirect_domains (Optional[str])**
Copy link
Collaborator

Choose a reason for hiding this comment

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

I would rather have this defined as a List[str] instead of Optional[str] (comma delimited list) to make the yaml configuration easier to read/write. I don't see a need for this to be a string when you are splitting it into a list on the backend anyway.

Also since the driving forces behind this is local development, it may be worth noting that use case here (adding the why to your example).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Great thought. I'd like to leave it as a str for now to match the aliases option, but would be open to changing both in the future. I realize it's a bit of a leaky abstraction (from CloudFormation) to have them be strings in the runway yaml. Shouldn't be too rough to support them either way (if isinstance(str) then split)) .

Somewhat related: at some point I'm probably going to PR and update to the CFN templates to process the values as CFNCommaDelimitedList Parameters (strings) again. Having used the pure-template versions now, I find that I miss the ChangeSet output when updating Parameters. I don't want to say that it's vastly better, but it's noticeable.

Also cleaned up Cognito App Client management process by removing the
post-deploy updater process from the majority of deployments (when an
alternate domain alias is specified)
@troyready troyready force-pushed the feature/staticsite_aae_addtl_redirect_domains branch from a4f93e0 to 72f8db7 Compare September 22, 2020 00:47
@troyready troyready marked this pull request as ready for review September 22, 2020 19:24
@troyready troyready merged commit d3079d0 into master Sep 22, 2020
@troyready troyready deleted the feature/staticsite_aae_addtl_redirect_domains branch September 22, 2020 21:52
@ITProKyle ITProKyle added the feature Request or pull request for a new feature label Sep 25, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature Request or pull request for a new feature
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants