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

fix: remove OIDC auth from Http Api #1491

Merged
merged 2 commits into from
Mar 5, 2020
Merged

fix: remove OIDC auth from Http Api #1491

merged 2 commits into from
Mar 5, 2020

Conversation

keetonian
Copy link
Contributor

@keetonian keetonian commented Mar 4, 2020 •
edited
Loading

Issue #, if available:
N/A

Description of changes:
OIDC and OAuth2 are the same authorization on the backend and the configuration for both is practically the same. This was causing some confusion for customers (and for us), so we decided to remove OIDC and only support the OAuth2 strategy for GA for Http Apis.

Description of how you validated changes:
Local and cloud testing

Checklist:

  • Write/update tests
  • make pr passes
  • Update documentation
  • Verify transformed template deploys and application functions as expected
  • Add/update example to examples/2016-10-31

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

@codecov-io
Copy link

Codecov Report

Merging #1491 into develop will increase coverage by 0.02%.
The diff coverage is 100%.

Impacted file tree graph

@@             Coverage Diff             @@
##           develop    #1491      +/-   ##
===========================================
+ Coverage     94.1%   94.12%   +0.02%     
===========================================
  Files           78       78              
  Lines         4900     4921      +21     
  Branches       988      989       +1     
===========================================
+ Hits          4611     4632      +21     
  Misses         134      134              
  Partials       155      155
Impacted Files Coverage Δ
samtranslator/model/api/http_api_generator.py 92.27% <ø> (+0.06%) ⬆️
samtranslator/model/apigatewayv2.py 93.93% <100%> (-0.8%) ⬇️
samtranslator/model/eventsources/push.py 90.78% <0%> (ø) ⬆️
...translator/plugins/api/implicit_rest_api_plugin.py 100% <0%> (ø) ⬆️
samtranslator/model/sam_resources.py 94.29% <0%> (ø) ⬆️
samtranslator/plugins/globals/globals.py 99.05% <0%> (ø) ⬆️
samtranslator/plugins/api/implicit_api_plugin.py 98.24% <0%> (+0.01%) ⬆️
...translator/plugins/api/implicit_http_api_plugin.py 97.43% <0%> (+0.94%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update ef92c3f...4dbc15a. Read the comment docs.

@alexw91
Copy link

alexw91 commented Mar 5, 2020

Codecov Report

Merging #1491 into develop will increase coverage by 0.02%.
The diff coverage is n/a.

Impacted file tree graph

@@             Coverage Diff             @@
##           develop    #1491      +/-   ##
===========================================
+ Coverage    94.10%   94.12%   +0.02%     
===========================================
  Files           78       78              
  Lines         4900     4921      +21     
  Branches       988      989       +1     
===========================================
+ Hits          4611     4632      +21     
  Misses         134      134              
  Partials       155      155
Impacted Files Coverage Δ
samtranslator/model/sam_resources.py 94.29% <0.00%> (ø)
samtranslator/model/eventsources/push.py 90.78% <0.00%> (ø)
samtranslator/plugins/globals/globals.py 99.05% <0.00%> (ø)
...translator/plugins/api/implicit_rest_api_plugin.py 100.00% <0.00%> (ø)
samtranslator/plugins/api/implicit_api_plugin.py 98.24% <0.00%> (+0.01%) ⬆️
samtranslator/model/api/http_api_generator.py 92.27% <0.00%> (+0.06%) ⬆️
samtranslator/model/apigatewayv2.py 93.93% <0.00%> (-0.80%) ⬇️
...translator/plugins/api/implicit_http_api_plugin.py 97.43% <0.00%> (+0.94%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update ef92c3f...4dbc15a. Read the comment docs.

@codecov-io
Copy link

Codecov Report

Merging #1491 into develop will increase coverage by 0.02%.
The diff coverage is 100%.

Impacted file tree graph

@@             Coverage Diff             @@
##           develop    #1491      +/-   ##
===========================================
+ Coverage     94.1%   94.12%   +0.02%     
===========================================
  Files           78       78              
  Lines         4900     4921      +21     
  Branches       988      989       +1     
===========================================
+ Hits          4611     4632      +21     
  Misses         134      134              
  Partials       155      155
Impacted Files Coverage Δ
samtranslator/model/api/http_api_generator.py 92.27% <ø> (+0.06%) ⬆️
samtranslator/model/apigatewayv2.py 93.93% <100%> (-0.8%) ⬇️
samtranslator/model/eventsources/push.py 90.78% <0%> (ø) ⬆️
...translator/plugins/api/implicit_rest_api_plugin.py 100% <0%> (ø) ⬆️
samtranslator/model/sam_resources.py 94.29% <0%> (ø) ⬆️
samtranslator/plugins/globals/globals.py 99.05% <0%> (ø) ⬆️
samtranslator/plugins/api/implicit_api_plugin.py 98.24% <0%> (+0.01%) ⬆️
...translator/plugins/api/implicit_http_api_plugin.py 97.43% <0%> (+0.94%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update ef92c3f...4dbc15a. Read the comment docs.

praneetap
praneetap reviewed Mar 5, 2020
View reviewed changes
Copy link
Contributor

@praneetap praneetap left a comment

Choose a reason for hiding this comment

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

I think, instead of breaking our customers with a invalid property error, we should try to put an error message that says this is no longer supported.

@keetonian
Copy link
Contributor Author

Added an explicit error in latest commit.

@alexw91
Copy link

alexw91 commented Mar 5, 2020 •
edited
Loading

Codecov Report

Merging #1491 into develop will increase coverage by 0.02%.
The diff coverage is n/a.

Impacted file tree graph

@@             Coverage Diff             @@
##           develop    #1491      +/-   ##
===========================================
+ Coverage    94.10%   94.13%   +0.02%     
===========================================
  Files           78       78              
  Lines         4900     4925      +25     
  Branches       988      990       +2     
===========================================
+ Hits          4611     4636      +25     
  Misses         134      134              
  Partials       155      155
Impacted Files Coverage Δ
samtranslator/model/apigatewayv2.py 93.93% <0.00%> (-0.80%) ⬇️
samtranslator/model/sam_resources.py 94.29% <0.00%> (ø)
samtranslator/model/eventsources/push.py 90.78% <0.00%> (ø)
samtranslator/plugins/globals/globals.py 99.05% <0.00%> (ø)
...translator/plugins/api/implicit_rest_api_plugin.py 100.00% <0.00%> (ø)
samtranslator/plugins/api/implicit_api_plugin.py 98.24% <0.00%> (+0.01%) ⬆️
samtranslator/model/api/http_api_generator.py 92.40% <0.00%> (+0.18%) ⬆️
...translator/plugins/api/implicit_http_api_plugin.py 97.43% <0.00%> (+0.94%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update ef92c3f...ea60b27. Read the comment docs.

@codecov-io
Copy link

Codecov Report

Merging #1491 into develop will increase coverage by 0.02%.
The diff coverage is 100%.

Impacted file tree graph

@@             Coverage Diff             @@
##           develop    #1491      +/-   ##
===========================================
+ Coverage     94.1%   94.13%   +0.02%     
===========================================
  Files           78       78              
  Lines         4900     4925      +25     
  Branches       988      990       +2     
===========================================
+ Hits          4611     4636      +25     
  Misses         134      134              
  Partials       155      155
Impacted Files Coverage Δ
samtranslator/model/apigatewayv2.py 93.93% <100%> (-0.8%) ⬇️
samtranslator/model/api/http_api_generator.py 92.4% <100%> (+0.18%) ⬆️
samtranslator/model/eventsources/push.py 90.78% <0%> (ø) ⬆️
...translator/plugins/api/implicit_rest_api_plugin.py 100% <0%> (ø) ⬆️
samtranslator/model/sam_resources.py 94.29% <0%> (ø) ⬆️
samtranslator/plugins/globals/globals.py 99.05% <0%> (ø) ⬆️
samtranslator/plugins/api/implicit_api_plugin.py 98.24% <0%> (+0.01%) ⬆️
...translator/plugins/api/implicit_http_api_plugin.py 97.43% <0%> (+0.94%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update ef92c3f...ea60b27. Read the comment docs.

@praneetap praneetap merged commit aba498f into aws:develop Mar 5, 2020
@ShreyaGangishetty ShreyaGangishetty mentioned this pull request Mar 6, 2020
Closed
@keetonian keetonian deleted the consolidate-auth branch March 11, 2020 18:57
@SampathDilhan SampathDilhan mentioned this pull request Dec 2, 2022
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ShreyaGangishetty ShreyaGangishetty ShreyaGangishetty approved these changes

@praneetap praneetap praneetap approved these changes

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

Successfully merging this pull request may close these issues.
5 participants
@keetonian @codecov-io @alexw91 @praneetap @ShreyaGangishetty