Add oauth2_authorization and openid_authentication #7617

sirlatrom · 2020-07-08T10:17:45Z

jackofallops · 2020-07-09T13:37:22Z

Hi @sirlatrom
Thanks for this PR.

Could you rebase against origin master and skip the commit that includes the giovani dependencies? That should work fine and will clean up the PR some too. I will loop back to review asap.

Thanks!

Signed-off-by: Sune Keller <absukl@almbrand.dk>

…ApiManagementApi_basicClassic Signed-off-by: Sune Keller <absukl@almbrand.dk>

…penidAuthentication Signed-off-by: Sune Keller <absukl@almbrand.dk>

sirlatrom · 2020-07-09T13:39:38Z

@jackofallops ~~Rebased.~~ Will rebase and try and find said commits to skip.

Update: Cannot find anything mentioning giovani in the git log or any files in the repo, so I think the outcome of the rebase was as expected.

Update 2: Found them by the name giovanni (two n's), but can see they are not in the current dependencies list.

jackofallops

Hi @sirlatrom
Thanks for this PR, apologies it took a while to get back to it after the initial comments.
I've left some comments and suggestions below, and a question on the validation for the OAuth scope. I'm not sure how the service would react to an update that contained an empty string there, rather than a nil value? Could we add update tests that exercise changes on these?

Thanks

azurerm/internal/services/apimanagement/api_management_api_resource.go

Co-authored-by: Steve <11830746+jackofallops@users.noreply.github.com>

Signed-off-by: Sune Keller <absukl@almbrand.dk>

jackofallops

Hi @sirlatrom
Thanks for the quick turnaround. Just a couple of errors in the tests to fix-up and this is good to go.

Thanks again!

azurerm/internal/services/apimanagement/tests/api_management_api_resource_test.go

Signed-off-by: Sune Keller <absukl@almbrand.dk> Co-authored-by: Steve <11830746+jackofallops@users.noreply.github.com>

sirlatrom · 2020-07-29T12:36:16Z

Hi @sirlatrom
Thanks for the quick turnaround. Just a couple of errors in the tests to fix-up and this is good to go.

Thanks again!

@jackofallops I've fixed the test errors as per your suggestions and the PR should be good to go now.

jackofallops

Hi @sirlatrom

Apologies for taking so long to get back to this, it's been a busy few weeks.

I was about to run the acceptance tests and I spotted a panic (commented below) which sparked some additional schema implementation thinking (sorry!). If you can respond or update as you feel appropriate I'll try and stay on top of this one as quickly as I can.

azurerm/internal/services/apimanagement/api_management_api_resource.go

jackofallops · 2020-08-07T13:05:19Z

azurerm/internal/services/apimanagement/api_management_api_resource.go

+							ValidateFunc: validate.ApiManagementChildName,
+						},
+						"bearer_token_sending_methods": {
+							Type:         schema.TypeSet,


Should this be a set? Thinking over it, there are only 2 possible values, would it not be more appropriate to have these as individual bools?

Signed-off-by: Sune Keller <absukl@almbrand.dk> Co-authored-by: Steve <11830746+jackofallops@users.noreply.github.com>

sirlatrom · 2020-08-07T13:20:35Z

Hi @sirlatrom

Apologies for taking so long to get back to this, it's been a busy few weeks.

I was about to run the acceptance tests and I spotted a panic (commented below) which sparked some additional schema implementation thinking (sorry!). If you can respond or update as you feel appropriate I'll try and stay on top of this one as quickly as I can.

Thanks for finding that! Sets are used so rarely that it's error prone to follow my usual procedure: copy & paste.

Signed-off-by: Sune Keller <absukl@almbrand.dk>

Geertvdc · 2020-08-12T11:42:35Z

Hi, i bumped into this issue because it was exactly what we were missing. Nice to see that @sirlatrom took the time to add this ❤

The only thing blocking this is the very rarely used property bearerTokenSendingMethods which is an array in the Azure rest API as well. so the current solution of @sirlatrom seems to fit well for me because it is closer to the actual Azure API compared to 2 separate booleans.

What do you think is needed to close this pr @jackofallops

jackofallops · 2020-08-14T13:46:27Z

Hi @sirlatrom - apologies for taking a while to get back to this again. I should have mentioned the knock on changes that would be needed to accept the previous suggestion, sorry. I've pushed what I think should sort that out so I can run the tests on this without tying up my laptop for hours (they take quite a while to run). I'll finish fixing up anything I've missed and get this merged asap.

sirlatrom · 2020-08-14T14:47:14Z

Hi @sirlatrom - apologies for taking a while to get back to this again. I should have mentioned the knock on changes that would be needed to accept the previous suggestion, sorry. I've pushed what I think should sort that out so I can run the tests on this without tying up my laptop for hours (they take quite a while to run). I'll finish fixing up anything I've missed and get this merged asap.

It looks to me like you made the right changes. If you need me to do anything more on this PR I'll be able to do so Monday CEST.

jackofallops · 2020-08-14T14:54:17Z

It looks to me like you made the right changes. If you need me to do anything more on this PR I'll be able to do so Monday CEST.

Thanks! I'm getting tired I think, made a silly mistake on the flatten so that panicked on the test run. I've fixed that and kicked it off again so I'll check the results first thing Monday.

jackofallops · 2020-08-17T07:30:14Z

Tests passing:

ghost · 2020-08-20T17:46:56Z

This has been released in version 2.24.0 of the provider. Please see the Terraform documentation on provider versioning or reach out if you need any assistance upgrading. As an example:

provider "azurerm" {
    version = "~> 2.24.0"
}
# ... other configuration ...

ghost · 2020-09-16T14:50:06Z

I'm going to lock this issue because it has been closed for 30 days ⏳. This helps our maintainers find and focus on the active issues.

If you feel this issue should be reopened, we encourage creating a new issue linking back to this one for added context. If you feel I made an error 🤖 🙉 , please reach out to my human friends 👉 hashibot-feedback@hashicorp.com. Thanks!

ghost added size/L documentation labels Jul 8, 2020

sirlatrom force-pushed the fix-3341 branch from 00e6130 to f16aef0 Compare July 8, 2020 10:28

ghost added size/XL dependencies size/XXL and removed size/L size/XL labels Jul 8, 2020

jackofallops added the service/api-management label Jul 9, 2020

sirlatrom added 3 commits July 9, 2020 15:39

Add oauth2_authorization and openid_authentication

f4ee1bb

Signed-off-by: Sune Keller <absukl@almbrand.dk>

Remove testAccAzureRMApiManagementApi_basicClassic and TestAccAzureRM…

db13fe5

…ApiManagementApi_basicClassic Signed-off-by: Sune Keller <absukl@almbrand.dk>

Fix number of args to fmt.Sprintf in testAccAzureRMApiManagementApi_o…

b709544

…penidAuthentication Signed-off-by: Sune Keller <absukl@almbrand.dk>

sirlatrom force-pushed the fix-3341 branch from 768d20f to b709544 Compare July 9, 2020 13:39

ghost added size/L and removed size/XXL labels Jul 9, 2020

jackofallops requested changes Jul 23, 2020

View reviewed changes

jackofallops added the waiting-response label Jul 23, 2020

jackofallops added this to the v2.21.0 milestone Jul 23, 2020

sirlatrom and others added 2 commits July 23, 2020 13:42

Check authorization_server_name for nil

d759803

Co-authored-by: Steve <11830746+jackofallops@users.noreply.github.com>

Check openid_provider_name for nil

bbf2523

Co-authored-by: Steve <11830746+jackofallops@users.noreply.github.com>

ghost added size/XL and removed size/L labels Jul 23, 2020

sirlatrom requested a review from jackofallops July 23, 2020 11:45

Add expand functions for OAuth2 and OpenID auth blocks

f260a23

Signed-off-by: Sune Keller <absukl@almbrand.dk>

sirlatrom force-pushed the fix-3341 branch from 778a388 to f260a23 Compare July 23, 2020 11:59

jackofallops requested changes Jul 23, 2020

View reviewed changes

azurerm/internal/services/apimanagement/tests/api_management_api_resource_test.go Outdated Show resolved Hide resolved

azurerm/internal/services/apimanagement/tests/api_management_api_resource_test.go Outdated Show resolved Hide resolved

Apply suggestions from code review

f60ef11

Signed-off-by: Sune Keller <absukl@almbrand.dk> Co-authored-by: Steve <11830746+jackofallops@users.noreply.github.com>

sirlatrom requested a review from jackofallops July 23, 2020 14:45

ghost removed the waiting-response label Jul 29, 2020

katbyte modified the milestones: v2.21.0, v2.22.0, v2.23.0 Jul 31, 2020

sirlatrom mentioned this pull request Aug 7, 2020

No way to configure security/ user authorization for an API using Azure API management API resource #3341

Closed

jackofallops requested changes Aug 7, 2020

View reviewed changes

Apply suggestions from code review

16d3a35

Signed-off-by: Sune Keller <absukl@almbrand.dk> Co-authored-by: Steve <11830746+jackofallops@users.noreply.github.com>

Make fmt

a42e60d

Signed-off-by: Sune Keller <absukl@almbrand.dk>

sirlatrom requested a review from jackofallops August 12, 2020 11:53

tombuildsstuff modified the milestones: v2.23.0, v2.24.0 Aug 13, 2020

adjusted for knock on effects of applying previous suggestion

cb31cb5

fixed type setting mistake from last commit

7c93267

jackofallops merged commit 10b165d into hashicorp:master Aug 17, 2020

jackofallops added a commit that referenced this pull request Aug 17, 2020

Updated for #7617

0c2b7d2

sirlatrom deleted the fix-3341 branch August 17, 2020 07:44

ghost locked and limited conversation to collaborators Sep 16, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add oauth2_authorization and openid_authentication #7617

Add oauth2_authorization and openid_authentication #7617

sirlatrom commented Jul 8, 2020

jackofallops commented Jul 9, 2020

sirlatrom commented Jul 9, 2020 •

edited

Loading

jackofallops left a comment

jackofallops left a comment

sirlatrom commented Jul 29, 2020

jackofallops left a comment

jackofallops Aug 7, 2020

sirlatrom commented Aug 7, 2020

Geertvdc commented Aug 12, 2020 •

edited

Loading

jackofallops commented Aug 14, 2020

sirlatrom commented Aug 14, 2020

jackofallops commented Aug 14, 2020

jackofallops commented Aug 17, 2020

ghost commented Aug 20, 2020

ghost commented Sep 16, 2020

Add oauth2_authorization and openid_authentication #7617

Add oauth2_authorization and openid_authentication #7617

Conversation

sirlatrom commented Jul 8, 2020

jackofallops commented Jul 9, 2020

sirlatrom commented Jul 9, 2020 • edited Loading

jackofallops left a comment

Choose a reason for hiding this comment

jackofallops left a comment

Choose a reason for hiding this comment

sirlatrom commented Jul 29, 2020

jackofallops left a comment

Choose a reason for hiding this comment

jackofallops Aug 7, 2020

Choose a reason for hiding this comment

sirlatrom commented Aug 7, 2020

Geertvdc commented Aug 12, 2020 • edited Loading

jackofallops commented Aug 14, 2020

sirlatrom commented Aug 14, 2020

jackofallops commented Aug 14, 2020

jackofallops commented Aug 17, 2020

ghost commented Aug 20, 2020

ghost commented Sep 16, 2020

sirlatrom commented Jul 9, 2020 •

edited

Loading

Geertvdc commented Aug 12, 2020 •

edited

Loading