scimmyjs / scimmy-routers

SCIMMY Express Routers
MIT License
13 stars 4 forks source link

Adding context to Incoming Requests #16

Closed always-akshat closed 7 months ago

always-akshat commented 8 months ago

This is more like a feature request.

Context

Currently, the auth middleware only allows returning a string that can be used to with /Me endpoint. I would like to set arbitrary data that can be used across the request lifecycle.

Use Case

My API uses an API key authentication and verifies and returns user information based on the API key.

Flow

This information is an object with information that can be used later

{
id: string,  
accessTypes: [string]
....
}

Since we don't pass the actual request to SimmyRouter handlers, this information is not accessible anywhere in the request lifecycle.

always-akshat commented 8 months ago

@sleelin, I can look at submitting a PR if you can point me to the right place to fix this.

sleelin commented 8 months ago

@always-akshat could you provide details on any other use cases you can think of for this? Specifically, where else you might want to be using the authentication data?

Presently the /Me endpoint expects the authentication handler to return a string as it then treats the request as if it were a GET request for the user resource of the authenticated user. This behaviour defined by the SCIM protocol itself (RFC7644§3.11). Albeit a bit scarce on details, it doesn't seem to suggest the /Me endpoint can return anything other than a fully-formed user resource representing the the currently authenticated subject.

See also https://github.com/scimmyjs/scimmy/issues/6#issuecomment-1684301780 as it may provide an alternative solution to your use case?

Thanks, Sam

always-akshat commented 8 months ago

Thanks for the reply @sleelin. My use case is exactly the one mentioned in the issue you linked. This should allow me to use a request scoped cache to get authorization data.

I wonder if you'd like to make this a first class feature in scimmy it self. I understand that you want to make scimmy as thin as possible but it might be useful to have the ability to pass the context downstream.

edorivai commented 8 months ago

First of all, thank you for this library! :pray:

I think this request is key for many SCIM implementations. Clearest example of why this is needed is multi-tenant environments; the tenant will be inferred from the Authentication step and needs to be used to scope down listing and resource responses.

The workaround is pretty brittle, see the issues on the underlying library: image

Basically; there are many situations where the request-scoped-context breaks. And if that happens, implementations relying on this context could now mix up tenant contexts, meaning one SCIM client could access and mutate data from other tenants.

remnin commented 8 months ago

I'm having the same multi-tenant problem. After testing and researching (!#¤%/"¤&!¤%#) other solutions , BIG and small, I found this library to be really nice....but I cant use it without being able to verifying the tenant.

sleelin commented 8 months ago

Thanks for the feedback everyone. I can see the demand is there for this feature, especially in multi-tenant environments. I'm hesitant to pass a full express request through to the ingress/egress/degress handlers as this could lead to other issues, but I have a few potential solutions that could be used to achieve the desired functionality:

Option 1: Passing req.params as an extra argument to the handlers

This would mean if the SCIMMYRouters instance was mounted at /:tenantId/scim, the handlers would be called with an argument containing an object similar to the following:

{"tenantId": "value-from-matched-route"}

[!NOTE] Requests for specific resources would also include a surplus id property on this object, and any preceding parameters also called id would be overwritten. The objects would also include any other preceding express route parameters.

Option 2: Adding a parameters method to the SCIMMYRouters config

This would be defined as a new property on the config, and called with the full express request object the same way the authentication handler is currently called. The method would be called for every route, and the returned value would be passed on to the handlers. The returned value may be of any type, as it would not be directly used by SCIMMY, so may be an arbitrary object with your desired properties, or a simple string with the tenant's ID.

[!TIP] To avoid double handling of authentication data, the value could be determined in advance by the authentication handler and stored on the request object. As the parameters handler would be called with the same request object, the method would just return the previously stored value.

Option 3: Utilising the existing authentication handler's returned value

This would involve expecting the authentication handler to return an object instead of a string, which is what is currently expected by the /Me route handler. The returned object would be required to have an id property to facilitate the ability to handle requests to the /Me route. As with option 2, the method would be called for every request to evaluate the returned value passed through to the resource handlers.

[!CAUTION] This would be a breaking change for those that currently use the /Me route handler, which presently expects an ID string value for the currently authenticated subject to be returned by the authentication handler.

I'm personally leaning towards option 2, as it would deliver the most flexibility in use case with the least confusion. Let me know which option is preferred and I'll implement it for v1.1.0.

Thanks, Sam

always-akshat commented 8 months ago

Thanks for coming up with potential solutions @sleelin.

My vote would also be on Option 2 since it doesn't touch any of the existing auth logic and allows flexibility on what can be passed on to the handlers.

remnin commented 8 months ago

Thank you for the quick response @sleelin.

My original thought where towards Option3, but I understand its complication, so over all I think your Option2 would be easy to use.

Sorry to ask, but any thoughts on timeline? As I'm in a position where I need a solution, or have to look else ware. As I do have 2 projects that could really benefit from this amazing project. And with this feature, I believe this would make it the go-to project for SCIM.

edorivai commented 8 months ago

Thanks @sleelin - I too vote for option 2. It's similar to how apollo-server handles this problem, and I think that's a good parallel for the problem in this library.

Leonscreative commented 8 months ago

Hey! We are having the same multi-tenant problem. Please it will be great to have it asap! Thank you! BTW Great job!

brassaw commented 8 months ago

I'd also vote for option 2. We're in the same multi-tenant situation as discussed above and in the process of spiking out a SCIM implementation. These libraries (scimmy and scimmmy-routers) seem like they'd be a huge time-saver with that support. Thank you!