Skip to content

[SignalR]: AuthorizeHelper should no-op if endpoint routing has already executed auth #9216

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

Closed
pranavkm opened this issue Apr 9, 2019 · 12 comments · Fixed by #10471
Closed
Assignees
Labels
accepted This issue has completed "acceptance" testing (including accessibility) area-signalr Includes: SignalR clients and servers bug This issue describes a behavior which is not expected - a bug.

Comments

@pranavkm
Copy link
Contributor

pranavkm commented Apr 9, 2019

Similar to the solution in #8387, we need to avoid re-running auth in SignalR if the middleware previously ran.

@pranavkm pranavkm added the area-signalr Includes: SignalR clients and servers label Apr 9, 2019
@pranavkm
Copy link
Contributor Author

pranavkm commented Apr 9, 2019

cc @rynowak \ @davidfowl \ @BrennanConroy

@analogrelay
Copy link
Contributor

analogrelay commented Apr 9, 2019

The work items here are:

  • Scoop up all attributes on the Hub and put them in the metadata (bonus: we get CorsAttribute for free!).
  • Remove our custom connection-level auth and use endpoint auth now (if endpoint routing is in play).

@davidfowl
Copy link
Member

We already did most of this work. The last piece is to skip if it already ran

@analogrelay
Copy link
Contributor

  • Scoop up all attributes on the Hub

We did this? @BrennanConroy wasn't so sure...

@davidfowl
Copy link
Member

@rynowak
Copy link
Member

rynowak commented Apr 9, 2019

I would be really careful about making things skip and be smart - rather than making sure that the app is configured correctly. The change in #8387 broke stuff in MVC because the user would configure the AuthorizeFilter manually and it would now no-op.

We landed on making it an error to have [Authorize] and not have the authorize middleware when using endpoint routing. Consider whether you can configre this behaviour based on whether or not endpoint routing is in use.

@pranavkm
Copy link
Contributor Author

pranavkm commented Apr 9, 2019

@rynowak the way I thought of doing this was to not add auth metadata to HttpConnectionDispatcherOptions as part of the extension method @davidfowl pointed. It would be analogous to how MVC would not set up a filter if you're doing endpoint routing.

@pranavkm pranavkm self-assigned this Apr 9, 2019
@rynowak
Copy link
Member

rynowak commented Apr 10, 2019

OK DOpe

@analogrelay
Copy link
Contributor

@pranavkm you assigned yourself to this, are you going to do it? If not, let me know and I'll find a victim person to fix it.

@pranavkm
Copy link
Contributor Author

Sure, I'd be happy to review the change if somebody else does the PR.

@analogrelay
Copy link
Contributor

Cool, just wanted to clarify the status of your assignment ;P. We'll take a look at this in preview 6

@analogrelay analogrelay added bug This issue describes a behavior which is not expected - a bug. cost: S labels May 7, 2019
@BrennanConroy BrennanConroy self-assigned this May 22, 2019
@BrennanConroy
Copy link
Member

Acceptance checklist (check one item)

  • We decided not to take this fix.
  • The fix is tests-only.
  • The fix contains product changes (check all items below).
    • Relevant XML documentation comments for new public APIs are present.
    • Narrative docs (docs.microsoft.com) are updated. (check one item below)
      • The change requires a new article. An issue with an outline has been filed here: [SignalR Endpoint Routing Article AspNetCore.Docs#11127]
      • The change requires a change to an existing article. A docs PR with these changes is linked here: [PR LINK]
      • The change requires no docs changes.
    • Verification has been completed. (check one item below)
      • The change is in the shared framework and was verified against the following versions
        • SDK installer: [3.0.100-preview6-012234]
        • ASP.NET Core Runtime: [3.0.0-preview6.19303.1]
        • .NET Core Runtime: [3.0.0-preview6-27730-01]
      • The change is in an OOB NuGet/NPM/JAR package and was verified against the following version of that package: [PACKAGE ID] [VERSION]

@BrennanConroy BrennanConroy added the accepted This issue has completed "acceptance" testing (including accessibility) label Jun 4, 2019
@ghost ghost locked as resolved and limited conversation to collaborators Dec 3, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
accepted This issue has completed "acceptance" testing (including accessibility) area-signalr Includes: SignalR clients and servers bug This issue describes a behavior which is not expected - a bug.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

6 participants