Skip to content

Conversation

@pboling
Copy link
Member

@pboling pboling commented Nov 5, 2025

  • e.g., REMOTE_USER

Closes and implements #87

@pboling pboling self-assigned this Nov 5, 2025
Copilot AI review requested due to automatic review settings November 5, 2025 10:39
Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull Request Overview

This PR adds support for header-based Single Sign-On (SSO) to the LDAP strategy, allowing trusted upstream gateways to authenticate users via headers like REMOTE_USER. When enabled, the strategy performs a directory lookup without requiring password authentication.

Key changes:

  • Added header_auth and header_name configuration options with security-focused documentation
  • Modified request and callback phases to support header-based authentication flow
  • Enhanced the filter method to accept optional username override for directory lookups

Reviewed Changes

Copilot reviewed 5 out of 5 changed files in this pull request and generated 1 comment.

Show a summary per file
File Description
lib/omniauth/strategies/ldap.rb Added header SSO logic in request/callback phases, new header_username and directory_lookup helper methods, and optional username_override parameter to filter method
spec/omniauth/strategies/ldap_spec.rb Comprehensive test suite for header SSO covering redirects, authentication, name_proc application, and failure scenarios
sig/omniauth/strategies/ldap.rbs Updated type signatures for new methods and modified filter signature to accept optional username parameter
README.md Added detailed documentation for header SSO configuration with security warnings and examples; fixed RFC reference formatting
CHANGELOG.md Added entry documenting the new header-based SSO feature

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

@ldap_user_info = entry
@user_info = self.class.map_user(CONFIG, @ldap_user_info)
return super
rescue => e
Copy link

Copilot AI Nov 5, 2025

Choose a reason for hiding this comment

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

Bare rescue catches all exceptions including syntax errors and system exits. Change to rescue StandardError => e to avoid catching non-application exceptions.

Copilot uses AI. Check for mistakes.
@pboling pboling force-pushed the feat/forward-sso-identity-from-http-header branch from b637223 to 15b911c Compare November 5, 2025 10:46
@github-actions
Copy link

github-actions bot commented Nov 5, 2025

Code Coverage

Package Line Rate Branch Rate Health
omniauth-ldap 98% 82%
Summary 98% (228 / 233) 82% (62 / 76)

Minimum allowed line rate is 97%

@codecov
Copy link

codecov bot commented Nov 5, 2025

Codecov Report

❌ Patch coverage is 96.42857% with 1 line in your changes missing coverage. Please review.
✅ Project coverage is 97.54%. Comparing base (5d67e0d) to head (119c7c3).
⚠️ Report is 3 commits behind head on main.

Files with missing lines Patch % Lines
lib/omniauth/strategies/ldap.rb 96.42% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main     #102      +/-   ##
==========================================
- Coverage   97.71%   97.54%   -0.18%     
==========================================
  Files           4        4              
  Lines         219      244      +25     
  Branches       59       67       +8     
==========================================
+ Hits          214      238      +24     
- Misses          5        6       +1     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@pboling pboling merged commit 1170a30 into main Nov 5, 2025
27 of 30 checks passed
@pboling pboling deleted the feat/forward-sso-identity-from-http-header branch November 5, 2025 10:51
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants