-
Notifications
You must be signed in to change notification settings - Fork 1.3k
[server] Introduce RequestContext #19023
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
Conversation
c3bde8b
to
e2d9077
Compare
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.
I skimmed through the code it looks reasonable. cc @svenefftinge to have a look as well
Yes, I pushed an attempt to fix it, by making |
b1546cc
to
afbc36b
Compare
17a92b9
to
04c23c3
Compare
@svenefftinge @akosyakov I addressed all the things mentioned above/we discussed. The current state should be complete now and deployed. @AlexTugarev It would be great if you could review of the AuthProviderService changes. @AlexTugarev @mustard-mh For reading the description, and getting acquainted with the changes to APIs (e.g. not using Please have a close look at the description for what this PR does and NOT does. |
@geropl Could you rebase please? 🙏 |
04c23c3
to
3624764
Compare
@akosyakov Done (again 🏃♂️ 😉 ). Build is still running. |
85b540c
to
7fb1c62
Compare
7fb1c62
to
4c03454
Compare
@geropl I've enabled debugging logging |
Interesting I never see |
Yeah, just found a bug in the redis handler. Pushing a fix... 🙄 |
Uh, I think |
@akosyakov The fixes for the above are pushed and deployed. |
components/server/src/websocket/websocket-connection-manager.ts
Outdated
Show resolved
Hide resolved
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.
I could not test analytics, the rest looks good.
Please test analytics if it is alright for all kind of calls, please unhold.
/hold
2443926
to
e8e0eee
Compare
Tested analytics and it works. ✔️ /unhold |
/unhold |
): Promise<ListAuthProviderDescriptionsResponse> { | ||
const user = context.user; | ||
const userId = ctxUserId(); |
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.
@geropl, just realized, this actually throws if unauthenticated.
Need to address it, as it breaks Login page with gRPC enabled.
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.
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.
@AlexTugarev Yes, this one throws. If we need one that does not throw, we should name it ctxTryUserId
, following the pattern in request-context.ts 👍
Description
Context:
Introduces
RequestContext
as primary method to pass request-local information between different pieces of code, incl. for authorization and request-local caching. This PR makes sure all code that calls some...Service
"request-handling" logic in the end is wrapped inside arunWithRequestContext
call:This also means that whenever we execute code with other permissions, we need to call
runWithSubjectId
(in the future). See the call sites in this PR for examples.To access the request based context, the unit exports a number of functions prefixed by
ctx...
, e.g.:ctxUserId
ctxCheckAborted
The idea for those is to be used sparingly, and almost exclusively in "leaf" code (authorizer, logging, tracing, etc.).
To pass authentication information independent of the
User
/userId
string, a newSubjectId
type is introduced. Currently there is only oneSubjectIdKind
("user"), which serializes to:user_<id>
.This is expected to be extended by a kind
apitokenv0_<id>
in subsequent PRs.Follow-up work
RequestContexts
everywherectx.subjectId
. Not passing that will be an "access denied" error.userId: string
that we are currently passing into...Service
s and down to theAuthorizer
. For now, in order to allow to split up work, and not interfere too much with other API-related work, we still pass that argument in the new top-level code (gRPC servicve implementations) usingctxUserId
.I will send out additional information about the future steps summarized here, and the expected changes to our code (patterns) today.
Summary generated by Copilot
🤖 Generated by Copilot at c3bde8b
This pull request introduces a new way of identifying users and organizations in the analytics system using subject ids, which are more reliable and consistent than user ids. It also refactors the request context mechanism to use subject ids and provide more information about the requests and the users. It updates various components and files to use the new request context and subject id features.
Related Issue(s)
Fixes EXP-915
How to test
For this PR, we need to make sure we don't break existing things.
Most interesting for review:
For testing:
Documentation
Preview status
Gitpod was successfully deployed to your preview environment.
Build Options
Build
Run the build with werft instead of GHA
Run Leeway with
--dont-test
Publish
Installer
Add desired feature flags to the end of the line above, space separated
Preview Environment / Integration Tests
If enabled this will build
install/preview
If enabled this will create the environment on GCE infra
Valid options are
all
,workspace
,webapp
,ide
,jetbrains
,vscode
,ssh
. If enabled,with-preview
andwith-large-vm
will be enabled./hold