Adding code only flow for sign in. #258

tushargupta51 · 2015-05-14T15:37:06Z

Also changes for integrating wilson logging to OIDC middleware.

kevinchalet · 2015-05-14T15:40:28Z

src/Microsoft.AspNet.Authentication.OpenIdConnect/OpenIdConnectAuthenticationHandler.cs

@@ -520,6 +540,14 @@ protected override AuthenticationTicket AuthenticateCore()
            }
        }

+        protected virtual IdentityModel.Clients.ActiveDirectory.AuthenticationResult RedeemAuthorizationCode(string authorizationCode)


Oh please no, no dependency on AAD stuff... specially for such a simple thing.
Why not simply using a good old HttpClient?

https://github.com/aspnet-contrib/AspNet.Security.OpenIdConnect.Server/blob/vNext/samples/Mvc/Mvc.Client/Startup.cs#L76-L103

@PinpointTownes We want to use ADAL for redeeming code for tokens instead of doing a direct httpClient call. That is why we have adal for 😄 We have plans to extend this to authorization servers beyond AAD but that will be the second step.

Then rename this middleware to ActiveDirectoryAuthenticationMiddleware and let the community offer a real provider-agnostic OpenID Connect middleware 😄

Out of curiosity, what's the advantage of using ADAL here?

Caching. That's one major advantage of using ADAL. I understand we need to be provider agnostic and that is the plan we have for GA. Adding @vibronet if he has anything to add here.

God no, not this mess. You can't even imagine how many people lost their hair on the #owin channel because of token caching 😄

And why would you use caching here? New code, new token. Absolutely no need for caching.

@vibronet hey, it's been a long time... 😄

kevinchalet · 2015-05-14T15:43:57Z

I have absolutely no idea what you're doing with the git history, but it's a real mess... you should really use fast forward merging or proper rebasing 😄

kevinchalet · 2015-05-14T15:45:30Z

src/Microsoft.AspNet.Authentication.OpenIdConnect/OpenIdConnectAuthenticationHandler.cs

+        {
+            AuthenticationContext authContext = new AuthenticationContext(Options.Authority, false);
+            ClientCredential credential = new ClientCredential(Options.ClientId, Options.ClientSecret);
+            var tokens = authContext.AcquireTokenByAuthorizationCodeAsync(authorizationCode, new Uri(Options.RedirectUri), credential).GetAwaiter().GetResult();


What? Why using a blocking .GetAwaiter().GetResult() when you can make the method fully async?

And BTW, given that Options.RedirectUri is not a mandatory setting (thought it should probably be, given that redirect_uri is required for OIDC authorization requests), Uri's constructor will throw an exception if you don't set it.

Nice catch. Thanks!

Same remark for Options.Authority. Can AuthenticationContext live with it if it's null?

tushargupta51 · 2015-05-14T17:19:32Z

@PinpointTownes Regarding the git history, are you referring to the merges because all other commits are relevant to the logging and code only flow changes?

kevinchalet · 2015-05-14T17:29:42Z

@tushargupta51 sure, my remark was only about the merge commits.

I'd like to play with the bits offered by this PR but the static ADAL version you specified doesn't seem to exist. Could you please upload it on the MyGet nightly build repository?

tushargupta51 · 2015-05-14T17:53:06Z

@PinpointTownes It is there on MyGet. Here: https://www.myget.org/gallery/azureadwebstacknightly

kevinchalet · 2015-05-14T18:03:25Z

Hum, I see, it failed because the Azure AD has been removed from NuGet.Config when the JWT security token handler bits have been published to aspnetvnext.

You should add it again if you plan to take a dependency on ADAL, or upload it to the aspnetvnext repository.

(BTW, that's why the build failed on AppVeyor: https://ci.appveyor.com/project/aspnetci/security/build/1.0.107)

vibronet · 2015-05-15T17:54:55Z

for the pure sign in flow, the plan is to move to HttpClient. When we'll look after improving the access token acquisition experience we will be using ADAL - mostly for its caching benefits.

kevinchalet · 2015-05-15T18:01:00Z

@vibronet I still don't understand why you would integrate ADAL directly in the OIDC client middleware instead of simply using the notifications hooks to store the access tokens exactly where you want to.

Tratcher · 2015-05-15T18:13:47Z

Agreed, OIDC should not take a direct AAD dependency, even temporarily.

vibronet · 2015-05-15T19:50:15Z

let me restate - for the sign in flow we won't have a dependency on ADAL. For the access token flow the notifications route is already available today, but requires far too much code and is prone to mistakes. We need a more integrated solution. I am not saying that such solution will show up in the OIDC middleware, though it is a possibility given that the code request and redemption machinery is largely the same

vibronet · 2015-05-15T19:59:13Z

also note, "store the access token" is a very simplistic way of describing what happens. It's not enough to provide a storage, you need a model to come with it. Token caching is a combination of caching access token, refresh tokens, and context (source, client app, resource, user and many others). When at a later time a new token is requested, there is very sophisticated logic that infers whether there is an access token and/or a refresh token and/or a multi resource refresh token and so on. All those rules can get very complicated and have a strong dependency on the provider. Generic classes that work across the board can't ingest this level of sophistication and surface the details to the developer. Conversely, ADAL's assumptions allow it to take care of all that logic without bothering the developer. Once again, I am not saying that this will end up in the OIDC middleware. But when it comes to clients and token management strategies, there is no such thing as a provider-agnostic approach AND a viable programming model. I am just back from BUILD and Ignite, during which I heard over and over again from customers how useless they find generic purpose libraries for OAuth2 (again, not OIDC).

kevinchalet · 2015-05-21T18:13:19Z

src/Microsoft.AspNet.Authentication.OpenIdConnect/project.json

        "Microsoft.Framework.NotNullAttribute.Sources": { "type": "build", "version": "1.0.0-*" },
-        "Microsoft.IdentityModel.Protocol.Extensions": "2.0.0-beta4-*"
+        "Microsoft.AspNet.Authentication.OAuth": "1.0.0-*"


Why taking a dependency on Microsoft.AspNet.Authentication.OAuth?

IMHO, you should use the super-cool OpenIdConnectMessage primitive instead of the more limited TokenResponse we created for the OAuth2 client middleware.

@PinpointTownes @tushargupta51 I think it helps in crafting the message url and avoiding missssspellings, since the properties like 'ClientId' avoid that category of error.

…pe value

…eanup

…point and some cleanup

kevinchalet · 2015-06-16T19:01:20Z

src/Microsoft.AspNet.Authentication.OpenIdConnect/OpenIdConnectAuthenticationHandler.cs

                if (properties == null)
                {
                    Logger.LogError(Resources.OIDCH_0005_MessageStateIsInvalid);
                    return null;
                }

+                var isCodeOnlyFlow = (properties.Items.ContainsKey(OpenIdConnectParameterNames.ResponseType) ? 


I'm still not sure it's the most appropriate way to determine whether an OIDC callback really corresponds to a "code only" flow: nothing prevents a server from ignoring your response_type and returning an id_token or a token.

IMHO, making sure there's no id_token or token in the message would be far more robust: https://github.com/aspnet/Security/pull/279/files#diff-5ba88763cd4e76f8ecfcccff1ca9984aR266

I do check for a null id_token in the incoming message along with this boolean before the code is used to redeem tokens. This boolean is an indication to take an action when a null id_token is received. If this boolean is not set and the id_token is null, we go directly to processing the code.

… minor changes.

brentschmaltz · 2015-06-23T21:50:49Z

src/Microsoft.AspNet.Authentication.OpenIdConnect/OpenIdConnectAuthenticationHandler.cs

+            var openIdMessage = new OpenIdConnectMessage()
+            {
+                ClientId = Options.ClientId,
+                ClientSecret = Options.ClientSecret,


We can start with ClientSecret, but we will need an issue for: Certs and SignedJwt (AssertionCredentials).

dnfclas · 2015-06-29T22:59:40Z

@tushargupta51, Thanks for signing the contribution license agreement so quickly! Actual humans will now validate the agreement and then evaluate the PR.

Thanks, DNFBOT;

tushargupta51 · 2015-07-02T21:11:42Z

Closing this, will send another PR later.

dnfclas added the cla-required label May 14, 2015

kevinchalet reviewed May 14, 2015
View reviewed changes

kevinchalet reviewed May 21, 2015
View reviewed changes

tushargupta51 added 8 commits June 14, 2015 15:51

Correcting the constant name

8e5c49a

replacing string with constant variable for code only flow reponse ty…

9c94b49

…pe value

Adding .designer.cs file

d430b5d

Removing adal dependency and addressing some github comments

c3b28a9

retrieving claims from userinfo endpoint

c6f45ba

removing changes to tokenresponse and cleanup

081ee8f

Addressing github comments - removing system dependencies and code cl…

a34370a

…eanup

Taking off the wilson logging adapter, adding checks for userinfo end…

334e66f

…point and some cleanup

tushargupta51 force-pushed the signinCodeFlow branch from 7a67300 to 334e66f Compare June 14, 2015 23:07

tushargupta51 added 2 commits June 14, 2015 16:10

Removing the .vs folder that was pushed by mistake

d350329

Cleanup

abd67c1

kevinchalet reviewed Jun 16, 2015
View reviewed changes

updating the redirect uri, adding test for getuserInfo and some other…

37208ce

… minor changes.

brentschmaltz reviewed Jun 23, 2015
View reviewed changes

dnfclas added cla-signed and removed cla-required labels Jun 29, 2015

brentschmaltz mentioned this pull request Jul 1, 2015

Incorporate community feedback from https://github.com/aspnet/Security/pull/112 #276

Merged

tushargupta51 closed this Jul 2, 2015

kevinchalet mentioned this pull request Jul 2, 2015

Integrated logs for aspnet and wilson #331

Closed

kevinchalet mentioned this pull request Dec 16, 2015

Add OIDC remote sign-out to the sample. #613

Closed

kevinchalet mentioned this pull request Nov 12, 2016

Provide an easy way to disable telemetry in the OpenID Connect middleware #1035

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Adding code only flow for sign in. #258

Adding code only flow for sign in. #258

tushargupta51 commented May 14, 2015

kevinchalet May 14, 2015

tushargupta51 May 14, 2015

kevinchalet May 14, 2015

tushargupta51 May 14, 2015

kevinchalet May 14, 2015

kevinchalet commented May 14, 2015

kevinchalet May 14, 2015

HaoK May 14, 2015

kevinchalet May 14, 2015

tushargupta51 May 14, 2015

kevinchalet May 14, 2015

tushargupta51 commented May 14, 2015

kevinchalet commented May 14, 2015

tushargupta51 commented May 14, 2015

kevinchalet commented May 14, 2015

vibronet commented May 15, 2015

kevinchalet commented May 15, 2015

Tratcher commented May 15, 2015

vibronet commented May 15, 2015

vibronet commented May 15, 2015

kevinchalet May 21, 2015

brentschmaltz May 21, 2015

kevinchalet Jun 16, 2015

tushargupta51 Jun 16, 2015

brentschmaltz Jun 23, 2015

dnfclas commented Jun 29, 2015

tushargupta51 commented Jul 2, 2015

Adding code only flow for sign in. #258

Adding code only flow for sign in. #258

Conversation

tushargupta51 commented May 14, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kevinchalet commented May 14, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tushargupta51 commented May 14, 2015

kevinchalet commented May 14, 2015

tushargupta51 commented May 14, 2015

kevinchalet commented May 14, 2015

vibronet commented May 15, 2015

kevinchalet commented May 15, 2015

Tratcher commented May 15, 2015

vibronet commented May 15, 2015

vibronet commented May 15, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dnfclas commented Jun 29, 2015

tushargupta51 commented Jul 2, 2015