-
-
Notifications
You must be signed in to change notification settings - Fork 7k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add LDAP authentication #4032
Add LDAP authentication #4032
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -31,6 +31,7 @@ | |
# last_emailed_at :datetime | ||
# otp_backup_codes :string is an Array | ||
# filtered_languages :string default([]), not null, is an Array | ||
# ldap_dn :text | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. yeah, I agree with Gargron that putting this on the user model doesn't make a lot of sense. How does discourse do it with their pluggable authentication methods? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The way I understand how it works, it's not how devise see things: An example (given in devise's documentation) of the use to have different models is to make a distinction between a simple user and an admin user, which clearly have different roles. In our case, an ldap user and an user would have exactly the same role, it's only the way they authenticate that change. It's sort of the same change as the two factor authentications: it lies on the user object and not outside, because once logged in you are that user. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. ldap_dn is explicitely stated here, because there is no devise LDAP authenticator that "hides" the thing. If you look in the database schema, you'll see otp / 2FA related informations, which may be null in case it's not relevent to that user. Here it's the same, except the attribute is stated explicitely in the model |
||
# | ||
|
||
class User < ApplicationRecord | ||
|
@@ -62,6 +63,7 @@ class User < ApplicationRecord | |
# It seems possible that a future release of devise-two-factor will | ||
# handle this itself, and this can be removed from our User class. | ||
attribute :otp_secret | ||
attribute :ldap_username | ||
|
||
has_many :session_activations, dependent: :destroy | ||
|
||
|
@@ -105,12 +107,24 @@ def session_active?(id) | |
session_activations.active? id | ||
end | ||
|
||
def ldap_user? | ||
Rails.configuration.x.use_ldap && ldap_dn.present? | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. yes, sorry, I was misreading. I was confused because of the attribute :ldap_username, above. |
||
end | ||
|
||
protected | ||
|
||
def send_devise_notification(notification, *args) | ||
devise_mailer.send(notification, self, *args).deliver_later | ||
end | ||
|
||
def confirmation_required? | ||
if ldap_user? | ||
!Rails.configuration.x.ldap_skip_email_confirmation | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. yep There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. uh nope, it would be |
||
else | ||
super | ||
end | ||
end | ||
|
||
private | ||
|
||
def sanitize_languages | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -29,7 +29,9 @@ | |
= render 'registration' | ||
- else | ||
.closed-registrations-message | ||
- if @instance_presenter.closed_registrations_message.blank? | ||
- if Rails.configuration.x.use_ldap | ||
%p= t('about.ldap_registrations') | ||
- elsif @instance_presenter.closed_registrations_message.blank? | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. so in this case, it's not possible to close registrations if you have ldap enabled? seems bad. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I don't think LDAP registrations ever need to be closed, because they're not really registrations - these people already have logins in LDAP, so it is expected that they can login on a connected Mastodon. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yes, if you don't want you users to use mastodon, then you disable them from LDAP, it's no more up to mastodon to decide that. (At least that's how I see LDAP) There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. that makes sense, but if ldap_only is false, then how would you prevent people from signing up with their email addresses? or is that handled elsewhere? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Right, I just understood your point. |
||
%p= t('about.closed_registrations') | ||
- else | ||
!= @instance_presenter.closed_registrations_message | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,109 @@ | ||
require 'net/ldap' | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. the fact that this is required to run before initializers/devise feels like a code smell and a bad hack. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yep, but how can we do otherwise? |
||
require 'devise/strategies/authenticatable' | ||
|
||
module Devise | ||
module Strategies | ||
class LdapAuthenticatable < Authenticatable | ||
def valid? | ||
Rails.configuration.x.use_ldap && params[:user].present? | ||
end | ||
|
||
def authenticate! | ||
config = Rails.configuration | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. maybe a better way to do this would be something like Honestly, it doesn't feel like Rails.configuration.x is the right place to put these variables... There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I believe it is probably okay to simply read the ENV variables in this very class, instead of doing so in an initializer. If nothing but this class will use them. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. devise.rb uses it to decide which key to use (ldap_username or email). I need to figure out how to make it not sensible to ordering though |
||
|
||
ldap = Net::LDAP.new | ||
ldap.host = config.x.ldap_host | ||
ldap.port = config.x.ldap_port | ||
|
||
if config.x.ldap_manager_bind | ||
ldap.auth config.x.ldap_manager_dn, config.x.ldap_manager_password | ||
|
||
if !ldap.bind | ||
return fail(:ldap_configuration_error) | ||
end | ||
|
||
search_filter = config.x.ldap_search_filter % { username: params[:user][:ldap_username] } | ||
|
||
result = ldap.search(base: config.x.ldap_search_base, filter: search_filter, result_set: true) | ||
|
||
if result.count != 1 | ||
return login_fail | ||
end | ||
|
||
user_dn = result.first.dn | ||
user_email = result.first[config.x.ldap_mail_attribute].first | ||
user_name = result.first[config.x.ldap_username_attribute].first | ||
else | ||
user_dn = config.x.ldap_bind_template % { username: params[:user][:ldap_username] } | ||
end | ||
|
||
ldap.auth user_dn, params[:user][:password] | ||
|
||
if ldap.bind | ||
user = User.find_by(ldap_dn: user_dn) | ||
|
||
# We don't want to trust too much the email attribute from | ||
# LDAP: if the user can edit it himself, he may login as | ||
# anyone | ||
if user.nil? | ||
if !config.x.ldap_manager_bind | ||
result = ldap.search(base: user_dn, scope: Net::LDAP::SearchScope_BaseObject, filter: "(objectClass=*)", result_set: true) | ||
user_email = result.first[config.x.ldap_mail_attribute].first | ||
user_name = result.first[config.x.ldap_username_attribute].first | ||
end | ||
|
||
if user_email.present? && User.find_by(email: user_email).nil? | ||
# Password is used for remember_me token | ||
user = User.new(email: user_email, ldap_dn: user_dn, password: SecureRandom.hex, account_attributes: { username: user_name }) | ||
user.locale = I18n.locale | ||
user.build_account if user.account.nil? | ||
user.save | ||
elsif User.find_by(email: user_email).present? | ||
return fail(:ldap_existing_email) | ||
else | ||
return fail(:ldap_cannot_create_account_without_email) | ||
end | ||
end | ||
|
||
success!(user) | ||
else | ||
return login_fail | ||
end | ||
end | ||
|
||
def login_fail | ||
if Rails.configuration.x.ldap_only | ||
return fail(:ldap_invalid_login) | ||
else | ||
return pass | ||
end | ||
end | ||
end | ||
end | ||
end | ||
|
||
Rails.application.configure do | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. these two things should be in separate files. |
||
config.x.use_ldap = ENV.fetch('USE_LDAP') { nil }.present? | ||
if config.x.use_ldap | ||
config.x.ldap_host = ENV.fetch('LDAP_HOST') { 'localhost' } | ||
config.x.ldap_port = ENV.fetch('LDAP_PORT') { 389 } | ||
config.x.ldap_only = ENV.fetch('LDAP_ONLY') { nil }.present? | ||
|
||
config.x.ldap_manager_bind = (ENV.fetch('LDAP_BIND_DN') { '' }).present? | ||
|
||
config.x.ldap_username_attribute = ENV.fetch('LDAP_USERNAME_ATTRIBUTE') { 'uid' } | ||
config.x.ldap_mail_attribute = ENV.fetch('LDAP_MAIL_ATTRIBUTE') { 'mail' } | ||
config.x.ldap_skip_email_confirmation = ENV.fetch('LDAP_SKIP_EMAIL_CONFIRMATION') { true } | ||
|
||
if config.x.ldap_manager_bind | ||
config.x.ldap_manager_dn = ENV.fetch('LDAP_BIND_DN') { '' } | ||
config.x.ldap_manager_password = ENV.fetch('LDAP_BIND_PW') { '' } | ||
config.x.ldap_search_filter = ENV.fetch('LDAP_SEARCH_FILTER') { 'uid=%{username}' } | ||
config.x.ldap_search_base = ENV.fetch('LDAP_SEARCH_BASE') { 'dc=example,dc=com' } | ||
else | ||
config.x.ldap_bind_template = ENV.fetch('LDAP_BIND_TEMPLATE') { 'uid=%{username},dc=example,dc=com' } | ||
end | ||
end | ||
end | ||
|
||
Warden::Strategies.add(:ldap_authenticatable, Devise::Strategies::LdapAuthenticatable) |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -18,6 +18,7 @@ | |
config.warden do |manager| | ||
manager.default_strategies(scope: :user).unshift :two_factor_authenticatable | ||
manager.default_strategies(scope: :user).unshift :two_factor_backupable | ||
manager.default_strategies(scope: :user).unshift :ldap_authenticatable | ||
end | ||
|
||
# The secret key used by Devise. Devise uses this key to generate | ||
|
@@ -50,7 +51,11 @@ | |
# session. If you need permissions, you should implement that in a before filter. | ||
# You can also supply a hash where the value is a boolean determining whether | ||
# or not authentication should be aborted when the value is not present. | ||
# config.authentication_keys = [:email] | ||
if Rails.configuration.x.use_ldap | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. this doesn't handle ldap_only properly, if i'm reading this correctly. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. the "ldap_username" should be named "ldap_username_or_email" actually, but it seems very long. Or yes we could have three keys depending on "ldap_only", "ldap", "no_ldap" ? |
||
config.authentication_keys = [:ldap_username] | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. is there a way to delay this configuration until later, so we don't have to worry about the load order? (and remove the ugly There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I tried in vain, but as I said I have little experience with that, if someone knows better... |
||
else | ||
config.authentication_keys = [:email] | ||
end | ||
|
||
# Configure parameters from the request object used for authentication. Each entry | ||
# given should be a request method and it will automatically be passed to the | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,6 @@ | ||
class AddLdapDnToUsers < ActiveRecord::Migration[5.0] | ||
def change | ||
add_column :users, :ldap_dn, :text, null: true, default: nil | ||
add_index :users, :ldap_dn | ||
end | ||
end |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this should probably be it's own method rather then overloading this one.