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

Starcounter.Authorization should not create Starcounter.Session unless required #91

Open
miyconst opened this issue Feb 28, 2019 · 14 comments

Comments

@miyconst
Copy link
Member

It turns out that Starcounter.Authorization will create new Starcounter.Session for every request, even if it's a simple REST API request.

This is very inefficient, because every REST API request would create a new Starcounter.Session which won't be used any more, but will consumer resources and eventually die with a timeout.

This session spawn leads to the following exception under high load of REST API.

"System.ArgumentOutOfRangeException: Specified argument was out of the range of valid values.
Parameter name: No free session indexes left.
   at Starcounter.Internal.SchedulerSessions.CreateNewSession(ScSessionStruct& ss, Session publicSession, UInt64 timeoutMinutes) in C:\    c_work\sc-19961\Level1\src\Starcounter.Sessions2x\SchedulerSessions.cs:line 112
   at Starcounter.Sessions2x.SessionFactory.Create(Session publicSession, ScSessionStruct& sss) in C:\    c_work\sc-19961\Level1\src\Starcounter.Sessions2x\SessionFactory.cs:line 20
   at Starcounter.Internal.XSONSessionDependencies.SessionFactoryWithSchedulers.Create(Session publicSession) in C:\    c_work\sc-19961\Level1\src\Starcounter\Apps\XSONDependencies.cs:line 93
   at Starcounter.Session..ctor(Flags flags, Nullable`1 includeNamespaces) in c:\github\Starcounter.XSON\src\Starcounter.XSON\Sessions\Session.cs:line 184
   at Starcounter.Session.Ensure() in c:\github\Starcounter.XSON\src\Starcounter.XSON\Sessions\Session.cs:line 373
   at Starcounter.Authorization.Authentication.TicketCreationStartupFilter.<>c__DisplayClass2_0.<Configure>b__1(Request request)
   at Starcounter.Handle.RunRequestFilters(Request req) in C:\    c_work\sc-19961\Level1\src\Starcounter.Internal\Handle.cs:line 555
   at Starcounter.Internal.AppsBootstrapper.ProcessExternalRequest(Request req) in C:\    c_work\sc-19961\Level1\src\Starcounter.Apps.JsonPatch\AppsBootstrapper.cs:line 345
ParamName=No free session indexes left.
HResult=-2146233086
"
@kegor
Copy link
Contributor

kegor commented Feb 28, 2019

I checked request object on this line https://github.com/Starcounter/Starcounter.Authorization/blob/3.x/Authorization/Authentication/TicketCreationStartupFilter.cs#L21 during REST-API requests mentioned on this page https://github.com/Starcounter/Blending/blob/master/docs/administration.md (as an well-known example for me) and during regular get request to one of the BlendingEditors pages.

For now i see 2 differences, in the case of REST-API we have:

  1. request.Method is PUT or DELETE.
  2. request has Authorization header with value Bearer ....

So for now as the solution i could suggest to use one of these options to filter such requests on the same line (https://github.com/Starcounter/Starcounter.Authorization/blob/3.x/Authorization/Authentication/TicketCreationStartupFilter.cs#L21) as we do for the request.IsStaticFileRequest, but i'm not sure that this is the best solution. @miyconst could you please share your opinion?

@miyconst
Copy link
Member Author

miyconst commented Mar 1, 2019

We can't filter API requests by the HTTP method. There are APIs which are GET as well.

I think, that Starcounter.Session and all sign in info (even if user is not signed in) shall only be created if requested.

Like a lazy loading, don't create it unless someone is trying to fetch it.

REST API may also use Starcounter.Session and rely on Starcounter.Authorization, but there should be a way to opt-out and don't waste resources creating Starcounter.Session when it's not needed.

@LarsTjernstrom
Copy link

LarsTjernstrom commented Mar 4, 2019

Hi @kegor.
Yes as @miyconst writes, there is no need to create a Starcounter.Session in Starcounter.Authorization when it’s rest call and not a viwmodel call.

Then the question is, what is the most simple way in Starcounter to know if the call i
just a rest call
Or a ViewModel call?

@LarsTjernstrom
Copy link

LarsTjernstrom commented Mar 4, 2019

@miyconst wrote. Rest API handler is not necessarily returns JSON, it may return nothing, just a string, xml, or even a view-model, which is going to be serialized into JSON.

@habib535
Copy link

Since its the Starcounter.Startup library that registers all the handlers from viewmodel, one way is to expose an extra flag that will set SkipRequestFilters = true there https://docs.starcounter.io/topic-guides/network/middleware#skip-request-filters

This can also have side effects though.

@LarsTjernstrom
Copy link

@habib535 in the case of Rest calls many times there will be no ViewModel...so we can't add a property that says skip me..when the viewmodel itself is not needed when doing rest calls.

@LarsTjernstrom
Copy link

Hi everyone, @kegor , @miyconst . In Cronofy I had Rest callbacks from Cronofy working before, but now they are blocked and never reach my code, Cronofy gets a 404 response.
I'm thinking of opt in vs opt out...We would like the authorization to protect our viewmodels when we have added an Authorize Attribute, if it's missing then access should be allowed = then it will work for Rest calls

@kegor
Copy link
Contributor

kegor commented Mar 25, 2019

Hi guys, on this line https://github.com/Starcounter/Starcounter.Authorization/blob/3.x/Authorization/Middleware/SecurityMiddleware.cs#L33 method CheckClass returns true (https://github.com/Starcounter/Starcounter.Authorization/blob/3.x/Authorization/PageSecurity/CheckersCreator.cs#L164) if there is no Authorize attribute found on the Type pageType, and therefore this middleware just returns next() in this case, i could re-test this part if you assume that this might work wrong.

@miyconst
Copy link
Member Author

@LarsTjernstrom could you debug Starcounter.Authorization and identify the cause of the issue?

@habib535
Copy link

@miyconst @LarsTjernstrom
If the goal of this issue to discuss how to skip authorization for particular requests then how #91 (comment) is not solving this issue? Its a middleware in the Authorization library that creates the session and go through the authorization code on every request. https://github.com/Starcounter/Starcounter.Authorization/blob/3.x/Authorization/Authentication/TicketCreationStartupFilter.cs#L19

If the request is not from a viewmodel (Startup library) but our own Handler.GET|POST etc then we can add the SkipRequestFilters = true ourselves.

@kegor
Please have a look at the habib branch which has a fix for the same 59d0f7f

@LarsTjernstrom
Copy link

@miyconst The root cause was Not Starcounter.Authorization, so we can delete those comments.
The root cause was that the customer wanted to be able to change the callback Url:s, so these could be edited from settings...but the app has to be restarted Or have code that does a new url registration. I have confirmed this with tests and by asking @alemoi.
E.g. if you have a variable in a Handle.POST(MyUrlVariable) a change of the variable will have no effect until the app is restarted Or you write code to specifically register it with the new url.

@miyconst
Copy link
Member Author

E.g. if you have a variable in a Handle.POST(MyUrlVariable) a change of the variable will have no effect until the app is restarted Or you write code to specifically register it with the new URL.

@LarsTjernstrom you can always register a dynamic URL, for example:

Handle.GET("/app/callback/{?}", (string uri) => 
{
    if (uri != this.GetCustomerCallbackUri())
    {
        return 404;
    }
    
    // Process callback.
    
    return 200;
});

@kegor
Copy link
Contributor

kegor commented Apr 17, 2019

Hi @miyconst, @mtornwall, @habib535 could you please share your vision on the things that are left here to do? I'm not sure on 100% that i have clear understanding.

@kegor kegor added the on hold label Apr 18, 2019
@kegor
Copy link
Contributor

kegor commented Apr 18, 2019

By the information from Konstantin:

Basically, what we want, is to exclude auth request filter for some of the handlers.
Which is currently impossible.

So this is on hold for now.

@kegor kegor removed their assignment Apr 18, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

5 participants