add scheme to graph call, clean up unused method, add randomness to d… #1180
Conversation
There was a problem hiding this comment.
LGTM.
Great work.
Good idea to remove the default scheme and not use OpenIdConnectDefault.AuthenticationScheme in the test app. This is pure now.
and yes :) ... I couldn't figure out how the Graph could work without doing anything when the scheme was not the OpenIdConnectDefault.AuthenticationScheme. ... it didn't :)
| @@ -10,7 +10,7 @@ | |||
|
|
|||
| namespace mvcwebapp_graph.Controllers | |||
| { | |||
| [Authorize(AuthenticationSchemes = OpenIdConnectDefaults.AuthenticationScheme)] | |||
| [Authorize(AuthenticationSchemes = "openid2")] | |||
There was a problem hiding this comment.
Maybe use a constant in this class for the Authentication scheme, as it starts to be used a lot :) #Resolved
| { | ||
| string scheme = OpenIdConnectDefaults.AuthenticationScheme; | ||
| // scheme ??= OpenIdConnectDefaults.AuthenticationScheme; |
There was a problem hiding this comment.
Good catch. Indeed, I think we'd want to remove this line #Pending
There was a problem hiding this comment.
i think it's okay, if there is no scheme, it will use the default. we can discuss though.
| @@ -44,8 +44,8 @@ public void ConfigureServices(IServiceCollection services) | |||
| .AddDownstreamWebApi("DownstreamB2CApi", Configuration.GetSection("DownstreamB2CApi")) | |||
| .AddInMemoryTokenCaches(); | |||
|
|
|||
| services.Configure<MicrosoftIdentityOptions>(OpenIdConnectDefaults.AuthenticationScheme, | |||
| o => o.ClientSecret = ""); | |||
| //services.Configure<MicrosoftIdentityOptions>(OpenIdConnectDefaults.AuthenticationScheme, | |||
There was a problem hiding this comment.
Yes, we want to remove these lines completely :) #Resolved
There was a problem hiding this comment.
i might leave, as it's the dev app, in case we want to test manually like this.
* multiple auth schemes (#1157) * initial commit for auth schemes * update tests part 1 * add basic web app for testing * updates to dev app * more updates to dev app and downstreamapi * update from merge conflict * update app integration tests * few more updates * merge options partial idea (#1162) * merge options partial idea * fix tests * fix warnings * fix validate options * fix credscan (#1172) * fix policheck * more updates to mergedoptions (#1177) * another possiblity for merged options * fix issue w/post configure * fix tests * fix a few more tests * fix warnings * fix integration tests * update merged options * fix warnings * fix push * add scheme to graph call, clean up unused method, add randomness to d… (#1180) * add scheme to graph call, clean up unused method, add randomness to dev ap * fix warnings * PR feedback & fix sign-out * add auth scheme integration tests (#1189) * add auth scheme integration tests * few more updates * few more updates * Adding the merged options generator * Adding generated methods for merged options * Removing code that isn't necessary * Taking into account the JwtBearer options (actually the authority) into the Microsoft identity options if needed. * Adding the Merged options generator to the solution * few more updates Co-authored-by: Jean-Marc Prieur <jmprieur@microsoft.com> * fix merge conflict * clean-up, pr feedback Co-authored-by: Jean-Marc Prieur <jmprieur@microsoft.com>
…ev ap
@jmprieur i'm trying out some stuff w/the dev app, so it's okay to commit that, as i will probably make more changes once i hear back from asp.net core team on how to handle the sign-out. also, you were right on this LOL 😆 ...so we scratched it a bit too early from the list.