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

feat: Setup AsyncLocalStoreage context for Express.User throughout the callstack #2199

Merged
merged 4 commits into from
Sep 20, 2023

Conversation

DafyddLlyr
Copy link
Contributor

@DafyddLlyr DafyddLlyr commented Sep 3, 2023

What does this PR do?

  • Creates a context object to store the user which will persist throughout the callstack of a request/response cycle
  • Uses the Node AsyncLocalStoreage API to acheive this (docs)

Why this approach?

We need to keep user details, specifically the JWT and user ID, for a number of operations that happen in the API. The specific issue we're hoping to address in the short term here is ensuring that any requests made the the API are scoped to the permission level of the user who made the request.

We always have req.user as the express-jwt middleware sets this up for us. If we wanted to access this value in a service, or a child function of that service, we'd need to "prop drill" the request or user object all the way through. This approach can get out of hand quite quickly and can be tricky to manage.

Using AsyncLocalStorage we get the following benefits -

  • We can easily access the userContext at any "depth" - a good expansion of this would be error logging - we can tell which user triggered an error now by calling userContext within our error handling operations.
  • We can easily add or remove data from the typed userContext object if needed
  • It's "thread safe" (for lack of a better term) so we can handle multiple users with multiple permission levels making requests all at once with confidence.

Testing

One thing I've not yet hit in any of our examples so far - it's likely we'll need to mock the userContext in some tests. Right now as we're using supertest and testing mostly via the API endpoints this is all set up for us. In more service-specific tests this might be a consideration.

Next steps...

  • There's a few $admin calls still remaining - tidying these up are low priority relatively so I've just marked as @depracted for now so we don't add any more and we can tidy these up as we go?

Diagram

This encapsulates the changes made in #2198 and #2199

sequenceDiagram
    participant Origin
    participant API

    box Middleware
    participant useJWT
    participant useRoleAuth
    end

    box userContext
    participant Controller
    participant Service
    participant Child Function
    end

    Origin ->> API: Send Request
    API ->> useJWT: 
    useJWT ->> useJWT: Validate & decode JWT

    alt Unauthorized
        useJWT ->> API: Error
        API ->> Origin: 401 Unauthorized
    end

    alt Forbidden
        useJWT ->> useRoleAuth: 
        useRoleAuth ->> useRoleAuth: Validate role
        useRoleAuth ->> API: Error
        API ->> Origin: 403 Forbidden
    end

    alt Permission Granted
        useRoleAuth ->> Controller: Setup userContext
        Controller ->> Service: 
        Service ->> Child Function: 
        Child Function -> Service: 
        Service ->> Controller: 
        Controller ->> API: Success
        API ->> Origin: 200 OK
    end
Loading

* This client instance ensures that all operations are restricted
* to the permissions of the user who initiated the request.
*/
export const getClient = () => {
Copy link
Contributor Author

@DafyddLlyr DafyddLlyr Sep 3, 2023

Choose a reason for hiding this comment

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

Not thrilled on the naming here - feels a little generic and meaningless.

Something like getUserRoleClient() might be more accurate but it's pretty wordy...

No strong opinions here on naming, but if we move to a future where the API only calls a $public client (pretty clear what this is) and a permissions-scoped $client then this might actually be fine?

@github-actions
Copy link

github-actions bot commented Sep 3, 2023

Removed vultr server and associated DNS entries

@DafyddLlyr DafyddLlyr changed the title feat: Setup AsyncLocalStoreage context for Express.User throughout the callstack feat: Setup AsyncLocalStoreage context for Express.User throughout the callstack Sep 3, 2023
@DafyddLlyr DafyddLlyr requested a review from a team September 3, 2023 16:00
@DafyddLlyr DafyddLlyr marked this pull request as ready for review September 3, 2023 16:00
@DafyddLlyr DafyddLlyr force-pushed the dp/use-roles-in-api branch 2 times, most recently from 6304ef2 to 2219fa1 Compare September 11, 2023 11:17
@DafyddLlyr DafyddLlyr removed the request for review from a team September 12, 2023 08:23
Base automatically changed from dp/use-roles-in-api to main September 19, 2023 13:34
@DafyddLlyr DafyddLlyr requested a review from a team September 19, 2023 13:42
* Should only be used by trusted service-to-service calls (e.g Hasura -> API).
* Calls made by users should be directed through $public or the role-scoped getClient().
*
* Consider removing this and replacing with an "api" role using "backend-only" operations in Hasura
Copy link
Member

Choose a reason for hiding this comment

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

@DafyddLlyr DafyddLlyr merged commit c323b46 into main Sep 20, 2023
@DafyddLlyr DafyddLlyr deleted the dp/use-api-context branch September 20, 2023 20:30
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants