feat: Azure groups#41
Conversation
|
Warning Review limit reached
Next review available in: 53 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
WalkthroughAdds Azure group retrieval and membership management through Graph helpers, tRPC procedures, and development HTTP routes. It also changes latest-guide query handling and logs the fetched result. ChangesAzure group management
Guide query adjustment
Sequence Diagram(s)sequenceDiagram
participant Client
participant TRPCRouter
participant HonoServer
participant AzureGroupHelpers
participant MicrosoftGraph
Client->>TRPCRouter: Request group operation
TRPCRouter->>AzureGroupHelpers: Validate and delegate request
Client->>HonoServer: Call development test route
HonoServer->>AzureGroupHelpers: Invoke group operation
AzureGroupHelpers->>MicrosoftGraph: Fetch groups or mutate membership
MicrosoftGraph-->>AzureGroupHelpers: Return Graph response
AzureGroupHelpers-->>TRPCRouter: ParsedGroup[] or boolean
AzureGroupHelpers-->>HonoServer: Groups or operation status
🚥 Pre-merge checks | ✅ 3 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (3 passed)
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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
🧹 Nitpick comments (1)
src/routers/azure/groups.ts (1)
10-10: 🔒 Security & Privacy | 🔵 Trivial | ⚡ Quick winValidate Azure Object IDs as UUIDs to prevent path traversal.
Since
groupIdanduserIdare interpolated directly into the Microsoft Graph API URL path without sanitization, using a generic string validator allows potential path injection. Azure Object IDs are standard UUIDs, so they should be strictly validated using Zod 4's new standalonez.uuid()schema.
src/routers/azure/groups.ts#L10-L10: ReplacegroupId: z.string(), userId: z.string()withgroupId: z.uuid(), userId: z.uuid().src/routers/azure/groups.ts#L15-L15: Apply the same strict UUID validation.src/server.ts#L100-L100: ReplacegroupId: z.string(), userId: z.string()withgroupId: z.uuid(), userId: z.uuid().🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/routers/azure/groups.ts` at line 10, Replace the generic string validators for groupId and userId with strict z.uuid() validation in src/routers/azure/groups.ts at lines 10 and 15, and in src/server.ts at line 100. Apply the same UUID schema consistently to all affected input definitions.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/azure/functions/groups.ts`:
- Around line 31-35: Await the asynchronous withRetry calls in both
addGroupMember (src/azure/functions/groups.ts lines 31-35) and removeGroupMember
(src/azure/functions/groups.ts line 45), so retries complete before returning
and failures reach each method’s local catch block.
- Line 21: Update the members mapping in the group transformation to handle
g.members being absent by using optional chaining with an empty-array fallback
before calling map, while preserving the existing displayName normalization.
- Around line 13-16: Update the group-fetching flow around the Graph client
request so it follows every `@odata.nextLink` page for both groups and each
group’s expanded members, rather than returning only the initial value. Preserve
the existing Group[] result shape and selected fields while aggregating all
pages before returning.
In `@src/routers/azure/groups.ts`:
- Around line 6-18: Replace publicProcedure with protectedProcedure or the
existing admin-only wrapper for getAll, addMember, and removeMember in the Azure
group router, ensuring all group reads and membership mutations require an
authenticated session.
In `@src/routers/web/guides_matricole.ts`:
- Line 34: Remove the raw console.log call in the public procedure; do not log
complete database rows or sensitive fields. If diagnostic logging is required,
use the existing logger at debug level and include only an explicit allowlist of
non-sensitive fields.
In `@src/server.ts`:
- Line 92: Update both production-disabled route guards in src/server.ts at
lines 92-92 and 102-102 to return c.notFound() instead of c.status(500),
preserving the existing NODE_ENV checks.
---
Nitpick comments:
In `@src/routers/azure/groups.ts`:
- Line 10: Replace the generic string validators for groupId and userId with
strict z.uuid() validation in src/routers/azure/groups.ts at lines 10 and 15,
and in src/server.ts at line 100. Apply the same UUID schema consistently to all
affected input definitions.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: f9493e70-7dd7-4f18-931a-6292899d4864
📒 Files selected for processing (6)
src/azure/functions/groups.tssrc/azure/types.tssrc/routers/azure/groups.tssrc/routers/azure/index.tssrc/routers/web/guides_matricole.tssrc/server.ts
| const res: Group[] = await client | ||
| .api("/groups?$select=id,displayName,mailNickname,mailEnabled&$expand=members($select=id,displayName)") | ||
| .get() | ||
| .then((r) => r.value) |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy lift
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Description: Look for existing Graph API pagination utilities.
rg -i 'nextLink|PageIterator|paginate' src/Repository: PoliNetworkOrg/backend
Length of output: 160
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== groups.ts outline =="
ast-grep outline src/azure/functions/groups.ts --view expanded || true
echo "\n== groups.ts excerpt =="
wc -l src/azure/functions/groups.ts
sed -n '1,220p' src/azure/functions/groups.ts
echo "\n== Graph pagination searches =="
rg -n "`@odata`\.nextLink|PageIterator|paginate|nextLink" src/azure || true
echo "\n== Graph client usages =="
rg -n "\.api\\(\"/groups|graph" src/azure/functions src/azure || trueRepository: PoliNetworkOrg/backend
Length of output: 4364
Handle Graph pagination for groups and members. This only reads the first value; later /groups pages and expanded members pages from @odata.nextLink will be dropped.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/azure/functions/groups.ts` around lines 13 - 16, Update the
group-fetching flow around the Graph client request so it follows every
`@odata.nextLink` page for both groups and each group’s expanded members, rather
than returning only the initial value. Preserve the existing Group[] result
shape and selected fields while aggregating all pages before returning.
| ...g, | ||
| displayName: displayName ?? "", | ||
| mailAddress: mailEnabled ? `${mailNickname}@polinetwork.org` : null, | ||
| members: g.members.map((m) => ({ ...m, displayName: m.displayName ?? "" })), |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
Safeguard against missing members array.
If a group has no members, the Graph API may omit the members property entirely despite the $expand directive. Calling .map directly on an undefined value will crash the application. Use optional chaining with a fallback array.
🛡️ Proposed defensive fallback
- members: g.members.map((m) => ({ ...m, displayName: m.displayName ?? "" })),
+ members: g.members?.map((m) => ({ ...m, displayName: m.displayName ?? "" })) ?? [],📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| members: g.members.map((m) => ({ ...m, displayName: m.displayName ?? "" })), | |
| members: g.members?.map((m) => ({ ...m, displayName: m.displayName ?? "" })) ?? [], |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/azure/functions/groups.ts` at line 21, Update the members mapping in the
group transformation to handle g.members being absent by using optional chaining
with an empty-array fallback before calling map, while preserving the existing
displayName normalization.
| const res = withRetry(() => | ||
| client.api(`/groups/${groupId}/members/$ref`).post({ | ||
| "@odata.id": `https://graph.microsoft.com/v1.0/directoryObjects/${userId}`, | ||
| }) | ||
| ) |
There was a problem hiding this comment.
🩺 Stability & Availability | 🔴 Critical | ⚡ Quick win
Await the asynchronous withRetry wrapper.
The withRetry utility returns a Promise, but it is being called synchronously. This execution bypasses the local catch block (resulting in Unhandled Promise Rejections on failure), breaks the intended backoff behavior, and returns true prematurely before the Graph API operation finishes.
src/azure/functions/groups.ts#L31-L35: Addawaitbefore thewithRetrycall insideaddGroupMember.src/azure/functions/groups.ts#L45-L45: Addawaitbefore thewithRetrycall insideremoveGroupMember.
📍 Affects 1 file
src/azure/functions/groups.ts#L31-L35(this comment)src/azure/functions/groups.ts#L45-L45
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/azure/functions/groups.ts` around lines 31 - 35, Await the asynchronous
withRetry calls in both addGroupMember (src/azure/functions/groups.ts lines
31-35) and removeGroupMember (src/azure/functions/groups.ts line 45), so retries
complete before returning and failures reach each method’s local catch block.
| getAll: publicProcedure.query(async () => { | ||
| return await getAllGroups() | ||
| }), | ||
| addMember: publicProcedure | ||
| .input(z.object({ groupId: z.string(), userId: z.string() })) | ||
| .mutation(async ({ input }) => { | ||
| return await addGroupMember(input.groupId, input.userId) | ||
| }), | ||
| removeMember: publicProcedure | ||
| .input(z.object({ groupId: z.string(), userId: z.string() })) | ||
| .mutation(async ({ input }) => { | ||
| return await removeGroupMember(input.groupId, input.userId) | ||
| }), |
There was a problem hiding this comment.
🔒 Security & Privacy | 🔴 Critical | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Description: Find available secured TRPC procedures.
rg -o 'export const [a-zA-Z0-9_]+Procedure' src/trpc.tsRepository: PoliNetworkOrg/backend
Length of output: 221
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '\n== src/routers/azure/groups.ts ==\n'
cat -n src/routers/azure/groups.ts
printf '\n== src/trpc.ts ==\n'
cat -n src/trpc.ts
printf '\n== group helper references ==\n'
rg -n "getAllGroups|addGroupMember|removeGroupMember|protectedProcedure|publicProcedure" src -SRepository: PoliNetworkOrg/backend
Length of output: 10947
🏁 Script executed:
#!/bin/bash
set -euo pipefail
sed -n '1,220p' src/routers/azure/groups.ts
printf '\n---\n'
sed -n '1,220p' src/trpc.ts
printf '\n---\n'
rg -n "function (getAllGroups|addGroupMember|removeGroupMember)|const (getAllGroups|addGroupMember|removeGroupMember)|export .* (getAllGroups|addGroupMember|removeGroupMember)" src -SRepository: PoliNetworkOrg/backend
Length of output: 3108
Restrict Azure group routes to authenticated access. publicProcedure leaves getAll, addMember, and removeMember callable without a session, so anyone can read group data and mutate Azure AD memberships. Switch these to protectedProcedure or an admin-only wrapper.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/routers/azure/groups.ts` around lines 6 - 18, Replace publicProcedure
with protectedProcedure or the existing admin-only wrapper for getAll,
addMember, and removeMember in the Azure group router, ensuring all group reads
and membership mutations require an authenticated session.
| getLatestGuide: publicProcedure.output(guideSchema.nullable()).query(async () => { | ||
| const [latestGuide] = await DB.select().from(GUIDES_MATRICOLE).orderBy(desc(GUIDES_MATRICOLE.date)).limit(1) | ||
| const res = await DB.select().from(GUIDES_MATRICOLE).orderBy(desc(GUIDES_MATRICOLE.date)).limit(1) | ||
| console.log(res, res[0]) |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
Remove raw database-row logging from the public procedure.
console.log(res, res[0]) logs the complete database row, including fields such as createdBy, modifiedBy, and timestamps that are not exposed by guideSchema. Remove it or use the existing logger at an appropriate debug level with an explicit, non-sensitive field allowlist.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/routers/web/guides_matricole.ts` at line 34, Remove the raw console.log
call in the public procedure; do not log complete database rows or sensitive
fields. If diagnostic logging is required, use the existing logger at debug
level and include only an explicit allowlist of non-sensitive fields.
| }) | ||
|
|
||
| app.get("/test/azure-groups", async (c) => { | ||
| if (env.NODE_ENV === "production") return c.status(500) |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Return 404 instead of 500 for disabled dev routes.
Returning a 500 (Internal Server Error) for routes that are intentionally disabled in production is semantically incorrect and will falsely trigger error monitors. Use a 403 or 404 response instead.
src/server.ts#L92-L92: Replacereturn c.status(500)withreturn c.notFound().src/server.ts#L102-L102: Replacereturn c.status(500)withreturn c.notFound().
📍 Affects 1 file
src/server.ts#L92-L92(this comment)src/server.ts#L102-L102
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/server.ts` at line 92, Update both production-disabled route guards in
src/server.ts at lines 92-92 and 102-102 to return c.notFound() instead of
c.status(500), preserving the existing NODE_ENV checks.
1 participant
No description provided.