feat: configurable SAML attributes and audience via environment variables#1200
Open
chipach wants to merge 3 commits into
Open
feat: configurable SAML attributes and audience via environment variables#1200chipach wants to merge 3 commits into
chipach wants to merge 3 commits into
Conversation
|
@chipach is attempting to deploy a commit to the ory Team on Vercel. A member of the Team first needs to authorize it. |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (6)
✅ Files skipped from review due to trivial changes (2)
🚧 Files skipped from review as they are similar to previous changes (3)
📝 WalkthroughWalkthroughThis PR adds support for custom SAML attributes in authentication responses. Configuration for audience and extra attributes is parsed from environment variables, the authentication endpoint dynamically builds claims from provided or configured attributes, and the login form includes a UI for managing attribute mappings with template placeholder support. ChangesSAML Custom Attributes Support
Sequence DiagramsequenceDiagram
actor User
participant Browser
participant LoginPage as Login Page<br/>(pages/saml/login.tsx)
participant AuthAPI as Auth API<br/>(pages/api/saml/auth.ts)
participant Config as Config<br/>(lib/env.ts)
participant IdP as SAML IdP
Browser->>LoginPage: Load /saml/login
LoginPage->>Config: getServerSideProps reads<br/>config.extraAttributes,<br/>config.audience
Config-->>LoginPage: Return defaultAttributes,<br/>defaultAudience
LoginPage-->>Browser: Render form with<br/>default attributes
User->>Browser: Edit attributes,<br/>enter credentials
Browser->>AuthAPI: POST /api/saml/auth<br/>{audience, acsUrl,<br/>attributes, ...}
AuthAPI->>Config: Read config.extraAttributes<br/>(fallback if needed)
AuthAPI->>AuthAPI: resolveTemplate()<br/>for each attribute value
AuthAPI->>AuthAPI: Build extraClaims<br/>from attributes
AuthAPI->>AuthAPI: Merge into claims:<br/>{...user, ...extraClaims}
AuthAPI->>AuthAPI: Sign SAML response<br/>with custom claims
AuthAPI-->>Browser: Return HTML form<br/>with SAML assertion
Browser->>IdP: Auto-POST SAML<br/>response with<br/>custom attributes
IdP-->>Browser: Validate & process<br/>custom claims
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes 🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Tip 💬 Introducing Slack Agent: The best way for teams to turn conversations into code.Slack Agent is built on CodeRabbit's deep understanding of your code, so your team can collaborate across the entire SDLC without losing context.
Built for teams:
One agent for your entire SDLC. Right inside Slack. 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 |
…bles Closes ory#156 Adds support for injecting custom SAML attributes and configuring the default audience via environment variables, with an editable UI on the login page. ## Environment variables - `SAML_ATTRIBUTE_<name>=<value>` — adds `<name>` as an extra attribute in the SAML assertion. Values support placeholders: `{id}`, `{email}`, `{firstName}`, `{lastName}`. - `SAML_AUDIENCE=<url>` — sets the default audience (SP entity ID) pre-filled on the login form (default: `https://saml.boxyhq.com`). ## Login page The Attributes section on the login form is seeded from env defaults but is fully editable before submitting: - Change any attribute name or value - Remove an attribute with ✕ - Add new attributes with the + row (Enter also commits a new row) Attribute values (including `{id}`) are resolved server-side so all placeholders work correctly regardless of edits made in the UI. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
chipach
force-pushed
the
feat/configurable-saml-attributes
branch
from
c37e4fe to
7013cbb
Compare
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (2)
next.config.js (1)
7-7: 💤 Low value
path.join(__dirname)with a single argument is a no-op — use__dirnamedirectly.
path.joinwith one argument simply normalises and returns that path unchanged, sopath.join(__dirname)is identical to__dirname. The intent is clear, but the wrapping call adds noise.✨ Proposed simplification
- outputFileTracingRoot: path.join(__dirname), + outputFileTracingRoot: __dirname,If
pathis no longer needed after this change, you can also drop therequire:-const path = require('path'); -🤖 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 `@next.config.js` at line 7, Replace the no-op path.join call by using __dirname directly for outputFileTracingRoot: change usages of path.join(__dirname) to __dirname (i.e., update the outputFileTracingRoot assignment referencing outputFileTracingRoot), and if the path module is no longer used elsewhere in the file, remove the unused require/import of path to keep the file clean.pages/saml/login.tsx (1)
6-6: ⚡ Quick winMove server-only
configimport insidegetServerSidePropsto prevent it being bundled client-side.
configis used exclusively ingetServerSideProps(line 276), yet the module-levelimport config from 'lib/env'causeslib/env.ts— which callsfetchPrivateKey()/fetchPublicKey()at module initialization — to be included in the client bundle. Next.js replaces non-NEXT_PUBLIC_*env vars withundefinedso key material won't literally appear in the bundle, but shipping the module unnecessarily increases bundle size and blurs the server/client boundary.♻️ Proposed refactor — scope the import to the server function
import Head from 'next/head'; import { useRouter } from 'next/router'; import type { GetServerSideProps } from 'next'; import type { FormEvent } from 'react'; import { useEffect, useRef, useState } from 'react'; -import config from 'lib/env'; // ... component unchanged ... export const getServerSideProps: GetServerSideProps<Props> = async () => { + const { default: config } = await import('lib/env'); const defaultAttributes = Object.entries(config.extraAttributes).map(([name, value]) => ({ name, value, })); return { props: { defaultAttributes, defaultAudience: config.audience } }; };Alternatively, add
server-onlyas a dependency and import it at the top oflib/env.tsto have Next.js enforce server-side-only usage at build time.🤖 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 `@pages/saml/login.tsx` at line 6, The module-level import "import config from 'lib/env'" pulls server-only key-fetching code into the client bundle; remove that top-level import and instead require/import config inside the server-side handler getServerSideProps (refer to getServerSideProps) so lib/env is only loaded on the server, or alternatively add and import the "server-only" helper at the top of lib/env.ts to enforce server-only usage; update any references in the file to use the locally imported config within getServerSideProps.
🤖 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 `@pages/api/saml/auth.ts`:
- Around line 8-14: The current resolveTemplate(template: string, user: User)
uses string replacements that allow special replacement patterns like $&/$' to
be interpreted, corrupting values when user fields (derived from email) contain
$. Change each .replace call to use the function form of
String.prototype.replace (e.g., replace(/\{firstName\}/g, () => user.firstName))
for all placeholders ({id},{email},{firstName},{lastName}) so the literal user
values are inserted verbatim and special $ sequences are not processed.
- Around line 36-39: The loop over attributes can throw when an element is null
because it blindly destructures each item; before destructuring, guard that each
element is a non-null object (or pre-filter attributes to only objects) so you
only access name/value for valid items—update the loop that iterates over
attributes (the Array.isArray(attributes) block) to skip null/primitive entries
and only call resolveTemplate(value ?? '', user) and assign extraClaims[name]
when name exists on a valid object.
In `@pages/saml/login.tsx`:
- Around line 221-228: The name input (<input ... value={newAttr.name}
onChange={...} />) currently lacks an Enter key guard so pressing Enter bubbles
to the enclosing form and triggers handleSubmit; add the same key handler used
on the value input to the name input: intercept the Enter key, call
e.preventDefault(), and invoke handleAttrAdd() (or the same handler function
used by the value input) so the new attribute is added instead of submitting the
login form, updating the element that uses newAttr.name and setNewAttr.
---
Nitpick comments:
In `@next.config.js`:
- Line 7: Replace the no-op path.join call by using __dirname directly for
outputFileTracingRoot: change usages of path.join(__dirname) to __dirname (i.e.,
update the outputFileTracingRoot assignment referencing outputFileTracingRoot),
and if the path module is no longer used elsewhere in the file, remove the
unused require/import of path to keep the file clean.
In `@pages/saml/login.tsx`:
- Line 6: The module-level import "import config from 'lib/env'" pulls
server-only key-fetching code into the client bundle; remove that top-level
import and instead require/import config inside the server-side handler
getServerSideProps (refer to getServerSideProps) so lib/env is only loaded on
the server, or alternatively add and import the "server-only" helper at the top
of lib/env.ts to enforce server-only usage; update any references in the file to
use the locally imported config within getServerSideProps.
🪄 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 Plus
Run ID: e88fe2c9-5186-4978-a0c1-cf28c5e9be29
📒 Files selected for processing (7)
.env.examplelib/env.tsnext.config.jspages/api/saml/auth.tspages/namespace/[namespace]/saml/login.tsxpages/saml/login.tsxtsconfig.json
chipach
commented
May 5, 2026
| module.exports = { | ||
| reactStrictMode: true, | ||
| output: 'standalone', | ||
| outputFileTracingRoot: path.join(__dirname), |
Author
There was a problem hiding this comment.
This was trying to look up a directory tree without this.
| } | ||
| }; | ||
|
|
||
| const inputBase = |
Author
There was a problem hiding this comment.
Not completely required for this PR, but since classes were being added for the attributes, a little DRYing was in order.
- Use function replacements in resolveTemplate to prevent $ special sequences in user field values from corrupting attribute output - Guard against null/non-object entries in the attributes array - Add Enter key handler to attribute name input to prevent accidental form submission - Simplify outputFileTracingRoot to use __dirname directly - Move lib/env import inside getServerSideProps to keep it server-side only Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
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
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.
Closes #156
Summary
SAML_ATTRIBUTE_<name>=<value>env vars inject extra attributes into the SAML assertion. Values support{id},{email},{firstName},{lastName}placeholders.SAML_AUDIENCE=<url>sets the default audience pre-filled on the login form (default:https://saml.boxyhq.com).Example
SAML_ATTRIBUTE_role=admin SAML_ATTRIBUTE_department=engineering SAML_ATTRIBUTE_uid={id} SAML_AUDIENCE=https://my-sp.example.comNotes
{id}) happens server-side, so all placeholders work correctly even after UI edits.next.config.jsgainsoutputFileTracingRootto silence a spurious workspace-root warning from Next.js 16 when a lockfile exists in a parent directory.tsconfig.jsonmoduleResolutionupdated fromnodetobundler— Next.js 16 flags this as a mandatory change and rewrites the file automatically on every build.🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Chores