React to Microsoft Entra ID suggestions

[hishamco]

Related to #14004

[hishamco]

First, you need to create an Microsoft Entra ID app on the Azure Portal for your Microsoft Entra ID tenant.

1. Go to the "Azure Active Directory" menu, which will open your organization's Active Directory settings.

@Piedone could you please confirm if the menu title changes in Azure or not, so I can react to the change

[Piedone]

It's still called AD in the Azure Portal, because they roll out the rename only starting in August.

[hishamco]

Is there anything missing or shall we merge this PR?

[Piedone]

I'll review it, just give me some time, please.

[Piedone]

When you request review the next time, please double-check that you've indeed addressed all my feedback.

[Piedone]

This is a breaking change, so we shouldn't do it (and it also needs the same rename in the step). However, I'm not sure the original implementation actually worked, because the name here is "AzureAD". while in AzureADSettingsStep it's nameof(AzureADSettings) (i.e. "AzureADSettings"). Let's continue the latter.

Similarly, the rename in AzureADDeploymentStep is a breaking change as well that we should avoid. However, for me it's not even showing up here, please check:

Also, the comments on AzureADDeploymentStep and AzureADSettingsStep are copy-paste leftovers, please fix those too (I know you didn't do them, just housekeeping).

[hishamco]

I change the names in the deployment steps before I submit the commit, I will revert the changes

[Piedone]

This is missing:

Also, the comments on AzureADDeploymentStep and AzureADSettingsStep are copy-paste leftovers, please fix those too (I know you didn't do them, just housekeeping).

[Piedone]

This isn't fully done: #14015 (comment)

[hishamco]

@Piedone if you look at the changed files, I revert to what I did in the deployment. Also, I remember your words " Let's continue the latter." for that I didn't touch anything there

[Piedone]

I didn't say "let's continue later" :).

This is a breaking change, so we shouldn't do it (and it also needs the same rename in the step). However, I'm not sure the original implementation actually worked, because the name here is "AzureAD". while in AzureADSettingsStep it's nameof(AzureADSettings) (i.e. "AzureADSettings"). Let's continue the latter.

"Latter" -> "AzureADSettings". Let's continue with that name. It's "AzureAD" now.

[hishamco]

I thought it was a typo :) I will change it now and we are ready to merge then

[hishamco]

Maybe we need to do some proper renaming in the upcoming major release

[jtkech]

@Piedone @hishamco

Yes, if in the ...Source.cs and ...Step the name was not the same, it was not working. But as it is done it still doesn't work, in the ...Source we set the settings in a property (whatever the name), but in the ...Step. we resolve the settings from the root without using this property.

---

Yes I know it's not easy to try everything, but here easy to simulate in a few lines what json data a source can provide and how a step can retrieve them, this is what I did.

So, the first solution is to change AzureADSettingsStepModel from

    public class AzureADSettingsStepModel
    {
        public string DisplayName { get; set; }
        public string AppId { get; set; }
        ...
    }

To

    public class AzureADSettingsStepModel
    {
        public AzureADSettings AzureADSettings { get; set; }
    }

And then in AzureADSettingsStep.cs use this property

    var model = context.Step.ToObject<AzureADSettingsStepModel>()
        .AzureADSettings;

---

The 2nd solution is to only change in AzureADDeploymentSource.cs, this

    result.Steps.Add(new JObject(
        new JProperty("name", "AzureADSettings"),
        new JProperty("AzureADSettings", JObject.FromObject(settings))
    ));

To that, so that we will be able to retrieve the settings from the root of the JObject.

    var step = new JObject(
        new JProperty("name", "AzureADSettings")
    );

    step.Merge(JObject.FromObject(settings));

    result.Steps.Add(step);

---

The first solution is what we use most of the time, and I saw that the 2nd solution is already used in OpenIdServerDeploymentSource.

---

Looks like there are the same kind of issues in the other authentication modules, including OpenId. For OpenId I saw one ...Source using the JObject.Merge() and the right name so that's okay, but another ...Source using a bad name and a child property while the related ...Step still resolves from the root, so this last one has the exact same issues we have here.

I think they have been done at the same time by copying from a ...Step not using a child property but then by copying from a ...Source using a child property.

[Piedone]

Please look into fixing this for this module, Hisham, and I've opened an issue for the rest: #14051

[hishamco]

Shall we use JT workaround for now to fix both?

[Piedone]

Yep.

[hishamco]

I go with the second suggestion, anything else or shall I go to fix the related issue?

[Piedone]

I don't think we need anything else here.

[jtkech]

Yes, here it's better to use the 2nd solution, meaning that the settings properties are at the same level than the step name property, because less changes to do, and because this is how it is documented, see https://docs.orchardcore.net/en/latest/docs/reference/modules/Microsoft.Authentication/#recipe-step

[hishamco]

I don't think we need anything else here.

So, let us approve and merge

[Piedone]

This is missing:

Also, the comments on AzureADDeploymentStep and AzureADSettingsStep are copy-paste leftovers, please fix those too (I know you didn't do them, just housekeeping).

[hishamco]

Also, the comments on AzureADDeploymentStep and AzureADSettingsStep are copy-paste leftovers, please fix those too (I know you didn't do them, just housekeeping).

Which comments?

[Piedone]

You'll see immediately if you open the files ;).

[hishamco]

I just opened the files and closed them so quickly :)

What about the comment here?

[hishamco]

You'll see immediately if you open the files ;).

Seems you refer to the docs :)

[hishamco]

Maybe I need to revise all the deployments after, seems @agriffard did a lot of copy paste here :)

[Piedone]

Seems you refer to the docs :)

The docs on the classes. As I mentioned, the comments, i.e. docs, yes, are outdated there. But I'm not talking about Markdown docs.

[hishamco]

The PR for weird functional test issue, I re-run the failed test hope it pass soon