Add more tests for EntraId/AzureAd providers

[sander1095]

Why make this change?

Closes #2407

This issue adds more tests for the EntraId authentication provider.

EntraId already seemed to be supported, so no business logic was needed for this change.

What is this change?

Several tests have been added.

How was this tested?

• Integration Tests
• Unit Tests

Sample Request(s)

Use EntraId and AzureAd providers like before.

[sander1095]

This PR still needs some discussions with the team as the issue's requirements don't really match DAB's behaviour. These changes can be made, but I doubt that such big changes are intended in this simple PR. Therefore, let's discuss what changes are still required:

• There is a TODO in the code where adding a synonym would cause behavioral differences, which isn't desired according to the issue. Please take a look at the comment and let's discuss!
• The issue mentions adding properties to the JSON Schema. However, the JSON schema currently doesn't validate any authentication.provider entries. I can add this, but is this desired in this PR?
  • Nor does it validate (in the json schema) that jwt should be null when the provider isn't AzureAd/EntraId/OAuth. This is validated in code. Is there a reason this is only validated at runtime and not in config? Are any changes desired in this PR?
• DAB's documentation isn't updated in this PR because the docs aren't part of this repo, it seems. If desired, I can write some docs, or is this usually done by people at Microsoft?
• Perhaps some more tests are desired. If so, please point out some locations!
• I'm running on Windows with the git pre commit hook installed. When I run dotnet format and try to commit, the hook fails. Perhaps because of CRLF changes? In any case, I disabled the hook and ran dotnet format manually but this might still need some looking into.

[sander1095]

@microsoft-github-policy-service agree

[RubenCerna2079]

/azp run

[JerryNixon]

Important

Before this is merged, we need to make a change. This is my fault.

In addition to None we need the synonym EasyAuth which is what StaticWebApps always meant.

This would be an additional synonym and what we recommend to users deploying to ACA and ACI.

[sander1095]

Hi @JerryNixon !

I've added some more code to the PR that implements your request. However, the PR is now littered with a few TODOs as your case seems to complicate some matters. I'd like to ask if someone from the team can take a look at it.

If my team agree that my TODO's require this PR to become more complex, it might be worth putting the generic EasyAuth auth provider in a seperate issue to keep this PR mergeable. Curious to hear what you think. I could simply revert the last 2 commits for this to happen (and then implement any comments the team might have)

Another reason to consider moving the EasyAuth provider to its own issue is that there's lots of places in the codebase that say StaticWebApps is the default (JSON Schema, XML Comments, etc..). Do you want EasyAuth to also be the new default? As in, should StaticWebApps actually become a synonym for EasyAuth, instead of the other way around?

[RubenCerna2079]

/azp run

[Aniruddh25]

• There is a TODO in the code where adding a synonym would cause behavioral differences, which isn't desired according to the issue. Please take a look at the comment and let's discuss!

Got it, lets discuss in line with respect to each TODO

• The issue mentions adding properties to the JSON Schema. However, the JSON schema currently doesn't validate any authentication.provider entries. I can add this, but is this desired in this PR?

Yes please add the properties in the schema in this PR or in an immediate subsequent one even though they are not being validated.

• Nor does it validate (in the json schema) that jwt should be null when the provider isn't AzureAd/EntraId/OAuth. This is validated in code. Is there a reason this is only validated at runtime and not in config? Are any changes desired in this PR?

Lets create a separate PR to validate the config using the schema. It was simply a miss by previous contributors.

• DAB's documentation isn't updated in this PR because the docs aren't part of this repo, it seems. If desired, I can write some docs, or is this usually done by people at Microsoft?

Could you please provide edits to the docs here:

https://learn.microsoft.com/en-us/azure/data-api-builder/authentication-local#use-the-simulator-provider

• Perhaps some more tests are desired. If so, please point out some locations!
• I'm running on Windows with the git pre commit hook installed. When I run dotnet format and try to commit, the hook fails. Perhaps because of CRLF changes? In any case, I disabled the hook and ran dotnet format manually but this might still need some looking into.

Will look into the PR this week. Thanks for your patience.

[RubenCerna2079]

/azp run

[azure-pipelines]

Azure Pipelines successfully started running 6 pipeline(s).

[sander1095]

@Aniruddh25 @JerryNixon @RubenCerna2079

This PR has been a wild ride ;)!

This PR now contains all the latest requested changes. Now that None, OAuth and EasyAuth have been removed from this PR's changeset, the resulting PR only contains some missing tests for EntraId; EntraId already existed as a synonym for AzureAd, so it seems no changes were required there. The test results will tell.

Some actionable items for @Aniruddh25 / @JerryNixon :
I suggest to create separate issues for adding None, OAuth and EasyAuth in the future, where each of these can get the attention they need. The comments in this PR can be used as discussion points, as I had already picked out several spots of code that would cause behavioral differences.

---

In an earlier comment, I discussed the issue of the JSON schema not having a list of auth providers to choose from, which also meant that weren't validated. I also talked about this at the MVP Summit. @Aniruddh25 said they could be added in this PR, but I disagree with this because this PR is currently nice, small and (hopefully) mergeable. I've created #2643 to further discuss this.

Let me know if any small changes are still needed.

[RubenCerna2079]

/azp run

[azure-pipelines]

Azure Pipelines successfully started running 6 pipeline(s).

[RubenCerna2079]

/azp run

[azure-pipelines]

Azure Pipelines successfully started running 6 pipeline(s).

[RubenCerna2079]

/azp run

[azure-pipelines]

Azure Pipelines successfully started running 6 pipeline(s).