fix: enable SPNEGO/Kerberos authentication on macOS #3259
fix: enable SPNEGO/Kerberos authentication on macOS #3259Ram-sah19 wants to merge 3 commits intoRocketChat:developfrom
Conversation
WalkthroughThis pull request enables Electron PDF rendering by adding plugin support to webPreferences and introduces macOS-specific Kerberos/SPNEGO authentication configuration for SAML support. Documentation files are added to explain both fixes. No public APIs are modified. Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Suggested labels
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 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 |
Ram-sah19
changed the title
Ram-sah19
changed the title
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/ui/main/serverView/index.ts (1)
248-255:⚠️ Potential issue | 🟡 MinorAdd
plugins: trueto the dedicated video-callBrowserWindowinsrc/videoCallWindow/ipc.ts.Two distinct code paths create video-call windows with inconsistent settings. The
setWindowOpenHandleroverride atsrc/ui/main/serverView/index.ts:248-255setswebPreferences.plugins: truefor "Video Call" frames, but the dedicated video-call window created via IPC atsrc/videoCallWindow/ipc.ts:102-114does not include this setting. The preload code insrc/servers/preload/internalVideoChatWindow.tsexplicitly uses the IPC path (video-call-window/open-window) instead ofwindow.open, making this the primary code path. Both paths should have consistentwebPreferencesto ensure uniform PDF rendering behavior.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/ui/main/serverView/index.ts` around lines 248 - 255, The dedicated video-call window created by the IPC handler for the 'video-call-window/open-window' path is missing webPreferences.plugins: true, causing inconsistent behavior versus the overrideBrowserWindowOptions (isVideoCall) branch; update the IPC handler that constructs the BrowserWindow in videoCallWindow ipc.ts (the 'video-call-window/open-window' handler) to include webPreferences.plugins: true (and mirror the same preload and sandbox settings used in overrideBrowserWindowOptions) so both creation paths use identical webPreferences for Video Call windows.
🧹 Nitpick comments (2)
FIXES_SUMMARY.md (1)
33-48: Please keep PR-local status notes out of the repo.These sections hard-code transient branch names, commit hashes, “create pull request” steps, and a placeholder date. They will be wrong on the next rebase/merge and add maintenance-only noise. Please move this material to the PR description or convert it into durable release notes/user docs.
Also applies to: 145-184, 301-304
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@FIXES_SUMMARY.md` around lines 33 - 48, Remove the PR-local status notes from the FIXES_SUMMARY.md by deleting or replacing the "## 📊 Branch Status" block that hard-codes Current Branch, Commit History, Changes Summary and any other transient snippets (e.g., create pull request steps or placeholder date) and instead move that content into the PR description or convert it into durable release notes/user docs; ensure the file only contains stable, long-lived information and remove the duplicate transient sections that match the same pattern elsewhere in the file ("Current Branch"/"Commit History"/"Changes Summary" blocks).MERGE_CONFLICT_RESOLUTION.md (1)
1-162: This belongs in the PR thread, not the repository.The file is a one-off merge/conflict-resolution log with git commands, branch names, and exact commit history. That context becomes stale immediately after merge and doesn't provide lasting value to the codebase. Please drop it from the tree, or fold only the durable testing/release notes into existing docs.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@MERGE_CONFLICT_RESOLUTION.md` around lines 1 - 162, Remove the transient merge/conflict log file MERGE_CONFLICT_RESOLUTION.md from the commit tree (delete the file and update the branch) and instead fold any durable testing or release notes into the existing PDF_FIX_TESTING_GUIDE.md or QUICK_START_PDF_FIX.md (or put the full merge log into the PR description/thread); ensure the repository only contains stable, evergreen docs and that the two created guides contain any necessary testing steps from the removed file.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/app/main/app.ts`:
- Around line 69-73: The hard-coded Kerberos/SPNEGO allowlist set via
app.commandLine.appendSwitch('auth-server-whitelist', ...) and
app.commandLine.appendSwitch('auth-negotiate-delegate-whitelist', ...) should be
made configurable; remove the fixed '*.local,*.domain.local' values and instead
read the allowlist from a configurable source (e.g., app config, environment
variable or the configured IdP/host list) and apply that value when calling
app.commandLine.appendSwitch in the same initialization branch (process.platform
=== 'darwin'); validate/fallback to an empty or safe default if no allowlist is
provided and document the new config key used to supply the allowlist.
---
Outside diff comments:
In `@src/ui/main/serverView/index.ts`:
- Around line 248-255: The dedicated video-call window created by the IPC
handler for the 'video-call-window/open-window' path is missing
webPreferences.plugins: true, causing inconsistent behavior versus the
overrideBrowserWindowOptions (isVideoCall) branch; update the IPC handler that
constructs the BrowserWindow in videoCallWindow ipc.ts (the
'video-call-window/open-window' handler) to include webPreferences.plugins: true
(and mirror the same preload and sandbox settings used in
overrideBrowserWindowOptions) so both creation paths use identical
webPreferences for Video Call windows.
---
Nitpick comments:
In `@FIXES_SUMMARY.md`:
- Around line 33-48: Remove the PR-local status notes from the FIXES_SUMMARY.md
by deleting or replacing the "## 📊 Branch Status" block that hard-codes Current
Branch, Commit History, Changes Summary and any other transient snippets (e.g.,
create pull request steps or placeholder date) and instead move that content
into the PR description or convert it into durable release notes/user docs;
ensure the file only contains stable, long-lived information and remove the
duplicate transient sections that match the same pattern elsewhere in the file
("Current Branch"/"Commit History"/"Changes Summary" blocks).
In `@MERGE_CONFLICT_RESOLUTION.md`:
- Around line 1-162: Remove the transient merge/conflict log file
MERGE_CONFLICT_RESOLUTION.md from the commit tree (delete the file and update
the branch) and instead fold any durable testing or release notes into the
existing PDF_FIX_TESTING_GUIDE.md or QUICK_START_PDF_FIX.md (or put the full
merge log into the PR description/thread); ensure the repository only contains
stable, evergreen docs and that the two created guides contain any necessary
testing steps from the removed file.
🪄 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: 3e266ffd-d7a3-4956-832f-a8af6167fb2f
📒 Files selected for processing (6)
FIXES_SUMMARY.mdKERBEROS_QUICK_REF.mdKERBEROS_SAML_FIX.mdMERGE_CONFLICT_RESOLUTION.mdsrc/app/main/app.tssrc/ui/main/serverView/index.ts
📜 Review details
🧰 Additional context used
📓 Path-based instructions (1)
**/*.{ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
**/*.{ts,tsx}: Use TypeScript strict mode enabled in TypeScript configuration
Use React functional components with hooks instead of class components
Follow FSA (Flux Standard Action) pattern for Redux actions
Use camelCase for file names and PascalCase for component file names
All code must pass ESLint and TypeScript checks
Write self-documenting code with clear naming; avoid unnecessary comments except for complex business logic or non-obvious decisions
Use Fuselage components from@rocket.chat/fuselagefor all UI work and only create custom components when Fuselage doesn't provide what's needed
CheckTheme.d.tsfor valid color tokens when using Fuselage components
Use defensive coding with optional chaining and fallbacks for Linux-only APIs (process.getuid(), process.getgid(), process.geteuid(), process.getegid()) to ensure cross-platform compatibility across Windows, macOS, and Linux
Files:
src/ui/main/serverView/index.tssrc/app/main/app.ts
🧠 Learnings (3)
📚 Learning: 2026-03-11T06:38:40.426Z
Learnt from: Ram-sah19
Repo: RocketChat/Rocket.Chat.Electron PR: 3254
File: .github/workflows/build-release.yml:80-94
Timestamp: 2026-03-11T06:38:40.426Z
Learning: In the RocketChat/Rocket.Chat.Electron repository, the issues flagged in `.github/workflows/build-release.yml` (e.g., `node12` runtime in the release action and missing `snapcraft_token` input), i18n files, and `electron-builder.json` are pre-existing in the `develop` branch and are pulled in during merge conflict resolution. Do not flag these as new issues introduced by PRs that only modify `src/injected.ts` and `src/ui/main/rootWindow.ts`.
Applied to files:
KERBEROS_SAML_FIX.mdFIXES_SUMMARY.mdMERGE_CONFLICT_RESOLUTION.md
📚 Learning: 2026-02-04T19:29:54.650Z
Learnt from: CR
Repo: RocketChat/Rocket.Chat.Electron PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-02-04T19:29:54.650Z
Learning: Applies to **/*.{ts,tsx} : Use defensive coding with optional chaining and fallbacks for Linux-only APIs (process.getuid(), process.getgid(), process.geteuid(), process.getegid()) to ensure cross-platform compatibility across Windows, macOS, and Linux
Applied to files:
src/app/main/app.ts
📚 Learning: 2026-02-04T19:29:54.650Z
Learnt from: CR
Repo: RocketChat/Rocket.Chat.Electron PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-02-04T19:29:54.650Z
Learning: Applies to **/*.{spec,main.spec}.ts : Only mock platform-specific APIs in tests when defensive coding patterns cannot be used; use Object.defineProperty to mock process.platform and environment variables
Applied to files:
src/app/main/app.ts
🪛 LanguageTool
KERBEROS_SAML_FIX.md
[style] ~69-~69: This phrase is redundant (‘OS’ stands for ‘operating system’). Use simply “macOS”.
Context: ...sting Instructions ### Prerequisites - macOS system - Rocket.Chat server with SAML configur...
(ACRONYM_TAUTOLOGY)
FIXES_SUMMARY.md
[style] ~114-~114: This phrase is redundant (‘OS’ stands for ‘operating system’). Use simply “macOS”.
Context: ...Kerberos/SAML Fix Prerequisites: - macOS system - Valid Kerberos ticket (klist) - SAM...
(ACRONYM_TAUTOLOGY)
KERBEROS_QUICK_REF.md
[style] ~29-~29: This phrase is redundant (‘OS’ stands for ‘operating system’). Use simply “macOS”.
Context: ... ## 🧪 Quick Test ### Prerequisites - macOS system - Valid Kerberos ticket: klist - SAML...
(ACRONYM_TAUTOLOGY)
🪛 markdownlint-cli2 (0.21.0)
KERBEROS_SAML_FIX.md
[warning] 98-98: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
[warning] 110-110: Spaces inside emphasis markers
(MD037, no-space-in-emphasis)
[warning] 156-156: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
[warning] 169-169: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
[warning] 240-240: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
FIXES_SUMMARY.md
[warning] 29-29: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
[warning] 41-41: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
[warning] 46-46: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
| // Enable Kerberos/SPNEGO authentication for SAML on macOS | ||
| if (process.platform === 'darwin') { | ||
| app.commandLine.appendSwitch('auth-server-whitelist', '*.local,*.domain.local'); | ||
| app.commandLine.appendSwitch('auth-negotiate-delegate-whitelist', '*.local,*.domain.local'); | ||
| } |
There was a problem hiding this comment.
Don't ship a hard-coded Kerberos allowlist.
This only enables SPNEGO for IdPs under .local / .domain.local, so deployments using hosts like idp.company.com will still fall back to the password prompt. It also delegates Kerberos to every host matching those wildcards. Please make the allowlist configurable, or derive it from the configured server/IdP, instead of baking fixed domains into the desktop app.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/app/main/app.ts` around lines 69 - 73, The hard-coded Kerberos/SPNEGO
allowlist set via app.commandLine.appendSwitch('auth-server-whitelist', ...) and
app.commandLine.appendSwitch('auth-negotiate-delegate-whitelist', ...) should be
made configurable; remove the fixed '*.local,*.domain.local' values and instead
read the allowlist from a configurable source (e.g., app config, environment
variable or the configured IdP/host list) and apply that value when calling
app.commandLine.appendSwitch in the same initialization branch (process.platform
=== 'darwin'); validate/fallback to an empty or safe default if no allowlist is
provided and document the new config key used to supply the allowlist.
1 participant
Description
This PR enables SPNEGO / Kerberos authentication for macOS users
in the Rocket.Chat Desktop app.
Electron does not enable SPNEGO authentication by default,
which causes SAML login to show a password prompt instead
of automatic authentication.
Changes
--auth-server-whitelist
--auth-negotiate-delegate-whitelist
Result
Kerberos SSO now works in the desktop app similar to browsers.
#3201
Summary by CodeRabbit
New Features
Documentation