From 03dfdbd83acd9cb14d9bf7d57637318e9ab55347 Mon Sep 17 00:00:00 2001 From: Dan Milne Date: Sun, 7 Jun 2026 15:53:27 +1000 Subject: [PATCH] Default-deny access control with group flags and access enumeration MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Replaces the implicit "empty allowed_groups means public" rule with explicit default-deny across both OIDC and ForwardAuth. Adds two boolean flags on Group — auto_assign (Keycloak-style auto-join on user create) and admin (members can reach the admin panel) — and drops the users.admin column entirely. Adds "Users with access" and "Accessible applications" panels with via-group badges on the application/user show pages. BEHAVIOR CHANGE: a ForwardAuth app with no allowed_groups previously bypassed authentication entirely; it now returns 403 like any other unauthorized request. The data migration seeds an "everyone" group and attaches it to all previously group-less apps to preserve behavior on existing installs. An "admins" group is seeded and backfilled from any user with the old admin column. Co-Authored-By: Claude Opus 4.7 (1M context) --- .../admin/applications_controller.rb | 6 ++ app/controllers/admin/groups_controller.rb | 2 +- app/controllers/admin/users_controller.rb | 43 ++++++++- app/controllers/users_controller.rb | 10 +- app/models/application.rb | 12 +-- app/models/group.rb | 12 +++ app/models/user.rb | 17 +++- app/views/admin/applications/show.html.erb | 37 +++++++- app/views/admin/groups/_form.html.erb | 16 ++++ app/views/admin/groups/show.html.erb | 10 +- app/views/admin/users/_form.html.erb | 35 ++++++- app/views/admin/users/show.html.erb | 95 +++++++++++++++++++ config/initializers/version.rb | 2 +- ...001_add_auto_assign_and_admin_to_groups.rb | 8 ++ ..._seed_default_groups_and_migrate_admins.rb | 44 +++++++++ .../20260607000003_remove_admin_from_users.rb | 5 + db/schema.rb | 7 +- .../admin/groups_controller_test.rb | 18 ++++ .../admin/users_controller_test.rb | 69 ++++++++++++++ .../api/forward_auth_bearer_test.rb | 1 + .../api/forward_auth_controller_test.rb | 32 ++++--- .../oidc_authorization_code_security_test.rb | 1 + test/controllers/oidc_pkce_controller_test.rb | 1 + test/fixtures/groups.yml | 6 ++ test/fixtures/user_groups.yml | 19 ++++ test/fixtures/users.yml | 4 - .../integration/forward_auth_advanced_test.rb | 40 ++++---- .../forward_auth_integration_test.rb | 13 +-- test/models/api_key_test.rb | 6 +- test/models/user_test.rb | 35 ++++--- test/services/oidc_jwt_service_test.rb | 3 +- test/test_helpers/session_test_helper.rb | 9 ++ 32 files changed, 530 insertions(+), 88 deletions(-) create mode 100644 app/views/admin/users/show.html.erb create mode 100644 db/migrate/20260607000001_add_auto_assign_and_admin_to_groups.rb create mode 100644 db/migrate/20260607000002_seed_default_groups_and_migrate_admins.rb create mode 100644 db/migrate/20260607000003_remove_admin_from_users.rb create mode 100644 test/controllers/admin/users_controller_test.rb diff --git a/app/controllers/admin/applications_controller.rb b/app/controllers/admin/applications_controller.rb index 2bf5878..885e89a 100644 --- a/app/controllers/admin/applications_controller.rb +++ b/app/controllers/admin/applications_controller.rb @@ -8,6 +8,12 @@ module Admin def show @allowed_groups = @application.allowed_groups + @users_with_access = User.where(status: User.statuses[:active]) + .joins(groups: :applications) + .where(applications: {id: @application.id}) + .distinct + .includes(:groups) + .order(:email_address) end def new diff --git a/app/controllers/admin/groups_controller.rb b/app/controllers/admin/groups_controller.rb index e165e1e..a8934f7 100644 --- a/app/controllers/admin/groups_controller.rb +++ b/app/controllers/admin/groups_controller.rb @@ -122,7 +122,7 @@ module Admin end def group_params - params.require(:group).permit(:name, :description, :custom_claims) + params.require(:group).permit(:name, :description, :custom_claims, :auto_assign, :admin) end end end diff --git a/app/controllers/admin/users_controller.rb b/app/controllers/admin/users_controller.rb index 694c0f0..24c1496 100644 --- a/app/controllers/admin/users_controller.rb +++ b/app/controllers/admin/users_controller.rb @@ -7,27 +7,38 @@ module Admin end def show + @accessible_applications = Application.where(active: true) + .joins(:allowed_groups) + .where(groups: {id: @user.groups}) + .distinct + .includes(:allowed_groups) + .order(:name) end def new @user = User.new + @available_groups = Group.order(:name) end def create @user = User.new(user_params) @user.password = SecureRandom.alphanumeric(16) if user_params[:password].blank? @user.status = :pending_invitation + @user.skip_auto_assign = true if params[:auto_assign] == "0" if @user.save + assign_groups_from_params(@user) InvitationsMailer.invite_user(@user).deliver_later redirect_to admin_users_path, notice: "User created successfully. Invitation email sent to #{@user.email_address}." else + @available_groups = Group.order(:name) render :new, status: :unprocessable_entity end end def edit @applications = Application.active.order(:name) + @available_groups = Group.order(:name) end def update @@ -43,6 +54,7 @@ module Admin rescue JSON::ParserError @user.errors.add(:custom_claims, "must be valid JSON") @applications = Application.active.order(:name) + @available_groups = Group.order(:name) render :edit, status: :unprocessable_entity return end @@ -52,9 +64,16 @@ module Admin end if @user.update(update_params) + unless assign_groups_from_params(@user) + @applications = Application.active.order(:name) + @available_groups = Group.order(:name) + render :edit, status: :unprocessable_entity + return + end redirect_to admin_users_path, notice: "User updated successfully." else @applications = Application.active.order(:name) + @available_groups = Group.order(:name) render :edit, status: :unprocessable_entity end end @@ -122,14 +141,28 @@ module Admin end def user_params - permitted = [:email_address, :username, :name, :password, :status, :totp_required, :custom_claims] + params.require(:user).permit(:email_address, :username, :name, :password, :status, :totp_required, :custom_claims) + end - # Only allow modifying admin status when editing other users (prevent self-demotion) - if params[:id] != Current.session.user.id.to_s - permitted << :admin + # Apply group_ids from the form, with a guard preventing self-demotion when + # the user is the last member of the last admin group. Returns true on + # success, false if a guard fired (caller should re-render). + def assign_groups_from_params(user) + return true unless params[:user].key?(:group_ids) + + raw_ids = Array(params[:user][:group_ids]).reject(&:blank?).map(&:to_i) + new_groups = Group.where(id: raw_ids) + + if user == Current.session.user + losing_admin = user.groups.where(admin: true).any? && new_groups.none?(&:admin?) + if losing_admin + user.errors.add(:base, "you cannot remove yourself from all administrator groups") + return false + end end - params.require(:user).permit(*permitted) + user.groups = new_groups + true end end end diff --git a/app/controllers/users_controller.rb b/app/controllers/users_controller.rb index 6018841..ef53070 100644 --- a/app/controllers/users_controller.rb +++ b/app/controllers/users_controller.rb @@ -8,12 +8,16 @@ class UsersController < ApplicationController def create @user = User.new(user_params) - - # First user becomes admin automatically - @user.admin = true if User.count.zero? @user.status = "active" + first_user = User.count.zero? if @user.save + # First user automatically becomes a member of every admin group, so they + # can reach the admin panel without an existing admin to grant access. + if first_user + Group.where(admin: true).each { |g| @user.groups << g } + end + start_new_session_for @user redirect_to root_path, notice: "Welcome to Clinch! Your account has been created." else diff --git a/app/models/application.rb b/app/models/application.rb index ad2d81b..e0fe4ab 100644 --- a/app/models/application.rb +++ b/app/models/application.rb @@ -118,14 +118,12 @@ class Application < ApplicationRecord end # Access control + # Default-deny: an empty allowed_groups list means no one gets in. + # To make an app accessible to "everyone", attach the seeded auto-assign + # group (or any group every user is in). def user_allowed?(user) return false unless active? return false unless user.active? - - # If no groups are specified, allow all active users - return true if allowed_groups.empty? - - # Otherwise, user must be in at least one of the allowed groups (user.groups & allowed_groups).any? end @@ -168,10 +166,6 @@ class Application < ApplicationRecord return "deny" unless active? return "deny" unless user.active? - # If no groups specified, bypass authentication - return "bypass" if allowed_groups.empty? - - # If user is in allowed groups, determine auth level if user_allowed?(user) # Require 2FA if user has TOTP configured, otherwise one factor user.totp_enabled? ? "two_factor" : "one_factor" diff --git a/app/models/group.rb b/app/models/group.rb index d7dac0f..700ade9 100644 --- a/app/models/group.rb +++ b/app/models/group.rb @@ -15,6 +15,11 @@ class Group < ApplicationRecord normalizes :name, with: ->(name) { name.strip.downcase } validate :no_reserved_claim_names + scope :auto_assign, -> { where(auto_assign: true) } + scope :admin, -> { where(admin: true) } + + before_destroy :ensure_other_admin_group_exists + # Parse custom_claims JSON field def parsed_custom_claims return {} if custom_claims.blank? @@ -23,6 +28,13 @@ class Group < ApplicationRecord private + def ensure_other_admin_group_exists + return unless admin? + return if Group.where(admin: true).where.not(id: id).exists? + errors.add(:base, "cannot delete the last administrators group") + throw :abort + end + def no_reserved_claim_names return if custom_claims.blank? diff --git a/app/models/user.rb b/app/models/user.rb index 0fa1cc4..5f07d71 100644 --- a/app/models/user.rb +++ b/app/models/user.rb @@ -42,7 +42,18 @@ class User < ApplicationRecord enum :status, {active: 0, disabled: 1, pending_invitation: 2} # Scopes - scope :admins, -> { where(admin: true) } + scope :admins, -> { joins(:groups).where(groups: {admin: true}).distinct } + + # Set true on a user (or on the user_params) to skip the auto-assign callback + # for that record. Used by the admin invite form (opt-out checkbox) and by + # tests that want a clean slate. + attr_accessor :skip_auto_assign + + after_create :add_to_auto_assign_groups, unless: :skip_auto_assign + + def admin? + groups.any?(&:admin?) + end # TOTP methods def totp_enabled? @@ -222,6 +233,10 @@ class User < ApplicationRecord private + def add_to_auto_assign_groups + Group.auto_assign.each { |g| groups << g } + end + def no_reserved_claim_names return if custom_claims.blank? diff --git a/app/views/admin/applications/show.html.erb b/app/views/admin/applications/show.html.erb index 286aff6..103a793 100644 --- a/app/views/admin/applications/show.html.erb +++ b/app/views/admin/applications/show.html.erb @@ -274,11 +274,11 @@
Allowed Groups
<% if @allowed_groups.empty? %> -
+
-

- No groups assigned - all active users can access this application. +

+ No groups assigned — no one can access this application. Attach a group to grant access.

@@ -299,4 +299,35 @@
+ + +
+
+

+ Users with access (<%= @users_with_access.count %>) +

+ <% if @users_with_access.any? %> +
    + <% @users_with_access.each do |user| %> + <% via = user.groups & @application.allowed_groups %> +
  • +
    +

    <%= user.email_address %>

    +
    + <% via.each do |g| %> + via <%= g.name %> + <% end %> +
    +
    + <%= link_to "View", admin_user_path(user), class: "text-blue-600 hover:text-blue-900 text-sm" %> +
  • + <% end %> +
+ <% else %> +
+

No users currently have access. Attach a group to grant access.

+
+ <% end %> +
+
diff --git a/app/views/admin/groups/_form.html.erb b/app/views/admin/groups/_form.html.erb index 447df34..4e8ef45 100644 --- a/app/views/admin/groups/_form.html.erb +++ b/app/views/admin/groups/_form.html.erb @@ -12,6 +12,22 @@ <%= form.text_area :description, rows: 3, class: "mt-1 block w-full rounded-md border-gray-300 dark:border-gray-600 dark:bg-gray-800 dark:text-gray-100 shadow-sm focus:border-blue-500 focus:ring-blue-500 sm:text-sm", placeholder: "Optional description of this group" %> +
+
+ <%= form.check_box :auto_assign, class: "h-4 w-4 rounded border-gray-300 dark:border-gray-600 text-blue-600 focus:ring-blue-500" %> + <%= form.label :auto_assign, "Auto Assign", class: "ml-2 text-sm text-gray-900 dark:text-gray-100" %> +
+

New users will be automatically added to this group when invited. You can mark multiple groups as auto-assigned.

+
+ +
+
+ <%= form.check_box :admin, class: "h-4 w-4 rounded border-gray-300 dark:border-gray-600 text-blue-600 focus:ring-blue-500" %> + <%= form.label :admin, "Administrators", class: "ml-2 text-sm text-gray-900 dark:text-gray-100" %> +
+

Members of this group can access the admin panel. Does not grant automatic access to applications.

+
+
<%= form.label :user_ids, "Group Members", class: "block text-sm font-medium text-gray-700 dark:text-gray-300" %>
diff --git a/app/views/admin/groups/show.html.erb b/app/views/admin/groups/show.html.erb index e1c8d3e..334e40a 100644 --- a/app/views/admin/groups/show.html.erb +++ b/app/views/admin/groups/show.html.erb @@ -1,7 +1,15 @@
-

<%= @group.name %>

+
+

<%= @group.name %>

+ <% if @group.auto_assign? %> + Auto Assign + <% end %> + <% if @group.admin? %> + Administrators + <% end %> +
<% if @group.description.present? %>

<%= @group.description %>

<% end %> diff --git a/app/views/admin/users/_form.html.erb b/app/views/admin/users/_form.html.erb index 8fb7668..927be18 100644 --- a/app/views/admin/users/_form.html.erb +++ b/app/views/admin/users/_form.html.erb @@ -33,11 +33,36 @@ <%= form.select :status, User.statuses.keys.map { |s| [s.titleize, s] }, {}, class: "mt-1 block w-full rounded-md border-gray-300 dark:border-gray-600 dark:bg-gray-800 dark:text-gray-100 shadow-sm focus:border-blue-500 focus:ring-blue-500 sm:text-sm" %>
-
- <%= form.check_box :admin, class: "h-4 w-4 rounded border-gray-300 dark:border-gray-600 text-blue-600 focus:ring-blue-500", disabled: (user == Current.session.user) %> - <%= form.label :admin, "Administrator", class: "ml-2 block text-sm text-gray-900 dark:text-gray-100" %> - <% if user == Current.session.user %> - (Cannot change your own admin status) +
+ <%= form.label :group_ids, "Group Memberships", class: "block text-sm font-medium text-gray-700 dark:text-gray-300" %> +
+ <% if @available_groups.any? %> + <% @available_groups.each do |group| %> +
+ <%= check_box_tag "user[group_ids][]", group.id, user.groups.include?(group), class: "h-4 w-4 rounded border-gray-300 dark:border-gray-600 text-blue-600 focus:ring-blue-500" %> + <%= label_tag "user_group_ids_#{group.id}", group.name, class: "ml-2 text-sm text-gray-900 dark:text-gray-100" %> + <% if group.admin? %> + Admin + <% end %> + <% if group.auto_assign? %> + Auto Assign + <% end %> +
+ <% end %> + <% else %> +

No groups available. Create a group first.

+ <% end %> +
+

Administrators are members of any group with the Admin flag set. You cannot remove yourself from your last administrator group.

+ <% unless user.persisted? %> + <% auto_names = Group.where(auto_assign: true).pluck(:name) %> + <% if auto_names.any? %> +
+ <%= check_box_tag "auto_assign", "1", true, class: "h-4 w-4 rounded border-gray-300 dark:border-gray-600 text-blue-600 focus:ring-blue-500" %> + <%= label_tag "auto_assign", "Auto-assign to default groups (#{auto_names.join(", ")})", class: "ml-2 text-sm text-gray-900 dark:text-gray-100" %> +
+

Uncheck to invite this user without auto-assigning the default group(s) — useful for restricted accounts.

+ <% end %> <% end %>
diff --git a/app/views/admin/users/show.html.erb b/app/views/admin/users/show.html.erb new file mode 100644 index 0000000..6fe6ccc --- /dev/null +++ b/app/views/admin/users/show.html.erb @@ -0,0 +1,95 @@ +
+
+
+
+

<%= @user.email_address %>

+ <% if @user.admin? %> + Admin + <% end %> + <% case @user.status %> + <% when "active" %> + Active + <% when "disabled" %> + Disabled + <% when "pending_invitation" %> + Pending Invitation + <% end %> +
+ <% if @user.name.present? %> +

<%= @user.name %>

+ <% end %> +
+
+ <%= link_to "Edit", edit_admin_user_path(@user), class: "rounded-md bg-white dark:bg-gray-700 px-3 py-2 text-sm font-semibold text-gray-900 dark:text-gray-200 shadow-sm ring-1 ring-inset ring-gray-300 dark:ring-gray-600 hover:bg-gray-50 dark:hover:bg-gray-600" %> +
+
+
+ +
+ +
+
+

+ Group memberships (<%= @user.groups.count %>) +

+ <% if @user.groups.any? %> +
    + <% @user.groups.order(:name).each do |group| %> +
  • +
    +

    <%= group.name %>

    + <% if group.admin? %> + Admin + <% end %> + <% if group.auto_assign? %> + Auto Assign + <% end %> +
    + <%= link_to "View", admin_group_path(group), class: "text-blue-600 hover:text-blue-900 text-sm" %> +
  • + <% end %> +
+ <% else %> +

This user is in no groups.

+ <% end %> +
+
+ + +
+
+

+ Accessible applications (<%= @accessible_applications.count %>) +

+ <% unless @user.active? %> +
+

+ User is <%= @user.status.humanize.downcase %> — access is denied regardless of group memberships. +

+
+ <% end %> + <% if @accessible_applications.any? %> +
    + <% @accessible_applications.each do |app| %> + <% via = app.allowed_groups & @user.groups %> +
  • +
    +

    <%= app.name %>

    +
    + <% via.each do |g| %> + via <%= g.name %> + <% end %> +
    +
    + <%= link_to "View", admin_application_path(app), class: "text-blue-600 hover:text-blue-900 text-sm" %> +
  • + <% end %> +
+ <% else %> +
+

No accessible applications. Add the user to a group that's attached to one or more applications.

+
+ <% end %> +
+
+
diff --git a/config/initializers/version.rb b/config/initializers/version.rb index bf027c7..a5c2565 100644 --- a/config/initializers/version.rb +++ b/config/initializers/version.rb @@ -1,5 +1,5 @@ # frozen_string_literal: true module Clinch - VERSION = "0.12.0" + VERSION = "0.13.0" end diff --git a/db/migrate/20260607000001_add_auto_assign_and_admin_to_groups.rb b/db/migrate/20260607000001_add_auto_assign_and_admin_to_groups.rb new file mode 100644 index 0000000..71d38ee --- /dev/null +++ b/db/migrate/20260607000001_add_auto_assign_and_admin_to_groups.rb @@ -0,0 +1,8 @@ +class AddAutoAssignAndAdminToGroups < ActiveRecord::Migration[8.1] + def change + add_column :groups, :auto_assign, :boolean, default: false, null: false + add_column :groups, :admin, :boolean, default: false, null: false + add_index :groups, :auto_assign, where: "auto_assign" + add_index :groups, :admin, where: "admin" + end +end diff --git a/db/migrate/20260607000002_seed_default_groups_and_migrate_admins.rb b/db/migrate/20260607000002_seed_default_groups_and_migrate_admins.rb new file mode 100644 index 0000000..9af0e8d --- /dev/null +++ b/db/migrate/20260607000002_seed_default_groups_and_migrate_admins.rb @@ -0,0 +1,44 @@ +class SeedDefaultGroupsAndMigrateAdmins < ActiveRecord::Migration[8.1] + # Data migration: seed "everyone" (auto_assign) and "admins" (admin) groups, + # backfill memberships from existing data, attach "everyone" to previously + # group-less applications. Idempotent. + # + # Must run before RemoveAdminFromUsers, because it reads the legacy + # users.admin column. + + def up + unless Group.exists?(auto_assign: true) + everyone = Group.create!( + name: "everyone", + description: "Auto-assigned to new users. Safe to rename or remove.", + auto_assign: true + ) + + User.where(status: 0).find_each do |u| + UserGroup.find_or_create_by!(user_id: u.id, group_id: everyone.id) + end + + Application.left_joins(:application_groups) + .where(application_groups: {id: nil}) + .find_each do |app| + ApplicationGroup.find_or_create_by!(application_id: app.id, group_id: everyone.id) + end + end + + unless Group.exists?(admin: true) + admins = Group.create!( + name: "admins", + description: "Members can access the admin panel.", + admin: true + ) + + User.where(admin: true).find_each do |u| + UserGroup.find_or_create_by!(user_id: u.id, group_id: admins.id) + end + end + end + + def down + Group.where(name: ["everyone", "admins"]).destroy_all + end +end diff --git a/db/migrate/20260607000003_remove_admin_from_users.rb b/db/migrate/20260607000003_remove_admin_from_users.rb new file mode 100644 index 0000000..5bb5802 --- /dev/null +++ b/db/migrate/20260607000003_remove_admin_from_users.rb @@ -0,0 +1,5 @@ +class RemoveAdminFromUsers < ActiveRecord::Migration[8.1] + def change + remove_column :users, :admin, :boolean, default: false, null: false + end +end diff --git a/db/schema.rb b/db/schema.rb index 85f953d..12ae5a0 100644 --- a/db/schema.rb +++ b/db/schema.rb @@ -10,7 +10,7 @@ # # It's strongly recommended that you check this file into your version control system. -ActiveRecord::Schema[8.1].define(version: 2026_04_20_080000) do +ActiveRecord::Schema[8.1].define(version: 2026_06_07_000003) do create_table "active_storage_attachments", force: :cascade do |t| t.bigint "blob_id", null: false t.datetime "created_at", null: false @@ -106,11 +106,15 @@ ActiveRecord::Schema[8.1].define(version: 2026_04_20_080000) do end create_table "groups", force: :cascade do |t| + t.boolean "admin", default: false, null: false + t.boolean "auto_assign", default: false, null: false t.datetime "created_at", null: false t.json "custom_claims", default: {}, null: false t.text "description" t.string "name", null: false t.datetime "updated_at", null: false + t.index ["admin"], name: "index_groups_on_admin", where: "admin" + t.index ["auto_assign"], name: "index_groups_on_auto_assign", where: "auto_assign" t.index ["name"], name: "index_groups_on_name", unique: true end @@ -225,7 +229,6 @@ ActiveRecord::Schema[8.1].define(version: 2026_04_20_080000) do end create_table "users", force: :cascade do |t| - t.boolean "admin", default: false, null: false t.json "backup_codes" t.datetime "created_at", null: false t.json "custom_claims", default: {}, null: false diff --git a/test/controllers/admin/groups_controller_test.rb b/test/controllers/admin/groups_controller_test.rb index 5e90bcd..57fd2c8 100644 --- a/test/controllers/admin/groups_controller_test.rb +++ b/test/controllers/admin/groups_controller_test.rb @@ -48,5 +48,23 @@ module Admin assert_equal [app], Group.find_by(name: "new group").applications end + + test "can mark a group as auto_assign and admin" do + patch admin_group_path(@group), params: { + group: {name: @group.name, auto_assign: "1", admin: "1"} + } + + @group.reload + assert @group.auto_assign? + assert @group.admin? + end + + test "cannot delete the last admin group" do + admins = groups(:admin_group) + + delete admin_group_path(admins) + # Destroy was aborted by the before_destroy guard + assert Group.exists?(admins.id), "admin group should not have been deleted" + end end end diff --git a/test/controllers/admin/users_controller_test.rb b/test/controllers/admin/users_controller_test.rb new file mode 100644 index 0000000..5c53b48 --- /dev/null +++ b/test/controllers/admin/users_controller_test.rb @@ -0,0 +1,69 @@ +require "test_helper" + +module Admin + class UsersControllerTest < ActionDispatch::IntegrationTest + setup do + @admin = users(:two) # in admin_group via fixtures + sign_in_as(@admin) + end + + test "show loads accessible applications via the user's groups" do + kavita = applications(:kavita_app) + # alice is in admin_group via fixtures; kavita is attached to admin_group via app_groups + get admin_user_path(users(:alice)) + assert_response :success + assert_match kavita.name, response.body + # The "via" badge mentions the granting group name + assert_match groups(:admin_group).name, response.body + end + + test "update assigns group memberships from group_ids" do + target = users(:bob) + editors = groups(:editor_group) + one = groups(:one) + + patch admin_user_path(target), params: { + user: {email_address: target.email_address, group_ids: [editors.id, one.id]} + } + + assert_redirected_to admin_users_path + assert_equal [editors, one].sort, target.reload.groups.sort + end + + test "cannot remove yourself from the last admin group" do + # @admin (users:two) is in admin_group. Removing them via the user form + # while no other admin exists is blocked. + sole_admin = users(:two) + # Strip alice (the other admin) so @admin is the last one. + users(:alice).groups.delete(groups(:admin_group)) + + patch admin_user_path(sole_admin), params: { + user: {email_address: sole_admin.email_address, group_ids: []} + } + + assert_response :unprocessable_entity + assert sole_admin.reload.admin?, "should still be admin" + end + + test "create with auto_assign=0 skips the auto-assign callback" do + post admin_users_path, params: { + user: {email_address: "restricted@example.com"}, + auto_assign: "0" + } + + assert_response :redirect + created = User.find_by(email_address: "restricted@example.com") + assert_not_includes created.groups, groups(:everyone) + end + + test "create without auto_assign param auto-joins everyone" do + post admin_users_path, params: { + user: {email_address: "newbie@example.com"} + } + + assert_response :redirect + created = User.find_by(email_address: "newbie@example.com") + assert_includes created.groups, groups(:everyone) + end + end +end diff --git a/test/controllers/api/forward_auth_bearer_test.rb b/test/controllers/api/forward_auth_bearer_test.rb index b503354..af60597 100644 --- a/test/controllers/api/forward_auth_bearer_test.rb +++ b/test/controllers/api/forward_auth_bearer_test.rb @@ -11,6 +11,7 @@ module Api domain_pattern: "webdav.example.com", active: true ) + grant_everyone_access(@app) @api_key = @user.api_keys.create!(name: "Test Key", application: @app) @token = @api_key.plaintext_token end diff --git a/test/controllers/api/forward_auth_controller_test.rb b/test/controllers/api/forward_auth_controller_test.rb index 4eba364..8526231 100644 --- a/test/controllers/api/forward_auth_controller_test.rb +++ b/test/controllers/api/forward_auth_controller_test.rb @@ -7,8 +7,8 @@ module Api @admin_user = users(:alice) @inactive_user = User.create!(email_address: "inactive@example.com", password: "password", status: :disabled) @group = groups(:admin_group) - @rule = Application.create!(name: "Test App", slug: "test-app", app_type: "forward_auth", domain_pattern: "test.example.com", active: true) - @inactive_rule = Application.create!(name: "Inactive App", slug: "inactive-app", app_type: "forward_auth", domain_pattern: "inactive.example.com", active: false) + @rule = grant_everyone_access(Application.create!(name: "Test App", slug: "test-app", app_type: "forward_auth", domain_pattern: "test.example.com", active: true)) + @inactive_rule = grant_everyone_access(Application.create!(name: "Inactive App", slug: "inactive-app", app_type: "forward_auth", domain_pattern: "inactive.example.com", active: false)) end # Authentication Tests @@ -65,7 +65,7 @@ module Api end test "should return 403 when rule exists but user not in allowed groups" do - @rule.allowed_groups << @group + @rule.allowed_groups = [@group] sign_in_as(@user) # User not in group get "/api/verify", headers: {"X-Forwarded-Host" => "test.example.com"} @@ -75,7 +75,7 @@ module Api end test "should return 200 when user is in allowed groups" do - @rule.allowed_groups << @group + @rule.allowed_groups = [@group] @user.groups << @group sign_in_as(@user) @@ -86,7 +86,7 @@ module Api # Domain Pattern Tests test "should match wildcard domains correctly" do - Application.create!(name: "Wildcard App", slug: "wildcard-app", app_type: "forward_auth", domain_pattern: "*.example.com", active: true) + grant_everyone_access Application.create!(name: "Wildcard App", slug: "wildcard-app", app_type: "forward_auth", domain_pattern: "*.example.com", active: true) sign_in_as(@user) get "/api/verify", headers: {"X-Forwarded-Host" => "app.example.com"} @@ -101,7 +101,7 @@ module Api end test "should match exact domains correctly" do - Application.create!(name: "Exact App", slug: "exact-app", app_type: "forward_auth", domain_pattern: "api.example.com", active: true) + grant_everyone_access Application.create!(name: "Exact App", slug: "exact-app", app_type: "forward_auth", domain_pattern: "api.example.com", active: true) sign_in_as(@user) get "/api/verify", headers: {"X-Forwarded-Host" => "api.example.com"} @@ -126,7 +126,7 @@ module Api end test "should return custom headers when configured" do - Application.create!( + grant_everyone_access Application.create!( name: "Custom App", slug: "custom-app", app_type: "forward_auth", @@ -151,7 +151,7 @@ module Api end test "should return no headers when all headers disabled" do - Application.create!( + grant_everyone_access Application.create!( name: "No Headers App", slug: "no-headers-app", app_type: "forward_auth", @@ -182,11 +182,19 @@ module Api assert_includes groups_header, "Editors" end - test "should not include groups header when user has no groups" do - @user.groups.clear # Remove fixture groups + test "should not include groups header when user has no groups beyond the granting one and groups header empty" do + # Under default-deny the user must be in at least one group to access the app. + # This rewritten test verifies that when an app's headers_config disables the + # groups header, no x-remote-groups is sent regardless of memberships. + app = grant_everyone_access Application.create!( + name: "Headers Hidden", slug: "headers-hidden", app_type: "forward_auth", + domain_pattern: "hidden.example.com", + active: true, + headers_config: {groups: ""} + ) sign_in_as(@user) - get "/api/verify", headers: {"X-Forwarded-Host" => "test.example.com"} + get "/api/verify", headers: {"X-Forwarded-Host" => "hidden.example.com"} assert_response 200 assert_nil response.headers["x-remote-groups"] @@ -705,7 +713,7 @@ module Api class FaTokenHostBindingTest < ActionDispatch::IntegrationTest setup do @user = users(:bob) - Application.create!(name: "Bound App", slug: "bound-app", app_type: "forward_auth", domain_pattern: "app.example.com", active: true) + grant_everyone_access Application.create!(name: "Bound App", slug: "bound-app", app_type: "forward_auth", domain_pattern: "app.example.com", active: true) @original_cache = Rails.cache Rails.cache = ActiveSupport::Cache::MemoryStore.new diff --git a/test/controllers/oidc_authorization_code_security_test.rb b/test/controllers/oidc_authorization_code_security_test.rb index d96e508..b158d02 100644 --- a/test/controllers/oidc_authorization_code_security_test.rb +++ b/test/controllers/oidc_authorization_code_security_test.rb @@ -17,6 +17,7 @@ class OidcAuthorizationCodeSecurityTest < ActionDispatch::IntegrationTest @application.generate_new_client_secret! @plain_client_secret = @application.client_secret @application.save! + grant_everyone_access(@application) end def teardown diff --git a/test/controllers/oidc_pkce_controller_test.rb b/test/controllers/oidc_pkce_controller_test.rb index 238c578..a6f5161 100644 --- a/test/controllers/oidc_pkce_controller_test.rb +++ b/test/controllers/oidc_pkce_controller_test.rb @@ -10,6 +10,7 @@ class OidcPkceControllerTest < ActionDispatch::IntegrationTest redirect_uris: ["http://localhost:4000/callback"].to_json, active: true ) + grant_everyone_access(@application) # Sign in the user using the test helper sign_in_as(@user) diff --git a/test/fixtures/groups.yml b/test/fixtures/groups.yml index 8e36c8a..1b987a5 100644 --- a/test/fixtures/groups.yml +++ b/test/fixtures/groups.yml @@ -11,7 +11,13 @@ two: admin_group: name: Administrators description: System administrators with full access + admin: true editor_group: name: Editors description: Content editors with limited access + +everyone: + name: everyone + description: Auto-assigned to new users. + auto_assign: true diff --git a/test/fixtures/user_groups.yml b/test/fixtures/user_groups.yml index 04bd86b..2b19d35 100644 --- a/test/fixtures/user_groups.yml +++ b/test/fixtures/user_groups.yml @@ -1,9 +1,28 @@ # Read about fixtures at https://api.rubyonrails.org/classes/ActiveRecord/FixtureSet.html +# All users belong to "everyone" so existing tests that create group-less apps +# can be made compatible by attaching that group. + +one_everyone: + user: one + group: everyone +two_everyone: + user: two + group: everyone +alice_everyone: + user: alice + group: everyone +bob_everyone: + user: bob + group: everyone alice_admin_group: user: alice group: admin_group +two_admin_group: + user: two + group: admin_group + bob_editor_group: user: bob group: editor_group diff --git a/test/fixtures/users.yml b/test/fixtures/users.yml index 738c183..c886737 100644 --- a/test/fixtures/users.yml +++ b/test/fixtures/users.yml @@ -3,23 +3,19 @@ one: email_address: one@example.com password_digest: <%= password_digest %> - admin: false status: 0 # active two: email_address: two@example.com password_digest: <%= password_digest %> - admin: true status: 0 # active alice: email_address: alice@example.com password_digest: <%= password_digest %> - admin: true status: 0 # active bob: email_address: bob@example.com password_digest: <%= password_digest %> - admin: false status: 0 # active diff --git a/test/integration/forward_auth_advanced_test.rb b/test/integration/forward_auth_advanced_test.rb index cc0f2f1..7aa96ad 100644 --- a/test/integration/forward_auth_advanced_test.rb +++ b/test/integration/forward_auth_advanced_test.rb @@ -12,7 +12,7 @@ class ForwardAuthAdvancedTest < ActionDispatch::IntegrationTest # End-to-End Authentication Flow Tests test "complete forward auth flow with default headers" do # Create an application with default headers - Application.create!(name: "App", slug: "app-system-test", app_type: "forward_auth", domain_pattern: "app.example.com", active: true) + grant_everyone_access Application.create!(name: "App", slug: "app-system-test", app_type: "forward_auth", domain_pattern: "app.example.com", active: true) # Step 1: Unauthenticated request to protected resource get "/api/verify", headers: { @@ -48,14 +48,14 @@ class ForwardAuthAdvancedTest < ActionDispatch::IntegrationTest test "multiple domain access with single session" do # Create applications for different domains - Application.create!(name: "App Domain", slug: "app-domain", app_type: "forward_auth", domain_pattern: "app.example.com", active: true) - Application.create!( + grant_everyone_access Application.create!(name: "App Domain", slug: "app-domain", app_type: "forward_auth", domain_pattern: "app.example.com", active: true) + grant_everyone_access Application.create!( name: "Grafana", slug: "grafana-system-test", app_type: "forward_auth", domain_pattern: "grafana.example.com", active: true, headers_config: {user: "X-WEBAUTH-USER", email: "X-WEBAUTH-EMAIL"} ) - Application.create!( + grant_everyone_access Application.create!( name: "Metube", slug: "metube-system-test", app_type: "forward_auth", domain_pattern: "metube.example.com", active: true, @@ -106,7 +106,7 @@ class ForwardAuthAdvancedTest < ActionDispatch::IntegrationTest # Should have access (in allowed group) get "/api/verify", headers: {"X-Forwarded-Host" => "admin.example.com"} assert_response 200 - assert_equal @group.name, response.headers["x-remote-groups"] + assert_includes response.headers["x-remote-groups"], @group.name # Add user to second group @user.groups << @group2 @@ -126,31 +126,27 @@ class ForwardAuthAdvancedTest < ActionDispatch::IntegrationTest assert_response 403 end - test "bypass mode when no groups assigned to rule" do - # Create bypass application (no groups) + test "default deny when no groups assigned to rule" do + # An app with no allowed_groups now denies all users (was: bypass mode). Application.create!( - name: "Public", slug: "public-system-test", app_type: "forward_auth", - domain_pattern: "public.example.com", + name: "No Groups", slug: "nogroups-system-test", app_type: "forward_auth", + domain_pattern: "nogroups.example.com", active: true ) - # Create user with no groups @user.groups.clear - # Sign in post "/signin", params: {email_address: @user.email_address, password: "password"} assert_response 303 - # Should have access (bypass mode) - get "/api/verify", headers: {"X-Forwarded-Host" => "public.example.com"} - assert_response 200 - assert_equal @user.email_address, response.headers["x-remote-user"] + get "/api/verify", headers: {"X-Forwarded-Host" => "nogroups.example.com"} + assert_response 403 end # Security System Tests test "session expiration and cleanup" do # Create test application - Application.create!( + grant_everyone_access Application.create!( name: "Test", slug: "test-system-test", app_type: "forward_auth", domain_pattern: "test.example.com", active: true @@ -179,7 +175,7 @@ class ForwardAuthAdvancedTest < ActionDispatch::IntegrationTest test "concurrent access with rate limiting considerations" do # Create wildcard application - Application.create!( + grant_everyone_access Application.create!( name: "Wildcard", slug: "wildcard-test", app_type: "forward_auth", domain_pattern: "*.example.com", active: true @@ -246,7 +242,11 @@ class ForwardAuthAdvancedTest < ActionDispatch::IntegrationTest active: true, headers_config: app[:headers_config] ) - app[:groups].each { |group| rule.allowed_groups << group } + if app[:groups].any? + app[:groups].each { |group| rule.allowed_groups << group } + else + grant_everyone_access(rule) + end rule end @@ -286,7 +286,7 @@ class ForwardAuthAdvancedTest < ActionDispatch::IntegrationTest ] patterns.each_with_index do |pattern_config, idx| - Application.create!( + grant_everyone_access Application.create!( name: "Pattern Test #{idx}", slug: "pattern-test-#{idx}", app_type: "forward_auth", domain_pattern: pattern_config[:pattern], active: true @@ -310,7 +310,7 @@ class ForwardAuthAdvancedTest < ActionDispatch::IntegrationTest # Performance System Tests test "system performance under load" do # Create test application with wildcard pattern - Application.create!(name: "Load Test", slug: "loadtest", app_type: "forward_auth", domain_pattern: "*.loadtest.example.com", active: true) + grant_everyone_access Application.create!(name: "Load Test", slug: "loadtest", app_type: "forward_auth", domain_pattern: "*.loadtest.example.com", active: true) # Sign in post "/signin", params: {email_address: @user.email_address, password: "password"} diff --git a/test/integration/forward_auth_integration_test.rb b/test/integration/forward_auth_integration_test.rb index f72ab3c..6193bf1 100644 --- a/test/integration/forward_auth_integration_test.rb +++ b/test/integration/forward_auth_integration_test.rb @@ -15,6 +15,7 @@ class ForwardAuthIntegrationTest < ActionDispatch::IntegrationTest domain_pattern: "test.example.com", active: true ) + grant_everyone_access(@test_app) end # Basic Authentication Flow Tests @@ -56,8 +57,8 @@ class ForwardAuthIntegrationTest < ActionDispatch::IntegrationTest # Domain and Rule Integration Tests test "different domain patterns with same session" do # Create test rules - Application.create!(name: "Wildcard App", slug: "wildcard-app", app_type: "forward_auth", domain_pattern: "*.example.com", active: true) - Application.create!(name: "Exact App", slug: "exact-app", app_type: "forward_auth", domain_pattern: "api.example.com", active: true) + grant_everyone_access Application.create!(name: "Wildcard App", slug: "wildcard-app", app_type: "forward_auth", domain_pattern: "*.example.com", active: true) + grant_everyone_access Application.create!(name: "Exact App", slug: "exact-app", app_type: "forward_auth", domain_pattern: "api.example.com", active: true) # Sign in post "/signin", params: {email_address: @user.email_address, password: "password"} @@ -103,14 +104,14 @@ class ForwardAuthIntegrationTest < ActionDispatch::IntegrationTest # Header Configuration Integration Tests test "different header configurations with same user" do # Create applications with different configs - Application.create!(name: "Default App", slug: "default-app", app_type: "forward_auth", domain_pattern: "default.example.com", active: true) - Application.create!( + grant_everyone_access Application.create!(name: "Default App", slug: "default-app", app_type: "forward_auth", domain_pattern: "default.example.com", active: true) + grant_everyone_access Application.create!( name: "Custom App", slug: "custom-app", app_type: "forward_auth", domain_pattern: "custom.example.com", active: true, headers_config: {user: "X-WEBAUTH-USER", groups: "X-WEBAUTH-ROLES"} ) - Application.create!( + grant_everyone_access Application.create!( name: "No Headers App", slug: "no-headers-app", app_type: "forward_auth", domain_pattern: "noheaders.example.com", active: true, @@ -196,7 +197,7 @@ class ForwardAuthIntegrationTest < ActionDispatch::IntegrationTest admin_user = users(:two) # Create restricted rule - Application.create!( + grant_everyone_access Application.create!( name: "Admin App", slug: "admin-app", app_type: "forward_auth", domain_pattern: "admin.example.com", active: true, diff --git a/test/models/api_key_test.rb b/test/models/api_key_test.rb index e44d3e7..db93849 100644 --- a/test/models/api_key_test.rb +++ b/test/models/api_key_test.rb @@ -10,6 +10,7 @@ class ApiKeyTest < ActiveSupport::TestCase domain_pattern: "webdav.example.com", active: true ) + @app.allowed_groups << groups(:everyone) end test "generates clk_ prefixed token on create" do @@ -78,9 +79,8 @@ class ApiKeyTest < ActiveSupport::TestCase end test "validates user must have access to application" do - group = groups(:admin_group) - @app.allowed_groups << group - # @user (bob) is not in admin_group + # Restrict the app to admin_group only — bob is not in admin_group. + @app.allowed_groups = [groups(:admin_group)] key = @user.api_keys.build(name: "No Access", application: @app) assert_not key.valid? assert_includes key.errors[:user], "does not have access to this application" diff --git a/test/models/user_test.rb b/test/models/user_test.rb index ad8862c..a143988 100644 --- a/test/models/user_test.rb +++ b/test/models/user_test.rb @@ -135,23 +135,36 @@ class UserTest < ActiveSupport::TestCase assert_equal user, found_user end - test "admin scope" do - admin_user = User.create!( - email_address: "admin@example.com", - password: "password123", - admin: true - ) - regular_user = User.create!( - email_address: "user@example.com", - password: "password123", - admin: false - ) + test "admin scope returns users in admin groups" do + admin_group = groups(:admin_group) + admin_user = User.create!(email_address: "admin@example.com", password: "password123") + admin_user.groups << admin_group + regular_user = User.create!(email_address: "user@example.com", password: "password123") admins = User.admins assert_includes admins, admin_user assert_not_includes admins, regular_user end + test "admin? reflects membership in any admin: true group" do + user = User.create!(email_address: "promoted@example.com", password: "password123") + assert_not user.admin? + user.groups << groups(:admin_group) + assert user.reload.admin? + end + + test "after_create auto-joins all auto_assign groups" do + user = User.create!(email_address: "newbie@example.com", password: "password123") + assert_includes user.groups, groups(:everyone) + end + + test "skip_auto_assign disables the after_create callback" do + user = User.new(email_address: "skipper@example.com", password: "password123") + user.skip_auto_assign = true + user.save! + assert_not_includes user.groups, groups(:everyone) + end + test "validates email address format" do user = User.new(email_address: "invalid-email", password: "password123") assert_not user.valid? diff --git a/test/services/oidc_jwt_service_test.rb b/test/services/oidc_jwt_service_test.rb index c91df38..72b0bb3 100644 --- a/test/services/oidc_jwt_service_test.rb +++ b/test/services/oidc_jwt_service_test.rb @@ -95,7 +95,8 @@ class OidcJwtServiceTest < ActiveSupport::TestCase end test "admin claim should not be included in token" do - @user.update!(admin: true) + # alice is already in admin_group via fixtures, so admin? is true here + assert @user.admin? token = @service.generate_id_token(@user, @application) diff --git a/test/test_helpers/session_test_helper.rb b/test/test_helpers/session_test_helper.rb index 0686378..47463db 100644 --- a/test/test_helpers/session_test_helper.rb +++ b/test/test_helpers/session_test_helper.rb @@ -12,6 +12,15 @@ module SessionTestHelper Current.session&.destroy! cookies.delete("session_id") end + + # Attach the auto-assign "everyone" group to the given app so existing tests + # written under the old "empty allowed_groups = public" rule keep working. + # New tests should attach groups explicitly to model real access intent. + def grant_everyone_access(app) + everyone = (groups(:everyone) rescue Group.find_by(auto_assign: true)) + app.allowed_groups << everyone unless app.allowed_groups.include?(everyone) + app + end end ActiveSupport.on_load(:action_dispatch_integration_test) do