Skip to content
This repository was archived by the owner on Dec 13, 2018. It is now read-only.

ChallengeContext will be null with [Authorize] attribute #137

Merged
merged 3 commits into from
Jan 30, 2015
Merged

ChallengeContext will be null with [Authorize] attribute #137

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
e5518e6
ChallengeContext will be null with [Authorize] attribute
brentschmaltz Jan 27, 2015
e04358f
Missing resource file.
brentschmaltz Jan 27, 2015
3483842
Rollback of setting Principal on AuthenticationTicket.
brentschmaltz Jan 28, 2015
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
28 changes: 22 additions & 6 deletions src/Microsoft.AspNet.Security.OpenIdConnect/OpenidConnectAuthenticationHandler.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -91,15 +91,17 @@ protected override async Task ApplyResponseGrantAsync()
{
ProtocolMessage = openIdConnectMessage
};

await Options.Notifications.RedirectToIdentityProvider(notification);

if (!notification.HandledResponse)
{
string redirectUri = notification.ProtocolMessage.CreateLogoutRequestUrl();
if (!Uri.IsWellFormedUriString(redirectUri, UriKind.Absolute))
{
_logger.WriteWarning("The logout redirect URI is malformed: " + redirectUri);
_logger.WriteWarning("The logout redirect URI is malformed: {0}", (redirectUri ?? "null"));
}

Response.Redirect(redirectUri);
}
}
Expand All @@ -116,15 +118,30 @@ protected override void ApplyResponseChallenge()
/// <returns></returns>
protected override async Task ApplyResponseChallengeAsync()
{
if ((Response.StatusCode != 401) || (ChallengeContext == null))
if (Response.StatusCode != 401)
{
return;
}

// Active middleware should redirect on 401 even if there wasn't an explicit challenge.
if (ChallengeContext == null && Options.AuthenticationMode == AuthenticationMode.Passive)
{
return;
}

// order for redirect_uri
// 1. challenge.Properties.RedirectUri
// 2. CurrentUri
AuthenticationProperties properties = new AuthenticationProperties(ChallengeContext.Properties);
AuthenticationProperties properties;
if (ChallengeContext == null)
{
properties = new AuthenticationProperties();
}
else
{
properties = new AuthenticationProperties(ChallengeContext.Properties);
}

if (string.IsNullOrEmpty(properties.RedirectUri))
{
properties.RedirectUri = CurrentUri;
Expand Down Expand Up @@ -154,7 +171,6 @@ protected override async Task ApplyResponseChallengeAsync()
State = OpenIdConnectAuthenticationDefaults.AuthenticationPropertiesKey + "=" + Uri.EscapeDataString(Options.StateDataFormat.Protect(properties))
};

// TODO - brentschmaltz, if INonceCache is set should we even consider if ProtocolValidator is set?
if (Options.ProtocolValidator.RequireNonce)
{
openIdConnectMessage.Nonce = Options.ProtocolValidator.GenerateNonce();
Expand All @@ -179,7 +195,7 @@ protected override async Task ApplyResponseChallengeAsync()
string redirectUri = notification.ProtocolMessage.CreateAuthenticationRequestUrl();
if (!Uri.IsWellFormedUriString(redirectUri, UriKind.Absolute))
{
_logger.WriteWarning("The authenticate redirect URI is malformed: " + redirectUri);
_logger.WriteWarning("Uri.IsWellFormedUriString(redirectUri, UriKind.Absolute) returned 'false', redirectUri is: {0}", (redirectUri ?? "null"));
}

Response.Redirect(redirectUri);
Expand Down Expand Up @@ -327,7 +343,7 @@ protected override async Task<AuthenticationTicket> AuthenticateCoreAsync()
throw new InvalidOperationException("No SecurityTokenValidator found for token: " + openIdConnectMessage.IdToken);
}

ticket = new AuthenticationTicket(principal, properties, Options.AuthenticationType);
ticket = new AuthenticationTicket(principal.Identity as ClaimsIdentity, properties);
if (!string.IsNullOrWhiteSpace(openIdConnectMessage.SessionState))
{
ticket.Properties.Dictionary[OpenIdConnectSessionProperties.SessionState] = openIdConnectMessage.SessionState;
Expand Down
132 changes: 132 additions & 0 deletions src/Microsoft.AspNet.Security.OpenIdConnect/Resources.resx
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,132 @@
<?xml version="1.0" encoding="utf-8"?>
<root>
Copy link
Contributor

@Praburaj Praburaj Jan 27, 2015

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍

<!--
Microsoft ResX Schema

Version 2.0

The primary goals of this format is to allow a simple XML format
that is mostly human readable. The generation and parsing of the
various data types are done through the TypeConverter classes
associated with the data types.

Example:

... ado.net/XML headers & schema ...
<resheader name="resmimetype">text/microsoft-resx</resheader>
<resheader name="version">2.0</resheader>
<resheader name="reader">System.Resources.ResXResourceReader, System.Windows.Forms, ...</resheader>
<resheader name="writer">System.Resources.ResXResourceWriter, System.Windows.Forms, ...</resheader>
<data name="Name1"><value>this is my long string</value><comment>this is a comment</comment></data>
<data name="Color1" type="System.Drawing.Color, System.Drawing">Blue</data>
<data name="Bitmap1" mimetype="application/x-microsoft.net.object.binary.base64">
<value>[base64 mime encoded serialized .NET Framework object]</value>
</data>
<data name="Icon1" type="System.Drawing.Icon, System.Drawing" mimetype="application/x-microsoft.net.object.bytearray.base64">
<value>[base64 mime encoded string representing a byte array form of the .NET Framework object]</value>
<comment>This is a comment</comment>
</data>

There are any number of "resheader" rows that contain simple
name/value pairs.

Each data row contains a name, and value. The row also contains a
type or mimetype. Type corresponds to a .NET class that support
text/value conversion through the TypeConverter architecture.
Classes that don't support this are serialized and stored with the
mimetype set.

The mimetype is used for serialized objects, and tells the
ResXResourceReader how to depersist the object. This is currently not
extensible. For a given mimetype the value must be set accordingly:

Note - application/x-microsoft.net.object.binary.base64 is the format
that the ResXResourceWriter will generate, however the reader can
read any of the formats listed below.

mimetype: application/x-microsoft.net.object.binary.base64
value : The object must be serialized with
: System.Runtime.Serialization.Formatters.Binary.BinaryFormatter
: and then encoded with base64 encoding.

mimetype: application/x-microsoft.net.object.soap.base64
value : The object must be serialized with
: System.Runtime.Serialization.Formatters.Soap.SoapFormatter
: and then encoded with base64 encoding.

mimetype: application/x-microsoft.net.object.bytearray.base64
value : The object must be serialized into a byte array
: using a System.ComponentModel.TypeConverter
: and then encoded with base64 encoding.
-->
<xsd:schema id="root" xmlns="" xmlns:xsd="http://www.w3.org/2001/XMLSchema" xmlns:msdata="urn:schemas-microsoft-com:xml-msdata">
<xsd:import namespace="http://www.w3.org/XML/1998/namespace" />
<xsd:element name="root" msdata:IsDataSet="true">
<xsd:complexType>
<xsd:choice maxOccurs="unbounded">
<xsd:element name="metadata">
<xsd:complexType>
<xsd:sequence>
<xsd:element name="value" type="xsd:string" minOccurs="0" />
</xsd:sequence>
<xsd:attribute name="name" use="required" type="xsd:string" />
<xsd:attribute name="type" type="xsd:string" />
<xsd:attribute name="mimetype" type="xsd:string" />
<xsd:attribute ref="xml:space" />
</xsd:complexType>
</xsd:element>
<xsd:element name="assembly">
<xsd:complexType>
<xsd:attribute name="alias" type="xsd:string" />
<xsd:attribute name="name" type="xsd:string" />
</xsd:complexType>
</xsd:element>
<xsd:element name="data">
<xsd:complexType>
<xsd:sequence>
<xsd:element name="value" type="xsd:string" minOccurs="0" msdata:Ordinal="1" />
<xsd:element name="comment" type="xsd:string" minOccurs="0" msdata:Ordinal="2" />
</xsd:sequence>
<xsd:attribute name="name" type="xsd:string" use="required" msdata:Ordinal="1" />
<xsd:attribute name="type" type="xsd:string" msdata:Ordinal="3" />
<xsd:attribute name="mimetype" type="xsd:string" msdata:Ordinal="4" />
<xsd:attribute ref="xml:space" />
</xsd:complexType>
</xsd:element>
<xsd:element name="resheader">
<xsd:complexType>
<xsd:sequence>
<xsd:element name="value" type="xsd:string" minOccurs="0" msdata:Ordinal="1" />
</xsd:sequence>
<xsd:attribute name="name" type="xsd:string" use="required" />
</xsd:complexType>
</xsd:element>
</xsd:choice>
</xsd:complexType>
</xsd:element>
</xsd:schema>
<resheader name="resmimetype">
<value>text/microsoft-resx</value>
</resheader>
<resheader name="version">
<value>2.0</value>
</resheader>
<resheader name="reader">
<value>System.Resources.ResXResourceReader, System.Windows.Forms, Version=4.0.0.0, Culture=neutral, PublicKeyToken=b77a5c561934e089</value>
</resheader>
<resheader name="writer">
<value>System.Resources.ResXResourceWriter, System.Windows.Forms, Version=4.0.0.0, Culture=neutral, PublicKeyToken=b77a5c561934e089</value>
</resheader>
<data name="ArgsException_BackchallelLessThanZero" xml:space="preserve">
<value>BackchannelTimeout cannot be less or equal to TimeSpan.Zero.</value>
</data>
<data name="Exception_OpenIdConnectMessageError" xml:space="preserve">
<value>"OpenIdConnectMessage.Error was not null, indicating an error. Error: '{0}'. Error_Description (may be empty): '{1}'. Error_Uri (may be empty): '{2}'."</value>
</data>
<data name="Exception_RedirectUri_LogoutQueryString_IsNotWellFormed" xml:space="preserve">
<value>OIDC_20001: The query string for Logout is not a well formed URI. The runtime cannot redirect. Redirect uri: '{0}'.</value>
Copy link
Member

@blowdart blowdart Jan 28, 2015

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Niggle - why are we embedding error codes again? This isn't 1995 any more. If an error message is not explicit enough to make sense without having an error code then the message is no good.

Copy link
Contributor

@Eilon Eilon Jan 28, 2015

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh I assumed that was something standard. If it's just something we made up then please remove it.

</data>
<data name="Exception_ValidatorHandlerMismatch" xml:space="preserve">
<value>An ICertificateValidator cannot be specified at the same time as an HttpMessageHandler unless it is a WebRequestHandler.</value>
Copy link
Contributor

@Eilon Eilon Jan 27, 2015

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

String resources should generally be kept to localizable content only. So, type names should be passed in programmatically through string.Format(), ideally using nameof(...).

Copy link
Contributor Author

@brentschmaltz brentschmaltz Jan 28, 2015

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think are asking for ICertificateValidator to be nameof(ICertificateValidator), but I am not sure.
Is there an example of the nameof(...) I can reference?

Copy link
Contributor

@Praburaj Praburaj Jan 28, 2015

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

an example here : https://github.com/aspnet/FileSystem/blob/56c87db7deaf657bde5bb6a7f3b16034aaf3cf28/src/Microsoft.AspNet.FileProviders/EmbeddedFileProvider.cs#L201

                throw new InvalidOperationException(string.Format("{0} does not support {1}.", nameof(EmbeddedFileProvider), nameof(WriteContent)));

Copy link
Contributor

@Eilon Eilon Jan 28, 2015

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah the idea is that instead of hard-coding a member name, you can programmatically pass it in. This makes localization easier (because localizers never see the unlocalizable text), as well as ensures that if the type name / member name changes we'll get a compiler error.

Here's an example:
https://github.com/aspnet/Mvc/blob/12c2759cec61674eb00f60a66a4df73b1af0e309/src/Microsoft.AspNet.Mvc.Core/Filters/ControllerActionFilter.cs#L27-L29

That example happens to use our resource formatting helpers where we code-gen a helper method that calls string.Format() for you.

</data>
</root>
7 changes: 5 additions & 2 deletions src/Microsoft.AspNet.Security/Infrastructure/AuthenticationHandler.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -77,9 +77,12 @@ protected async Task BaseInitializeAsync(AuthenticationOptions options, HttpCont
if (BaseOptions.AuthenticationMode == AuthenticationMode.Active)
{
AuthenticationTicket ticket = await AuthenticateAsync();
if (ticket != null && ticket.Identity != null)
if (ticket != null)
{
SecurityHelper.AddUserIdentity(Context, ticket.Identity);
if ( ticket.Identity != null)
Copy link
Contributor

@Praburaj Praburaj Jan 27, 2015

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Format the file.

Copy link
Contributor Author

@brentschmaltz brentschmaltz Jan 28, 2015

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

See comment to Chris above, we will defer this change for now.

Copy link
Contributor

@Praburaj Praburaj Jan 28, 2015

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should this file be excluded from the PR?

SecurityHelper.AddUserIdentity(Context, ticket.Identity);
Copy link
Member

@Tratcher Tratcher Jan 27, 2015

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this duplication needs to be resolved inside of AuthTicket, we don't want every consumer to have to check both properties.

Copy link
Contributor Author

@brentschmaltz brentschmaltz Jan 28, 2015

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

OIDC sets the Principal property.
Since changes w.r.t. that property are still in the early development, I agree to change OIDC until we have a comprehensive fix.

Copy link
Member

@blowdart blowdart Jan 28, 2015

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yea, I just noticed this. No no no no no. You can't just slap a Principal and an Identity property to AuthTicket. This needs reverting before Beta 3 please.

Copy link
Contributor

@Praburaj Praburaj Jan 28, 2015

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@blowdart - ClaimsPrincipal was introduced in AuthenticationTicket with OIDC - 49e66f0. @blowdart mentioned in a conversation having both Principal and Identity on AuthenticationTicket is going to confuse users. So until we decide on whether it is Principal or Identity to be in AuthenticationTicket he suggested to remove the Principal property from AuthenticationTicket for beta3.

@brentschmaltz @Eilon @Tratcher

Copy link
Member

@blowdart blowdart Jan 28, 2015

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@brentschmaltz Is there a reason you needed a full blown principal?

Copy link
Contributor

@kevinchalet kevinchalet Jan 29, 2015

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@blowdart #112 (comment)

else if (ticket.Principal != null)
SecurityHelper.AddUserIdentity(Context, ticket.Principal.Identity);
}
}
}
Expand Down