Skip to content
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

Inconsistent requirement to specify auth scheme #23325

Closed
Tratcher opened this issue Jun 24, 2020 · 2 comments · Fixed by #23604
Closed

Inconsistent requirement to specify auth scheme #23325

Tratcher opened this issue Jun 24, 2020 · 2 comments · Fixed by #23604
Labels
area-mvc Includes: MVC, Actions and Controllers, Localization, CORS, most templates bug This issue describes a behavior which is not expected - a bug. Done This issue has been fixed good first issue Good for newcomers. help wanted Up for grabs. We would accept a PR to help resolve this issue

Comments

@Tratcher
Copy link
Member

MVC provides several ActionResults for interacting with the authentication infrastructure.

  • ChallengResult
  • ForbidResult
  • SignInResult
  • SignOutResult

These mirror and call similar methods on the HttpContext:
https://github.com/dotnet/aspnetcore/blob/5155e11120cf7ee2e07383225057f66512f00fde/src/Http/Authentication.Abstractions/src/AuthenticationHttpContextExtensions.cs

Problem: These results and their associated methods are inconsistent about their requirement to specify the AuthenticationScheme. E.g. ChallengeResult, ForbidResult, and SignOutResult do not require an auth scheme to be specified, but SignInResult does. The authentication scheme is optional on all of the underlying HttpContext APIs, falling back to the application defaults for each.

Proposal: AuthenticationScheme should be optional for SignInResult as well, and ControllerBase should add the appropriate overload to match.

        // Existing
        public virtual SignInResult SignIn(ClaimsPrincipal principal, AuthenticationProperties properties, string authenticationScheme);
        public virtual SignInResult SignIn(ClaimsPrincipal principal, string authenticationScheme);
        // New
        public virtual SignInResult SignIn(ClaimsPrincipal principal, AuthenticationProperties properties);
        public virtual SignInResult SignIn(ClaimsPrincipal principal);

This makes the APIs consistent and reduces the verbosity in the Actions.

Example in the wild:
https://github.com/AzureAD/microsoft-identity-web/blob/17f48c62410e08ea0d4c1fdb4f15a0713ed7f00c/src/Microsoft.Identity.Web.UI/Areas/MicrosoftIdentity/Controllers/AccountController.cs#L41-L48

@Tratcher Tratcher added bug This issue describes a behavior which is not expected - a bug. area-mvc Includes: MVC, Actions and Controllers, Localization, CORS, most templates labels Jun 24, 2020
@javiercn javiercn added good first issue Good for newcomers. help wanted Up for grabs. We would accept a PR to help resolve this issue labels Jun 25, 2020
@javiercn
Copy link
Member

@Tratcher just to be clear, you are asking to add two additional overloads, not to change the existing ones. Is that correct?

@Tratcher
Copy link
Member Author

Add two new ones, and the existing two should accept a null scheme.

@mkArtakMSFT mkArtakMSFT added this to the Backlog milestone Jun 30, 2020
@Tratcher Tratcher modified the milestones: Backlog, 5.0.0-preview8 Jul 14, 2020
@Tratcher Tratcher added the Done This issue has been fixed label Jul 14, 2020
@ghost ghost locked as resolved and limited conversation to collaborators Aug 13, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
area-mvc Includes: MVC, Actions and Controllers, Localization, CORS, most templates bug This issue describes a behavior which is not expected - a bug. Done This issue has been fixed good first issue Good for newcomers. help wanted Up for grabs. We would accept a PR to help resolve this issue
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants