Update pre MFA workflow example#25
Conversation
📝 WalkthroughWalkthroughAdds an MFAPolicy mapping for standardized MFA enforcement values, imports Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~8 minutes Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
🧪 Generate unit tests (beta)
No actionable comments were generated in the recent review. 🎉 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 |
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
preMFA/gracePeriodWorkflow.ts (1)
51-55:⚠️ Potential issue | 🟡 Minor
MFA_GRACE_PERIOD_IN_MSsilently becomesNaNwhen the env var is missing.If
MFA_GRACE_PERIOD_IN_HOURSis not set,getEnvironmentVariablereturnsundefined, andNumber(undefined)isNaN. The grace-period comparison then always evaluates tofalse, so MFA is always required — a safe default, but this fails silently and could confuse operators debugging why the grace period "isn't working."Consider adding an early guard with a log to surface the misconfiguration.
🛡️ Proposed guard
- const MFA_GRACE_PERIOD_IN_MS = - Number(getEnvironmentVariable("MFA_GRACE_PERIOD_IN_HOURS")?.value) * - 60 * - 60 * - 1000; + const gracePeriodHours = Number( + getEnvironmentVariable("MFA_GRACE_PERIOD_IN_HOURS")?.value + ); + + if (isNaN(gracePeriodHours)) { + console.log( + "MFA_GRACE_PERIOD_IN_HOURS is not set or invalid — MFA will always be required" + ); + setEnforcementPolicy("required"); + return; + } + + const MFA_GRACE_PERIOD_IN_MS = gracePeriodHours * 60 * 60 * 1000;
🧹 Nitpick comments (1)
preMFA/gracePeriodWorkflow.ts (1)
11-14: Tighten the type ofMFAPolicyto preserve compile-time key safety.The
{[key: string]: MFAEnforcementPolicy}index signature meansMFAPolicy.Typosilently evaluates toundefinedat runtime without a TypeScript error. Usingas const(with a satisfies check if you want theMFAEnforcementPolicyconstraint) retains both literal types and key safety.♻️ Suggested tighter typing
-const MFAPolicy: {[key: string]: MFAEnforcementPolicy} = { - Required: "required", - Skip: "skip" -} +const MFAPolicy = { + Required: "required", + Skip: "skip", +} as const satisfies Record<string, MFAEnforcementPolicy>;
Add missing import;
Define MFAPolicy constant as MFAPolicy and MFAEnforcementPolicy are differing string unions;
Add kinde.mfa binding;
Explain your changes
Simple update primarily to add the kinde.mfa binding which is required to use setEnforcementPolicy.
Also adds a missing import (createKindeAPI) and defines a constant for MFAPolicy since the MFAPolicy and MFAEnforcementPolicy types from the infrastructure library are actually differing string unions which will save time for anyone not too familiar with typescript.
Happy to add a PR in infrastructure for the last one but I figured this would do for an example workflow.
Checklist
Summary by CodeRabbit