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_app_service: Adds authentication support #2831

Merged

Conversation

joakimhew
Copy link
Contributor

This PR fixes #1992 and allows users to configure authentication for azurerm_app_service. This PR supports all of the providers in azure (Active Directory, Facebook, Google, Microsoft, Twitter).

It uses the UpdateAuthSetting as mentioned in the issue. More information about the API here: Web Apps - Update Auth Settings

Each provider is defined in it's own block inside of an auth_settings section.
Example:

auth_settings {
  enabled                       = true
  issuer                        = "https://sts.windows.net/d13958f6-b541-4dad-97b9-5a39c6b01297"
  default_provider              = "AzureActiveDirectory"
  unauthenticated_client_action = "RedirectToLoginPage"

  active_directory {
    client_id     = "aadclientid"
    client_secret = "aadsecret"

    allowed_audiences = [
      "someallowedaudience",
    ]
  }
    
  microsoft {
    client_id     = "microsoftclientid"
    client_secret = "microsoftclientsecret"

    oauth_scopes = [
      "somescope",
    ]
  }
}

Copy link

@a144359 a144359 left a comment

Choose a reason for hiding this comment

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

can we do this for Azure function app as well? Looks like much of the changes are applicable to functions_app

@tombuildsstuff tombuildsstuff modified the milestones: 1.23.0, 1.25.0 Mar 5, 2019
@tombuildsstuff tombuildsstuff modified the milestones: v1.25.0, v1.24.0 Mar 14, 2019
@joakimhew
Copy link
Contributor Author

@a144359 I'll look into this 👍

@tombuildsstuff
Copy link
Member

@joakimhew sorry for the delay reviewing this PR (I'd missed this hadn't been reviewed) - I'm taking a look now

@tombuildsstuff tombuildsstuff modified the milestones: v1.24.0, v1.25.0 Apr 1, 2019
@joakimhew
Copy link
Contributor Author

@tombuildsstuff Thank you buddy. I can see that there are a couple of conflicts now as it's been a while. I'll wait for your review before I resolve those conflicts! 😄

Copy link
Member

@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.

hey @joakimhew

Thanks for this PR - apologies for the delayed review here, I'd got most of the way through this and then forgot to hit submit, so I've taken a fresh look this morning.

On the whole this looks pretty good - if we can fix up the comments (and resolve the merge conflicts) this otherwise LGTM 👍

Thanks!

azurerm/helpers/azure/app_service.go Outdated Show resolved Hide resolved
azurerm/helpers/azure/app_service.go Outdated Show resolved Hide resolved
azurerm/helpers/azure/app_service.go Outdated Show resolved Hide resolved
azurerm/helpers/azure/app_service.go Outdated Show resolved Hide resolved
azurerm/helpers/azure/app_service.go Outdated Show resolved Hide resolved
azurerm/helpers/azure/app_service.go Outdated Show resolved Hide resolved
azurerm/helpers/azure/app_service.go Outdated Show resolved Hide resolved
azurerm/helpers/azure/app_service.go Outdated Show resolved Hide resolved
azurerm/helpers/azure/app_service.go Outdated Show resolved Hide resolved
website/docs/r/app_service.html.markdown Outdated Show resolved Hide resolved
@joakimhew
Copy link
Contributor Author

@tombuildsstuff Thank you for the review Tom. I'll try to get by it this week! 👍

@ghost ghost removed the waiting-response label Apr 15, 2019
@tombuildsstuff tombuildsstuff modified the milestones: v1.25.0, v1.26.0 Apr 16, 2019
@katbyte katbyte modified the milestones: v1.26.0, v1.27.0 Apr 17, 2019
@joakimhew joakimhew force-pushed the feature/app_service_site_auth_settings branch 4 times, most recently from dc701ed to 6c5e481 Compare April 23, 2019 13:13
@tombuildsstuff tombuildsstuff self-assigned this May 7, 2019
@joakimhew joakimhew force-pushed the feature/app_service_site_auth_settings branch from cb8d8c1 to 517b906 Compare May 19, 2019 18:01
@joakimhew
Copy link
Contributor Author

@tombuildsstuff I've resolved your requested changes and hopefully it's good to go now 👍

@ghost ghost removed the waiting-response label May 19, 2019
Copy link
Member

@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.

hey @joakimhew

Thanks for pushing those changes - taking a look through besides some minor documentation tweaks (to ensure this matches the other resources, which I hope you don't mind but I'm going to push a commit for) - this LGTM 👍

Thanks!

website/docs/r/app_service.html.markdown Outdated Show resolved Hide resolved
website/docs/r/app_service.html.markdown Outdated Show resolved Hide resolved
website/docs/r/app_service.html.markdown Outdated Show resolved Hide resolved
website/docs/r/app_service.html.markdown Outdated Show resolved Hide resolved
website/docs/r/app_service.html.markdown Outdated Show resolved Hide resolved
website/docs/r/app_service.html.markdown Outdated Show resolved Hide resolved
website/docs/r/app_service.html.markdown Outdated Show resolved Hide resolved
website/docs/r/app_service.html.markdown Outdated Show resolved Hide resolved
website/docs/r/app_service.html.markdown Outdated Show resolved Hide resolved
website/docs/r/app_service.html.markdown Outdated Show resolved Hide resolved
@joakimhew
Copy link
Contributor Author

@tombuildsstuff Wonderful, thank you for resolving the documentation stuff. And as always, thanks for the amount of work you put into reviewing this! 😄

@tombuildsstuff
Copy link
Member

tombuildsstuff commented May 22, 2019

@joakimhew anytime :) the tests identified a couple more minor things, which I've pushed a commit to fix

@tombuildsstuff
Copy link
Member

tombuildsstuff commented May 22, 2019

Tests pass:

Screenshot 2019-05-22 at 15 49 24

Thanks again @joakimhew :)

@tombuildsstuff tombuildsstuff merged commit ef1914f into hashicorp:master May 22, 2019
tombuildsstuff added a commit that referenced this pull request May 22, 2019
@ghost
Copy link

ghost commented May 26, 2019

This has been released in version 1.29.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 = "~> 1.29.0"
}
# ... other configuration ...

@ghost
Copy link

ghost commented Jun 22, 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!

@hashicorp hashicorp locked and limited conversation to collaborators Jun 22, 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.

Support Auth users on web apps using siteAuthSettings
4 participants