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.

Sign up for GitHub

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

Jump to bottom

feat: support @httpApiKeyAuth trait #473

Merged
merged 5 commits into from
Jan 25, 2022
Merged

feat: support @httpApiKeyAuth trait #473

merged 5 commits into from
Jan 25, 2022

Conversation

glb
Copy link
Contributor

@glb glb commented Dec 7, 2021 •
edited
Loading

Issue #, if available: Fixes #453.

Description of changes: This adds codegen support for services and operations that use the @httpApiKeyAuth trait.

The generated client will have a new apiKey config attribute. This value will be injected into the header/query parameter that's specified in the trait by new middleware that's included in the codegen client.

⚠️ I've done my best to base this on existing patterns but I'm sure that there will be comments and things that need to change. Please let me know and I'll happily make adjustments.

By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.

kstich
kstich previously requested changes Dec 7, 2021
View reviewed changes
@glb
Copy link
Contributor Author

glb commented Dec 7, 2021

First round of fixes pushed, will rebase & squash them once I've got all the comments addressed.

@glb glb requested a review from kstich December 8, 2021 19:56
@glb
Copy link
Contributor Author

glb commented Dec 10, 2021

@kstich @mtdowling @JordonPhillips what are the next steps I need to take for this PR? Is there any chance you'll have an opportunity to review soon? I don't mean to be pushy, just don't know what timeline to expect here as I haven't heard anything since Tuesday afternoon.

I left the changes from code review comments as separate commits so they would be easier to review, but I could rebase, squash back to a single commit, and mark ready for review. I think I've addressed all of Kevin's comments and the tests (unit tests as well as manual testing of a generated client) seem to indicate this does what I hoped it would.

@glb
Copy link
Contributor Author

glb commented Dec 12, 2021

Pushed a change (f0ee2e5) to delete a test; not 100% sure I understood the intended behaviour of the auth traits so backed off on this one as I discovered it was failing and it seemed like the expectation was incorrect. Would love to hear if the test was legitimately wrong or if there's a bug in the code.

kstich
kstich reviewed Dec 13, 2021
View reviewed changes
@glb
Copy link
Contributor Author

glb commented Dec 14, 2021

Pushed changes to address most comments; still need to do the work to support an API key in a query parameter.

@glb
Copy link
Contributor Author

glb commented Dec 14, 2021

I think I've addressed all of the review comments so far. I've realized, though, that I'm missing the validator to check for conflicts. I probably won't get to that today, maybe tomorrow.

@glb
Copy link
Contributor Author

glb commented Dec 17, 2021

Silly me, I got confused, the validator is for a different PR. I think this is ready to be re-reviewed.

@glb glb requested a review from kstich December 17, 2021 15:02
@kstich
Copy link
Contributor

kstich commented Dec 20, 2021

I think this is ready to move to non-draft, I've initiated the CI run.

@glb
feat: support @httpApiKeyAuth trait
7684950 
This adds codegen support for services and operations that use the
@httpApiKeyAuth trait.

The generated client will have a new `apiKey` config attribute. This
value will be injected into the header that's specified in the trait
by new middleware that's included in the codegen client.
@glb glb force-pushed the 453-support-http-apikey-auth-trait branch from eae855f to 7684950 Compare December 21, 2021 00:07
@glb glb marked this pull request as ready for review December 21, 2021 00:08
@glb glb requested a review from a team as a code owner December 21, 2021 00:08
@glb
Copy link
Contributor Author

glb commented Dec 21, 2021

Thanks @kstich !

@glb
Copy link
Contributor Author

glb commented Dec 22, 2021 •
edited
Loading

Pushed fixup commits to address comments and fix missing @jest/types dependency so that the generated code can build & test. Will squash once folks have had a chance to review.

@glb glb requested a review from trivikr December 22, 2021 14:39
@glb
Copy link
Contributor Author

glb commented Dec 25, 2021

Pushed a behavioural change in f2f40d3 so that the middleware doesn't throw an error when the API key is not available, because the middleware doesn't know if there is another auth mechanism available (for example if an operation can be authorized by an API key or a bearer token, it shouldn't fail because the API key isn't present).

@glb glb force-pushed the 453-support-http-apikey-auth-trait branch from f2f40d3 to 5dd6606 Compare December 27, 2021 20:06
@trivikr trivikr dismissed kstich’s stale review January 25, 2022 21:34

kstich had provided approval over comment in #473 (comment)

@trivikr trivikr merged commit a1a37e6 into smithy-lang:main Jan 25, 2022
@eduardomourar eduardomourar mentioned this pull request Oct 3, 2022
Merged
eduardomourar added a commit to eduardomourar/aws-sdk-js-v3 that referenced this pull request Oct 25, 2022
@eduardomourar
feat(middleware-api-key): add new auth middleware
a14a5b3 
We will create a new middleware to handle API keys in the
context of Smithy generated models (probably not
applicable for AWS specific services). The code is based
heavily on [this PR](
smithy-lang/smithy-typescript#473
) from the Smithy TypeScript repository.

In order for it to work, we need to ensure the Bearer token
middleware succeeds even when the `token` is not set.
eduardomourar added a commit to eduardomourar/aws-sdk-js-v3 that referenced this pull request Oct 25, 2022
@eduardomourar
feat(middleware-api-key): add new auth middleware
43d2a1f 
We will create a new middleware to handle API keys in the
context of Smithy generated models (probably not
applicable for AWS specific services). The code is based
heavily on [this PR](
smithy-lang/smithy-typescript#473
) from the Smithy TypeScript repository.

In order for it to work, we need to ensure the Bearer token
middleware succeeds even when the `token` is not set.
eduardomourar added a commit to eduardomourar/aws-sdk-js-v3 that referenced this pull request Oct 26, 2022
@eduardomourar
feat(middleware-api-key): add new auth middleware
7f7a613 
We will create a new middleware to handle API keys in the
context of Smithy generated models (probably not
applicable for AWS specific services). The code is based
heavily on [this PR](
smithy-lang/smithy-typescript#473
) from the Smithy TypeScript repository.

In order for it to work, we need to ensure the Bearer token
middleware succeeds even when the `token` is not set.
eduardomourar added a commit to eduardomourar/aws-sdk-js-v3 that referenced this pull request Oct 27, 2022
@eduardomourar
feat(middleware-api-key): add new auth middleware
a7bab48 
We will create a new middleware to handle API keys in the
context of Smithy generated models (probably not
applicable for AWS specific services). The code is based
heavily on [this PR](
smithy-lang/smithy-typescript#473
) from the Smithy TypeScript repository.

In order for it to work, we need to ensure the Bearer token
middleware succeeds even when the `token` is not set.
eduardomourar added a commit to eduardomourar/aws-sdk-js-v3 that referenced this pull request Nov 1, 2022
@eduardomourar
feat(middleware-api-key): add new auth middleware
67fd30e 
We will create a new middleware to handle API keys in the
context of Smithy generated models (probably not
applicable for AWS specific services). The code is based
heavily on [this PR](
smithy-lang/smithy-typescript#473
) from the Smithy TypeScript repository.

In order for it to work, we need to ensure the Bearer token
middleware succeeds even when the `token` is not set.
eduardomourar added a commit to eduardomourar/aws-sdk-js-v3 that referenced this pull request Dec 14, 2022
@eduardomourar
feat(middleware-api-key): add new auth middleware
9cd5fe4 
We will create a new middleware to handle API keys in the
context of Smithy generated models (probably not
applicable for AWS specific services). The code is based
heavily on [this PR](
smithy-lang/smithy-typescript#473
) from the Smithy TypeScript repository.

In order for it to work, we need to ensure the Bearer token
middleware succeeds even when the `token` is not set.
eduardomourar added a commit to eduardomourar/aws-sdk-js-v3 that referenced this pull request Dec 14, 2022
@eduardomourar
feat(middleware-api-key): add new auth middleware
8db1050 
We will create a new middleware to handle API keys in the
context of Smithy generated models (probably not
applicable for AWS specific services). The code is based
heavily on [this PR](
smithy-lang/smithy-typescript#473
) from the Smithy TypeScript repository.

In order for it to work, we need to ensure the Bearer token
middleware succeeds even when the `token` is not set.
eduardomourar added a commit to eduardomourar/aws-sdk-js-v3 that referenced this pull request Dec 15, 2022
@eduardomourar
feat(middleware-api-key): add new auth middleware
d8e9485 
We will create a new middleware to handle API keys in the
context of Smithy generated models (probably not
applicable for AWS specific services). The code is based
heavily on [this PR](
smithy-lang/smithy-typescript#473
) from the Smithy TypeScript repository.

In order for it to work, we need to ensure the Bearer token
middleware succeeds even when the `token` is not set.
eduardomourar added a commit to eduardomourar/aws-sdk-js-v3 that referenced this pull request Dec 23, 2022
@eduardomourar
feat(middleware-api-key): add new auth middleware
db8e8e4 
We will create a new middleware to handle API keys in the
context of Smithy generated models (probably not
applicable for AWS specific services). The code is based
heavily on [this PR](
smithy-lang/smithy-typescript#473
) from the Smithy TypeScript repository.

In order for it to work, we need to ensure the Bearer token
middleware succeeds even when the `token` is not set.
@eduardomourar eduardomourar mentioned this pull request Dec 23, 2022
Closed
srchase pushed a commit to srchase/smithy-typescript that referenced this pull request Mar 17, 2023
@glb @trivikr @srchase
feat: support @httpApiKeyAuth trait (smithy-lang#473)
7513c94 
Co-authored-by: Trivikram Kamat <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@trivikr trivikr trivikr approved these changes

@kstich kstich Awaiting requested review from kstich

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Feature request: support @httpApiKeyAuth with scheme
3 participants
@glb @kstich @trivikr