Merge pull request #1128 from ElixirTeSS/url-warning

Warn on inaccessible website URL instead of throwing error

Author: fbacall

app/assets/javascripts/url_checker.js

@@ -10,6 +10,9 @@ var UrlChecker = {
 
         $(this).addClass('loading');
 
+        if (!checkExistsUrl) {
+            return UrlChecker.validUrl(input, url, testValidUrl);
+        }
         $.ajax({
             method: 'POST',
             dataType: 'json',

app/models/profile.rb

@@ -6,7 +6,7 @@ class Profile < ApplicationRecord
 
   before_validation :normalize_orcid
   validates :firstname, :surname, :description, presence: true, if: :public?
-  validates :website, url: true, http_url: true, allow_blank: true
+  validates :website, url: true, http_url: { allow_inaccessible: true }, allow_blank: true
   validates :orcid, orcid: true, allow_blank: true
   after_validation :check_public
   after_commit :reindex_trainer

app/validators/http_url_validator.rb

@@ -1,30 +1,35 @@
 class HttpUrlValidator < ActiveModel::EachValidator
-  def self.blocked?(value)
+  def blocked?(value)
     (TeSS::Config.blocked_domains || []).any? do |regex|
       value =~ regex
     end
   end
 
-  def self.accessible?(value)
+  def accessible?(value)
+    code = nil
     begin
       uri = URI.parse(value) rescue nil
+      return false if PrivateAddressCheck.resolves_to_private_address?(uri.hostname)
       if uri && (uri.scheme == 'http' || uri.scheme == 'https')
         PrivateAddressCheck.only_public_connections do
           res = HTTParty.get(value, { timeout: Rails.env.test? ? 1 : 5 })
-          res.code == 200
+          code = res.code
         end
       end
-    rescue PrivateAddressCheck::PrivateConnectionAttemptedError, Net::OpenTimeout, Net::ReadTimeout, SocketError,
+    rescue PrivateAddressCheck::PrivateConnectionAttemptedError
+      return false
+    rescue Net::OpenTimeout, Net::ReadTimeout, SocketError,
       Errno::ECONNREFUSED, Errno::EHOSTUNREACH, OpenSSL::SSL::SSLError, URI::InvalidURIError
-      false
+      code = 999
     end
+    options[:allow_inaccessible] || code == 200
   end
 
   def validate_each(record, attribute, value)
     if value.present?
-      if self.class.blocked?(value)
+      if blocked?(value)
         record.errors.add(attribute, 'is blocked')
-      elsif !self.class.accessible?(value)
+      elsif !accessible?(value)
         record.errors.add(attribute, 'is not accessible')
       end
     end

app/views/users/_form.html.erb

@@ -14,7 +14,10 @@
 
         <%= f.input :surname, as: :string %>
 
-        <%= f.input :website, as: :string, input_html: { title: t('profile.hints.website') } %>
+        <%# Blank hint is to render a "help-block" in which errors will be displayed.
+            Blank "data-url-check" property is just so the event gets bound %>
+        <%= f.input :website, as: :string, hint: ' ', input_html: { title: t('profile.hints.website'),
+                                                                    data: { 'url-check': '', 'url-valid': test_url_path } } %>
 
         <%= f.input :orcid, as: :string, label: 'ORCID', input_html: { title: t('profile.hints.orcid') } %>
 

test/controllers/application_controller_test.rb

@@ -37,7 +37,7 @@ class ApplicationControllerTest < ActionController::TestCase
     assert_equal 'Could not access the given URL', JSON.parse(response.body)['message']
 
     with_net_connection do
-      get :test_url, params: { url: 'http://127.0.0.1', format: :json }
+      get :test_url, params: { url: 'http://192.168.0.1', format: :json }
       assert_equal 'Could not access the given URL', JSON.parse(response.body)['message']
     end
   end

test/controllers/users_controller_test.rb

@@ -305,8 +305,8 @@ class UsersControllerTest < ActionController::TestCase
 
     # check errors
     profile = assigns(:user).profile
-    assert_equal 3, profile.errors.size, 'invalid number of errors'
-    assert_equal 2, profile.errors.full_messages_for(:website).size, 'invalid error count for: website'
+    assert_equal 2, profile.errors.size, 'invalid number of errors'
+    assert_equal 1, profile.errors.full_messages_for(:website).size, 'invalid error count for: website'
     assert_equal 1, profile.errors.full_messages_for(:orcid).size, 'invalid error count for: orcid'
     assert_equal "Website is not a valid URL", profile.errors.full_messages_for(:website).first
     assert_equal "ORCID isn't a valid ORCID identifier", profile.errors.full_messages_for(:orcid).first

test/models/profile_test.rb

@@ -73,24 +73,22 @@ class ProfileTest < ActiveSupport::TestCase
     # accessible domain
     assert profile.update(website: 'http://200host.com')
 
-    # blocked domain
+    # blocked domain is not OK
     refute profile.update(website: 'https://bad-domain.example/')
     assert profile.errors.added?(:website, 'is blocked')
-    refute profile.errors.added?(:website, 'is not accessible'), 'connection should not be attempted to blocked domains'
 
-    # inaccessible domain
-    refute profile.update(website: 'http://404host.com')
-    assert profile.errors.added?(:website, 'is not accessible')
+    # inaccessible domain is OK
+    assert profile.update(website: 'http://404host.com')
     refute profile.errors.added?(:website, 'is blocked')
 
-    # private address
+    # private address is not OK
     with_net_connection do # Allow request through to be caught by private_address_check
-      refute profile.update(website: 'http://127.0.0.1')
+      refute profile.update(website: 'http://192.168.0.1')
       assert profile.errors.added?(:website, 'is not accessible')
     end
 
-    # address that times out
-    refute profile.update(website: 'http://slowhost.com')
-    assert profile.errors.added?(:website, 'is not accessible')
+    # address that times out is OK
+    assert profile.update(website: 'http://slowhost.com')
+    refute profile.errors.added?(:website, 'is not accessible')
   end
 end