refactor(api): pass this.user to permission checks in REST endpoints - #41367
Conversation
|
Looks like this PR is ready to merge! 🎉 |
|
WalkthroughThis change updates API authorization calls to pass the authenticated user object ( ChangesAuthorization principal updates
Estimated code review effort: 3 (Moderate) | ~20 minutes Possibly related PRs
Suggested labels: Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
Warning Review ran into problems🔥 ProblemsErrors were encountered while retrieving linked issues. Errors (1)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## develop #41367 +/- ##
===========================================
- Coverage 68.44% 68.44% -0.01%
===========================================
Files 4092 4092
Lines 158213 158212 -1
Branches 28645 28682 +37
===========================================
- Hits 108288 108281 -7
- Misses 44890 44905 +15
+ Partials 5035 5026 -9
Flags with carried forward coverage won't be shown. Click here to find out more. 🚀 New features to boost your workflow:
|
REST endpoints already load `this.user` with getDefaultUserFields (which includes `roles`), so passing it to the permission helpers instead of `this.userId` skips an extra Users.findOneById per check on these hot paths. Swapped `hasPermissionAsync(this.userId, …)` (and the hasAll/hasAtLeastOne and Authorization.hasPermission variants) to pass `this.user` across the v1 and EE API endpoints where `this.user` is a non-nullable IUser in scope. Left as `this.userId` where the endpoint context types `this.user` as optional (e.g. im.ts view-room-administration) — the type checker gates those.
ff2daea to
1dbe136
Compare
|
/jira ARCH-1464 |
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
1 issue found across 21 files
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="apps/meteor/server/api/v1/teams.ts">
<violation number="1" location="apps/meteor/server/api/v1/teams.ts:134">
P3: Permission checks now stringify the entire authenticated user to build the roles-cache key, including unrelated settings/custom fields and service data, on every call. Normalize object inputs to `{ _id, roles }` in `hasPermissionAsync`/related wrappers so hot-path cache-key work stays bounded to permission data.</violation>
</file>
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
| const { name, type, members, room, owner } = this.bodyParams; | ||
|
|
||
| if (room?.id && !(await hasAllPermissionAsync(this.userId, ['create-team', 'edit-room'], room.id))) { | ||
| if (room?.id && !(await hasAllPermissionAsync(this.user, ['create-team', 'edit-room'], room.id))) { |
There was a problem hiding this comment.
P3: Permission checks now stringify the entire authenticated user to build the roles-cache key, including unrelated settings/custom fields and service data, on every call. Normalize object inputs to { _id, roles } in hasPermissionAsync/related wrappers so hot-path cache-key work stays bounded to permission data.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At apps/meteor/server/api/v1/teams.ts, line 134:
<comment>Permission checks now stringify the entire authenticated user to build the roles-cache key, including unrelated settings/custom fields and service data, on every call. Normalize object inputs to `{ _id, roles }` in `hasPermissionAsync`/related wrappers so hot-path cache-key work stays bounded to permission data.</comment>
<file context>
@@ -131,7 +131,7 @@ const teamsEndpoints = API.v1
const { name, type, members, room, owner } = this.bodyParams;
- if (room?.id && !(await hasAllPermissionAsync(this.userId, ['create-team', 'edit-room'], room.id))) {
+ if (room?.id && !(await hasAllPermissionAsync(this.user, ['create-team', 'edit-room'], room.id))) {
return API.v1.forbidden();
}
</file context>
There was a problem hiding this comment.
This is handled by #41371 (already merged): getRolesCached no longer stringifies the whole user — the cache key is now ${_id}/${scope} for string ids and ${_id}/${scope}/${JSON.stringify(roles)} for objects (just the roles array), and object-without-scope bypasses the cache entirely via resolveRoles. So the hot-path cache-key work is bounded to permission data.
ggazzo
added
the
stat: QA assured
dionisio-bot
Bot
added
the
stat: ready to merge
2 participants
What
REST endpoints run with
this.useralready loaded viagetDefaultUserFields()— which includesroles(apps/meteor/app/utils/server/functions/getBaseUserFields.ts). SohasPermissionAsync(this.userId, …)forces a redundantUsers.findOneByIdinside the check, when the full user is already in hand.This swaps
this.userId→this.userin the permission helpers (hasPermissionAsync/hasAllPermissionAsync/hasAtLeastOnePermissionAsyncand theAuthorization.hasPermission*variants) across the v1 + EE API endpoints — one fewer DB round-trip per permission check on these hot paths.Scope
~20 files under
server/api/v1andee/server/api(channels, teams, users, groups, rooms, chat, settings, e2e, roles, oauthapps, omnichannel/*, licenses, ldap, …).Safety
this.usercarriesrolesat runtime AND is typed non-nullableIUserin the standard authenticated endpoint context, so each swap typechecks with no cast. Where the endpoint context typesthis.useras optional (anonymous-capable route, e.g.im.tsview-room-administration), the swap is left asthis.userId— the type checker rejects the object form there, which is the safety gate. Every touched file was verified clean via diagnostics and prettier before pushing.Not touched: custom-integration webhook API (
integrations/server/api/api.ts) whosethis.userpopulation isn't the standard ApiClass; and sites whose in-scope user object is a rolesless projection.Task: ARCH-2252
Summary by CodeRabbit