-
Couldn't load subscription status.
- Fork 1.9k
fix(api-gateway): make securityContext available to extendContext
#10050
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
base: master
Are you sure you want to change the base?
Conversation
securityContext to extendContext
securityContext to extendContextsecurityContext available to extendContext
|
@igorlukanin - would you be able to assign this to the appropriate reviewers? 🙏 |
@KSDaemon perhaps? 🙏 |
|
I'd say @ovr is better here :) |
|
@KSDaemon - sorry for another ping but any chance you could help bump this? hoping to get unblocked on some SQL API usage here |
|
Hi @morford-brex, we'll try to have a look asap. Sorry for the delays, too busy... |
| } | ||
|
|
||
| public async contextByReq(req: Request, securityContext, requestId: string): Promise<ExtendedRequestContext> { | ||
| req.securityContext = { |
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.
First, I am sorry for the late reply.
@morford-brex Good catch! However, I believe it's correct to assign securityContext for req.securityContext instead of merging.
I assume that securityContext is correct for checkAuth in HTTP (see pic 1), while it's incorrect for WebSocket server and checkSqlAuth (SQL API).
I am OK with changing it once as the last mile instead of fixing it in (sql-server.ts#contextByNativeReq and SubscriptionServer.ts#processMessage), but I suggest using req.securityContext = securityContext instead of merging.
There is no need to merge it, because:
- HTTP checkAuth will receive the same object.
this.contextByReq(req, req.securityContext, getRequestIdFromRequest(req)), see pic 1 - WS request doesn't have
securityContext, it stores it separately - SQL API request doesn't have
securityContext, it stores it separately
Thanks
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.
no worries at all - thanks for the follow up!
i think that makes sense and just updated here to skip the merging and directly set it
Check List
Issue Reference this PR resolves
#10049 (comment)