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

azurerm_azuread_application urls are now required to be https #1960

Merged
merged 3 commits into from
Sep 21, 2018

Conversation

katbyte
Copy link
Collaborator

@katbyte katbyte commented Sep 20, 2018

fixes #1953

existing tests are failing:

gofmt -w $(find . -name '*.go' |grep -v vendor)
==> Checking that code complies with gofmt requirements...
TF_ACC=1 go test ./azurerm -v -test.run=TestAccAzureRMActiveDirectoryApplication -timeout 180m
=== RUN   TestAccAzureRMActiveDirectoryApplication_basic
--- PASS: TestAccAzureRMActiveDirectoryApplication_basic (25.68s)
=== RUN   TestAccAzureRMActiveDirectoryApplication_availableToOtherTenants
--- PASS: TestAccAzureRMActiveDirectoryApplication_availableToOtherTenants (28.07s)
=== RUN   TestAccAzureRMActiveDirectoryApplication_complete
--- FAIL: TestAccAzureRMActiveDirectoryApplication_complete (9.09s)
	testing.go:513: Step 0 error: Error applying: 1 error(s) occurred:

		* azurerm_azuread_application.test: 1 error(s) occurred:

		* azurerm_azuread_application.test: graphrbac.ApplicationsClient#Create: Failure responding to request: StatusCode=400 -- Original Error: autorest/azure: Service returned an error. Status=400 Code="Unknown" Message="Unknown service error" Details=[{"odata.error":{"code":"Request_BadRequest","date":"2018-09-20T23:06:55","message":{"lang":"en","value":"The URI scheme in property  is invalid or unsupported."},"requestId":"c859cdd5-633a-47d1-86c9-32f5bd47214b","values":null}}]
=== RUN   TestAccAzureRMActiveDirectoryApplication_update
--- FAIL: TestAccAzureRMActiveDirectoryApplication_update (36.61s)
	testing.go:513: Step 1 error: Error applying: 1 error(s) occurred:

		* azurerm_azuread_application.test: 1 error(s) occurred:

		* azurerm_azuread_application.test: Error patching Azure AD Application with ID "076b47b2-ede2-491b-a188-aa108714116a": graphrbac.ApplicationsClient#Patch: Failure responding to request: StatusCode=400 -- Original Error: autorest/azure: Service returned an error. Status=400 Code="Unknown" Message="Unknown service error" Details=[{"odata.error":{"code":"Request_BadRequest","date":"2018-09-20T23:07:21","message":{"lang":"en","value":"The URI scheme in property  is invalid or unsupported."},"requestId":"11689938-958c-4262-af80-c8fc4ac55f90","values":null}}]
FAIL
FAIL	github.com/terraform-providers/terraform-provider-azurerm/azurerm	99.494s
make: *** [testacc] Error 1

@katbyte katbyte added the bug label Sep 20, 2018
@katbyte katbyte added this to the 1.16.0 milestone Sep 20, 2018
@katbyte katbyte requested a review from a team September 20, 2018 23:17
@ghost ghost added the size/L label Sep 20, 2018
@katbyte
Copy link
Collaborator Author

katbyte commented Sep 20, 2018

tests pass:

gofmt -w $(find . -name '*.go' |grep -v vendor)
==> Checking that code complies with gofmt requirements...
TF_ACC=1 go test ./azurerm -v -test.run=TestAccAzureRMActiveDirectoryApplication -timeout 180m
=== RUN   TestAccAzureRMActiveDirectoryApplication_basic
--- PASS: TestAccAzureRMActiveDirectoryApplication_basic (34.13s)
=== RUN   TestAccAzureRMActiveDirectoryApplication_availableToOtherTenants
--- PASS: TestAccAzureRMActiveDirectoryApplication_availableToOtherTenants (35.62s)
=== RUN   TestAccAzureRMActiveDirectoryApplication_complete
--- PASS: TestAccAzureRMActiveDirectoryApplication_complete (29.10s)
=== RUN   TestAccAzureRMActiveDirectoryApplication_update
--- PASS: TestAccAzureRMActiveDirectoryApplication_update (43.48s)
PASS
ok  	github.com/terraform-providers/terraform-provider-azurerm/azurerm	142.373s

Copy link
Contributor

@tombuildsstuff tombuildsstuff left a comment

Choose a reason for hiding this comment

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

LGTM 👍

@katbyte katbyte merged commit 06119dc into master Sep 21, 2018
@katbyte katbyte deleted the b-adapp-homepage branch September 21, 2018 19:52
katbyte added a commit that referenced this pull request Sep 21, 2018
@tomasaschan
Copy link
Contributor

tomasaschan commented Oct 17, 2018

Hi!

I'm wondering if the root cause of this behavior was incorrectly diagnosed, or if maybe Azure have changed their API again. If i peg my azurerm provider version to = 1.15 (i.e. the latest release before this change was merged) I can apply the following template without problems:

provider "azurerm" {
  version = "= 1.15"
}

resource "azurerm_azuread_application" "ad_app" {
  name = "tomas-testing"
}

In other words, I cannot reproduce the original issue (#1953).

However, as of this PR and version 1.16, no URLs on the application can be http, which disallows something like this:

resource "azurerm_azuread_application" "ad_app" {
  name = "tomas-testing"

  reply_urls = ["http://localhost:8080"]
}

Again, this template applies successfully with 1.15 but fails on 1.16. This effectively makes impossible to e.g. set reply urls for AzureAD applications that enable local testing of OAuth flows (without configuring your local development environment to run https).

Is there any chance this PR could be reverted for 1.17, to re-enable this configuration?

PS. This template, touching all three types of URLs and setting them to HTTP-schemed values, also works without problems on 1.15:

provider "azurerm" {
  version = "= 1.15"
}

resource "azurerm_azuread_application" "ad_app" {
  name     = "tomas-testing"
  homepage = "http://tomas-testar.com"

  identifier_uris = [
    "http://tomas-testar",
  ]

  reply_urls = ["http://localhost:8080"]
}

@tomasaschan
Copy link
Contributor

Ping @katbyte @tombuildsstuff - just making sure this didn't fall under the radar because the issue is already closed :)

@tombuildsstuff
Copy link
Contributor

@tomasaschan since this issue is closed - would you open a separate issue about that? Thanks!

@tomasaschan
Copy link
Contributor

@tombuildsstuff No probs - see #2130 :)

@ghost
Copy link

ghost commented Mar 6, 2019

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 ghost locked and limited conversation to collaborators Mar 6, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

azurerm_azuread_application's default value for the homepage field is no longer valid
3 participants