From 254a8364d2f59ccddc701755ba97a383968adfb2 Mon Sep 17 00:00:00 2001 From: Andy Allan Date: Wed, 9 Sep 2026 19:52:56 +0100 Subject: [PATCH] Create a UI for managing ACLs This allows administrators to manage ACLs, without having to either use the console or use the postgresql cli. --- app/abilities/ability.rb | 1 + app/controllers/acls_controller.rb | 57 ++++++ app/helpers/acls_helper.rb | 17 ++ app/models/acl.rb | 1 + app/validators/inet_validator.rb | 11 ++ app/views/acls/_acl.html.erb | 15 ++ app/views/acls/_form.html.erb | 11 ++ app/views/acls/_navigation.html.erb | 12 ++ app/views/acls/edit.html.erb | 7 + app/views/acls/index.html.erb | 27 +++ app/views/acls/new.html.erb | 7 + config/locales/en.yml | 44 ++++- config/routes.rb | 3 + test/abilities/administrator_ability_test.rb | 8 + test/abilities/moderator_ability_test.rb | 8 + test/abilities/user_ability_test.rb | 8 + test/controllers/acls_controller_test.rb | 176 +++++++++++++++++++ test/helpers/acls_helper_test.rb | 13 ++ test/models/acl_test.rb | 13 ++ test/system/acls_test.rb | 99 +++++++++++ test/validators/inet_validator_test.rb | 28 +++ 21 files changed, 565 insertions(+), 1 deletion(-) create mode 100644 app/controllers/acls_controller.rb create mode 100644 app/helpers/acls_helper.rb create mode 100644 app/validators/inet_validator.rb create mode 100644 app/views/acls/_acl.html.erb create mode 100644 app/views/acls/_form.html.erb create mode 100644 app/views/acls/_navigation.html.erb create mode 100644 app/views/acls/edit.html.erb create mode 100644 app/views/acls/index.html.erb create mode 100644 app/views/acls/new.html.erb create mode 100644 test/controllers/acls_controller_test.rb create mode 100644 test/helpers/acls_helper_test.rb create mode 100644 test/system/acls_test.rb create mode 100644 test/validators/inet_validator_test.rb diff --git a/app/abilities/ability.rb b/app/abilities/ability.rb index 9953cb0f5..8669b046a 100644 --- a/app/abilities/ability.rb +++ b/app/abilities/ability.rb @@ -78,6 +78,7 @@ class Ability can [:update], :user_status can [:read, :update], :users_list can [:create, :destroy], UserRole + can [:read, :create, :update, :destroy], Acl end end end diff --git a/app/controllers/acls_controller.rb b/app/controllers/acls_controller.rb new file mode 100644 index 000000000..2abfee729 --- /dev/null +++ b/app/controllers/acls_controller.rb @@ -0,0 +1,57 @@ +# frozen_string_literal: true + +class AclsController < ApplicationController + layout :site_layout + + before_action :authorize_web + before_action :set_locale + + authorize_resource + + before_action :check_database_readable + before_action :check_database_writable, :except => [:index] + before_action :set_acl, :only => [:edit, :update, :destroy] + + def index + @acls = Acl.order(:id) + end + + def new + @acl = Acl.new + end + + def edit; end + + def create + @acl = Acl.new(acl_params) + + if @acl.save + redirect_to acls_path, :notice => t(".success") + else + render :new, :status => :unprocessable_content + end + end + + def update + if @acl.update(acl_params) + redirect_to acls_path, :notice => t(".success"), :status => :see_other + else + render :edit, :status => :unprocessable_content + end + end + + def destroy + @acl.destroy + redirect_to acls_path, :notice => t(".success"), :status => :see_other + end + + private + + def set_acl + @acl = Acl.find(params.expect(:id)) + end + + def acl_params + params.expect(:acl => [:address, :domain, :mx, :k, :v]) + end +end diff --git a/app/helpers/acls_helper.rb b/app/helpers/acls_helper.rb new file mode 100644 index 000000000..7d5c03f13 --- /dev/null +++ b/app/helpers/acls_helper.rb @@ -0,0 +1,17 @@ +# frozen_string_literal: true + +module AclsHelper + # Format an IPAddr the way PostgreSQL's inet type does: host addresses + # are shown bare, while networks carry their prefix length. + def acl_address(address) + return nil if address.nil? + + host_prefix = address.ipv4? ? 32 : 128 + + if address.prefix == host_prefix + address.to_s + else + "#{address}/#{address.prefix}" + end + end +end diff --git a/app/models/acl.rb b/app/models/acl.rb index e3079718d..9b13ba074 100644 --- a/app/models/acl.rb +++ b/app/models/acl.rb @@ -21,6 +21,7 @@ class Acl < ApplicationRecord validates :k, :presence => true + validates :address, :inet => true def self.match(address, options = {}) acls = Acl.where("address >>= ?", address) diff --git a/app/validators/inet_validator.rb b/app/validators/inet_validator.rb new file mode 100644 index 000000000..71f69d246 --- /dev/null +++ b/app/validators/inet_validator.rb @@ -0,0 +1,11 @@ +# frozen_string_literal: true + +# Validates that an inet/cidr column was given parseable input. ActiveRecord +# casts anything unparseable to nil, so the cast value alone can't tell a +# blank field from a typo; compare it against the raw input instead. +class InetValidator < ActiveModel::EachValidator + def validate_each(record, attribute, value) + raw = record.read_attribute_before_type_cast(attribute) + record.errors.add(attribute, options[:message] || :invalid) if raw.present? && value.nil? + end +end diff --git a/app/views/acls/_acl.html.erb b/app/views/acls/_acl.html.erb new file mode 100644 index 000000000..158fe428d --- /dev/null +++ b/app/views/acls/_acl.html.erb @@ -0,0 +1,15 @@ +<%# locals: (acl:) %> + + + <%= acl_address(acl.address) %> + <%= acl.domain %> + <%= acl.mx %> + <%= acl.k %> + <%= acl.v %> + +
+ <%= link_to t(".edit_button"), edit_acl_path(acl), :class => "btn btn-sm btn-primary" %> + <%= button_to t(".destroy_button"), acl_path(acl), :method => :delete, :class => "btn btn-sm btn-danger", :form => { :data => { :turbo => true, :turbo_confirm => t(".confirm") } } %> +
+ + diff --git a/app/views/acls/_form.html.erb b/app/views/acls/_form.html.erb new file mode 100644 index 000000000..e02c7d0df --- /dev/null +++ b/app/views/acls/_form.html.erb @@ -0,0 +1,11 @@ +<%# locals: (acl:) %> + +<%= bootstrap_form_with(:model => acl) do |form| %> + <%= form.text_field :address, :value => acl.address_before_type_cast %> + <%= form.text_field :domain %> + <%= form.text_field :mx %> + <%= form.text_field :k %> + <%= form.text_field :v %> + <%= form.primary %> + <%= link_to t(".cancel"), acls_path, :class => "btn btn-link" %> +<% end %> diff --git a/app/views/acls/_navigation.html.erb b/app/views/acls/_navigation.html.erb new file mode 100644 index 000000000..76aa7d8f0 --- /dev/null +++ b/app/views/acls/_navigation.html.erb @@ -0,0 +1,12 @@ +<%# locals: () %> + + diff --git a/app/views/acls/edit.html.erb b/app/views/acls/edit.html.erb new file mode 100644 index 000000000..960d864a0 --- /dev/null +++ b/app/views/acls/edit.html.erb @@ -0,0 +1,7 @@ +<% @title = t(".title") %> + +<% content_for :heading do %> +

<%= @title %>

+<% end %> + +<%= render "form", :acl => @acl %> diff --git a/app/views/acls/index.html.erb b/app/views/acls/index.html.erb new file mode 100644 index 000000000..aa32b5215 --- /dev/null +++ b/app/views/acls/index.html.erb @@ -0,0 +1,27 @@ +<% @title = t(".title") %> + +<% content_for :heading_class, "pb-0" %> +<% content_for :heading do %> +

<%= @title %>

+ <%= render "navigation" %> +<% end %> + +<% if @acls.empty? %> +

<%= t(".empty") %>

+<% else %> + + + + + + + + + + + + + <%= render @acls %> + +
<%= t(".address") %><%= t(".domain") %><%= t(".mx") %><%= t(".k") %><%= t(".v") %><%= t(".actions") %>
+<% end %> diff --git a/app/views/acls/new.html.erb b/app/views/acls/new.html.erb new file mode 100644 index 000000000..960d864a0 --- /dev/null +++ b/app/views/acls/new.html.erb @@ -0,0 +1,7 @@ +<% @title = t(".title") %> + +<% content_for :heading do %> +

<%= @title %>

+<% end %> + +<%= render "form", :acl => @acl %> diff --git a/config/locales/en.yml b/config/locales/en.yml index a301d3a41..03331d51d 100644 --- a/config/locales/en.yml +++ b/config/locales/en.yml @@ -55,7 +55,7 @@ en: cannot_change_to_legacy: "can't be changed back to a legacy value" # Translates all the model names, which is used in error handling on the website models: - acl: "Access Control List" + acl: "ACL" changeset: "Changeset" changeset_tag: "Changeset Tag" country: "Country" @@ -93,6 +93,12 @@ en: # Translates all the model attributes, which is used in error handling on the website # Only the ones that are used on the website are translated at the moment attributes: + acl: + address: Address + domain: Domain + mx: MX server + k: Key + v: Value diary_comment: body: "Body" diary_entry: @@ -155,6 +161,12 @@ en: pass_crypt: "Password" pass_crypt_confirmation: "Confirm Password" help: + acl: + address: An IP address or CIDR network, e.g. 192.0.2.0/24 + domain: An email domain; subdomains are matched too + mx: The host name of an MX server for the email domain + k: "One of: no_account_creation, allow_account_creation, no_note_comment, no_trace_download" + v: Optional note, e.g. the reason for this entry doorkeeper/application: confidential: "Application will be used where the client secret can be kept confidential (native mobile apps and single page apps are not confidential)" redirect_uri: "Use one line per URI" @@ -3914,6 +3926,36 @@ en: not_empty: "Redaction is not empty. Please un-redact all versions belonging to this redaction before destroying it." flash: "Redaction destroyed." error: "There was an error destroying this redaction." + acls: + navigation: + new_title: Add a new ACL + new: New ACL + actions: ACL actions + index: + title: ACLs + empty: No ACLs to show. + address: Address + domain: Domain + mx: MX server + k: Key + v: Value + actions: Actions + acl: + edit_button: Edit + destroy_button: Delete + confirm: Are you sure? + form: + cancel: Cancel + new: + title: New ACL + create: + success: ACL created. + edit: + title: Editing ACL + update: + success: ACL updated. + destroy: + success: ACL deleted. validations: leading_whitespace: "has leading whitespace" trailing_whitespace: "has trailing whitespace" diff --git a/config/routes.rb b/config/routes.rb index 60cd94f0a..28279ee18 100644 --- a/config/routes.rb +++ b/config/routes.rb @@ -432,6 +432,9 @@ OpenStreetMap::Application.routes.draw do # redactions resources :redactions + # ACLs + resources :acls, :except => [:show] + # moderation zones resources :moderation_zones, :only => [:index, :new, :create, :edit, :update] diff --git a/test/abilities/administrator_ability_test.rb b/test/abilities/administrator_ability_test.rb index 27ba4fb95..3ee233754 100644 --- a/test/abilities/administrator_ability_test.rb +++ b/test/abilities/administrator_ability_test.rb @@ -21,4 +21,12 @@ class AdministratorAbilityTest < ActiveSupport::TestCase assert ability.can?(action, UserRole), "should be able to #{action} UserRoles" end end + + test "ACL permissions for an administrator" do + ability = Ability.new create(:administrator_user) + + [:index, :create, :edit, :update, :destroy].each do |action| + assert ability.can?(action, Acl), "should be able to #{action} ACLs" + end + end end diff --git a/test/abilities/moderator_ability_test.rb b/test/abilities/moderator_ability_test.rb index b93c0c5f9..37c89f3fb 100644 --- a/test/abilities/moderator_ability_test.rb +++ b/test/abilities/moderator_ability_test.rb @@ -24,6 +24,14 @@ class ModeratorAbilityTest < ActiveSupport::TestCase end end + test "ACL permissions" do + ability = Ability.new create(:moderator_user) + + [:index, :create, :edit, :update, :destroy].each do |action| + assert ability.cannot?(action, Acl), "should not be able to #{action} ACLs" + end + end + test "Active block update permissions" do creator_user = create(:moderator_user) other_moderator_user = create(:moderator_user) diff --git a/test/abilities/user_ability_test.rb b/test/abilities/user_ability_test.rb index da6b71ef3..384b5ea26 100644 --- a/test/abilities/user_ability_test.rb +++ b/test/abilities/user_ability_test.rb @@ -22,4 +22,12 @@ class UserAbilityTest < ActiveSupport::TestCase assert ability.cannot?(action, Issue), "should not be able to #{action} Issues" end end + + test "ACL permissions" do + ability = Ability.new create(:user) + + [:index, :create, :edit, :update, :destroy].each do |action| + assert ability.cannot?(action, Acl), "should not be able to #{action} ACLs" + end + end end diff --git a/test/controllers/acls_controller_test.rb b/test/controllers/acls_controller_test.rb new file mode 100644 index 000000000..ea27cfc8b --- /dev/null +++ b/test/controllers/acls_controller_test.rb @@ -0,0 +1,176 @@ +# frozen_string_literal: true + +require "test_helper" + +class AclsControllerTest < ActionDispatch::IntegrationTest + ## + # test all routes which lead to this controller + def test_routes + assert_routing( + { :path => "/acls", :method => :get }, + { :controller => "acls", :action => "index" } + ) + assert_routing( + { :path => "/acls/new", :method => :get }, + { :controller => "acls", :action => "new" } + ) + assert_routing( + { :path => "/acls", :method => :post }, + { :controller => "acls", :action => "create" } + ) + assert_routing( + { :path => "/acls/1/edit", :method => :get }, + { :controller => "acls", :action => "edit", :id => "1" } + ) + assert_routing( + { :path => "/acls/1", :method => :put }, + { :controller => "acls", :action => "update", :id => "1" } + ) + assert_routing( + { :path => "/acls/1", :method => :delete }, + { :controller => "acls", :action => "destroy", :id => "1" } + ) + end + + def test_index + get acls_path + assert_redirected_to login_path(:referer => acls_path) + end + + def test_index_non_administrator + session_for(create(:user)) + + get acls_path + assert_redirected_to :controller => "errors", :action => "forbidden" + end + + def test_index_administrator + session_for(create(:administrator_user)) + + get acls_path + assert_response :success + assert_template :index + end + + def test_new + get new_acl_path + assert_redirected_to login_path(:referer => new_acl_path) + end + + def test_new_non_administrator + session_for(create(:user)) + + get new_acl_path + assert_redirected_to :controller => "errors", :action => "forbidden" + end + + def test_new_administrator + session_for(create(:administrator_user)) + + get new_acl_path + assert_response :success + assert_template :new + end + + def test_create + assert_no_difference "Acl.count" do + post acls_path(:acl => { :address => "192.0.2.0/24", :k => "no_account_creation" }) + end + assert_response :forbidden + end + + def test_create_non_administrator + session_for(create(:user)) + + assert_no_difference "Acl.count" do + post acls_path(:acl => { :address => "192.0.2.0/24", :k => "no_account_creation" }) + end + assert_redirected_to :controller => "errors", :action => "forbidden" + end + + def test_create_administrator + session_for(create(:administrator_user)) + + assert_difference "Acl.count", 1 do + post acls_path(:acl => { :address => "192.0.2.0/24", :k => "no_account_creation" }) + end + assert_redirected_to acls_path + end + + def test_edit + acl = create(:acl) + + get edit_acl_path(acl) + assert_redirected_to login_path(:referer => edit_acl_path(acl)) + end + + def test_edit_non_administrator + session_for(create(:user)) + + get edit_acl_path(create(:acl)) + assert_redirected_to :controller => "errors", :action => "forbidden" + end + + def test_edit_administrator + session_for(create(:administrator_user)) + + get edit_acl_path(create(:acl)) + assert_response :success + assert_template :edit + end + + def test_update + acl = create(:acl, :k => "no_account_creation") + + put acl_path(acl, :acl => { :k => "no_note_comment" }) + assert_response :forbidden + assert_equal "no_account_creation", acl.reload.k + end + + def test_update_non_administrator + session_for(create(:user)) + acl = create(:acl, :k => "no_account_creation") + + put acl_path(acl, :acl => { :k => "no_note_comment" }) + assert_redirected_to :controller => "errors", :action => "forbidden" + assert_equal "no_account_creation", acl.reload.k + end + + def test_update_administrator + session_for(create(:administrator_user)) + acl = create(:acl, :k => "no_account_creation") + + put acl_path(acl, :acl => { :k => "no_note_comment" }) + assert_redirected_to acls_path + assert_equal "no_note_comment", acl.reload.k + end + + def test_destroy + acl = create(:acl) + + assert_no_difference "Acl.count" do + delete acl_path(acl) + end + assert_response :forbidden + end + + def test_destroy_non_administrator + session_for(create(:user)) + acl = create(:acl) + + assert_no_difference "Acl.count" do + delete acl_path(acl) + end + assert_redirected_to :controller => "errors", :action => "forbidden" + end + + def test_destroy_administrator + session_for(create(:administrator_user)) + acl = create(:acl) + + assert_difference "Acl.count", -1 do + delete acl_path(acl) + end + assert_redirected_to acls_path + end +end diff --git a/test/helpers/acls_helper_test.rb b/test/helpers/acls_helper_test.rb new file mode 100644 index 000000000..42c855448 --- /dev/null +++ b/test/helpers/acls_helper_test.rb @@ -0,0 +1,13 @@ +# frozen_string_literal: true + +require "test_helper" + +class AclsHelperTest < ActionView::TestCase + def test_acl_address + assert_nil acl_address(nil) + assert_equal "192.0.2.7", acl_address(IPAddr.new("192.0.2.7")) + assert_equal "192.0.2.0/24", acl_address(IPAddr.new("192.0.2.0/24")) + assert_equal "2001:db8::1", acl_address(IPAddr.new("2001:db8::1")) + assert_equal "2001:db8::/32", acl_address(IPAddr.new("2001:db8::/32")) + end +end diff --git a/test/models/acl_test.rb b/test/models/acl_test.rb index 79a1422be..f596fe77a 100644 --- a/test/models/acl_test.rb +++ b/test/models/acl_test.rb @@ -10,6 +10,19 @@ class AclTest < ActiveSupport::TestCase assert_not_predicate acl, :valid? end + def test_address_must_be_valid + acl = build(:acl, :address => "192.0.2.0/24") + assert_predicate acl, :valid? + + acl = build(:acl, :address => "") + assert_predicate acl, :valid? + assert_nil acl.address + + acl = build(:acl, :address => "not-an-address") + assert_not_predicate acl, :valid? + assert_includes acl.errors[:address], "is invalid" + end + def test_no_account_creation_by_subnet assert_not Acl.no_account_creation?("192.168.1.1") create(:acl, :address => "192.168.0.0/16", :k => "no_account_creation") diff --git a/test/system/acls_test.rb b/test/system/acls_test.rb new file mode 100644 index 000000000..b6c0d3a3d --- /dev/null +++ b/test/system/acls_test.rb @@ -0,0 +1,99 @@ +# frozen_string_literal: true + +require "application_system_test_case" + +class AclsTest < ApplicationSystemTestCase + test "index shows a placeholder when there are no acls" do + sign_in_as(create(:administrator_user)) + + visit acls_path + assert_title "ACLs" + assert_text "No ACLs to show." + end + + test "index lists acls with formatted addresses" do + create(:acl, :address => "192.0.2.0/24", :k => "no_account_creation", :v => "spam") + create(:acl, :address => "198.51.100.7", :k => "no_note_comment") + create(:acl, :domain => "example.com", :mx => "mail.example.com", :k => "allow_account_creation") + sign_in_as(create(:administrator_user)) + + visit acls_path + within_table "acl_list" do + assert_selector "tbody tr", :count => 3 + assert_selector "tr", :text => "192.0.2.0/24 no_account_creation spam" + assert_selector "tr", :text => "198.51.100.7 no_note_comment" + assert_selector "tr", :text => "example.com mail.example.com allow_account_creation" + end + end + + test "create an acl" do + sign_in_as(create(:administrator_user)) + + visit acls_path + click_on "New ACL" + assert_title "New ACL" + + fill_in "Address", :with => "192.0.2.0/24" + fill_in "Domain", :with => "example.com" + fill_in "MX server", :with => "mail.example.com" + fill_in "Key", :with => "no_account_creation" + fill_in "Value", :with => "spam" + click_on "Create ACL" + + assert_text "ACL created." + within_table "acl_list" do + assert_selector "tr", :text => "192.0.2.0/24 example.com mail.example.com no_account_creation spam" + end + end + + test "create an acl with an invalid address" do + sign_in_as(create(:administrator_user)) + + visit new_acl_path + fill_in "Address", :with => "not-an-address" + fill_in "Key", :with => "no_account_creation" + click_on "Create ACL" + + assert_title "New ACL" + assert_text "is invalid" + assert_field "Address", :with => "not-an-address" + assert_equal 0, Acl.count + end + + test "edit an acl" do + create(:acl, :address => "192.0.2.0/24", :k => "no_account_creation") + sign_in_as(create(:administrator_user)) + + visit acls_path + within_table "acl_list" do + click_on "Edit" + end + assert_title "Editing ACL" + assert_field "Address", :with => "192.0.2.0/24" + assert_field "Key", :with => "no_account_creation" + + fill_in "Address", :with => "198.51.100.7" + fill_in "Key", :with => "no_note_comment" + click_on "Update ACL" + + assert_text "ACL updated." + within_table "acl_list" do + assert_selector "tr", :text => "198.51.100.7 no_note_comment" + assert_no_text "192.0.2.0/24" + end + end + + test "delete an acl" do + create(:acl, :address => "192.0.2.0/24", :k => "no_account_creation") + sign_in_as(create(:administrator_user)) + + visit acls_path + accept_confirm do + click_on "Delete" + end + + assert_text "ACL deleted." + assert_text "No ACLs to show." + assert_equal 0, Acl.count + end +end diff --git a/test/validators/inet_validator_test.rb b/test/validators/inet_validator_test.rb new file mode 100644 index 000000000..0feded531 --- /dev/null +++ b/test/validators/inet_validator_test.rb @@ -0,0 +1,28 @@ +# frozen_string_literal: true + +require "test_helper" + +class InetValidatorTest < ActiveSupport::TestCase + def test_valid_addresses + ["192.0.2.7", "192.0.2.0/24", "2001:db8::1", "2001:db8::/32"].each do |address| + acl = Acl.new(:k => "test", :address => address) + assert_predicate acl, :valid?, "'#{address}' should be valid" + end + end + + def test_blank_addresses + [nil, ""].each do |address| + acl = Acl.new(:k => "test", :address => address) + assert_predicate acl, :valid?, "#{address.inspect} should be valid" + assert_nil acl.address + end + end + + def test_invalid_addresses + ["not-an-address", "192.0.2", "192.0.2.256", "192.0.2.0/33"].each do |address| + acl = Acl.new(:k => "test", :address => address) + assert_not_predicate acl, :valid?, "'#{address}' should not be valid" + assert_includes acl.errors[:address], "is invalid" + end + end +end -- 2.47.3