LDAP start tls support closes #9952

make start tls the default if not otherwise specified in the API, and
the default for new integrations in the UI. still support booleans
in the API.

test plan:
 * test non-tls, simple tls, and start tls LDAP servers
 * ensure new LDAP settings default to start tls

Change-Id: I60b2f2d6cbdd32beff14d198c92efbfd6705b041
Reviewed-on: https://gerrit.instructure.com/12923
Reviewed-by: Cody Cutrer <cody@instructure.com>
Tested-by: Cody Cutrer <cody@instructure.com>

app/controllers/account_authorization_configs_controller.rb

@@ -24,6 +24,7 @@ class AccountAuthorizationConfigsController < ApplicationController
     @account_configs = @account.account_authorization_configs.to_a
     while @account_configs.length < 2
       @account_configs << @account.account_authorization_configs.new
+      @account_configs.last.auth_over_tls = :start_tls
     end
     @saml_identifiers = Onelogin::Saml::NameIdentifiers::ALL_IDENTIFIERS
     @saml_login_attributes = AccountAuthorizationConfig.saml_login_attributes
@@ -102,10 +103,11 @@ class AccountAuthorizationConfigsController < ApplicationController
   #
   #   The LDAP server's TCP port. (default: 389)
   #
-  # - auth_over_tls [Optional, Boolean]
+  # - auth_over_tls [Optional]
   #
-  #   Whether to use simple TLS encryption. Only simple TLS encryption is
-  #   supported at this time. (default: false)
+  #   Whether to use TLS. Can be '', 'simple_tls', or 'start_tls'. For backwards
+  #   compatibility, booleans are also accepted, with true meaning simple_tls.
+  #   If not provided, it will default to start_tls.
   #
   # - auth_base [Optional]
   #
@@ -322,6 +324,12 @@ class AccountAuthorizationConfigsController < ApplicationController
   end
 
   def filter_data(data)
-    data ? data.slice(*recognized_params(data[:auth_type])) : {}
+    data ||= {}
+    data = data.slice(*recognized_params(data[:auth_type]))
+    if data[:auth_type] == 'ldap'
+      data[:auth_over_tls] = 'start_tls' unless data.has_key?(:auth_over_tls)
+      data[:auth_over_tls] = AccountAuthorizationConfig.auth_over_tls_setting(data[:auth_over_tls])
+    end
+    data
   end
 end

app/models/account_authorization_config.rb

@@ -35,10 +35,27 @@ class AccountAuthorizationConfig < ActiveRecord::Base
   # if the config changes, clear out last_timeout_failure so another attempt can be made immediately
   before_save :clear_last_timeout_failure
 
+  def self.auth_over_tls_setting(value)
+    case value
+      when nil, '', false, 'false', 'f', 0, '0'
+        nil
+      when true, 'true', 't', 1, '1', 'simple_tls', :simple_tls
+        'simple_tls'
+      when 'start_tls', :start_tls
+        'start_tls'
+      else
+        raise ArgumentError("invalid auth_over_tls setting: #{value}")
+    end
+  end
+
+  def auth_over_tls
+    self.class.auth_over_tls_setting(read_attribute(:auth_over_tls))
+  end
+
   def ldap_connection
     raise "Not an LDAP config" unless self.auth_type == 'ldap'
     require 'net/ldap'
-    ldap = Net::LDAP.new(:encryption => (self.auth_over_tls ? :simple_tls : nil))
+    ldap = Net::LDAP.new(:encryption => self.auth_over_tls.try(:to_sym))
     ldap.host = self.auth_host
     ldap.port = self.auth_port
     ldap.base = self.auth_base

app/views/account_authorization_configs/_ldap_settings_fields.html.erb

@@ -18,8 +18,22 @@
 <tr>
   <td style="vertical-align: top; width: 200px;"><%= f.blabel :auth_over_tls, :en => "Over TLS?" %></td>
   <td style="vertical-align: top;" class="nobr">
-    <%= f.check_box :auth_over_tls, :class => "auth_form" %>
-    <span class="auth_info auth_over_tls"><%= account_config.auth_over_tls %></span>
+    <%= f.radio_button :auth_over_tls, 'start_tls', :class => "auth_form" %>
+    <%= f.label :auth_over_tls_start_tls, :auth_over_tls_start_tls, :en => "StartTLS", :class => "auth_form" %>
+    <%= f.radio_button :auth_over_tls, 'simple_tls', :class => "auth_form" %>
+    <%= f.label :auth_over_tls_simple_tls, :auth_over_tls_simple_tls, :en => "Simple TLS", :class => "auth_form" %>
+    <%= f.radio_button :auth_over_tls, 'false', :class => "auth_form", :checked => !account_config.auth_over_tls %>
+    <%= f.label :auth_over_tls_false, :auth_over_tls_false, :en => "No TLS", :class => "auth_form" %>
+    <span class="auth_info auth_over_tls">
+    <%= case account_config.auth_over_tls
+        when 'start_tls'
+          t 'labels.start_tls', "StartTLS"
+        when 'simple_tls'
+          t 'labels.simple_tls', "Simple TLS"
+        else
+          t 'labels.no_tls', "No TLS"
+        end
+     %></span>
   </td>
 </tr>
 <tr>

db/migrate/20120814205244_change_auth_over_tls_to_string.rb

@@ -0,0 +1,15 @@
+class ChangeAuthOverTlsToString < ActiveRecord::Migration
+  tag :predeploy
+
+  def self.up
+    # existing Rails process will continue seeing it as boolean until they restart;
+    # this is fine, since they fetch as a string anyway
+    change_column :account_authorization_configs, :auth_over_tls, :string
+  end
+
+  def self.down
+    # technically it is reversible, but requires db specific syntax in postgres,
+    # and probably in mysql as well
+    raise ActiveRecord::IrreversibleMigration
+  end
+end

spec/selenium/accounts_spec.rb

@@ -19,7 +19,7 @@ describe "account" do
 
       ldap_form.find_element(:id, 'account_authorization_config_0_auth_host').send_keys('primary.host.example.com')
       ldap_form.find_element(:id, 'account_authorization_config_0_auth_port').send_keys('1')
-      ldap_form.find_element(:id, 'account_authorization_config_0_auth_over_tls').click
+      ldap_form.find_element(:id, 'account_authorization_config_0_auth_over_tls_simple_tls').click
       ldap_form.find_element(:id, 'account_authorization_config_0_auth_base').send_keys('primary base')
       ldap_form.find_element(:id, 'account_authorization_config_0_auth_filter').send_keys('primary filter')
       ldap_form.find_element(:id, 'account_authorization_config_0_auth_username').send_keys('primary username')
@@ -32,7 +32,7 @@ describe "account" do
       config = Account.default.account_authorization_configs.first
       config.auth_host.should == 'primary.host.example.com'
       config.auth_port.should == 1
-      config.auth_over_tls.should == true
+      config.auth_over_tls.should == 'simple_tls'
       config.auth_base.should == 'primary base'
       config.auth_filter.should == 'primary filter'
       config.auth_username.should == 'primary username'
@@ -51,16 +51,18 @@ describe "account" do
       ldap_form.find_element(:id, 'account_authorization_config_1_auth_filter').send_keys('secondary filter')
       ldap_form.find_element(:id, 'account_authorization_config_1_auth_username').send_keys('secondary username')
       ldap_form.find_element(:id, 'account_authorization_config_1_auth_password').send_keys('secondary password')
+
       expect_new_page_load { submit_form(ldap_form) }
 
       Account.default.account_authorization_configs.length.should == 2
       config = Account.default.account_authorization_configs.first
       config.auth_host.should == 'primary.host.example.com'
+      config.auth_over_tls.should == 'simple_tls'
 
       config = Account.default.account_authorization_configs[1]
       config.auth_host.should == 'secondary.host.example.com'
       config.auth_port.should == 2
-      config.auth_over_tls.should == false
+      config.auth_over_tls.should == 'start_tls'
       config.auth_base.should == 'secondary base'
       config.auth_filter.should == 'secondary filter'
       config.auth_username.should == 'secondary username'