Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion package.json
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
{
"name": "backend",
"version": "0.16.4",
"version": "0.17.0",
"description": "PoliNetwork backend server",
"private": true,
"keywords": [],
Expand Down
2 changes: 1 addition & 1 deletion package/package.json
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
{
"name": "@polinetwork/backend",
"version": "0.16.4",
"version": "0.17.0",
"description": "Utils to interact with the backend.",
"repository": {
"type": "git",
Expand Down
52 changes: 52 additions & 0 deletions src/azure/functions/groups.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,52 @@
import type { Group as TGroup, User as TUser } from "@microsoft/microsoft-graph-types"
import { logger } from "@/logger"
import { withRetry } from "@/utils/wait"
import { client } from "../client"
import type { ParsedGroup } from "../types"

export type Group = Pick<Required<TGroup>, "id" | "displayName" | "mailNickname" | "mailEnabled"> & {
members: Array<Pick<Required<TUser>, "id" | "displayName">>
}

export async function getAllGroups(): Promise<ParsedGroup[]> {
try {
const res: Group[] = await client
.api("/groups?$select=id,displayName,mailNickname,mailEnabled&$expand=members($select=id,displayName)")
.get()
.then((r) => r.value)
Comment on lines +13 to +16

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🗄️ 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 || true

Repository: 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.

return res.map(({ mailNickname, mailEnabled, displayName, ...g }) => ({
...g,
displayName: displayName ?? "",
mailAddress: mailEnabled ? `${mailNickname}@polinetwork.org` : null,
members: g.members.map((m) => ({ ...m, displayName: m.displayName ?? "" })),

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🩺 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.

Suggested change
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.

}))
} catch (error) {
logger.error({ error }, "[MS Graph API] Error in getAllGroups call")
return []
}
}

export async function addGroupMember(groupId: string, userId: string): Promise<boolean> {
try {
const res = withRetry(() =>
client.api(`/groups/${groupId}/members/$ref`).post({
"@odata.id": `https://graph.microsoft.com/v1.0/directoryObjects/${userId}`,
})
)
Comment on lines +31 to +35

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🩺 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: Add await before the withRetry call inside addGroupMember.
  • src/azure/functions/groups.ts#L45-L45: Add await before the withRetry call inside removeGroupMember.
📍 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.

logger.debug({ res, userId, groupId }, "[MS Graph API] OK addGroupMember call")
return true
} catch (error) {
logger.error({ error, userId, groupId }, "[MS Graph API] Error in addGroupMember call")
return false
}
}
export async function removeGroupMember(groupId: string, userId: string): Promise<boolean> {
try {
withRetry(() => client.api(`/groups/${groupId}/members/${userId}/$ref`).delete())
logger.debug({ userId, groupId }, "[MS Graph API] OK removeGroupMember call")
return true
} catch (error) {
logger.error({ error, userId, groupId }, "[MS Graph API] Error in removeGroupMember call")
return false
}
}
10 changes: 10 additions & 0 deletions src/azure/types.ts
Original file line number Diff line number Diff line change
Expand Up @@ -32,3 +32,13 @@ export type ParsedUser = {
isMember: boolean
assignedLicensesIds: string[]
}

export type ParsedGroup = {
id: string
displayName: string
mailAddress: string | null
members: Array<{
id: string
displayName: string
}>
}
19 changes: 19 additions & 0 deletions src/routers/azure/groups.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,19 @@
import { z } from "zod"
import { addGroupMember, getAllGroups, removeGroupMember } from "@/azure/functions/groups"
import { createTRPCRouter, publicProcedure } from "@/trpc"

export default createTRPCRouter({
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)
}),
Comment on lines +6 to +18

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🔒 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.ts

Repository: 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 -S

Repository: 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 -S

Repository: 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.

})
2 changes: 2 additions & 0 deletions src/routers/azure/index.ts
Original file line number Diff line number Diff line change
@@ -1,6 +1,8 @@
import { createTRPCRouter } from "@/trpc"
import groups from "./groups"
import members from "./members"

export const azureRouter = createTRPCRouter({
members,
groups,
})
5 changes: 3 additions & 2 deletions src/routers/web/guides_matricole.ts
Original file line number Diff line number Diff line change
Expand Up @@ -30,9 +30,10 @@ export default createTRPCRouter({
}),

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])

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🔒 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.


return latestGuide || null
return res[0] || null
}),

addGuide: publicProcedure
Expand Down
20 changes: 20 additions & 0 deletions src/server.ts
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@ import z from "zod"
import { auth } from "./auth"
import { getMembers } from "./azure/functions/members"
import "./azure/blob"
import { addGroupMember, getAllGroups, removeGroupMember } from "./azure/functions/groups"
import { AUTH_PATH, TRPC_PATH, WS_PATH } from "./constants"
import { cron } from "./cron"
import { DB, SCHEMA } from "./db"
Expand Down Expand Up @@ -87,6 +88,25 @@ app.get("/test/members", async (c) => {
return c.json({ users })
})

app.get("/test/azure-groups", async (c) => {
if (env.NODE_ENV === "production") return c.status(500)

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

📐 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: Replace return c.status(500) with return c.notFound().
  • src/server.ts#L102-L102: Replace return c.status(500) with return 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.


const groups = await getAllGroups()
return c.json({ groups })
})

app.post(
"/test/azure-group-member",
zValidator("json", z.object({ groupId: z.string(), userId: z.string(), mode: z.enum(["add", "remove"]) })),
async (c) => {
if (env.NODE_ENV === "production") return c.status(500)
const { userId, groupId, mode } = c.req.valid("json")

const ok = mode === "add" ? await addGroupMember(groupId, userId) : await removeGroupMember(groupId, userId)
return c.json({ ok })
}
)

app.all(`${WS_PATH}/`, (c) => {
return wssEngine.handleRequest(c.req.raw, server)
})
Expand Down