Skip to content
This repository has been archived by the owner on Nov 20, 2018. It is now read-only.

Deprecate the IAuthenticationHandler property #863

Merged
merged 3 commits into from
Jun 7, 2017
Merged

Conversation

davidfowl
Copy link
Member

@davidfowl davidfowl commented Jun 7, 2017

@@ -9,6 +10,7 @@ public interface IHttpAuthenticationFeature
{
ClaimsPrincipal User { get; set; }

[Obsolete("This is obsolete and will be removed in a future version. The recommended alternative is to use Microsoft.AspNetCore.Authentication.AuthenticationHttpContextExtensions. See https://go.microsoft.com/fwlink/?linkid=845470.")]
Copy link
Member

Choose a reason for hiding this comment

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

Drop "The recommended alternative is to use Microsoft.AspNetCore.Authentication.AuthenticationHttpContextExtensions."? It's not a direct replacement for this API.

Copy link
Member

@HaoK HaoK left a comment

Choose a reason for hiding this comment

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

Looks fine, how far do we want to go down the obsolete path? For preview 2 we just deprecated the most common httpContext.Authentication. Do we want to deprecate all of the classes/properties for RTM? Its a bit redundant since there is no way for them to have used any of the other stuff without having a context.Authentication call so...

@davidfowl
Copy link
Member Author

@HaoK Just the entry points.

@Tratcher
Copy link
Member

Tratcher commented Jun 7, 2017

We should at least do AuthenticationManager. That would get any code that was accepting it as a parameter.

@HaoK
Copy link
Member

HaoK commented Jun 7, 2017

@Tratcher just the class and not all of the methods right?

@Tratcher
Copy link
Member

Tratcher commented Jun 7, 2017

At least the class. And at that point why not do the methods too?

@HaoK
Copy link
Member

HaoK commented Jun 7, 2017

How would they ever call these methods without going through context.Authentication?

@Tratcher
Copy link
Member

Tratcher commented Jun 7, 2017

public void DoAuth(AuthenticationManager manager);

Yes someone would have to call context.Authentication before calling this method, but that may not even be in the same project as a method accepting AuthenticationManager.

@HaoK
Copy link
Member

HaoK commented Jun 7, 2017

So in that usage it would be enough to have it on the class then right? They'd only have access to the methods via context.Authentication or an explicit type reference

@Tratcher
Copy link
Member

Tratcher commented Jun 7, 2017

Right, but at that point why not just do all the methods? It's just some copy-paste?

@davidfowl davidfowl merged commit 9dedc98 into dev Jun 7, 2017
@HaoK
Copy link
Member

HaoK commented Jun 7, 2017

I'm just trying to avoid the viral spread of obsolete/pragmas, if the methods don't spread pragamas sure...

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.

4 participants