Enable local dev OIDC

[los93sol]

This is a continuation of #6469 I needed to move the fork over to a branch to work on some other things and inadvertently closed out the original PR

Microsoft Reviewers: Open in CodeFlow

[halter73]

Is this relying on the default options binding in OpenIdConnectConfigureOptions? If so, would it make more sense to forward the entire "Authentication:Schemes:OpenIdConnect" IConfigurationSection via environment variables instead of just what we remember to include in "OpenIdConnectSettings"?

Also, would it make sense to make the scheme name and/or configuration section prefix configurable?

[los93sol]

Originally it was, but earlier feedback was to switch to POCO to avoid the dependency on it. If there was a way to copy that entire section in then I guess you don't need the reference and could just copy that section in directly, but since there's currently no way to bring the whole section in I think POCO is a reasonable approach for now that doesn't really bind you in any way long term.

[halter73]

@davidfowl Can you explain your earlier feedback? I don't like that it's using the same default configuration section OpenIdConnectConfigureOptions does but only supports a subset of the options supported by that. If the goal is just to remove Microsoft.AspNetCore.Authentication.OpenIdConnect PackageReference, I would just forward the entire "Authentication:Schemes:OpenIdConnect" IConfigurationSection via environment variables.

[halter73]

I agree with @JamesNK comment on the earlier PR that these two related sections seem a bit too disconnected. Just because this is the section normal web apps use to configure OpenIdConnect doesn't mean that we have to do that for the dashboard, right? Could we include everything under "Dashboard:Frontend:OpenIdConnect"? We could forward it to "Authentication:Schemes:OpenIdConnect" in the orchestrated apps.

Another option might be to add the NameClaimType/UsernameClaimType/RequiredClaimType/RequiredClaimValue logic from aspire to ASP.NET Core's OIDC config binding logic and include everything under "Authentication:Schemes:OpenIdConnect" except for the AuthMode=OpenIdConnect part.

[los93sol]

Is it really just the configuration keys you think need to be changed over?

Currently the appsettings config would look something like this...

"Dashboard": {
  "Frontend": {
    "AuthMode": "OpenIdConnect",
    "OpenIdConnect": {
      "RequiredClaimType": "",
      "RequiredClaimValue": ""
    }
  }
},
"Authentication": {
  "Schemes": {
    "OpenIdConnect": {
      "Scope": [ "openid", "profile" ],
      "MetadataAddress": "",
      "ClientId": "",
      "ClientSecret": ""
    }
  }
}

I'm guessing you'd rather see something like....

"Dashboard": {
  "AuthMode": "OpenIdConnect",
  "OpenIdConnect": {
    "Policy": {
      "RequiredClaimType": "",
      "RequiredClaimValue": ""
    },
    "Settings": {
      "Scope": [ "openid", "profile" ],
      "MetadataAddress": "",
      "ClientId": "",
      "ClientSecret": ""
    }
  }
}

[los93sol]

@halter73 Let me know if I’m interpreting this correctly and I will make the changes

[halter73]

Sorry for the delay. I just got back from vacation. I don't have much background on this PR, I only dropped a review because @eerhardt requested it. I have a deeper understanding of the inner workings of the OpenIdConnectHandler than the Aspire dashboard.

Is the intention of this PR to only enable/configure OIDC for the dashboard? Or can this configuration also be used by the projects orchestrated by aspire?

If it's the former, I would like to see something like your second example. It makes it much more clear the configuration only applies to the dashboard. The "AuthMode" might not even be necessary since the presence of the "Dashboard:OpenIdConnect" configuration section ought to be enough to indicate that you want to use it.

However, if the idea is to be able to share OIDC configuration between the dashboard and other projects, it does make more sense to have it split out the way you currently have it. But then, I don't see what things like NameClaimType and UsernameClaimType would be specific to the dashboard. Wouldn't you want to share that between all projects?

I can see why RequiredClaimType and RequiredClaimValue might be specific to the dashboard, but I'm not sure that would belong in an "OpenIdConnect" subsection. What if we added support for another authentication mode later that also provides claims? Would it be more future proof to put RequiredClaimType and RequiredClaimValue in a "Dashboard:Frontend:AuthorizationPolicy" section to avoid duplication?

[los93sol]

This is just for the dashboard, that’s why I thought it made sense to put it in the normal location you’d use for any app instead of encapsulating it. If there was a way to forward the entire section to the context then I think we can avoid taking a dependency on the OpenIdConnect package and it should support every option in whatever version of the package the user is consuming. I tried to sort that out but came up empty. It seemed like it would be there already since you can take config and bind it, but I don’t see a clear path to just copying the config section since the config builder in the context doesn’t even exist at that point

[halter73]

If there was a way to forward the entire section to the context then I think we can avoid taking a dependency on the OpenIdConnect package and it should support every option in whatever version of the package the user is consuming.

It would be nice if there was a one-liner like Bind that supported Dictonary<string, object> and created nested dictionaries when necessary, but that's not currently supported. Plus, that's not exactly what we need here since it wouldn't create UNDERSCORE__DELIMITED__SECTION__NAMES for the keys.

It doesn't seem like it would be too hard to write custom code that calls IConfguration.GetChildren() and builds up the dictionary with the right keys though.

[los93sol]

Yes! That’s exactly what I was trying originally but the only way I could think of to do it was feed a root like authentication__ and go grab all the env variables that start with that and bring them over directly. Doesn’t seem like a very good solution, but it might be doable

[los93sol]

@danmoseley You mentioned you would welcome more PR’s and I’m ready to deliver. What do I need to do to get some traction on this one?

[danmoseley]

Looks like we need guidance from @halter73

[los93sol]

@halter73 Please clarify what changes need to happen for this to be merged.