diff --git a/.rubocop_gradual.lock b/.rubocop_gradual.lock index 23e729b..bd5000d 100644 --- a/.rubocop_gradual.lock +++ b/.rubocop_gradual.lock @@ -30,15 +30,14 @@ [47, 7, 38, "RSpec/AnyInstance: Avoid stubbing using `allow_any_instance_of`.", 3627954156], [84, 7, 48, "RSpec/AnyInstance: Avoid stubbing using `allow_any_instance_of`.", 2759780562] ], - "spec/omniauth/strategies/ldap_spec.rb:1788355205": [ - [14, 3, 54, "RSpec/LeakyConstantDeclaration: Stub class constant instead of declaring explicitly.", 2419068710], - [90, 13, 9, "RSpec/ContextWording: Context description should match /^when\\b/, /^with\\b/, or /^without\\b/.", 1130140517], - [145, 17, 28, "RSpec/ContextWording: Context description should match /^when\\b/, /^with\\b/, or /^without\\b/.", 3444838747], - [154, 17, 23, "RSpec/ContextWording: Context description should match /^when\\b/, /^with\\b/, or /^without\\b/.", 1584148894], - [165, 17, 32, "RSpec/ContextWording: Context description should match /^when\\b/, /^with\\b/, or /^without\\b/.", 1515076977], - [174, 19, 19, "RSpec/ContextWording: Context description should match /^when\\b/, /^with\\b/, or /^without\\b/.", 2526348694], - [187, 17, 56, "RSpec/ContextWording: Context description should match /^when\\b/, /^with\\b/, or /^without\\b/.", 2413495789], - [202, 13, 9, "RSpec/ContextWording: Context description should match /^when\\b/, /^with\\b/, or /^without\\b/.", 3182939526], - [235, 15, 19, "RSpec/ContextWording: Context description should match /^when\\b/, /^with\\b/, or /^without\\b/.", 2526348694] + "spec/omniauth/strategies/ldap_spec.rb:783052937": [ + [93, 13, 9, "RSpec/ContextWording: Context description should match /^when\\b/, /^with\\b/, or /^without\\b/.", 1130140517], + [148, 17, 28, "RSpec/ContextWording: Context description should match /^when\\b/, /^with\\b/, or /^without\\b/.", 3444838747], + [157, 17, 23, "RSpec/ContextWording: Context description should match /^when\\b/, /^with\\b/, or /^without\\b/.", 1584148894], + [168, 17, 32, "RSpec/ContextWording: Context description should match /^when\\b/, /^with\\b/, or /^without\\b/.", 1515076977], + [177, 19, 19, "RSpec/ContextWording: Context description should match /^when\\b/, /^with\\b/, or /^without\\b/.", 2526348694], + [203, 17, 56, "RSpec/ContextWording: Context description should match /^when\\b/, /^with\\b/, or /^without\\b/.", 2413495789], + [218, 13, 9, "RSpec/ContextWording: Context description should match /^when\\b/, /^with\\b/, or /^without\\b/.", 3182939526], + [251, 15, 19, "RSpec/ContextWording: Context description should match /^when\\b/, /^with\\b/, or /^without\\b/.", 2526348694] ] } diff --git a/README.md b/README.md index ba11cab..a7f96c5 100644 --- a/README.md +++ b/README.md @@ -312,6 +312,66 @@ provider :ldap, password: ENV["LDAP_SEARCH_PASSWORD"] ``` +What `:filter` actually does + +- If `:filter` is provided, the strategy constructs an LDAP filter string by substituting `%{username}` with the submitted username after applying `:name_proc`, escaping special characters per RFC 4515, and passes it to the directory search. +- In the normal password flow, a successful search returns the user's DN and we then bind as that DN with the submitted password. +- In trusted header SSO flow (`header_auth: true`), we only perform the search and skip the user password bind; if the search returns no entry, authentication fails. +- If `:filter` is not provided, the strategy falls back to a simple equality filter using `:uid` (e.g. `(uid=alice)`). + +Notes on escaping and safety + +- We escape the interpolated username with `Net::LDAP::Filter.escape`, which protects against LDAP injection and handles special characters like `(`, `)`, `*`, and `\`. +- Your static filter text is used as-is — keep it to a valid LDAP filter expression and only use `%{username}` for substitution. + +Group-based recipes + +- Active Directory (simple group): + + ```text + (&(sAMAccountName=%{username})(memberOf=cn=myapp-users,ou=groups,dc=example,dc=com)) + ``` + +- Active Directory (nested groups via matchingRuleInChain): + + ```text + (&(sAMAccountName=%{username})(memberOf:1.2.840.113556.1.4.1941:=cn=myapp-users,ou=groups,dc=example,dc=com)) + ``` + +- OpenLDAP (groupOfNames): + + ```text + (&(uid=%{username})(memberOf=cn=myapp-users,ou=groups,dc=example,dc=com)) + ``` + + or, if you can't use `memberOf` overlays, filter on the group and member DN: + + ```text + (&(uid=%{username})(|(uniqueMember=uid=%{username},ou=people,dc=example,dc=com)(member=uid=%{username},ou=people,dc=example,dc=com))) + ``` + +Username normalization examples + +- If your users sign in with an email but the directory expects a short name, combine `:name_proc` with `:filter`: + + ```ruby + provider :ldap, + name_proc: proc { |n| n.split("@").first }, + filter: "(&(sAMAccountName=%{username})(memberOf=cn=myapp-users,ou=groups,dc=example,dc=com))" + # other settings... + ``` + +Discourse plugin (jonmbake/discourse-ldap-auth) + +- That plugin forwards its `filter` setting to this gem. You can therefore paste the same filter strings shown above. +- Example (allow only members of `forum-users`): + + ```text + (&(uid=%{username})(memberOf=cn=forum-users,ou=groups,dc=example,dc=com)) + ``` + +- If users type an email address but your directory matches on a short user id, also configure `name_proc` accordingly in your app (or the plugin, if supported). + ### SASL (advanced) SASL enables alternative bind mechanisms. Only enable if you understand the server-side requirements. @@ -325,7 +385,7 @@ provider :ldap, uid: "uid" ``` -Supported mechanisms include `"DIGEST-MD5"` and `"GSS-SPNEGO"` depending on your environment and gems. +Supported mechanisms include "DIGEST-MD5" and "GSS-SPNEGO" depending on your environment and gems. ### Name processing and examples diff --git a/spec/omniauth/strategies/ldap_spec.rb b/spec/omniauth/strategies/ldap_spec.rb index 9f1ebb8..bf2564f 100644 --- a/spec/omniauth/strategies/ldap_spec.rb +++ b/spec/omniauth/strategies/ldap_spec.rb @@ -11,7 +11,10 @@ # :name_proc => Proc.new {|name| name.gsub(/@.*$/,'')} # :bind_dn => 'default_bind_dn' # :password => 'password' - class MyLdapProvider < OmniAuth::Strategies::LDAP; end + before do + ldap_strategy = Class.new(OmniAuth::Strategies::LDAP) + stub_const("MyLdapProvider", ldap_strategy) + end let(:app) do Rack::Builder.new { @@ -181,6 +184,19 @@ def make_env(path = "/auth/ldap", props = {}) expect(last_response.headers["Location"]).to match("invalid_credentials") expect(last_request.env["omniauth.error"].message).to eq("Invalid credentials for ping") end + + it "supports group restriction filters and applies name_proc" do + # Complex filter with %{username} placeholder and group membership + group_filter = "(&(uid=%{username})(memberOf=cn=forum-users,ou=groups,dc=example,dc=com))" + allow(@adaptor).to receive(:filter).and_return(group_filter) + # username has a domain part; name_proc on strategy under test strips it + expect(Net::LDAP::Filter).to receive(:construct).with("(&(uid=alice)(memberOf=cn=forum-users,ou=groups,dc=example,dc=com))") + + post("/auth/ldap/callback", {username: "alice@example.com", password: "password"}) + + expect(last_response).to be_redirect + expect(last_response.headers["Location"]).to match("invalid_credentials") + end end end @@ -240,6 +256,29 @@ def make_env(path = "/auth/ldap", props = {}) expect(last_response).not_to be_redirect end + + it "escapes special characters in username when building filter" do + allow(@adaptor).to receive(:filter).and_return("uid=%{username}") + # '(' => \28 and ')' => \29 per RFC 4515 escaping + expect(Net::LDAP::Filter).to receive(:construct).with("uid=al\\28ice\\29") + post("/auth/ldap/callback", {username: "al(ice)", password: "secret"}) + end + + it "binds with complex group filter and applies name_proc" do + allow(@adaptor).to receive(:bind_as) { + Net::LDAP::Entry.from_single_ldif_string( + %{dn: cn=alice, dc=example, dc=com +uid: alice +}, + ) + } + allow(@adaptor).to receive(:filter).and_return("(&(uid=%{username})(memberOf=cn=forum-users,ou=groups,dc=example,dc=com))") + expect(Net::LDAP::Filter).to receive(:construct).with("(&(uid=alice)(memberOf=cn=forum-users,ou=groups,dc=example,dc=com))") + + post("/auth/ldap/callback", {username: "alice@example.com", password: "secret"}) + expect(last_response).not_to be_redirect + expect(last_request.env["omniauth.auth"].info.nickname).to eq "alice" + end end it "maps user info to Auth Hash" do @@ -440,11 +479,41 @@ def connection_returning(entry) expect(last_response).not_to be_redirect end + it "escapes special characters in header SSO username when building filter" do + entry = Net::LDAP::Entry.from_single_ldif_string(%{dn: cn=al\\28ice\\29, dc=example, dc=com +uid: al(ice) +}) + allow(@adaptor).to receive_messages( + connection: connection_returning(entry), + filter: "uid=%{username}", + ) + expect(Net::LDAP::Filter).to receive(:construct).with("uid=al\\28ice\\29").and_call_original + + post "/auth/ldap/callback", nil, {"REMOTE_USER" => "al(ice)"} + expect(last_response).not_to be_redirect + end + it "fails when directory lookup returns no entry" do allow(@adaptor).to receive(:connection).and_return(connection_returning(nil)) post "/auth/ldap/callback", nil, {"REMOTE_USER" => "missing"} expect(last_response).to be_redirect expect(last_response.headers["Location"]).to match(/invalid_credentials/) end + + it "supports complex group filter with %{username} in header SSO path" do + # Expect that the complex filter string is constructed with the processed username + expect(Net::LDAP::Filter).to receive(:construct).with("(&(uid=alice)(memberOf=cn=forum-users,ou=groups,dc=example,dc=com))").and_call_original + + entry = Net::LDAP::Entry.from_single_ldif_string(%{dn: cn=alice, dc=example, dc=com +uid: alice +}) + allow(@adaptor).to receive_messages( + filter: "(&(uid=%{username})(memberOf=cn=forum-users,ou=groups,dc=example,dc=com))", + connection: connection_returning(entry), + ) + + post "/auth/ldap/callback", nil, {"REMOTE_USER" => "alice@example.com"} + expect(last_response).not_to be_redirect + end end end