Merge pull request #5636 from coslajohn/admin_restrict

Restrict access to Admin functions by IP Address

.rubocop.yml

@@ -781,6 +781,9 @@ Style/RaiseArgs:
 Style/RedundantArgument:
   Enabled: true
 
+Style/RedundantBegin:
+  Enabled: true
+
 Style/RedundantCondition:
   Enabled: true
 

app/controllers/admin/base_controller.rb

@@ -1,4 +1,5 @@
 class Admin::BaseController < ApplicationController
+  include IpDeniedHandler
   layout "admin"
   before_action :authenticate_user!
 

app/controllers/concerns/ip_denied_handler.rb

@@ -0,0 +1,17 @@
+module IpDeniedHandler
+  extend ActiveSupport::Concern
+
+  included do
+    before_action :restrict_ip, unless: :allowed_ip?
+  end
+
+private
+
+  def restrict_ip
+    redirect_to root_path, alert: t("ip_denied_handler.unauthorized")
+  end
+
+  def allowed_ip?
+    RestrictAdminIps.new(request.remote_ip).allowed?
+  end
+end

app/controllers/graphql_controller.rb

@@ -9,7 +9,6 @@ class GraphqlController < ApplicationController
   class QueryStringError < StandardError; end
 
   def execute
-    begin
     raise GraphqlController::QueryStringError if query_string.nil?
 
     result = ConsulSchema.execute(
@@ -28,7 +27,6 @@ class GraphqlController < ApplicationController
   rescue ArgumentError => e
     render json: { message: e.message }, status: :bad_request
   end
-  end
 
   private
 

app/lib/restrict_admin_ips.rb

@@ -0,0 +1,37 @@
+class RestrictAdminIps
+  attr_reader :ip
+
+  def initialize(ip)
+    @ip = ip
+  end
+
+  def allowed?
+    unrestricted_access? || allowed_ip?
+  end
+
+  private
+
+    def unrestricted_access?
+      allowed_ips.blank?
+    end
+
+    def allowed_ips
+      Array(Tenant.current_secrets.dig(:security, :allowed_admin_ips))
+    end
+
+    def allowed_ip?
+      normalized_allowed_ips.any? { |allowed_ip| allowed_ip.include?(ip) }
+    rescue IPAddr::Error
+      false
+    end
+
+    def normalized_allowed_ips
+      allowed_ips.map do |allowed_ip|
+        IPAddr.new(allowed_ip)
+      rescue IPAddr::Error
+        Rails.logger.warn "Your allowed_admin_ips configuration includes the " \
+                          "address \"#{allowed_ip}\", which is not valid"
+        nil
+      end.compact
+    end
+end

app/models/legislation/annotation.rb

@@ -25,7 +25,6 @@ class Legislation::Annotation < ApplicationRecord
   end
 
   def store_context
-    begin
     html = draft_version.body_html
     doc = Nokogiri::HTML(html)
 
@@ -42,7 +41,6 @@ class Legislation::Annotation < ApplicationRecord
   rescue
     "<span class=annotator-hl>#{quote}</span>"
   end
-  end
 
   def create_first_comment
     comments.create(body: text, user: author)

app/models/machine_learning.rb

@@ -15,7 +15,6 @@ class MachineLearning
   end
 
   def run
-    begin
     export_proposals_to_json
     export_budget_investments_to_json
     export_comments_to_json
@@ -66,7 +65,6 @@ class MachineLearning
     handle_error(e)
     raise e
   end
-  end
   handle_asynchronously :run, queue: "machine_learning"
 
   class << self

config/deploy.rb

@@ -84,7 +84,6 @@ end
 task :install_ruby do
   on roles(:app) do
     within release_path do
-      begin
       current_ruby = capture(:rvm, "current")
     rescue SSHKit::Command::Failed
       after "install_ruby", "rvm1:install:rvm"
@@ -99,12 +98,10 @@ task :install_ruby do
     end
   end
 end
-end
 
 task :install_node do
   on roles(:app) do
     with rails_env: fetch(:rails_env) do
-      begin
       execute fetch(:fnm_install_node_command)
     rescue SSHKit::Command::Failed
       begin
@@ -119,7 +116,6 @@ task :install_node do
     end
   end
 end
-end
 
 task :map_node_bins do
   on roles(:app) do

config/locales/en/general.yml

@@ -759,6 +759,8 @@ en:
     youtube: "%{org} YouTube"
     telegram: "%{org} Telegram"
     instagram: "%{org} Instagram"
+  ip_denied_handler:
+    unauthorized: "Access denied. Your IP address is not allowed."
   unauthorized:
     default: You do not have permission to access this page.
     manage:

config/locales/es/general.yml

@@ -759,6 +759,8 @@ es:
     youtube: "YouTube de %{org}"
     telegram: "Telegram de %{org}"
     instagram: "Instagram de %{org}"
+  ip_denied_handler:
+    unauthorized: "Acceso denegado. Tu IP no tiene permiso para ver este contenido."
   unauthorized:
     default: No tienes permiso para acceder a esta página.
     manage:

config/secrets.yml.example

@@ -23,6 +23,7 @@ development:
   devise_lockable: false
   multitenancy: false
   security:
+    # allowed_admin_ips: ["123.45.67.89", "192.168.1.0/24"]
     last_sign_in: false
     password_complexity: false
     # lockable:
@@ -64,6 +65,7 @@ staging:
   managers_application_key: ""
   multitenancy: false
   security:
+    # allowed_admin_ips: ["123.45.67.89", "192.168.1.0/24"]
     last_sign_in: false
     password_complexity: false
     # lockable:
@@ -118,6 +120,7 @@ preproduction:
   managers_application_key: ""
   multitenancy: false
   security:
+    # allowed_admin_ips: ["123.45.67.89", "192.168.1.0/24"]
     last_sign_in: false
     password_complexity: false
     # lockable:
@@ -171,6 +174,7 @@ production:
   managers_application_key: ""
   multitenancy: false
   security:
+    # allowed_admin_ips: ["123.45.67.89", "192.168.1.0/24"]
     last_sign_in: false
     password_complexity: false
     # lockable:

spec/components/account/sign_in_info_component_spec.rb

@@ -5,9 +5,7 @@ describe Account::SignInInfoComponent do
 
   context "Security secret for render last sign in is enabled" do
     it "shows a sign in info" do
-      allow(Rails.application).to receive(:secrets).and_return(ActiveSupport::OrderedOptions.new.merge(
+      stub_secrets(security: { last_sign_in: true })
-        security: { last_sign_in: true }
-      ))
 
       render_inline Account::SignInInfoComponent.new(account)
 

spec/controllers/admin/base_controller_spec.rb

@@ -0,0 +1,31 @@
+require "rails_helper"
+
+describe Admin::BaseController, :admin do
+  controller do
+    def index
+      render plain: "Index"
+    end
+  end
+
+  describe "#restrict_ip" do
+    before do
+      stub_secrets(security: { allowed_admin_ips: ["1.2.3.4", "5.6.7.8"] })
+    end
+
+    it "renders the content when the IP is allowed" do
+      request.env["REMOTE_ADDR"] = "1.2.3.4"
+      get :index
+
+      expect(response).to be_successful
+      expect(response.body).to eq "Index"
+    end
+
+    it "redirects to the root path when the IP isn't allowed" do
+      request.env["REMOTE_ADDR"] = "9.10.11.12"
+      get :index
+
+      expect(response).to redirect_to root_path
+      expect(flash[:alert]).to eq "Access denied. Your IP address is not allowed."
+    end
+  end
+end

spec/controllers/officing/voters_controller_spec.rb

@@ -11,14 +11,12 @@ describe Officing::VotersController do
 
       2.times.map do
         Thread.new do
-          begin
           post :create, params: {
             voter: { poll_id: poll.id, user_id: user.id },
             format: :js
           }
         rescue ActionDispatch::IllegalStateError
         end
-        end
       end.each(&:join)
 
       expect(Poll::Voter.count).to eq 1

spec/controllers/polls/answers_controller_spec.rb

@@ -8,7 +8,6 @@ describe Polls::AnswersController do
 
       2.times.map do
         Thread.new do
-          begin
           post :create, params: {
             question_id: question.id,
             option_id: question.question_options.find_by(title: "Answer A").id,
@@ -16,7 +15,6 @@ describe Polls::AnswersController do
           }
         rescue ActionDispatch::IllegalStateError, ActiveRecord::RecordInvalid
         end
-        end
       end.each(&:join)
 
       expect(Poll::Answer.count).to eq 1

spec/lib/restrict_admin_ips_spec.rb

@@ -0,0 +1,81 @@
+require "rails_helper"
+
+describe RestrictAdminIps do
+  it "allows any IP when allowed_admin_ips isn't configured" do
+    stub_secrets(security: {})
+
+    expect(RestrictAdminIps.new("1.2.3.4")).to be_allowed
+    expect(RestrictAdminIps.new("whatever")).to be_allowed
+  end
+
+  it "allows any IP when allowed_admin_ips is empty" do
+    stub_secrets(security: { allowed_admin_ips: [] })
+
+    expect(RestrictAdminIps.new("1.2.3.4")).to be_allowed
+    expect(RestrictAdminIps.new("whatever")).to be_allowed
+  end
+
+  it "only allows IPs present in allowed_admin_ips" do
+    stub_secrets(security: { allowed_admin_ips: ["1.2.3.4", "5.6.7.8"] })
+
+    expect(RestrictAdminIps.new("1.2.3.4")).to be_allowed
+    expect(RestrictAdminIps.new("5.6.7.8")).to be_allowed
+    expect(RestrictAdminIps.new("9.9.9.9")).not_to be_allowed
+    expect(RestrictAdminIps.new("whatever")).not_to be_allowed
+  end
+
+  it "restricts every IP when there are only malformed IPs on the list" do
+    stub_secrets(security: { allowed_admin_ips: ["not_an_ip"] })
+
+    expect(RestrictAdminIps.new("1.2.3.4")).not_to be_allowed
+    expect(RestrictAdminIps.new("not_an_ip")).not_to be_allowed
+  end
+
+  it "ignores malformed IPs in the allowed_admin_ips list" do
+    stub_secrets(security: { allowed_admin_ips: ["1.2.3.4", "not_an_ip"] })
+
+    expect(RestrictAdminIps.new("1.2.3.4")).to be_allowed
+    expect(RestrictAdminIps.new("not_an_ip")).not_to be_allowed
+  end
+
+  it "supports ranges of IPs" do
+    stub_secrets(security: { allowed_admin_ips: ["1.2.3.0/16"] })
+
+    expect(RestrictAdminIps.new("1.2.3.4")).to be_allowed
+    expect(RestrictAdminIps.new("1.2.3.5")).to be_allowed
+    expect(RestrictAdminIps.new("5.6.7.8")).not_to be_allowed
+  end
+
+  context "tenant overwriting secrets" do
+    before do
+      stub_secrets({
+        security: {
+          allowed_admin_ips: ["1.2.3.4", "5.6.7.8"]
+        },
+        tenants: {
+          private: {
+            security: {
+              allowed_admin_ips: ["127.0.0.1", "192.168.1.1"]
+            }
+          }
+        }
+      })
+    end
+
+    it "uses the general secrets for the main tenant" do
+      expect(RestrictAdminIps.new("1.2.3.4")).to be_allowed
+      expect(RestrictAdminIps.new("5.6.7.8")).to be_allowed
+      expect(RestrictAdminIps.new("127.0.0.1")).not_to be_allowed
+      expect(RestrictAdminIps.new("192.168.1.1")).not_to be_allowed
+    end
+
+    it "uses the tenant secrets for a tenant" do
+      allow(Tenant).to receive(:current_schema).and_return("private")
+
+      expect(RestrictAdminIps.new("127.0.0.1")).to be_allowed
+      expect(RestrictAdminIps.new("192.168.1.1")).to be_allowed
+      expect(RestrictAdminIps.new("1.2.3.4")).not_to be_allowed
+      expect(RestrictAdminIps.new("5.6.7.8")).not_to be_allowed
+    end
+  end
+end

spec/mailers/mailer_spec.rb

@@ -85,12 +85,12 @@ describe Mailer do
       let(:super_settings) { { address: "super.consul.dev", username: "super" } }
 
       before do
-        allow(Rails.application).to receive(:secrets).and_return(ActiveSupport::OrderedOptions.new.merge(
+        stub_secrets(
           smtp_settings: default_settings,
           tenants: {
             supermailer: { smtp_settings: super_settings }
           }
-        ))
+        )
       end
 
       it "does not overwrite the settings for the default tenant" do

spec/models/poll/answer_spec.rb

@@ -187,11 +187,9 @@ describe Poll::Answer do
 
       [answer, other_answer].map do |poll_answer|
         Thread.new do
-          begin
           poll_answer.save_and_record_voter_participation
         rescue ActiveRecord::RecordInvalid
         end
-        end
       end.each(&:join)
 
       expect(Poll::Voter.count).to be 1

spec/models/tenant_spec.rb

@@ -243,10 +243,7 @@ describe Tenant do
   describe ".current_secrets" do
     context "same secrets for all tenants" do
       before do
-        allow(Rails.application).to receive(:secrets).and_return(ActiveSupport::OrderedOptions.new.merge(
+        stub_secrets(star: "Sun", volume: "Medium")
-          star: "Sun",
-          volume: "Medium"
-        ))
       end
 
       it "returns the default secrets for the default tenant" do
@@ -266,11 +263,11 @@ describe Tenant do
 
     context "tenant overwriting secrets" do
       before do
-        allow(Rails.application).to receive(:secrets).and_return(ActiveSupport::OrderedOptions.new.merge(
+        stub_secrets(
           star: "Sun",
           volume: "Medium",
           tenants: { proxima: { star: "Alpha Centauri" }}
-        ))
+        )
       end
 
       it "returns the default secrets for the default tenant" do

spec/models/user_spec.rb

@@ -994,7 +994,7 @@ describe User do
 
     context "when secrets are configured" do
       before do
-        allow(Rails.application).to receive(:secrets).and_return(ActiveSupport::OrderedOptions.new.merge(
+        stub_secrets(
           security: {
             password_complexity: true
           },
@@ -1005,7 +1005,7 @@ describe User do
               }
             }
           }
-        ))
+        )
       end
 
       it "uses the general secrets for the main tenant" do
@@ -1027,7 +1027,7 @@ describe User do
 
     context "when secrets are configured" do
       before do
-        allow(Rails.application).to receive(:secrets).and_return(ActiveSupport::OrderedOptions.new.merge(
+        stub_secrets(
           security: {
             lockable: { maximum_attempts: "14" }
           },
@@ -1038,7 +1038,7 @@ describe User do
               }
             }
           }
-        ))
+        )
       end
 
       it "uses the general secrets for the main tenant" do
@@ -1060,7 +1060,7 @@ describe User do
 
     context "when secrets are configured" do
       before do
-        allow(Rails.application).to receive(:secrets).and_return(ActiveSupport::OrderedOptions.new.merge(
+        stub_secrets(
           security: {
             lockable: { unlock_in: "2" }
           },
@@ -1071,7 +1071,7 @@ describe User do
               }
             }
           }
-        ))
+        )
       end
 
       it "uses the general secrets for the main tenant" do

spec/support/common_actions.rb

@@ -14,6 +14,7 @@ module CommonActions
   include Polls
   include Proposals
   include RemoteCensusMock
+  include Secrets
   include Tags
   include Translations
   include Users

spec/support/common_actions/graphql_api.rb

@@ -18,14 +18,12 @@ module GraphQLAPI
   def extract_fields(response, collection_name, field_chain)
     fields = field_chain.split(".")
     dig(response, "data.#{collection_name}.edges").map do |node|
-      begin
       if fields.size > 1
         node["node"][fields.first][fields.second]
       else
         node["node"][fields.first]
       end
     rescue NoMethodError
-      end
     end.compact
   end
 end

spec/support/common_actions/secrets.rb

@@ -0,0 +1,7 @@
+module Secrets
+  def stub_secrets(configuration)
+    allow(Rails.application).to receive(:secrets).and_return(
+      ActiveSupport::OrderedOptions.new.merge(configuration)
+    )
+  end
+end

spec/system/users_auth_spec.rb

@@ -661,9 +661,7 @@ describe "Users" do
 
   context "Regular authentication with password complexity enabled" do
     before do
-      allow(Rails.application).to receive(:secrets).and_return(ActiveSupport::OrderedOptions.new.merge(
+      stub_secrets(security: { password_complexity: true })
-        security: { password_complexity: true }
-      ))
     end
 
     context "Sign up" do