Skip to content

add Azure Active Directory as sql server admin authentication#1106

Merged
AlexanderSehr merged 10 commits intoAzure:mainfrom
mvbugge:users/maive/sqlServerAADAuthen
Mar 10, 2022
Merged

add Azure Active Directory as sql server admin authentication#1106
AlexanderSehr merged 10 commits intoAzure:mainfrom
mvbugge:users/maive/sqlServerAADAuthen

Conversation

@mvbugge
Copy link
Copy Markdown
Contributor

@mvbugge mvbugge commented Mar 8, 2022

Change

Add the configuration to use AAD as sql server admin authentication.
SQL admin credentials were previously required parameters, this is changed to conditional as they are no longer necessary if AAD is configured to be only authentication method.

Type of Change

  • New feature (non-breaking change which adds functionality)

Checklist

  • I'm sure there are no other open Pull Requests for the same update/change
  • My corresponding pipelines / checks run clean and green without any errors or warnings
  • My code follows the style guidelines of this project
  • I have commented my code, particularly in hard-to-understand areas
  • I have made corresponding changes to the documentation (readme)
  • I did format my code

@ghost
Copy link
Copy Markdown

ghost commented Mar 8, 2022

CLA assistant check
All CLA requirements met.

@mvbugge mvbugge changed the title add Azure Active Directory as admin authentication add Azure Active Directory as sql server admin authentication Mar 8, 2022
@AlexanderSehr AlexanderSehr added [cat] modules category: modules enhancement New feature or request labels Mar 8, 2022
@AlexanderSehr
Copy link
Copy Markdown
Contributor

Hey @mvbugge, thanks for the great contribution. Just a few comments and asks.

mvbugge and others added 6 commits March 8, 2022 13:05
Co-authored-by: Alexander Sehr <ASehr@hotmail.de>
Co-authored-by: Alexander Sehr <ASehr@hotmail.de>
Co-authored-by: Alexander Sehr <ASehr@hotmail.de>
Co-authored-by: Alexander Sehr <ASehr@hotmail.de>
Co-authored-by: Alexander Sehr <ASehr@hotmail.de>
@AlexanderSehr
Copy link
Copy Markdown
Contributor

I'll give the template a spin as soon as I can. If the change works in the pipeline, we can merge it

mvbugge and others added 2 commits March 10, 2022 13:10
Co-authored-by: Alexander Sehr <ASehr@hotmail.de>
Co-authored-by: Alexander Sehr <ASehr@hotmail.de>
@AlexanderSehr
Copy link
Copy Markdown
Contributor

AlexanderSehr commented Mar 10, 2022

Hey @mvbugge , I created a branch with your code to test the changes using pipelines. To make it transparent, I opened a draft PR which is not supposed to be merged.
Anyhow, I added a test file to specifically test the adminitrator variant, but it fails the login. Any idea why this could be these case?

Parameters
"administrators": {
    "value": {
        "azureADOnlyAuthentication": false,
        "login": "U85Z90R82n110u117M7",
        "sid": "<<deploymentSpId>>",
        "principalType": "Application",
        "tenantId": "<<tenantId>>"
    }
}

Error
Status Message: Invalid value given for parameter Login. Specify a valid parameter value. (Code:InvalidParameterValue)

@mvbugge
Copy link
Copy Markdown
Contributor Author

mvbugge commented Mar 10, 2022

@MrMCake I've added the suggestions to the new PR. I suggest trying "azureADOnlyAuthentication": true

@AlexanderSehr
Copy link
Copy Markdown
Contributor

@mvbugge You nailed it. I successfully tested it in the test PR.

@AlexanderSehr AlexanderSehr merged commit 1df9524 into Azure:main Mar 10, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

[cat] modules category: modules enhancement New feature or request

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants