New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Support POST OpenID Connect authentication/logout requests #392

Merged
merged 1 commit into from Sep 1, 2015

Conversation

Projects
None yet
6 participants
@PinpointTownes
Contributor

PinpointTownes commented Aug 7, 2015

Fixes #295.

@Eilon as promised in my last mail, here are the first bits of the new POST authentication/authorization requests support for OIDC.

/cc @Eilon @HaoK @Tratcher

@Tratcher

This comment has been minimized.

Show comment
Hide comment
@Tratcher

Tratcher Aug 13, 2015

Member

Forking the comment thread so GitHub will stop hiding it: #392 (comment)
@PinpointTownes, I've forwarded your question to our OIDC spec guy. We'll see what they have to say.

Member

Tratcher commented Aug 13, 2015

Forking the comment thread so GitHub will stop hiding it: #392 (comment)
@PinpointTownes, I've forwarded your question to our OIDC spec guy. We'll see what they have to say.

@PinpointTownes

This comment has been minimized.

Show comment
Hide comment
@PinpointTownes

PinpointTownes Aug 13, 2015

Contributor

@Tratcher thanks!

[sarcastic mode]
I hope your OIDC spec guy will come with a more useful answer than the one he had for response_mode=query because I had the weird impression I was treated like a complete noob last time 😄
[/sarcastic mode]

Contributor

PinpointTownes commented Aug 13, 2015

@Tratcher thanks!

[sarcastic mode]
I hope your OIDC spec guy will come with a more useful answer than the one he had for response_mode=query because I had the weird impression I was treated like a complete noob last time 😄
[/sarcastic mode]

Show outdated Hide outdated NuGet.Config Outdated
@PinpointTownes

This comment has been minimized.

Show comment
Hide comment
@PinpointTownes

PinpointTownes Aug 20, 2015

Contributor

@Tratcher hey. Did you hear back from your OIDC specs specialist?

Contributor

PinpointTownes commented Aug 20, 2015

@Tratcher hey. Did you hear back from your OIDC specs specialist?

@Tratcher

This comment has been minimized.

Show comment
Hide comment
@Tratcher

Tratcher Aug 20, 2015

Member

There is still some debate, but they recommended supporting it.

Member

Tratcher commented Aug 20, 2015

There is still some debate, but they recommended supporting it.

@PinpointTownes

This comment has been minimized.

Show comment
Hide comment
@PinpointTownes

PinpointTownes Aug 20, 2015

Contributor

No words about the methods the logout endpoint should support?

#392 (comment)

Contributor

PinpointTownes commented Aug 20, 2015

No words about the methods the logout endpoint should support?

#392 (comment)

@Tratcher

This comment has been minimized.

Show comment
Hide comment
@Tratcher

Tratcher Aug 20, 2015

Member

That's what I meant, they said both endpoint should support POSTs.

Member

Tratcher commented Aug 20, 2015

That's what I meant, they said both endpoint should support POSTs.

@PinpointTownes

This comment has been minimized.

Show comment
Hide comment
@PinpointTownes

PinpointTownes Aug 20, 2015

Contributor

Ah, interesting. Should we add another property to control the logout method, or should we use AuthenticationMethod for that?

Contributor

PinpointTownes commented Aug 20, 2015

Ah, interesting. Should we add another property to control the logout method, or should we use AuthenticationMethod for that?

@Tratcher

This comment has been minimized.

Show comment
Hide comment
@Tratcher

Tratcher Aug 20, 2015

Member

I'd control them both from the same option. I don't know why you'd want to separate them.

Member

Tratcher commented Aug 20, 2015

I'd control them both from the same option. I don't know why you'd want to separate them.

@PinpointTownes

This comment has been minimized.

Show comment
Hide comment
@PinpointTownes

PinpointTownes Aug 20, 2015

Contributor

Well, I guess an authentication server could support POST for the authorization endpoint (since it's mandatory 😄) but not for the end session endpoint.

Contributor

PinpointTownes commented Aug 20, 2015

Well, I guess an authentication server could support POST for the authorization endpoint (since it's mandatory 😄) but not for the end session endpoint.

@PinpointTownes

This comment has been minimized.

Show comment
Hide comment
@PinpointTownes

PinpointTownes Aug 25, 2015

Contributor

PR rebased and updated to support POST logout requests.

FYI, I'll be absent this week, but I guess this PR is not critical and can wait a bit. I'll react to your feedback next week 👍

Contributor

PinpointTownes commented Aug 25, 2015

PR rebased and updated to support POST logout requests.

FYI, I'll be absent this week, but I guess this PR is not critical and can wait a bit. I'll react to your feedback next week 👍

@PinpointTownes

This comment has been minimized.

Show comment
Hide comment
@PinpointTownes

PinpointTownes Aug 30, 2015

Contributor

I'm back! 😄

@blowdart no security concern with this PR?

Contributor

PinpointTownes commented Aug 30, 2015

I'm back! 😄

@blowdart no security concern with this PR?

@PinpointTownes PinpointTownes changed the title from Support POST OpenID Connect authentication requests to Support POST OpenID Connect authentication/logout requests Aug 30, 2015

var issuer = Options.HtmlEncoder.HtmlEncode(message.IssuerAddress);
var content = string.Format(CultureInfo.InvariantCulture, HtmlFormFormat, issuer, inputs);

This comment has been minimized.

@blowdart

blowdart Aug 31, 2015

Member

Use string interpolation rather than {0}, {1}

@blowdart

blowdart Aug 31, 2015

Member

Use string interpolation rather than {0}, {1}

This comment has been minimized.

@PinpointTownes

PinpointTownes Aug 31, 2015

Contributor

Then we'll have to de-constant-ize (I love that verb 😄) HtmlFormFormat.

See this comment for why I prefer avoiding using string interpolation on .NET 4.5.1 (though it's a great feature): #392 (comment)

@PinpointTownes

PinpointTownes Aug 31, 2015

Contributor

Then we'll have to de-constant-ize (I love that verb 😄) HtmlFormFormat.

See this comment for why I prefer avoiding using string interpolation on .NET 4.5.1 (though it's a great feature): #392 (comment)

inputs.AppendLine(input);
}
var issuer = Options.HtmlEncoder.HtmlEncode(message.IssuerAddress);

This comment has been minimized.

@blowdart

blowdart Aug 31, 2015

Member

Are we assuming IssuerAddress is correctly Url encoded first?

@blowdart

blowdart Aug 31, 2015

Member

Are we assuming IssuerAddress is correctly Url encoded first?

This comment has been minimized.

@PinpointTownes

PinpointTownes Aug 31, 2015

Contributor

Wouldn't URL-encoding it make the resulting URL unusable (since it's used as the form action)?

#392 (comment)

@PinpointTownes

PinpointTownes Aug 31, 2015

Contributor

Wouldn't URL-encoding it make the resulting URL unusable (since it's used as the form action)?

#392 (comment)

This comment has been minimized.

@blowdart

blowdart Aug 31, 2015

Member

Which is why I'm asking :) This is configured by the dev right? So they should get it tight. Hopefully.

@blowdart

blowdart Aug 31, 2015

Member

Which is why I'm asking :) This is configured by the dev right? So they should get it tight. Hopefully.

This comment has been minimized.

@PinpointTownes

PinpointTownes Aug 31, 2015

Contributor

The issuer address (i.e the authorization or the logout endpoint) is either extracted from the OIDC provider metadata or manually configured by the dev. In both cases, it's not supposed to be URL-encoded.

It can also be replaced from the RedirectToIdentityProvider notification but I'm not sure there's something we could do to avoid manual encoding.

@PinpointTownes

PinpointTownes Aug 31, 2015

Contributor

The issuer address (i.e the authorization or the logout endpoint) is either extracted from the OIDC provider metadata or manually configured by the dev. In both cases, it's not supposed to be URL-encoded.

It can also be replaced from the RedirectToIdentityProvider notification but I'm not sure there's something we could do to avoid manual encoding.

@Tratcher

This comment has been minimized.

Show comment
Hide comment
@Tratcher

Tratcher Aug 31, 2015

Member

:shipit: pending any remaining comments from @blowdart?

Member

Tratcher commented Aug 31, 2015

:shipit: pending any remaining comments from @blowdart?

@Tratcher Tratcher added this to the 1.0.0-beta8 milestone Aug 31, 2015

@Tratcher Tratcher self-assigned this Aug 31, 2015

@blowdart

This comment has been minimized.

Show comment
Hide comment
@blowdart

blowdart Sep 1, 2015

Member

Nothing from me.

Member

blowdart commented Sep 1, 2015

Nothing from me.

@Tratcher Tratcher merged commit d9b3ea2 into aspnet:dev Sep 1, 2015

0 of 2 checks passed

continuous-integration/appveyor AppVeyor build failed
Details
continuous-integration/travis-ci/pr The Travis CI build failed
Details
@@ -30,6 +32,22 @@ public class OpenIdConnectAuthenticationHandler : AuthenticationHandler<OpenIdCo
{
private const string NonceProperty = "N";
private const string UriSchemeDelimiter = "://";
private const string InputTagFormat = @"<input type=""hidden"" name=""{0}"" value=""{1}"" />";

This comment has been minimized.

@Eilon

Eilon Sep 1, 2015

Member

@blowdart please make sure you review these codez for HTML injection / CSRF / etc.

@Eilon

Eilon Sep 1, 2015

Member

@blowdart please make sure you review these codez for HTML injection / CSRF / etc.

This comment has been minimized.

@blowdart

blowdart Sep 1, 2015

Member

I did, it's used further down at line 132, with encoding happening in the previous 2 lines.

@blowdart

blowdart Sep 1, 2015

Member

I did, it's used further down at line 132, with encoding happening in the previous 2 lines.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment