-
Notifications
You must be signed in to change notification settings - Fork 905
refactor(coderd): fetch owner information when authorizing workspace agent #9123
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
ae20c36
to
cee1af8
Compare
cee1af8
to
e6ce0f5
Compare
…ndOwnerByAuthToken
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.
This looks alright to me, but would love for @Emyrk to give his stamp as well.
ON | ||
group_members.user_id = users.id | ||
WHERE | ||
-- TODO: we can add more conditions here, such as: |
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.
nit: out of curiosity, why is it left for later improvement?
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.
It would change the existing behaviour; right now we just get the agent by the token without further restrictions.
ON | ||
workspace_agents.resource_id = workspace_resources.id | ||
INNER JOIN -- every user is a member of some org | ||
organization_members |
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'm curious if we should start testing users being part of 2 organizations.
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.
TBH I'd honestly defer it until we start having multiple organizations. I'm not sure if we expose multi-orgs at a higher level, or plan to?
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.
My comment was rather preventing the accumulation of technical debt, but we can leave it for now.
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.
@mtojek we have quite a bit of the 1 org assumption.
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.
System rbac calls are OP 😄
When authorizing a workspace agent, we currently perform a number of queries to
See TODO comment in
getAgentSubject()
:This PR:
GetWorkspaceAgentByAuthToken
to return the owner and associated roles, removing the need for additional queries.Fixes #8065