Skip to content

[typescript-axios] Add detection for AWS IAM from schema #21356

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

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

NiltiakSivad
Copy link

@NiltiakSivad NiltiakSivad commented May 30, 2025

Previous Efforts

Here is the open issue.

I've taken over the effort that was originally started, here. It was suggested in this PR that rather than use a CLI flag like useAwsIamAuth, that we detect IAM Auth from the schema itself.

Summary

This is the first PR of 2. I thought it would be good to talk about the IAM auth detection mechanism before talking about the implementation method in the templates.

At a high-level I'm proposing 3 patterns for detecting AWS IAM Auth.

  1. Check for AWS-specific extension
  2. Check for common AWS V4 signature scheme names
  3. Check for AWS API Gateway URL patterns in servers

It is up for discussion the pro/cons of each, and perhaps we end up with only 1 or a subset of the 3.

Deployment

This is a non-breaking change, as it is adding a variable to be returned to the templates, so I think merging into master is sufficient, but let me know if you think differently! I really hate Java and first PR contributing to this repo so looking for any and all feedback to follow standards/conventions. Any deviation from standards/conventions is not intentional so please lmk.

PR checklist

  • Read the contribution guidelines.
  • Pull Request title clearly describes the work in the pull request and Pull Request description provides details about how to validate the work. Missing information here may result in delayed response from the community.
  • Run the following to build the project and update samples:
    ./mvnw clean package || exit
    ./bin/generate-samples.sh ./bin/configs/*.yaml || exit
    ./bin/utils/export_docs_generators.sh || exit
    
    (For Windows users, please run the script in WSL)
    Commit all changed files.
    This is important, as CI jobs will verify all generator outputs of your HEAD commit as it would merge with master.
    These must match the expectations made by your contribution.
    You may regenerate an individual generator by passing the relevant config(s) as an argument to the script, for example ./bin/generate-samples.sh bin/configs/java*.
    IMPORTANT: Do NOT purge/delete any folders/files (e.g. tests) when regenerating the samples as manually written tests may be removed.
  • File the PR against the correct branch: master (upcoming 7.x.0 minor release - breaking changes with fallbacks), 8.0.x (breaking changes without fallbacks)
  • If your PR is targeting a particular programming language, @mention the technical committee members, so they are more likely to review the pull request.

@TiFu (2017/07) @taxpon (2017/07) @sebastianhaas (2017/07) @kenisteward (2017/07) @Vrolijkx (2017/09) @macjohnny (2018/01) @topce (2018/10) @akehir (2019/07) @petejohansonxo (2019/11) @amakhrov (2020/02) @davidgamero (2022/03) @mkusaka (2022/04) @joscha (2024/10)

templateDir: modules/openapi-generator/src/main/resources/typescript-axios
additionalProperties:
npmVersion: 1.0.0
npmName: '@openapitools/typescript-axios-with-aws-iam'
Copy link
Author

Choose a reason for hiding this comment

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

This is my setup, but let me know if you think I should add a specific combination

@@ -26,7 +26,9 @@
"prepare": "npm run build"
},
"dependencies": {
"axios": "{{axiosVersion}}"
"axios": "{{axiosVersion}}"{{#withAWSV4Signature}},
Copy link
Author

@NiltiakSivad NiltiakSivad May 30, 2025

Choose a reason for hiding this comment

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

Can remove this before merge but wanted to show it working with a conditional

version: 1.0.0
description: Test API for AWS IAM authentication detection
servers:
- url: https://abc123.execute-api.us-east-1.amazonaws.com/prod
Copy link
Author

Choose a reason for hiding this comment

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

Pattern 3

Copy link
Member

Choose a reason for hiding this comment

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

can you add comments to each pattern so its still clear after the PR is merged?

Copy link
Author

Choose a reason for hiding this comment

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

Totally! Added

type: apiKey
name: Authorization
in: header
x-amazon-apigateway-authtype: awsSigv4
Copy link
Author

Choose a reason for hiding this comment

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

Pattern 1

@@ -30,7 +30,7 @@ public class CodegenSecurity {
// Those are to differentiate basic and bearer authentication
// isHttpSignature is to support HTTP signature authorization scheme.
// https://datatracker.ietf.org/doc/draft-cavage-http-signatures/
public Boolean isBasicBasic, isBasicBearer, isHttpSignature;
public Boolean isBasicBasic, isBasicBearer, isHttpSignature, isAWSV4Signature;
Copy link
Author

@NiltiakSivad NiltiakSivad May 30, 2025

Choose a reason for hiding this comment

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

Surprised this wasn't already here given AWS IAM is supported in the go generator? Anticipating some feedback 👂

servers:
- url: https://abc123.execute-api.us-east-1.amazonaws.com/prod
security:
- iam: []
Copy link
Author

@NiltiakSivad NiltiakSivad May 30, 2025

Choose a reason for hiding this comment

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

Pattern 2, which reflects on line 64, 16, 30, 51

@NiltiakSivad NiltiakSivad changed the title [typescript-axios] Add detection for AWS IAM [typescript-axios] Add detection for AWS IAM from schema May 30, 2025
const localVarQueryParameter = {} as any;

// authentication iam required
await setApiKeyToObject(localVarHeaderParameter, "Authorization", configuration)
Copy link
Author

Choose a reason for hiding this comment

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

In the next PR this would be replaced with something that returns the signed AWS headers

@OpenAPITools OpenAPITools deleted a comment May 31, 2025
@NiltiakSivad NiltiakSivad force-pushed the typescript-axios-detect-iam-auth branch from f9c7ce4 to 910a15d Compare June 3, 2025 19:19
@NiltiakSivad
Copy link
Author

Rebased against master to capture latest updates

@NiltiakSivad
Copy link
Author

Hey @wing328 @jimschubert @cbornet @jmini @etherealjoy, apologies to tag you here, but looking for a PR review. Did I not tag the right people or follow the correct process?

version: 1.0.0
description: Test API for AWS IAM authentication detection
servers:
- url: https://abc123.execute-api.us-east-1.amazonaws.com/prod
Copy link
Member

Choose a reason for hiding this comment

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

can you add comments to each pattern so its still clear after the PR is merged?

@@ -56,6 +56,7 @@ public CodegenSecurity(CodegenSecurity original) {
this.isBasic = original.isBasic;
this.isBasicBasic = original.isBasicBasic;
this.isHttpSignature = original.isHttpSignature;
this.isAWSV4Signature = original.isAWSV4Signature;
Copy link
Member

Choose a reason for hiding this comment

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

is this only used in the test or also in the template?

Copy link
Author

@NiltiakSivad NiltiakSivad Jun 20, 2025

Choose a reason for hiding this comment

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

This will be used in the template to dynamically generate different code.

I figured stopping at the detection of using AWS Auth was a good way to separate out for smaller PRs for earlier feedback since what I have here in this PR doesn't break anything and only expands functionality

@@ -121,6 +128,76 @@ private static String getRelativeToRoot(String path) {
return sb.toString();
}

@Override
public void preprocessOpenAPI(OpenAPI openAPI) {
Copy link
Member

Choose a reason for hiding this comment

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

i would suggest to move it to the core, since its not typescript-specific, right?
also, i don't understand where the withAWSV4Signature is used in the template?

Copy link
Author

Choose a reason for hiding this comment

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

@macjohnny Thanks for your review! It isn't used in the templates yet, I was saving that for a Part 2 in an effort to keep the PR small and to collect feedback early. However, in package.mustache I made a small modification to show that the variable is being passed.

By moving to "core" do you mean modifying the DefaultCodegen or making changes to the openapi-generator-core module?

Copy link
Member

Choose a reason for hiding this comment

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

agreed with johnny suggestion to move it to the "core" : https://github.com/OpenAPITools/openapi-generator/blob/master/modules/openapi-generator/src/main/java/org/openapitools/codegen/DefaultCodegen.java#L5623-L5718

also agreed with your reply to do it in another PR instead to keep this PR small for easier review

@NiltiakSivad NiltiakSivad force-pushed the typescript-axios-detect-iam-auth branch from c95d147 to d145b9e Compare June 20, 2025 18:55

@Test
public void testDetectsAwsIamAuthenticationWithNaming() {
OpenAPI openAPI = TestUtils.parseFlattenSpec("src/test/resources/3_0/typescript-axios/with-aws-iam.yaml");
Copy link
Author

Choose a reason for hiding this comment

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

This test is technically the same as the above at the moment. I wanted to see if others felt good about the 3 different detection mechanisms. I can split the example out into multiple so that all 3 patterns are not found in 1 file, and each can be tested in isolation without the potential for a false positive.

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.

3 participants