-
Notifications
You must be signed in to change notification settings - Fork 2
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
Conversation
* This client instance ensures that all operations are restricted | ||
* to the permissions of the user who initiated the request. | ||
*/ | ||
export const getClient = () => { |
There was a problem hiding this comment.
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?
Removed vultr server and associated DNS entries |
AsyncLocalStoreage
context for Express.User throughout the callstack
6304ef2
to
2219fa1
Compare
1c2465b
to
347cb52
Compare
347cb52
to
ea011d1
Compare
* 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 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
➕
ea011d1
to
872891b
Compare
What does this PR do?
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 theexpress-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 -userContext
at any "depth" - a good expansion of this would be error logging - we can tell which user triggered an error now by callinguserContext
within our error handling operations.userContext
object if neededTesting
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 usingsupertest
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...
$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