diff --git a/README.md b/README.md index 16c28e2..fdd7288 100644 --- a/README.md +++ b/README.md @@ -354,7 +354,9 @@ Há também uma tela em `/users` (menu "Acessos" no topo, visível a líder e ad - o cadastro de um novo usuário exige nome, e-mail, senha e a permissão (executor, líder ou admin) — não há autocadastro; - a edição permite alterar nome, e-mail e a permissão de um usuário existente — a troca de senha em si não é feita por aqui (ver seção "Primeiro acesso e redefinição de senha"); - **o campo Chat ID do Telegram só aparece pra quem está logado como admin** — nem no formulário, nem na edição, um líder vê ou consegue alterar esse campo de outro usuário (reforçado também do lado do servidor, não só escondido na tela); -- **só um admin concede ou remove o papel de admin, e ninguém altera a própria permissão** (`User#validar_atribuicao_de_papel`). O líder continua alternando qualquer outro usuário entre executor e líder — o que ele não pode é criar um admin, promover alguém a admin ou rebaixar um admin existente. Sem essa regra o `cannot :manage, WebhookSubscription` do líder (ver `app/models/ability.rb`) seria decorativo: como o líder gerencia usuários, bastaria um `PATCH` no próprio usuário com `role=admin` pra contorná-lo num request. A trava de "não altera a própria permissão" vale inclusive pro admin, e de quebra impede o último admin de se rebaixar e deixar os webhooks sem ninguém que possa gerenciá-los; +- **só um admin concede ou remove o papel de admin, e ninguém altera a própria permissão** (`User#validar_atribuicao_de_papel`). O líder continua alternando qualquer outro usuário entre executor e líder — o que ele não pode é criar um admin, promover alguém a admin ou rebaixar um admin existente. Sem essa regra o `cannot :manage, WebhookSubscription` do líder (ver `app/models/ability.rb`) seria decorativo: como o líder gerencia usuários, bastaria um `PATCH` no próprio usuário com `role=admin` pra contorná-lo num request. A trava de "não altera a própria permissão" vale inclusive pro admin, e de quebra impede o último admin de se rebaixar e deixar os webhooks sem ninguém que possa gerenciá-los. + + A regra é **fail-closed**: quem está fazendo a alteração chega no model pelo `User#ator`, que não é coluna — os controllers preenchem —, e **papel definido ou alterado sem ator é recusado**. Antes era o contrário (ator ausente dispensava a validação), e a diferença não era teórica: a invariante valia só porque os dois controllers de hoje lembram de preencher o ator, e qualquer caminho novo que esquecesse — um job, um importador, uma rake task, um endpoint futuro — passava direto e em silêncio. Os poucos lugares legítimos que escrevem papel fora de uma requisição (`db/seeds.rb`, o console, a factory da suíte) declaram isso com `User#ator_dispensado`. O nome é incômodo de propósito: usá-lo é afirmar "não há requisição aqui", não silenciar uma validação chata; - líder e admin também podem **excluir** outros usuários (com confirmação via Turbo). Duas travas de segurança: não dá pra excluir a própria conta, nem excluir um usuário que já tenha demandas cadastradas (é preciso reatribuir ou excluir as demandas dele antes); - um formulário de busca (com autocomplete por nome/e-mail já cadastrados + filtro por permissão) e paginação (10 por página). diff --git a/app/models/user.rb b/app/models/user.rb index 35c3fe6..c127427 100644 --- a/app/models/user.rb +++ b/app/models/user.rb @@ -27,12 +27,28 @@ class User < ApplicationRecord # Quem está cadastrando/editando este usuário. Não é coluna — os # controllers preenchem (ver UsersController#set_user/#create e - # Api::V1::UsersController#create) só pra #validar_atribuicao_de_papel - # poder decidir. Fica nil no console, nos seeds e nos testes que não - # tratam dessa regra, e aí a validação não roda: quem chega no console - # já chega no banco, não há fronteira a defender ali. + # Api::V1::UsersController#create) pra #validar_atribuicao_de_papel + # poder decidir. attr_accessor :ator + # Declara que este registro está sendo escrito FORA de um contexto de + # requisição: seeds, console, factory de teste. Sem isto, definir ou + # mudar +role+ sem +ator+ é recusado (ver + # #validar_atribuicao_de_papel). + # + # O nome é deliberadamente incômodo de escrever, e é assim de propósito: + # quem o usa está afirmando "não há requisição aqui, não há fronteira a + # defender". Isso precisa ser uma decisão consciente, não o atalho para + # calar uma validação que apareceu no caminho. + attr_accessor :ator_dispensado + + # Mensagem de erro de programação, não de usuário: nenhuma tela leva a + # ela, porque os controllers sempre preenchem o ator. Se ela aparecer, o + # caminho que a produziu é novo e precisa decidir de qual lado da + # fronteira está — por isso ela nomeia as duas saídas. + ERRO_SEM_ATOR = 'só pode ser definido informando quem está fazendo a alteração ' \ + '(User#ator) — fora de uma requisição, use User#ator_dispensado' + # Usado por TelegramNotifier para avisar o usuário sobre demandas # atrasadas (ver app/services/telegram_notifier.rb). É opcional — nem # todo usuário precisa configurar. Só o admin cadastra/edita esse valor @@ -129,11 +145,24 @@ def self.ransackable_associations(_auth_object = nil) # # O que o líder continua podendo fazer, porque é a regra documentada: # alternar qualquer outro usuário entre executor e líder. + # + # A ausência de ator é RECUSA, não dispensa. Antes era o contrário + # (`return if ator.blank?`), e a diferença não é teórica: a invariante + # valia só porque os dois controllers de hoje lembram de preencher o + # ator. Qualquer caminho novo que esquecesse — um job, um importador, + # uma rake task, um endpoint futuro — passava direto, em silêncio, e o + # silêncio é o problema: um esquecimento virava brecha em vez de erro. + # + # Os lugares legítimos que escrevem papel sem ator (seeds, console, + # factory) declaram isso com +ator_dispensado+. São poucos, e é neles + # que o custo deve cair. def validar_atribuicao_de_papel - return if ator.blank? return unless papel_mudou? + return if ator_dispensado - if proprio_usuario? + if ator.blank? + errors.add(:role, ERRO_SEM_ATOR) + elsif proprio_usuario? errors.add(:role, 'não pode ser alterado por você mesmo — peça a outro líder ou admin') elsif envolve_papel_admin? && !ator.admin? errors.add(:role, 'de admin só pode ser concedido ou removido por outro admin') diff --git a/db/seeds.rb b/db/seeds.rb index b2f859e..3bb6163 100644 --- a/db/seeds.rb +++ b/db/seeds.rb @@ -10,11 +10,18 @@ return if Rails.env.test? # Seeds de exemplo para ambiente de desenvolvimento. +# +# O `u.ator_dispensado = true` dos três usuários abaixo declara o óbvio +# para o model: seeds rodam fora de qualquer requisição, não há ator e não +# há fronteira a defender. Sem essa declaração a criação é recusada — ver +# User#validar_atribuicao_de_papel, que trata ator ausente como recusa e +# não como dispensa. User.find_or_create_by!(email: 'admin@task-keeper.local') do |u| u.name = 'Admin Exemplo' u.password = 'senhaSegura123' u.password_confirmation = 'senhaSegura123' u.role = :admin + u.ator_dispensado = true end lider = User.find_or_create_by!(email: 'lider@task-keeper.local') do |u| @@ -22,6 +29,7 @@ u.password = 'senhaSegura123' u.password_confirmation = 'senhaSegura123' u.role = :lider + u.ator_dispensado = true end executor = User.find_or_create_by!(email: 'executor@task-keeper.local') do |u| @@ -29,6 +37,7 @@ u.password = 'senhaSegura123' u.password_confirmation = 'senhaSegura123' u.role = :executor + u.ator_dispensado = true end Demanda.find_or_create_by!(title: 'Preparar relatório semanal') do |d| diff --git a/spec/factories/users.rb b/spec/factories/users.rb index 55e9383..fa5c7cc 100644 --- a/spec/factories/users.rb +++ b/spec/factories/users.rb @@ -15,6 +15,21 @@ # abaixo (ver spec/requests/definir_senha_spec.rb). must_change_password { false } + # A criação de um usuário de teste acontece fora de qualquer + # requisição, então declara isso (ver User#ator_dispensado) — sem a + # marca, todo `create(:user)` da suíte falharia na validação de papel. + # + # O after(:create) apaga a marca em seguida, e isso é o ponto: um spec + # que mude o papel DEPOIS está exercitando a regra, e precisa informar + # o ator como qualquer requisição informaria. Sem essa limpeza, os + # specs de spec/models/user_spec.rb passariam por vacuidade — todos + # eles partem de um usuário criado por aqui. + ator_dispensado { true } + + after(:create) do |usuario| + usuario.ator_dispensado = false + end + trait :lider do role { :lider } end diff --git a/spec/models/user_spec.rb b/spec/models/user_spec.rb index dab5151..6a09a7a 100644 --- a/spec/models/user_spec.rb +++ b/spec/models/user_spec.rb @@ -129,9 +129,38 @@ def alterar(usuario, para:, por:) expect(admin).to be_valid end - it 'não roda sem ator (seeds e console seguem funcionando)' do + # A invariante é fail-CLOSED: o esquecimento vira erro visível, não + # brecha silenciosa. Antes esta validação começava com + # `return if ator.blank?`, e valia só porque os controllers de hoje + # lembram de preencher o ator — qualquer caminho novo passava direto. + it 'recusa a mudança de papel quando ninguém informou o ator' do alvo.role = 'admin' + expect(alvo).not_to be_valid + expect(alvo.errors[:role].join).to include('quem está fazendo a alteração') + end + + it 'recusa também a criação de um usuário com papel, sem ator' do + novo = build(:user, role: :admin, ator_dispensado: false) + + expect(novo).not_to be_valid + end + + # A saída declarada, para os poucos lugares legítimos: seeds, console + # e a própria factory desta suíte. + it 'permite quando o chamador declara que está fora de uma requisição' do + alvo.ator_dispensado = true + alvo.role = 'admin' + + expect(alvo).to be_valid + end + + # Sem esta guarda, exigir ator em toda escrita quebraria qualquer + # atualização de usuário feita fora de requisição (rake task, job) que + # não tem nada a ver com papel. + it 'não exige ator quando o papel não está mudando' do + alvo.name = 'Outro Nome' + expect(alvo).to be_valid end