Let Redaktion grant and revoke its own role

Any holder may add or remove another account, witnessed as
redaktion_grant/revoke so the vouching is legible. Not behind elevation:
onboarding must not wait for a keyholder, and a compromised Redaktion
account can already publish.

9 files changed, 134 insertions, 9 deletions

diff --git a/app/controllers/users_controller.rb b/app/controllers/users_controller.rb
index 583ebac0..9b9d64e5 100644
--- a/app/controllers/users_controller.rb
+++ b/app/controllers/users_controller.rb
@@ -5,17 +5,17 @@ class UsersController < ApplicationController
   # Private
 
   before_action :login_required
-  before_action :find_user,     :only => [:show, :edit, :update, :reset_otp, :deactivate, :reactivate]
-  before_action :require_admin, :only => [:index, :new, :create, :reset_otp, :deactivate, :reactivate]
+  before_action :find_user,         :only => [:show, :edit, :update, :reset_otp, :deactivate, :reactivate, :grant_redaktion, :revoke_redaktion]
+  before_action :require_redaktion, :only => [:index]
+  before_action :require_admin,     :only => [:new, :create, :reset_otp, :deactivate, :reactivate]
   before_action :require_elevation, :only => [:new, :create, :reset_otp, :deactivate, :reactivate]
-  before_action :verify_status, :except => [:index]
+  before_action :verify_status,   :except => [:index, :grant_redaktion, :revoke_redaktion]
 
   layout 'admin'
 
   ROLE_PRESETS = {
     "editor"    => [],
     "redaktion" => ["redaktion"],
-    "admin"     => ["admin", "redaktion"]
   }.freeze
 
   GROUP_ORDER = [:admin, :redaktion, :editor, :alumni].freeze
@@ -74,6 +74,28 @@ class UsersController < ApplicationController
     redirect_to users_path
   end
 
+  def grant_redaktion
+    return deny_role_access(:redaktion_required) unless current_user.redaktion?
+
+    case @user.grant_redaktion!(:actor => current_user)
+    when :granted           then flash[:notice] = t("flash.users.redaktion_granted", :login => @user.login)
+    when :no_second_factor  then flash[:error]  = t("flash.users.redaktion_needs_otp", :login => @user.login)
+    end
+
+    redirect_to users_path
+  end
+
+  def revoke_redaktion
+    return deny_role_access(:redaktion_required) unless current_user.redaktion?
+
+    case @user.revoke_redaktion!(:actor => current_user)
+    when :revoked then flash[:notice] = t("flash.users.redaktion_revoked", :login => @user.login)
+    when :self    then flash[:error]  = t("flash.users.redaktion_not_self")
+    end
+
+    redirect_to users_path
+  end
+
   def reset_otp
     @user.disable_otp!(:actor => current_user)
     flash[:notice] = t("flash.users.otp_reset", :login => @user.login)
diff --git a/app/helpers/node_actions_helper.rb b/app/helpers/node_actions_helper.rb
index f57ef84f..53f6ecd0 100644
--- a/app/helpers/node_actions_helper.rb
+++ b/app/helpers/node_actions_helper.rb
@@ -20,7 +20,9 @@ module NodeActionsHelper
     "otp_disable"        => "shield-off",
     "otp_reset"          => "shield-x",
     "user_deactivate"    => "user-off",
-    "user_reactivate"    => "user-check"
+    "user_reactivate"    => "user-check",
+    "redaktion_grant"    => "user-plus",
+    "redaktion_revoke"   => "user-minus"
   }.freeze
 
   def verb_icon action
@@ -283,4 +285,14 @@ module NodeActionsHelper
     t("node_actions.user_reactivate", :actor => actor_ref(action),
        :target => user_participant_ref(action)).html_safe
   end
+
+  def summarize_redaktion_grant action
+    t("node_actions.redaktion_grant", :actor => actor_ref(action),
+       :target => user_participant_ref(action)).html_safe
+  end
+
+  def summarize_redaktion_revoke action
+    t("node_actions.redaktion_revoke", :actor => actor_ref(action),
+       :target => user_participant_ref(action)).html_safe
+  end
 end
diff --git a/app/models/user.rb b/app/models/user.rb
index bf0f40ee..c3035a02 100644
--- a/app/models/user.rb
+++ b/app/models/user.rb
@@ -25,6 +25,7 @@ class User < ApplicationRecord
                             :message => Authentication.bad_email_message
 
   validate :roles_are_known
+  validate :admin_needs_second_factor
 
   # Authenticates a user by their login name and unencrypted password. Returns the user or nil.
   def self.authenticate(login, password)
@@ -136,6 +137,30 @@ class User < ApplicationRecord
     true
   end
 
+  def grant_redaktion!(actor:)
+    return :already if redaktion?
+    return :no_second_factor unless otp_enrolled?
+
+    transaction do
+      update_column(:roles, (roles | ["redaktion"]).sort)
+      NodeAction.record!(:participants => [self], :user => actor,
+                          :action => "redaktion_grant", :target_login => login)
+    end
+    :granted
+  end
+
+  def revoke_redaktion!(actor:)
+    return :already unless redaktion?
+    return :self unless actor != self
+
+    transaction do
+      update_column(:roles, (roles - ["redaktion"]).sort)
+      NodeAction.record!(:participants => [self], :user => actor,
+                          :action => "redaktion_revoke", :target_login => login)
+    end
+    :revoked
+  end
+
   # otp_secret present == enrolled. otp_pending_secret holds the secret
   # between QR display and first-code confirmation. otp_consumed_timestep
   # makes every accepted code single-use (replay guard within the drift
@@ -213,4 +238,10 @@ class User < ApplicationRecord
       unknown = roles.to_a - ROLES
       errors.add(:roles, :unknown, :list => unknown.join(", ")) if unknown.any?
     end
+
+    def admin_needs_second_factor
+      return unless roles.include?("admin")
+      return if otp_secret.present?
+      errors.add(:roles, :admin_needs_otp)
+    end
 end
diff --git a/app/views/users/_user.html.erb b/app/views/users/_user.html.erb
index ff9d4e37..ba82375d 100644
--- a/app/views/users/_user.html.erb
+++ b/app/views/users/_user.html.erb
@@ -26,5 +26,22 @@
       <% end %>
     <% end %>
   </td>
+  <td>
+    <% if current_user.redaktion? && !user.alumni? %>
+      <% if user.redaktion? %>
+        <% unless user == current_user %>
+          <%= button_to t(".revoke_redaktion"), revoke_redaktion_user_path(user), method: :put,
+                form: { data: { confirm: t(".confirm_revoke_redaktion", :login => user.login) },
+                        class: 'button_to destructive' } %>
+        <% end %>
+      <% elsif user.otp_enrolled? %>
+        <%= button_to t(".grant_redaktion"), grant_redaktion_user_path(user), method: :put,
+              form: { data: { confirm: t(".confirm_grant_redaktion", :login => user.login) },
+                      class: 'button_to state_changing' } %>
+      <% else %>
+        <span class="field_hint"><%= t(".needs_otp") %></span>
+      <% end %>
+    <% end %>
+  </td>
 </tr>
 <% end %>
diff --git a/app/views/users/index.html.erb b/app/views/users/index.html.erb
index 854811a2..2936bbea 100644
--- a/app/views/users/index.html.erb
+++ b/app/views/users/index.html.erb
@@ -8,6 +8,7 @@
     <% end %>
   <% end %>
 </p>
+<p class="field_hint"><%= t(".admin_hint") %></p>
 
 <% UsersController::GROUP_ORDER.each do |group| %>
   <% members = @users[group] || [] %>
diff --git a/config/locales/de.yml b/config/locales/de.yml
index 6b5c97cc..67117945 100644
--- a/config/locales/de.yml
+++ b/config/locales/de.yml
@@ -150,6 +150,7 @@ de:
           attributes:
             roles:
               unknown: "enthält unbekannte Rollen: %{list}"
+              admin_needs_otp: "Die Administrator-Rolle setzt einen zweiten Faktor voraus. Der Nutzer muss diesen zuerst selbst unter »Mein Konto« einrichten"
         related_asset:
           attributes:
             headline:
@@ -226,6 +227,8 @@ de:
     otp_reset: "%{actor} hat den zweiten Faktor von %{target} zurückgesetzt"
     user_deactivate: "%{actor} hat %{target} deaktiviert"
     user_reactivate: "%{actor} hat %{target} reaktiviert"
+    redaktion_grant: "%{actor} hat %{target} in die Redaktion aufgenommen"
+    redaktion_revoke: "%{actor} hat %{target} aus der Redaktion entfernt"
 
   open_gallery: "Gallerie anzeigen"
   asset_licenses:
@@ -291,6 +294,11 @@ de:
       deactivate: "Deaktivieren"
       reactivate: "Reaktivieren"
       confirm_deactivate: "%{login} deaktivieren? Die Anmeldung wird sofort verweigert, Zuschreibungen bleiben erhalten."
+      grant_redaktion: "In die Redaktion aufnehmen"
+      revoke_redaktion: "Aus der Redaktion entfernen"
+      confirm_grant_redaktion: "%{login} in die Redaktion aufnehmen? Damit darf %{login} in den geschützten Bereichen veröffentlichen."
+      confirm_revoke_redaktion: "%{login} aus der Redaktion entfernen?"
+      needs_otp: "zweiter Faktor fehlt"
     index:
       title: "Benutzerkonten"
       create_editor: "Editor-Konto anlegen"
@@ -301,6 +309,7 @@ de:
       group_editor: "Editors"
       group_alumni: "Ehemalige"
       group_empty: "— keine —"
+      admin_hint: "Administrative Rechte lassen sich erst vergeben, nachdem der Nutzer sich angemeldet und einen zweiten Faktor eingerichtet hat."
     labels:
       roles: "Rollen"
     roles:
@@ -595,6 +604,10 @@ de:
       deactivated: "%{login} ist jetzt alumni und kann sich nicht mehr anmelden."
       reactivated: "%{login} kann sich wieder anmelden."
       cannot_deactivate_self: "Das eigene Konto kann nicht deaktiviert werden."
+      redaktion_granted: "%{login} gehört jetzt zur Redaktion."
+      redaktion_revoked: "%{login} gehört nicht mehr zur Redaktion."
+      redaktion_needs_otp: "%{login} braucht zuerst einen zweiten Faktor."
+      redaktion_not_self: "Die eigene Redaktions-Rolle kann nicht abgegeben werden."
     assets:
       created: "Asset wurde angelegt."
       updated: "Asset wurde aktualisiert."
diff --git a/config/locales/en.yml b/config/locales/en.yml
index e4c7ecc4..95eb7e94 100644
--- a/config/locales/en.yml
+++ b/config/locales/en.yml
@@ -97,6 +97,7 @@ en:
           attributes:
             roles:
               unknown: "contains unknown roles: %{list}"
+              admin_needs_otp: "Administrator role requires an enrolled second factor. The user has to set one up under My account first"
         related_asset:
           attributes:
             headline:
@@ -170,6 +171,8 @@ en:
     otp_reset: "%{actor} reset the second factor of %{target}"
     user_deactivate: "%{actor} deactivated %{target}"
     user_reactivate: "%{actor} reactivated %{target}"
+    redaktion_grant: "%{actor} added %{target} to Redaktion"
+    redaktion_revoke: "%{actor} removed %{target} from Redaktion"
 
   open_gallery: "Open gallery"
   asset_licenses:
@@ -235,6 +238,12 @@ en:
       deactivate: "Deactivate"
       reactivate: "Reactivate"
       confirm_deactivate: "Deactivate %{login}? Sign-in is refused immediately; attributions are preserved."
+      grant_redaktion: "Add to Redaktion"
+      revoke_redaktion: "Remove from Redaktion"
+      confirm_grant_redaktion: "Add %{login} to Redaktion? They will be able to publish in the protected sections."
+      confirm_revoke_redaktion: "Remove %{login} from Redaktion?"
+      needs_otp: "no second factor"
+
     index:
       title: "User accounts"
       create_editor: "Create editor account"
@@ -245,6 +254,7 @@ en:
       group_editor: "Editors"
       group_alumni: "Alumni"
       group_empty: "— none —"
+      admin_hint: "Administrative rights can only be granted after the account has signed in and set up a second factor."
     labels:
       roles: "Roles"
     roles:
@@ -554,6 +564,11 @@ en:
       deactivated: "%{login} is now an alumnus and can no longer sign in."
       reactivated: "%{login} can sign in again."
       cannot_deactivate_self: "You cannot deactivate your own account."
+      redaktion_granted: "%{login} is now part of Redaktion."
+      redaktion_revoked: "%{login} is no longer part of Redaktion."
+      redaktion_needs_otp: "%{login} needs a second factor first."
+      redaktion_not_self: "You cannot give up your own Redaktion role."
+
     assets:
       created: "Asset was successfully created."
       updated: "Asset was successfully updated."
diff --git a/config/routes.rb b/config/routes.rb
index 58e1e632..d19ac25b 100644
--- a/config/routes.rb
+++ b/config/routes.rb
@@ -102,6 +102,8 @@ Cccms::Application.routes.draw do
         put :reset_otp
         put :deactivate
         put :reactivate
+        put :grant_redaktion
+        put :revoke_redaktion
       end
     end
     resource :otp_enrollment, :only => [:show, :create, :update, :destroy]
diff --git a/test/controllers/users_controller_test.rb b/test/controllers/users_controller_test.rb
index b6f0970d..d7d8b9a6 100644
--- a/test/controllers/users_controller_test.rb
+++ b/test/controllers/users_controller_test.rb
@@ -6,7 +6,7 @@ class UsersControllerTest < ActionController::TestCase
     login_as :quentin
     get :index
     assert_redirected_to admin_path
-    assert_equal I18n.t("flash.common.admin_required"), flash[:error]
+    assert_equal I18n.t("flash.common.redaktion_required"), flash[:error]
   end
 
   test "get index as admin shows every group with per-row actions" do
@@ -53,7 +53,7 @@ class UsersControllerTest < ActionController::TestCase
     assert !User.last.admin
   end
   
-  test "creating new admin users being logged in as admin" do
+  test "creating a Redaktion account" do
     login_as :aaron
     elevate_session!
     assert_difference "User.count", +1 do
@@ -63,13 +63,14 @@ class UsersControllerTest < ActionController::TestCase
           :email                  => "foo@bar.com",
           :password               => "xxxzzz",
           :password_confirmation  => "xxxzzz",
-          :roles                  => ["admin", "redaktion"]
+          :roles                  => ["redaktion"]
         }
       }
     end
     
     assert_redirected_to user_path(User.last)
-    assert User.last.admin
+    assert User.last.redaktion?
+    assert_not User.last.is_admin?
   end
   
   test "creating new users not being logged as regular user wont work" do
@@ -196,6 +197,7 @@ class UsersControllerTest < ActionController::TestCase
     login_as :aaron
     elevate_session!
     user = users(:quentin)
+    user.update_column(:otp_secret, ROTP::Base32.random)
     put :update, params: { :id => user.id, :user => {:roles => ["admin", "redaktion"]} }
     
     assert_equal true, user.reload.is_admin?
@@ -261,4 +263,14 @@ class UsersControllerTest < ActionController::TestCase
 
     assert_not user.reload.is_admin?
   end
+
+  test "an account without a second factor cannot be promoted to admin" do
+    login_as :aaron
+    elevate_session!
+    user = users(:quentin)
+
+    put :update, params: { :id => user.id, :user => { :roles => ["admin"] } }
+
+    assert_not user.reload.is_admin?
+  end
 end