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

Conversation

@brentschmaltz
Copy link
Contributor

OpenIdConnect set Ticket.Principal, get identity from there.

OpenIdConnect set Ticket.Principal, get identity from there.
Copy link
Member

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

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

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

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

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

Choose a reason for hiding this comment

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

@Tratcher
Copy link
Member

@Eilon @muratg Triage for beta3. OIDC does not currently work with the Authorize attribute because the attribute only sets 401, it does not call Challenge.

Copy link
Contributor

Choose a reason for hiding this comment

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

use _logger.WriteWarning("{0}", redirectUri ?? "null") overload of logger to substitute parameters.

Copy link
Contributor

Choose a reason for hiding this comment

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

We write the same warning in ApplyResponseGrantAsync method in the same file. Just for consistency.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed both.

Copy link
Contributor

Choose a reason for hiding this comment

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

👍

Copy link
Member

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

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.

Praburaj pushed a commit that referenced this pull request Jan 30, 2015
ChallengeContext will be null with [Authorize] attribute
@Praburaj Praburaj merged commit a249cbb into aspnet:release Jan 30, 2015
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants