feat: get and create members on Azure #28
Draft
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Contributor
|
Important Review skippedDraft detected. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the
WalkthroughThis PR extends Azure Graph API integration to support user and member management. It adds functions for fetching members, creating users with license assignment, and managing licenses. The changes introduce TRPC router endpoints, utility functions for password generation and delays, and update email templates to align with new user creation workflows. Changes
Sequence DiagramsequenceDiagram
participant Client
participant TRPCRouter as TRPC Router<br/>(members.create)
participant CreateMember as Azure Functions<br/>(createMember)
participant AzureGraph as Azure Graph API
participant EmailService as Email Service<br/>(sendWelcomeEmail)
Client->>TRPCRouter: mutation create({firstName, lastName, assocNumber, sendEmailTo})
TRPCRouter->>CreateMember: createMember({firstName, lastName, assocNumber})
CreateMember->>AzureGraph: POST /users (create user)
AzureGraph-->>CreateMember: userId, credentials
CreateMember->>AzureGraph: POST /groups/{GruppoSociID}/members (add to group)
AzureGraph-->>CreateMember: success
CreateMember->>AzureGraph: POST /users/{userId}/assignLicense (assign OFFICE_365, POWER_AUTOMATE)
AzureGraph-->>CreateMember: licenses assigned
CreateMember-->>TRPCRouter: {id, email, credentials}
TRPCRouter->>EmailService: sendWelcomeEmail(sendEmailTo, credentials)
EmailService-->>TRPCRouter: {success: boolean}
TRPCRouter-->>Client: {error: null, id, email, welcomeMailSent}
Possibly related PRs
🚥 Pre-merge checks | ✅ 1 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. 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}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 11
🤖 Fix all issues with AI agents
In `@src/azure/functions.ts`:
- Around line 64-68: The current transformation that builds outMembers sets
assignedLicensesIds by mapping assignedLicenses to FlippedLicenses[al.skuId],
which can yield undefined when a skuId has no match; update the logic in the
members.map block (the outMembers construction) to explicitly handle
missing/unknown SKUs—either filter out undefined values (e.g., map then filter)
or replace undefined with a sentinel (null or a specific string) so
assignedLicensesIds contains only valid entries or known placeholders; ensure
you update the expression that references FlippedLicenses and assignedLicenses
so unknown licenses are not silently left as undefined.
- Around line 77-125: The createMember function currently creates the user then
performs multiple post-pcreation steps (client.api(`/users/${id}`).patch,
client.api(`/groups/${GruppoSociID}/members/$ref`).post, and manageLicenses)
without error handling; wrap those post-creation steps in a try-catch so that on
any failure you perform cleanup (delete the newly created user via
client.api(`/users/${id}`).delete()) and then rethrow the original error to the
caller, or alternatively implement compensating actions for each failed step
before returning; ensure you reference createMember, the user creation response
id, the patch/group post calls and manageLicenses when adding the try-catch and
cleanup logic.
- Line 11: Remove the unused import "which" from the module import statement
(import { which } from "bun") since it is not referenced anywhere in this file;
simply delete that named import (or the entire import if nothing else is
imported from "bun") and run the linter/TS build to verify the unused-import
warning is resolved.
- Around line 140-143: The array passed to the Microsoft Graph assignLicense
call is currently building removeLicenses as objects; change the construction so
removeLicenses is an array of SKU ID strings instead of objects—replace the
mapping that creates { disabledPlans: [], skuId: Licenses[l] } with a map (or
similar) that yields just Licenses[l]; keep addLicenses as-is (objects with
disabledPlans) and update any variable/parameter named removeLicenses used in
the assignLicense call (the code around removeLicenses mapping in the function
that calls assignLicense) to pass the string array expected by the API.
In `@src/routers/azure/members.ts`:
- Around line 1-4: Reorder the import statements in members.ts to satisfy Biome:
group external packages first (e.g., z) then internal/absolute imports, and sort
each group alphabetically by module specifier; ensure the imported symbols are
preserved (z, createTRPCRouter, publicProcedure, createMember, getMembers,
sendWelcomeEmail) so the file still imports z, createTRPCRouter,
publicProcedure, createMember/getMembers, and sendWelcomeEmail in the new sorted
order.
- Around line 7-12: The handler getAll currently treats an empty array from
getMembers() as NOT_FOUND, which conflates a real empty result with an API
failure; update getMembers (in src/azure/functions.ts) to either throw an error
on failure or return a discriminated result type (e.g., { ok: true, members: []
} | { ok: false, error }) and then change getAll to check that result: if the
call errored return the actual error (or propagate/throw) and only return
NOT_FOUND when ok is true and members.length === 0; reference getAll and
getMembers to locate and implement the change.
- Around line 13-60: The create mutation currently defined as create:
publicProcedure allows unauthenticated creation of members; change it to create:
protectedProcedure and update the import from "@/trpc" to include
protectedProcedure alongside publicProcedure so the mutation requires
authentication; ensure the rest of the mutation (createMember, sendWelcomeEmail,
and the .input/.output schemas) remains unchanged so only the procedure guardian
is swapped to protect the create flow.
- Around line 56-59: Replace the console.error and JSON.stringify(err) in the
catch block with the project logger: call logger.error with a descriptive
message and the error object (e.g., logger.error("Failed to <operation>", err))
so the full stack is recorded in logs, and return a non-sensitive client-facing
error (e.g., { error: "Internal server error" }) instead of JSON.stringify(err);
update the catch block in members.ts accordingly to use the imported logger from
"@/logger" and avoid leaking error details to clients.
In `@src/routers/index.ts`:
- Around line 1-4: Reorder the import statements at the top of the file so they
are sorted per the project's import-sorting rule (e.g., alphabetically by module
name) — specifically ensure createTRPCRouter, azureRouter, testRouter, and
tgRouter imports are in the correct sorted order and grouped consistently; you
can run the project's formatter/linter (e.g., prettier/eslint --fix or the
configured import sorter) to apply the fix automatically and then commit the
updated imports.
In `@src/server.ts`:
- Line 16: The import for getMembers is out of Biome-sorted order; in
src/server.ts reorder the import statements so they follow Biome's
alphabetical/grouping rules (typically third-party packages first, then
absolute/alias, then relative paths) and place the line importing getMembers
from "./azure/functions" into the correct sorted position among the other
imports to satisfy the linter.
In `@src/utils/password.ts`:
- Around line 20-26: Replace the insecure Math.random() in the Fisher–Yates
shuffle (the for-loop that iterates over the characters array and computes const
j = Math.floor(Math.random() * (i + 1))) with a cryptographically secure random
integer call (use crypto.getRandomValues or crypto.randomInt depending on
runtime): generate j via secureRandomInt(0, i) (in Node import crypto and use
crypto.randomInt(0, i + 1); in browser use globalThis.crypto.getRandomValues to
produce a random uint32 and map into range). Also clean up the stray
formatting/semicolons around the swap line (the leading semicolon before
[characters[i], characters[j]] = …) and run the formatter so the file
(src/utils/password.ts) conforms to project style after replacing Math.random().
🧹 Nitpick comments (3)
src/emails/templates/welcome.tsx (1)
84-86: Consider making the signature configurable.Hardcoding a specific person's name ("Lorenzo Corallo") in the email template creates maintenance overhead—any personnel or role changes require code modifications. Consider extracting this to a configuration value or prop.
Additionally, the signature is in Italian while the rest of the email content is in English, which creates a language inconsistency. If this is intentional for your audience, please disregard.
src/utils/password.ts (1)
3-5: Consider adding special characters for stronger passwords.The current password is 12 characters with only alphanumeric characters. For Azure AD accounts, consider including at least one special character to meet common password policy requirements and improve entropy.
💡 Optional: Add special characters pool
const a1 = customAlphabet("abcdefghijklmnopqrstuvwxyz", 4) const a2 = customAlphabet("ABCDEFGHIJKLMNOPQRSTUVWXYZ", 4) -const a3 = customAlphabet("1234567890", 4) +const a3 = customAlphabet("1234567890", 3) +const a4 = customAlphabet("!@#$%&*", 1) export function generatePassword() { - return shuffleString(`${a1()}${a2()}${a3()}`) + return shuffleString(`${a1()}${a2()}${a3()}${a4()}`) }src/server.ts (1)
82-87: Consider using 403 or 404 instead of 500 for production guard.Returning HTTP 500 suggests an internal server error, but the endpoint is intentionally disabled in production. A 403 (Forbidden) or 404 (Not Found) would be more semantically accurate. This also applies to the existing
/test/welcome-emailendpoint at line 66.♻️ Suggested fix
app.get("/test/members", async (c) => { - if (env.NODE_ENV === "production") return c.status(500) + if (env.NODE_ENV === "production") return c.notFound() const users = await getMembers() return c.json({ users }) })
src/azure/functions.ts
Outdated
| import { generatePassword } from "@/utils/password" | ||
| import { wait } from "@/utils/wait" | ||
| import { GraphError } from "@microsoft/microsoft-graph-client" | ||
| import { which } from "bun" |
Contributor
There was a problem hiding this comment.
Remove unused import.
The which import from "bun" is not used anywhere in this file. The pipeline has flagged this as well.
-import { which } from "bun"📝 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
| import { which } from "bun" |
🧰 Tools
🪛 GitHub Actions: Test
[warning] 11-11: Biome: Unused imports.
🤖 Prompt for AI Agents
In `@src/azure/functions.ts` at line 11, Remove the unused import "which" from the
module import statement (import { which } from "bun") since it is not referenced
anywhere in this file; simply delete that named import (or the entire import if
nothing else is imported from "bun") and run the linter/TS build to verify the
unused-import warning is resolved.
src/azure/functions.ts
Outdated
Comment on lines
64
to
68
| const outMembers = members.map((m) => ({ | ||
| ...m, | ||
| assignedLicensesIds: m.assignedLicenses.map((al) => (al.skuId ? FlippedLicenses[al.skuId] : null)), | ||
| assignedLicenses: undefined, | ||
| })) |
Contributor
There was a problem hiding this comment.
Potential undefined values in assignedLicensesIds array.
When FlippedLicenses[al.skuId] doesn't find a match, it returns undefined (not null as the ternary suggests for missing skuId). Consider filtering out unknown licenses or handling them explicitly.
💡 Suggested improvement
const outMembers = members.map((m) => ({
...m,
- assignedLicensesIds: m.assignedLicenses.map((al) => (al.skuId ? FlippedLicenses[al.skuId] : null)),
+ assignedLicensesIds: m.assignedLicenses
+ .map((al) => (al.skuId ? FlippedLicenses[al.skuId] : null))
+ .filter((id): id is string => id != null),
assignedLicenses: undefined,
}))📝 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
| const outMembers = members.map((m) => ({ | |
| ...m, | |
| assignedLicensesIds: m.assignedLicenses.map((al) => (al.skuId ? FlippedLicenses[al.skuId] : null)), | |
| assignedLicenses: undefined, | |
| })) | |
| const outMembers = members.map((m) => ({ | |
| ...m, | |
| assignedLicensesIds: m.assignedLicenses | |
| .map((al) => (al.skuId ? FlippedLicenses[al.skuId] : null)) | |
| .filter((id): id is string => id != null), | |
| assignedLicenses: undefined, | |
| })) |
🤖 Prompt for AI Agents
In `@src/azure/functions.ts` around lines 64 - 68, The current transformation that
builds outMembers sets assignedLicensesIds by mapping assignedLicenses to
FlippedLicenses[al.skuId], which can yield undefined when a skuId has no match;
update the logic in the members.map block (the outMembers construction) to
explicitly handle missing/unknown SKUs—either filter out undefined values (e.g.,
map then filter) or replace undefined with a sentinel (null or a specific
string) so assignedLicensesIds contains only valid entries or known
placeholders; ensure you update the expression that references FlippedLicenses
and assignedLicenses so unknown licenses are not silently left as undefined.
Comment on lines
+77
to
+125
| export async function createMember({ | ||
| firstName, | ||
| lastName, | ||
| assocNumber, | ||
| }: { | ||
| firstName: string | ||
| lastName: string | ||
| assocNumber: number | ||
| }) { | ||
| const password = generatePassword() | ||
| const mailNickname = `${firstName.replaceAll(" ", "")}.${lastName.replaceAll(" ", "")}`.toLowerCase() | ||
| const mail = `${mailNickname}@polinetwork.org` | ||
|
|
||
| // create user | ||
| const { id }: { id: string } = await client.api("/users").post({ | ||
| accountEnabled: true, | ||
| displayName: `${firstName} ${lastName}`, | ||
| mailNickname, | ||
| userPrincipalName: mail, | ||
| passwordProfile: { | ||
| forceChangePasswordNextSignIn: true, | ||
| password, | ||
| }, | ||
| }) | ||
|
|
||
| // set firstName and lastName | ||
| await client.api(`/users/${id}`).patch({ | ||
| givenName: firstName, | ||
| surname: lastName, | ||
| usageLocation: "IT", | ||
| employeeId: assocNumber.toString(), | ||
| }) | ||
|
|
||
| // add to group Soci | ||
| await client.api(`/groups/${GruppoSociID}/members/$ref`).post({ | ||
| "@odata.id": `https://graph.microsoft.com/v1.0/directoryObjects/${id}`, | ||
| }) | ||
|
|
||
| // assign licenses | ||
| await manageLicenses(id, ["OFFICE_365", "POWER_AUTOMATE"], []) | ||
|
|
||
| return { | ||
| firstName, | ||
| lastName, | ||
| mail, | ||
| id, | ||
| password, | ||
| } | ||
| } |
Contributor
There was a problem hiding this comment.
createMember lacks error handling for partial failures.
If any step fails after user creation (patching profile, adding to group, or assigning licenses), the user will exist in an incomplete state. Consider wrapping the operations in a try-catch and implementing cleanup on failure, or at minimum, propagating errors to the caller.
🛡️ Suggested approach with error handling
export async function createMember({
firstName,
lastName,
assocNumber,
}: {
firstName: string
lastName: string
assocNumber: number
}) {
const password = generatePassword()
const mailNickname = `${firstName.replaceAll(" ", "")}.${lastName.replaceAll(" ", "")}`.toLowerCase()
const mail = `${mailNickname}@polinetwork.org`
+ let id: string | undefined
+ try {
// create user
- const { id }: { id: string } = await client.api("/users").post({
+ const result = await client.api("/users").post({
accountEnabled: true,
displayName: `${firstName} ${lastName}`,
mailNickname,
userPrincipalName: mail,
passwordProfile: {
forceChangePasswordNextSignIn: true,
password,
},
})
+ id = result.id
// set firstName and lastName
await client.api(`/users/${id}`).patch({
givenName: firstName,
surname: lastName,
usageLocation: "IT",
employeeId: assocNumber.toString(),
})
// add to group Soci
await client.api(`/groups/${GruppoSociID}/members/$ref`).post({
"@odata.id": `https://graph.microsoft.com/v1.0/directoryObjects/${id}`,
})
// assign licenses
await manageLicenses(id, ["OFFICE_365", "POWER_AUTOMATE"], [])
return {
firstName,
lastName,
mail,
id,
password,
}
+ } catch (error) {
+ logger.error({ error, id, mail }, "[Azure Graph API] createMember failed")
+ throw error
+ }
}📝 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
| export async function createMember({ | |
| firstName, | |
| lastName, | |
| assocNumber, | |
| }: { | |
| firstName: string | |
| lastName: string | |
| assocNumber: number | |
| }) { | |
| const password = generatePassword() | |
| const mailNickname = `${firstName.replaceAll(" ", "")}.${lastName.replaceAll(" ", "")}`.toLowerCase() | |
| const mail = `${mailNickname}@polinetwork.org` | |
| // create user | |
| const { id }: { id: string } = await client.api("/users").post({ | |
| accountEnabled: true, | |
| displayName: `${firstName} ${lastName}`, | |
| mailNickname, | |
| userPrincipalName: mail, | |
| passwordProfile: { | |
| forceChangePasswordNextSignIn: true, | |
| password, | |
| }, | |
| }) | |
| // set firstName and lastName | |
| await client.api(`/users/${id}`).patch({ | |
| givenName: firstName, | |
| surname: lastName, | |
| usageLocation: "IT", | |
| employeeId: assocNumber.toString(), | |
| }) | |
| // add to group Soci | |
| await client.api(`/groups/${GruppoSociID}/members/$ref`).post({ | |
| "@odata.id": `https://graph.microsoft.com/v1.0/directoryObjects/${id}`, | |
| }) | |
| // assign licenses | |
| await manageLicenses(id, ["OFFICE_365", "POWER_AUTOMATE"], []) | |
| return { | |
| firstName, | |
| lastName, | |
| mail, | |
| id, | |
| password, | |
| } | |
| } | |
| export async function createMember({ | |
| firstName, | |
| lastName, | |
| assocNumber, | |
| }: { | |
| firstName: string | |
| lastName: string | |
| assocNumber: number | |
| }) { | |
| const password = generatePassword() | |
| const mailNickname = `${firstName.replaceAll(" ", "")}.${lastName.replaceAll(" ", "")}`.toLowerCase() | |
| const mail = `${mailNickname}@polinetwork.org` | |
| let id: string | undefined | |
| try { | |
| // create user | |
| const result = await client.api("/users").post({ | |
| accountEnabled: true, | |
| displayName: `${firstName} ${lastName}`, | |
| mailNickname, | |
| userPrincipalName: mail, | |
| passwordProfile: { | |
| forceChangePasswordNextSignIn: true, | |
| password, | |
| }, | |
| }) | |
| id = result.id | |
| // set firstName and lastName | |
| await client.api(`/users/${id}`).patch({ | |
| givenName: firstName, | |
| surname: lastName, | |
| usageLocation: "IT", | |
| employeeId: assocNumber.toString(), | |
| }) | |
| // add to group Soci | |
| await client.api(`/groups/${GruppoSociID}/members/$ref`).post({ | |
| "@odata.id": `https://graph.microsoft.com/v1.0/directoryObjects/${id}`, | |
| }) | |
| // assign licenses | |
| await manageLicenses(id, ["OFFICE_365", "POWER_AUTOMATE"], []) | |
| return { | |
| firstName, | |
| lastName, | |
| mail, | |
| id, | |
| password, | |
| } | |
| } catch (error) { | |
| logger.error({ error, id, mail }, "[Azure Graph API] createMember failed") | |
| throw error | |
| } | |
| } |
🤖 Prompt for AI Agents
In `@src/azure/functions.ts` around lines 77 - 125, The createMember function
currently creates the user then performs multiple post-pcreation steps
(client.api(`/users/${id}`).patch,
client.api(`/groups/${GruppoSociID}/members/$ref`).post, and manageLicenses)
without error handling; wrap those post-creation steps in a try-catch so that on
any failure you perform cleanup (delete the newly created user via
client.api(`/users/${id}`).delete()) and then rethrow the original error to the
caller, or alternatively implement compensating actions for each failed step
before returning; ensure you reference createMember, the user creation response
id, the patch/group post calls and manageLicenses when adding the try-catch and
cleanup logic.
Comment on lines
+140
to
+143
| removeLicenses: removeLicenses.map((l) => ({ | ||
| disabledPlans: [], | ||
| skuId: Licenses[l], | ||
| })), |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
Microsoft Graph API assignLicense endpoint removeLicenses format documentation
💡 Result:
For assignLicense, removeLicenses is an array of SKU GUIDs (skuId) to remove, and you must include both addLicenses and removeLicenses in the JSON payload (either can be an empty array). [1]
POST https://graph.microsoft.com/v1.0/users/{user-id}/assignLicense
Content-Type: application/json
{
"addLicenses": [],
"removeLicenses": [
"f30db892-07e9-47e9-837c-80727f46fd3d",
"84a661c4-e949-4bd2-a560-ed7766fcaf2b"
]
}(Equivalent example also works for POST https://graph.microsoft.com/v1.0/me/assignLicense.) [1]
Official documentation: Microsoft Learn – user: assignLicense (updated 2025-07-23). [1]
Sources
[1] https://learn.microsoft.com/en-us/graph/api/user-assignlicense?view=graph-rest-1.0
🏁 Script executed:
head -n 150 src/azure/functions.ts | tail -n 20Repository: PoliNetworkOrg/backend
Length of output: 655
Fix removeLicenses format in the API call.
The Microsoft Graph assignLicense endpoint expects removeLicenses to be an array of SKU IDs (strings), not objects with disabledPlans and skuId. Only addLicenses uses the object format with disabledPlans.
🐛 Fix removeLicenses format
await client.api(`/users/${userId}/assignLicense`).post({
addLicenses: addLicenses.map((l) => ({
disabledPlans: [],
skuId: Licenses[l],
})),
- removeLicenses: removeLicenses.map((l) => ({
- disabledPlans: [],
- skuId: Licenses[l],
- })),
+ removeLicenses: removeLicenses.map((l) => Licenses[l]),
})🤖 Prompt for AI Agents
In `@src/azure/functions.ts` around lines 140 - 143, The array passed to the
Microsoft Graph assignLicense call is currently building removeLicenses as
objects; change the construction so removeLicenses is an array of SKU ID strings
instead of objects—replace the mapping that creates { disabledPlans: [], skuId:
Licenses[l] } with a map (or similar) that yields just Licenses[l]; keep
addLicenses as-is (objects with disabledPlans) and update any variable/parameter
named removeLicenses used in the assignLicense call (the code around
removeLicenses mapping in the function that calls assignLicense) to pass the
string array expected by the API.
src/routers/azure/members.ts
Outdated
Comment on lines
1
to
4
| import { createMember, getMembers } from "@/azure/functions" | ||
| import { sendWelcomeEmail } from "@/emails/mailer" | ||
| import { createTRPCRouter, publicProcedure } from "@/trpc" | ||
| import z from "zod" |
Contributor
There was a problem hiding this comment.
Sort imports to satisfy linter.
The pipeline reports that imports are not sorted. Reorder according to Biome's sorting rules.
🧰 Tools
🪛 GitHub Actions: Test
[warning] 1-1: Biome: The imports and exports are not sorted.
🤖 Prompt for AI Agents
In `@src/routers/azure/members.ts` around lines 1 - 4, Reorder the import
statements in members.ts to satisfy Biome: group external packages first (e.g.,
z) then internal/absolute imports, and sort each group alphabetically by module
specifier; ensure the imported symbols are preserved (z, createTRPCRouter,
publicProcedure, createMember, getMembers, sendWelcomeEmail) so the file still
imports z, createTRPCRouter, publicProcedure, createMember/getMembers, and
sendWelcomeEmail in the new sorted order.
Comment on lines
+13
to
+60
| create: publicProcedure | ||
| .input( | ||
| z.object({ | ||
| firstName: z.string().min(1), | ||
| lastName: z.string().min(1), | ||
| assocNumber: z.number(), | ||
| sendEmailTo: z.email(), | ||
| }) | ||
| ) | ||
| .output( | ||
| z.union([ | ||
| z.object({ error: z.string() }), | ||
| z.object({ | ||
| error: z.null(), | ||
| id: z.string(), | ||
| email: z.email(), | ||
| welcomeMailSent: z.boolean(), | ||
| }), | ||
| ]) | ||
| ) | ||
| .mutation(async ({ input }) => { | ||
| try { | ||
| const member = await createMember({ | ||
| firstName: input.firstName, | ||
| lastName: input.lastName, | ||
| assocNumber: input.assocNumber, | ||
| }) | ||
|
|
||
| const mailOk = await sendWelcomeEmail( | ||
| input.sendEmailTo, | ||
| { email: member.mail, password: member.password }, | ||
| { | ||
| firstName: input.firstName, | ||
| assocNumber: input.assocNumber, | ||
| } | ||
| ) | ||
|
|
||
| return { | ||
| error: null, | ||
| id: member.id, | ||
| email: member.mail, | ||
| welcomeMailSent: mailOk, | ||
| } | ||
| } catch (err) { | ||
| console.error(err) | ||
| return { error: JSON.stringify(err) } | ||
| } | ||
| }), |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Search for authenticated/protected procedures in the TRPC setup
rg -n "protectedProcedure|adminProcedure|authProcedure" --type tsRepository: PoliNetworkOrg/backend
Length of output: 151
🏁 Script executed:
#!/bin/bash
# Examine the actual file to confirm the vulnerability
cat -n src/routers/azure/members.ts | head -70Repository: PoliNetworkOrg/backend
Length of output: 2097
🏁 Script executed:
#!/bin/bash
# Look for how other routers/mutations handle authentication
rg -n "publicProcedure|protectedProcedure" --type ts -A 2 -B 2 | head -100Repository: PoliNetworkOrg/backend
Length of output: 4845
Replace publicProcedure with protectedProcedure to require authentication.
The create mutation allows unauthenticated users to create Azure members, generate credentials, and trigger welcome emails. This is a critical security vulnerability enabling unauthorized account creation, license cost abuse, and credential exposure.
Change line 13 from create: publicProcedure to create: protectedProcedure, and update the import at line 3 to include protectedProcedure from @/trpc.
🤖 Prompt for AI Agents
In `@src/routers/azure/members.ts` around lines 13 - 60, The create mutation
currently defined as create: publicProcedure allows unauthenticated creation of
members; change it to create: protectedProcedure and update the import from
"@/trpc" to include protectedProcedure alongside publicProcedure so the mutation
requires authentication; ensure the rest of the mutation (createMember,
sendWelcomeEmail, and the .input/.output schemas) remains unchanged so only the
procedure guardian is swapped to protect the create flow.
Comment on lines
+56
to
+59
| } catch (err) { | ||
| console.error(err) | ||
| return { error: JSON.stringify(err) } | ||
| } |
Contributor
There was a problem hiding this comment.
Use the project logger and avoid exposing raw error details.
- Use the project's
logger(imported from@/logger) instead ofconsole.errorfor consistency with the rest of the codebase. JSON.stringify(err)on an Error object typically produces"{}"(empty object) since Error properties aren't enumerable, and it may also leak sensitive stack traces or internal details to clients.
♻️ Suggested fix
+import { logger } from "@/logger"
...
} catch (err) {
- console.error(err)
- return { error: JSON.stringify(err) }
+ logger.error({ err }, "[Azure Members] Failed to create member")
+ return { error: err instanceof Error ? err.message : "Unknown error" }
}📝 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
| } catch (err) { | |
| console.error(err) | |
| return { error: JSON.stringify(err) } | |
| } | |
| import { logger } from "@/logger" | |
| // ... (rest of existing imports and code) | |
| } catch (err) { | |
| logger.error({ err }, "[Azure Members] Failed to create member") | |
| return { error: err instanceof Error ? err.message : "Unknown error" } | |
| } |
🤖 Prompt for AI Agents
In `@src/routers/azure/members.ts` around lines 56 - 59, Replace the console.error
and JSON.stringify(err) in the catch block with the project logger: call
logger.error with a descriptive message and the error object (e.g.,
logger.error("Failed to <operation>", err)) so the full stack is recorded in
logs, and return a non-sensitive client-facing error (e.g., { error: "Internal
server error" }) instead of JSON.stringify(err); update the catch block in
members.ts accordingly to use the imported logger from "@/logger" and avoid
leaking error details to clients.
src/routers/index.ts
Outdated
Comment on lines
1
to
4
| import { createTRPCRouter } from "@/trpc" | ||
| import { testRouter } from "./test" | ||
| import { tgRouter } from "./tg" | ||
| import { azureRouter } from "./azure" |
Contributor
There was a problem hiding this comment.
Address import sorting per pipeline warning.
The pipeline reports that imports are not sorted. Consider running the formatter to fix this.
-import { createTRPCRouter } from "@/trpc"
-import { testRouter } from "./test"
-import { tgRouter } from "./tg"
import { azureRouter } from "./azure"
+import { createTRPCRouter } from "@/trpc"
+import { testRouter } from "./test"
+import { tgRouter } from "./tg"📝 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
| import { createTRPCRouter } from "@/trpc" | |
| import { testRouter } from "./test" | |
| import { tgRouter } from "./tg" | |
| import { azureRouter } from "./azure" | |
| import { createTRPCRouter } from "@/trpc" | |
| import { azureRouter } from "./azure" | |
| import { testRouter } from "./test" | |
| import { tgRouter } from "./tg" |
🧰 Tools
🪛 GitHub Actions: Test
[warning] 1-1: Biome: The imports and exports are not sorted.
🤖 Prompt for AI Agents
In `@src/routers/index.ts` around lines 1 - 4, Reorder the import statements at
the top of the file so they are sorted per the project's import-sorting rule
(e.g., alphabetically by module name) — specifically ensure createTRPCRouter,
azureRouter, testRouter, and tgRouter imports are in the correct sorted order
and grouped consistently; you can run the project's formatter/linter (e.g.,
prettier/eslint --fix or the configured import sorter) to apply the fix
automatically and then commit the updated imports.
src/server.ts
Outdated
| import { logger } from "./logger" | ||
| import { appRouter } from "./routers" | ||
| import { WebSocketServer, engine as wssEngine } from "./websocket" | ||
| import { getMembers } from "./azure/functions" |
Contributor
There was a problem hiding this comment.
Sort imports to satisfy linter.
The pipeline reports that imports are not sorted. The new import should be placed according to Biome's sorting rules.
🤖 Prompt for AI Agents
In `@src/server.ts` at line 16, The import for getMembers is out of Biome-sorted
order; in src/server.ts reorder the import statements so they follow Biome's
alphabetical/grouping rules (typically third-party packages first, then
absolute/alias, then relative paths) and place the line importing getMembers
from "./azure/functions" into the correct sorted position among the other
imports to satisfy the linter.
Comment on lines
20
to
26
| for (let i = characters.length - 1; i > 0; i--) { | ||
| // Generate a random index from 0 to i | ||
| const j = Math.floor(Math.random() * (i + 1)) | ||
|
|
||
| // Swap elements at indices i and j | ||
| ;[characters[i], characters[j]] = [characters[j], characters[i]] | ||
| } |
Contributor
There was a problem hiding this comment.
Use cryptographically secure randomness for password shuffling.
Math.random() is not cryptographically secure and should not be used for security-sensitive operations like password generation. While nanoid uses crypto.getRandomValues() internally, the shuffle step undermines this by using Math.random().
Additionally, the pipeline reports a formatting issue on these lines.
🔐 Proposed fix using crypto for secure shuffling
+import { randomInt } from "crypto"
import { customAlphabet } from "nanoid"
const a1 = customAlphabet("abcdefghijklmnopqrstuvwxyz", 4)
const a2 = customAlphabet("ABCDEFGHIJKLMNOPQRSTUVWXYZ", 4)
const a3 = customAlphabet("1234567890", 4)
export function generatePassword() {
return shuffleString(`${a1()}${a2()}${a3()}`)
}
/**
* Shuffles a string using the Fisher-Yates algorithm.
* `@param` text The string to shuffle
* `@returns` A new shuffled string
*/
function shuffleString(text: string): string {
// Convert string to an array of characters
const characters = text.split("")
for (let i = characters.length - 1; i > 0; i--) {
// Generate a cryptographically secure random index from 0 to i
- const j = Math.floor(Math.random() * (i + 1))
-
- // Swap elements at indices i and j
- ;[characters[i], characters[j]] = [characters[j], characters[i]]
+ const j = randomInt(0, i + 1)
+ ;[characters[i], characters[j]] = [characters[j], characters[i]]
}
// Join the array back into a string
return characters.join("")
}📝 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
| for (let i = characters.length - 1; i > 0; i--) { | |
| // Generate a random index from 0 to i | |
| const j = Math.floor(Math.random() * (i + 1)) | |
| // Swap elements at indices i and j | |
| ;[characters[i], characters[j]] = [characters[j], characters[i]] | |
| } | |
| import { randomInt } from "crypto" | |
| import { customAlphabet } from "nanoid" | |
| const a1 = customAlphabet("abcdefghijklmnopqrstuvwxyz", 4) | |
| const a2 = customAlphabet("ABCDEFGHIJKLMNOPQRSTUVWXYZ", 4) | |
| const a3 = customAlphabet("1234567890", 4) | |
| export function generatePassword() { | |
| return shuffleString(`${a1()}${a2()}${a3()}`) | |
| } | |
| /** | |
| * Shuffles a string using the Fisher-Yates algorithm. | |
| * `@param` text The string to shuffle | |
| * `@returns` A new shuffled string | |
| */ | |
| function shuffleString(text: string): string { | |
| // Convert string to an array of characters | |
| const characters = text.split("") | |
| for (let i = characters.length - 1; i > 0; i--) { | |
| // Generate a cryptographically secure random index from 0 to i | |
| const j = randomInt(0, i + 1) | |
| ;[characters[i], characters[j]] = [characters[j], characters[i]] | |
| } | |
| // Join the array back into a string | |
| return characters.join("") | |
| } |
🧰 Tools
🪛 GitHub Actions: Test
[error] 22-24: Biome: File content differs from formatting output.
🤖 Prompt for AI Agents
In `@src/utils/password.ts` around lines 20 - 26, Replace the insecure
Math.random() in the Fisher–Yates shuffle (the for-loop that iterates over the
characters array and computes const j = Math.floor(Math.random() * (i + 1)))
with a cryptographically secure random integer call (use crypto.getRandomValues
or crypto.randomInt depending on runtime): generate j via secureRandomInt(0, i)
(in Node import crypto and use crypto.randomInt(0, i + 1); in browser use
globalThis.crypto.getRandomValues to produce a random uint32 and map into
range). Also clean up the stray formatting/semicolons around the swap line (the
leading semicolon before [characters[i], characters[j]] = …) and run the
formatter so the file (src/utils/password.ts) conforms to project style after
replacing Math.random().
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
No description provided.