diff --git a/app/avo/resources/user.rb b/app/avo/resources/user.rb index 1b31c19f..566cdf74 100644 --- a/app/avo/resources/user.rb +++ b/app/avo/resources/user.rb @@ -13,6 +13,9 @@ def fields field :id, as: :id field :email, as: :text, required: true field :company, as: :belongs_to + # El registro público crea la cuenta sin aprobar (Auth::RegisterUser): + # hasta que se tilde acá, no puede loguearse. + field :approved, as: :boolean field :created_at, as: :date_time, only_on: :index end diff --git a/app/controllers/api/v1/auth/registrations_controller.rb b/app/controllers/api/v1/auth/registrations_controller.rb index f28c290e..de1e95e7 100644 --- a/app/controllers/api/v1/auth/registrations_controller.rb +++ b/app/controllers/api/v1/auth/registrations_controller.rb @@ -5,7 +5,10 @@ module V1 module Auth class RegistrationsController < ApplicationController include TenantFromSlug + include AttemptLimit + rate_limit to: MAX_ATTEMPTS, within: WINDOW, only: :create, + with: :render_too_many_attempts skip_before_action :authenticate_user! skip_after_action :verify_authorized, :verify_policy_scoped @@ -13,14 +16,22 @@ def create company = tenant_company return render_unknown_tenant if company.nil? - user = ::Auth::RegisterUser.new(params: user_params, company: company).call - render json: UserSerializer.render(user), status: :created + ::Auth::RegisterUser.new(params: user_params, company: company).call + render_request_received rescue ActiveRecord::RecordInvalid => e render json: { errors: e.record.errors.full_messages }, status: :unprocessable_content end private + # 202 y el mismo cuerpo, se haya creado la solicitud o no porque el email + # ya tenía cuenta: ver Auth::RegisterUser. Tampoco devuelve la cuenta + # (antes devolvía su id y su company_id): una solicitud pendiente no es + # todavía nada que el que llama pueda usar. + def render_request_received + render json: { status: 'pending_approval' }, status: :accepted + end + # `company_id` ya no se permitea: el tenant sale del slug del request. # Mandarlo en el body no hace nada — no es un error, simplemente se # ignora, como cualquier atributo desconocido. diff --git a/app/controllers/api/v1/auth/sessions_controller.rb b/app/controllers/api/v1/auth/sessions_controller.rb index 60e19a52..b0ee97c2 100644 --- a/app/controllers/api/v1/auth/sessions_controller.rb +++ b/app/controllers/api/v1/auth/sessions_controller.rb @@ -5,7 +5,9 @@ module V1 module Auth class SessionsController < ApplicationController include TenantFromSlug + include AttemptLimit + before_action :refuse_exhausted_attempts, only: :create skip_before_action :authenticate_user!, only: :create skip_after_action :verify_authorized, :verify_policy_scoped @@ -24,6 +26,7 @@ def create if token render json: { token: token }, status: :ok else + count_failed_attempt render json: { error: 'Invalid email or password' }, status: :unauthorized end end @@ -39,6 +42,16 @@ def destroy head :no_content end + + private + + # El logout pasa aunque la empresa esté dada de baja o la cuenta haya + # perdido la aprobación. Es lo único que quien quedó afuera todavía quiere + # hacer, y si se le respondiera 401 el token nunca entraría a la denylist: + # con la empresa reactivada o la cuenta aprobada de nuevo dentro de sus + # 24 h, cualquier copia de ese token volvería a servir. El token sí tiene + # que ser válido: eso lo sigue exigiendo Devise. + def session_refusal = nil end end end diff --git a/app/controllers/api/v1/integrations_controller.rb b/app/controllers/api/v1/integrations_controller.rb index 8b84bd49..790b6c50 100644 --- a/app/controllers/api/v1/integrations_controller.rb +++ b/app/controllers/api/v1/integrations_controller.rb @@ -3,7 +3,11 @@ module Api module V1 class IntegrationsController < ApplicationController - skip_after_action :verify_authorized, :verify_policy_scoped + # El listado no pasa por Pundit: lo usa el widget de nodos del panel + # aunque la empresa no tenga la feature `integrations`, y sólo muestra las + # plantillas globales con el estado de la propia empresa. El alta y la + # modificación sí: ver CompanyIntegrationPolicy. + skip_after_action :verify_policy_scoped def index integrations = current_company.company_integrations.index_by(&:service_id) @@ -13,6 +17,7 @@ def index end def update + authorize CompanyIntegration integration = Integrations::UpsertIntegration.new( company: current_company, service_id: params[:service_id], diff --git a/app/controllers/api/v1/warehouses_controller.rb b/app/controllers/api/v1/warehouses_controller.rb index 0b482fd9..3eaad3dd 100644 --- a/app/controllers/api/v1/warehouses_controller.rb +++ b/app/controllers/api/v1/warehouses_controller.rb @@ -46,20 +46,30 @@ def set_warehouse authorize @warehouse end + # `expect` y no `require` + `permit`: si `warehouse` llega como String o + # como Array, `require` lo devuelve igual y `permit` revienta con + # NoMethodError, que salía como 500. `expect` responde 400. + # + # Un company_id en el body se sigue ignorando: `expect` descarta en + # silencio las claves que no permite. (El comentario anterior decía que + # respondía 400; en Rails 8.1 no es así, y el spec de company_id lo fija.) def warehouse_params - # permit (no expect) es intencional y load-bearing: expect usa - # on_unpermitted: :raise, así que un body con company_id daría 400 en - # vez de ignorarlo — rompiendo el requisito de la card. - # rubocop:disable-next Rails/StrongParametersExpect - params.require(:warehouse).permit(:name, :zip_code, :address) + params.expect(warehouse: %i[name zip_code address]) end - # Dos cosas bloquean el borrado y el mensaje tiene que decir cuál. Se + # Tres cosas bloquean el borrado y el mensaje tiene que decir cuál. Se # pregunta en el mismo orden en que el modelo las declara, que es el orden # en que `restrict_with_error` corta: si hay stock, el motivo es el stock. def render_conflict(_exception) - reason = @warehouse.stocks.exists? ? 'existing stock' : 'order lines taken from it' - render json: { error: "Cannot delete warehouse with #{reason}" }, status: :conflict + render json: { error: "Cannot delete warehouse with #{blocking_reason}" }, + status: :conflict + end + + def blocking_reason + return 'existing stock' if @warehouse.stocks.exists? + return 'order lines taken from it' if @warehouse.order_items.exists? + + 'stock transfers from or to it' end end end diff --git a/app/controllers/application_controller.rb b/app/controllers/application_controller.rb index eeb59afc..be176281 100644 --- a/app/controllers/application_controller.rb +++ b/app/controllers/application_controller.rb @@ -42,6 +42,28 @@ class MalformedParameterError < StandardError; end def index_action? = action_name == 'index' + # Devise valida el token (firma, vencimiento, denylist); acá se suma lo que el + # token no puede saber porque pudo cambiar después de emitido: si la empresa + # sigue activa y si la cuenta sigue aprobada. El JWT sigue siendo válido hasta + # vencer, y sin este chequeo la sesión seguía operando hasta 24 h después de + # la baja. + # + # Va sobre authenticate_user! y no en active_for_authentication? de Devise a + # propósito: ese hook corta con 401 cualquier request que traiga el token, + # también los que no exigen sesión (login, registro, tenant-config). Acá sólo + # corre donde se pide sesión. + def authenticate_user!(*) + super + refusal = session_refusal + render json: { error: refusal }, status: :unauthorized if refusal + end + + def session_refusal + return 'The company of this account is not active' unless current_company.is_active? + + 'This account is pending approval' unless current_user.approved? + end + def set_current_tenant Current.company_id = current_user&.company_id Current.user = current_user diff --git a/app/controllers/concerns/api/v1/auth/attempt_limit.rb b/app/controllers/concerns/api/v1/auth/attempt_limit.rb new file mode 100644 index 00000000..58a3ee75 --- /dev/null +++ b/app/controllers/concerns/api/v1/auth/attempt_limit.rb @@ -0,0 +1,68 @@ +# frozen_string_literal: true + +module Api + module V1 + module Auth + # Límite de intentos para los endpoints públicos de auth: login y registro. + # + # Sin esto se podían probar contraseñas sin freno: después de 30 incorrectas + # seguidas, la correcta entraba igual. Se limita por IP y no con el + # :lockable de Devise, que bloquea la cuenta: con él, cualquiera podría + # dejar afuera a otro usuario tipeando mal su password a propósito. + # + # Cada endpoint cuenta distinto: + # - Login: sólo los intentos fallidos (before_action + # :refuse_exhausted_attempts + count_failed_attempt en la rama del 401). + # El que entra no está adivinando nada, y contar los exitosos dejaba + # afuera al undécimo operario de un depósito detrás de un mismo NAT + # aunque pusiera bien su password. + # - Registro: todos, con el rate_limit de Rails. Cada pedido crea una + # solicitud, y no hay un «pedido exitoso» que se repita en el uso normal. + # + # El contador vive en la cache de la app, que en producción es Solid Cache y + # la comparten todos los procesos. En test la cache es :null_store y el + # límite nunca se alcanza, así los specs que loguean muchas veces no chocan + # con él; el spec del límite le da una cache de verdad a propósito. + module AttemptLimit + extend ActiveSupport::Concern + + MAX_ATTEMPTS = 10 + WINDOW = 3.minutes + + private + + # Una vez agotados se rechaza sin mirar la password, también la correcta: + # si se la evaluara, el que prueba contraseñas seguiría probando y la + # correcta le daría el token igual. + def refuse_exhausted_attempts + render_too_many_attempts if failed_attempts >= MAX_ATTEMPTS + end + + # La ventana es fija: el increment de la cache conserva el vencimiento + # que tomó la entrada con el primer fallo. + def count_failed_attempt + attempt_store.increment(failed_attempts_key, 1, expires_in: WINDOW) + end + + # `raw: true` porque el valor lo escribió increment, que en algunos stores + # guarda el entero sin serializar. + def failed_attempts + attempt_store.read(failed_attempts_key, raw: true).to_i + end + + def failed_attempts_key + "auth-failed-attempts:#{request.remote_ip}" + end + + def attempt_store + self.class.cache_store + end + + def render_too_many_attempts + response.set_header('Retry-After', WINDOW.to_i.to_s) + render json: { error: 'Too many attempts, try again later' }, status: :too_many_requests + end + end + end + end +end diff --git a/app/models/company.rb b/app/models/company.rb index 66ca3ddb..e3272bf6 100644 --- a/app/models/company.rb +++ b/app/models/company.rb @@ -33,6 +33,13 @@ def self.find_active_by_slug(slug) active.find_by(slug: normalized) end + # Un flag ausente es un flag apagado, mismo criterio que el front + # (isFeatureEnabled): la empresa que no compró la feature no la tiene + # declarada, y el default nunca puede ser habilitarla. + def feature_enabled?(feature) + features[feature.to_s] == true + end + private def assign_default_slug diff --git a/app/models/user.rb b/app/models/user.rb index 7ebff7e4..69c21a47 100644 --- a/app/models/user.rb +++ b/app/models/user.rb @@ -3,6 +3,10 @@ class User < ApplicationRecord belongs_to :company + # `approved` tiene default true: sólo el registro público crea cuentas sin + # aprobar (Auth::RegisterUser), que no pueden loguearse hasta que alguien las + # apruebe desde el backoffice. + # Include default devise modules. Others available are: # :confirmable, :lockable, :timeoutable, :trackable and :omniauthable devise :database_authenticatable, :registerable, diff --git a/app/models/warehouse.rb b/app/models/warehouse.rb index 82b0bab8..fdd6eef5 100644 --- a/app/models/warehouse.rb +++ b/app/models/warehouse.rb @@ -12,6 +12,17 @@ class Warehouse < ApplicationRecord # devolución sin destino. La FK es restrict por lo mismo; esto la adelanta a # un 409 legible en vez de un InvalidForeignKey. has_many :order_items, dependent: :restrict_with_error + # Tampoco si una transferencia lo tiene como origen o como destino: la FK de + # stock_transfers también es restrict, y sin esto el borrado llegaba a la base + # y respondía 500. El caso real es el destino de una transferencia en + # tránsito: todavía no tiene stock propio, porque las unidades se le suman + # recién al recibirla. + has_many :outgoing_transfers, class_name: 'StockTransfer', inverse_of: :origin_warehouse, + foreign_key: :origin_warehouse_id, + dependent: :restrict_with_error + has_many :incoming_transfers, class_name: 'StockTransfer', inverse_of: :destination_warehouse, + foreign_key: :destination_warehouse_id, + dependent: :restrict_with_error validates :name, presence: true validates :zip_code, presence: true diff --git a/app/policies/company_integration_policy.rb b/app/policies/company_integration_policy.rb new file mode 100644 index 00000000..a658233e --- /dev/null +++ b/app/policies/company_integration_policy.rb @@ -0,0 +1,11 @@ +# frozen_string_literal: true + +class CompanyIntegrationPolicy < ApplicationPolicy + # El flag `integrations` de la empresa sólo lo aplicaba el front: una empresa + # sin la feature configuraba integraciones llamando al endpoint directo (QA de + # TESIS-82). Se compara con `== true` en Company#feature_enabled?, igual que + # el front, así los dos deciden lo mismo. + def update? + user.present? && user.company.feature_enabled?(:integrations) + end +end diff --git a/app/poros/auth/authenticate_user.rb b/app/poros/auth/authenticate_user.rb index 32661c83..eb8951ed 100644 --- a/app/poros/auth/authenticate_user.rb +++ b/app/poros/auth/authenticate_user.rb @@ -10,26 +10,53 @@ def initialize(email:, password:, company:) end # Devuelve nil ante cualquier fallo — tenant no resuelto, email inexistente - # en ese tenant, usuario de otro tenant o password incorrecta. El caller no - # puede distinguir los casos, que es justamente el punto: el 401 tiene que - # ser idéntico para todos. + # en ese tenant, usuario de otro tenant, password incorrecta o cuenta que + # todavía no aprobaron. El caller no puede distinguir los casos, que es + # justamente el punto: el 401 tiene que ser idéntico para todos. def call - return nil if @company.nil? - user = find_user - return nil unless user&.valid_password?(@password) + return nil unless password_matches?(user) && user.approved? Warden::JWTAuth::UserEncoder.new.call(user, :user, nil).first end + # Digest descartable contra el que se compara cuando no hay cuenta. Se arma + # con el mismo Devise::Encryptor (y por lo tanto el mismo costo) que las + # passwords reales. + def self.dummy_digest + @dummy_digest ||= Devise::Encryptor.digest(User, SecureRandom.hex(32)) + end + private + # Se corre bcrypt aunque no haya cuenta que comparar. Si no, el 401 de un + # email inexistente (o de un tenant no resuelto) volvía mucho antes que el + # de una password incorrecta, y el tiempo de respuesta decía qué emails + # existen aunque el cuerpo fuera idéntico. + def password_matches?(user) + return user.valid_password?(@password) if user + + Devise::Encryptor.compare(User, self.class.dummy_digest, @password) + false + end + # `unscoped` explícito: User incluye CompanyScoped, y si el request de login # llegara con un JWT viejo en el header, el default scope filtraría por el # tenant de ese token y no por el que se está intentando. El scope acá es el # de la company resuelta por slug, y sólo ese. def find_user - User.unscoped.find_by(company_id: @company.id, email: @email) + return nil if @company.nil? + + User.unscoped.find_by(company_id: @company.id, email: normalized_email) + end + + # Devise guarda el email en minúsculas y sin espacios alrededor + # (case_insensitive_keys y strip_whitespace_keys), pero esa normalización + # sólo corre al guardar y en sus propios finders, y este find_by es nuestro. + # Sin esto, quien se registró como «Ana@Norte.com» no podía entrar tipeando + # el email igual que al registrarse. + def normalized_email + @email.to_s.strip.downcase end end end diff --git a/app/poros/auth/register_user.rb b/app/poros/auth/register_user.rb index 48eb4416..fb98d57e 100644 --- a/app/poros/auth/register_user.rb +++ b/app/poros/auth/register_user.rb @@ -8,18 +8,50 @@ def initialize(params:, company:) @company = company end - # La company es la que resolvió el slug del request, nunca la que venga en - # el body: hasta TESIS-120 `company_id` era un parámetro permitido y con eso - # cualquiera podía darse de alta dentro de cualquier tenant. + # Registrarse es pedir acceso (S02: «Solicitá acceso al espacio de operación + # de tu organización»): la cuenta nace sin aprobar y no puede loguearse + # hasta que la aprueben desde el backoffice. Antes el alta daba acceso + # inmediato, y como el slug es público (es el subdominio), cualquiera podía + # entrar a leer los datos de cualquier empresa. # - # El `Current.set` no es decorativo: User incluye CompanyScoped, que en el - # create pisa `company_id` con `Current.company_id` si está seteado. Sin - # esto, un register con un JWT de otro tenant en el header crearía el - # usuario en ese otro tenant en vez de en el del slug. + # Devuelve la cuenta creada, o nil si el email ya tenía una. El controller + # responde lo mismo en los dos casos: el email es único en toda la base, y + # si la respuesta cambiara, el registro diría qué emails existen en esta + # empresa o en cualquier otra. + # + # La company es la que resolvió el slug, nunca la del body (TESIS-120). El + # `Current.set` no es decorativo: CompanyScoped pisa `company_id` con + # `Current.company_id` en el create, y un register con un JWT de otro tenant + # en el header crearía la cuenta en ese otro tenant. def call Current.set(company_id: @company.id) do - User.create!(@params.merge(company: @company)) + user = User.new(@params.merge(company: @company, approved: false)) + next if email_taken?(user) + + user.save! + user end + rescue ActiveRecord::RecordNotUnique + # Dos registros simultáneos del mismo email nuevo pasan los dos la + # validación y el segundo choca contra el índice único. Para el que llama + # es lo mismo que un email que ya tenía cuenta. + nil + end + + private + + # Valida lo que escribió el que llama y dice si el email ya tenía cuenta. + # Los errores de formato (email mal escrito, password corta) se levantan + # siempre, y ANTES de mirar si el email existe: al revés, una password + # inválida respondería distinto según el email estuviera tomado o no, y la + # diferencia volvería a delatarlo. + def email_taken?(user) + user.validate + taken = user.errors.of_kind?(:email, :taken) + user.errors.delete(:email, :taken) + raise ActiveRecord::RecordInvalid, user if user.errors.any? + + taken end end end diff --git a/db/migrate/20260924120000_add_approved_to_users.rb b/db/migrate/20260924120000_add_approved_to_users.rb new file mode 100644 index 00000000..8bdae7d0 --- /dev/null +++ b/db/migrate/20260924120000_add_approved_to_users.rb @@ -0,0 +1,14 @@ +# frozen_string_literal: true + +class AddApprovedToUsers < ActiveRecord::Migration[8.1] + # Registrarse pasa a ser pedir acceso (S02: «Solicitá acceso al espacio de + # operación de tu organización»). La cuenta que crea el registro público nace + # sin aprobar y no puede loguearse hasta que la aprueben. + # + # Default true a propósito: las cuentas que ya existen, y las que crean el + # backoffice, los seeds o la consola, quedan habilitadas como hasta ahora. + # Sólo Auth::RegisterUser crea cuentas con false. + def change + add_column :users, :approved, :boolean, default: true, null: false + end +end diff --git a/db/schema.rb b/db/schema.rb index 338cf76c..74dd0156 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_09_21_130000) do +ActiveRecord::Schema[8.1].define(version: 2026_09_24_120000) do # These are extensions that must be enabled in order to support this database enable_extension "pg_catalog.plpgsql" @@ -224,6 +224,7 @@ end create_table "users", force: :cascade do |t| + t.boolean "approved", default: true, null: false t.bigint "company_id", null: false t.datetime "created_at", null: false t.string "email", default: "", null: false diff --git a/docs/adr/ADR-002-authentication.md b/docs/adr/ADR-002-authentication.md index 64ed64aa..9f89a295 100644 --- a/docs/adr/ADR-002-authentication.md +++ b/docs/adr/ADR-002-authentication.md @@ -84,3 +84,100 @@ una pestaña. Hay un spec dedicado a ese caso. puramente stateless: es el precio de poder revocar - ⚠️ La tabla es global y **no** lleva `CompanyScoped`. Filtrar por empresa dejaría pasar un token revocado desde otro contexto de tenant + + +--- + +## Actualización — QA del módulo de auth (2026-09-24, TESIS-82) + +La validación del módulo encontró cuatro huecos en cómo se entra y cómo se +sigue adentro. Esta sección registra cómo se cerró cada uno. + +### El registro es una solicitud de acceso + +`POST /auth/register` creaba una cuenta que podía loguearse en el acto, dentro +de la empresa que nombrara el header `X-Tenant-Slug`. El slug es público (es el +subdominio), así que cualquiera podía darse de alta en cualquier empresa y leer +todos sus datos. TESIS-120 ya había sacado `company_id` del body, pero el slug +que lo reemplazó también lo elige quien llama. + +Registrarse pasa a ser pedir acceso, que es lo que dice la pantalla S02 +(«Solicitá acceso al espacio de operación de tu organización»): + +- `users.approved`, booleano con **default `true`**. Las cuentas que ya existían + y las que crean el backoffice, los seeds o la consola nacen habilitadas; sólo + `Auth::RegisterUser` crea cuentas en `false`. +- Una cuenta sin aprobar no obtiene token: el login le responde el mismo 401 que + a una password incorrecta. Se aprueba desde el backoffice (campo `approved` del + recurso User). +- El endpoint responde **202 con el mismo cuerpo** haya creado la solicitud o no. + El email es único en toda la base, y el viejo 422 «Email has already been + taken» decía qué emails tenían cuenta en cualquier empresa. Los errores de + formato se siguen informando, pero antes de mirar si el email existe: si no, + una password corta respondería distinto según el email estuviera tomado. + +Se descartó hacer el email único por empresa: resolvía la enumeración pero +pedía reemplazar la validación de Devise y una migración de índices, y el 202 +indistinguible ya la cierra. + +### La sesión se revisa en cada request, no sólo al loguearse + +El JWT sigue siendo válido hasta vencer aunque cambie algo que el token no +puede saber. Una empresa dada de baja seguía operando hasta 24 h con los tokens +que ya tenía. `ApplicationController#authenticate_user!` revisa ahora, después +de Devise, que la empresa siga activa y la cuenta aprobada, y responde 401 si +no. + +Va sobre `authenticate_user!` y no en `active_for_authentication?` a propósito: +el hook de Devise corta con 401 cualquier request que traiga el token, también +los que no exigen sesión (login, registro, tenant-config). + +El logout es la excepción: `DELETE /auth/logout` sólo exige un token válido, y +revoca aunque la empresa esté inactiva o la cuenta sin aprobar. Si respondiera +401, el token nunca entraría a la denylist y, como el corte es reversible, +cualquier copia de él volvería a servir al reactivarse la empresa o aprobarse +de nuevo la cuenta dentro de sus 24 h. + +El corte es una suspensión, no una baja: reactivar la empresa devuelve el +acceso a los tokens que siguen vivos y que nadie cerró. Revocar todos los tokens +de la empresa al desactivarla se descartó por ahora: la denylist guarda `jti` +emitidos, no sesiones abiertas, y no hay de dónde sacar los que no se cerraron. + +### Límite de intentos por IP + +No había freno: después de 30 passwords incorrectas seguidas, la correcta +entraba. Login y registro aceptan ahora 10 intentos cada 3 minutos por IP +(`Api::V1::Auth::AttemptLimit`) y después responden 429 con `Retry-After`. + +- En el login cuentan **sólo los intentos fallidos**. Contar todos (el + `rate_limit` de Rails cuenta requests) dejaba afuera al undécimo operario que + entra al turno detrás del mismo NAT, con la password correcta. Agotados los + intentos se rechaza sin evaluar la password, también la correcta. +- En el registro cuentan todos, con el `rate_limit` de Rails: cada pedido crea + una solicitud y no hay un uso normal que lo repita. + +- Se prefirió al `:lockable` de Devise porque bloquear la cuenta deja que + cualquiera deje afuera a otro usuario tipeando mal su password a propósito. +- El contador vive en la cache de la app: Solid Cache en producción, compartida + por todos los procesos. ⚠️ El entorno desplegado necesita la base de cache + (`proyecto_api_production_cache`), y `request.remote_ip` tiene que ser la IP + del cliente y no la del proxy: si no, todos comparten un solo contador. + +### El email del login se normaliza + +Devise guarda el email en minúsculas y sin espacios, pero sólo normaliza al +guardar y en sus propios finders. `Auth::AuthenticateUser` busca con su propio +`find_by`, así que quien se registró como «Ana@Norte.com» recibía 401 con la +password correcta. Ahora normaliza igual antes de buscar. + +### El tiempo del 401 no delata qué emails existen + +Con un email inexistente (o un tenant no resuelto) no había password contra la +cual correr bcrypt, y el 401 volvía mucho antes que el de una password +incorrecta: el cuerpo era idéntico, pero el tiempo de respuesta enumeraba +usuarios. `Auth::AuthenticateUser` compara ahora contra un digest descartable, +armado con el mismo `Devise::Encryptor` y el mismo costo, cuando no encuentra la +cuenta. + +- ⚠️ En `Auth::RegisterUser` queda una diferencia más chica: el camino del email + ya tomado se saltea el INSERT. Se deja como limitación conocida. diff --git a/spec/models/company_spec.rb b/spec/models/company_spec.rb index 1cf0200f..a6323cb2 100644 --- a/spec/models/company_spec.rb +++ b/spec/models/company_spec.rb @@ -41,6 +41,25 @@ expect(company.reload.branding['primary_color']).to eq('#2E7D32') end + + describe '#feature_enabled?' do + it 'is true for a feature turned on' do + company.features = { 'integrations' => true } + + expect(company.feature_enabled?(:integrations)).to be(true) + end + + it 'is false for a feature turned off' do + company.features = { 'integrations' => false } + + expect(company.feature_enabled?(:integrations)).to be(false) + end + + # La empresa que no compró la feature no la tiene declarada. + it 'is false for a feature that is not declared' do + expect(company.feature_enabled?(:integrations)).to be(false) + end + end end describe 'slug' do diff --git a/spec/models/user_spec.rb b/spec/models/user_spec.rb index b2fe7017..da96f3ce 100644 --- a/spec/models/user_spec.rb +++ b/spec/models/user_spec.rb @@ -14,6 +14,12 @@ expect(user).not_to be_valid end + # Las cuentas que crean el backoffice, los seeds o la consola quedan + # habilitadas. Sólo el registro público las crea sin aprobar. + it 'is approved unless it is created as an access request' do + expect(user.approved).to be(true) + end + it 'belongs to a company' do expect(described_class.reflect_on_association(:company).macro).to eq(:belongs_to) end diff --git a/spec/policies/company_integration_policy_spec.rb b/spec/policies/company_integration_policy_spec.rb new file mode 100644 index 00000000..b1281f2e --- /dev/null +++ b/spec/policies/company_integration_policy_spec.rb @@ -0,0 +1,42 @@ +# frozen_string_literal: true + +require 'rails_helper' + +RSpec.describe CompanyIntegrationPolicy, type: :policy do + subject(:policy) { described_class.new(user, CompanyIntegration) } + + let(:company) do + Company.create!(name: 'Tenant A', tax_id: '30-11111111-1', features: { 'integrations' => true }) + end + let(:user) { User.create!(email: 'a@example.com', password: 'password123', company: company) } + + it 'lets a company with the integrations feature configure them' do + expect(policy.update?).to be(true) + end + + context 'when the company does not have the feature' do + before { company.update!(features: { 'integrations' => false }) } + + it 'denies configuring them' do + expect(policy.update?).to be(false) + end + end + + # Igual que el front (`features?.[feature] === true`): un "true" string es + # apagado en los dos lados. + context 'when the flag is not a real boolean' do + before { company.update!(features: { 'integrations' => 'true' }) } + + it 'denies configuring them' do + expect(policy.update?).to be(false) + end + end + + context 'without a user' do + let(:user) { nil } + + it 'denies configuring them' do + expect(policy.update?).to be(false) + end + end +end diff --git a/spec/poros/auth/authenticate_user_spec.rb b/spec/poros/auth/authenticate_user_spec.rb index 3e359f8f..1f9da194 100644 --- a/spec/poros/auth/authenticate_user_spec.rb +++ b/spec/poros/auth/authenticate_user_spec.rb @@ -12,6 +12,14 @@ expect(token).to be_present end + # Devise guarda el email normalizado; el que se tipea en el login puede venir + # con otras mayúsculas o con espacios alrededor. + it 'finds the account whatever the case and the surrounding spaces of the email' do + token = described_class.new(email: ' Log@TEST.com ', password: 'password123', + company: company).call + expect(token).to be_present + end + it 'returns nil for a wrong password' do token = described_class.new(email: 'log@test.com', password: 'wrong', company: company).call expect(token).to be_nil @@ -31,6 +39,15 @@ expect(token).to be_nil end + # Una cuenta del registro público que todavía no aprobaron: credenciales + # correctas, pero no hay token hasta que la aprueben. + it 'returns nil for an account pending approval' do + User.create!(email: 'pend@test.com', password: 'password123', company: company, approved: false) + + token = described_class.new(email: 'pend@test.com', password: 'password123', company: company).call + expect(token).to be_nil + end + # El controller pasa el resultado de resolver el slug, que es nil cuando el # tenant no existe o está inactivo. El PORO no puede confundir eso con un # login global. @@ -39,6 +56,24 @@ expect(token).to be_nil end + # Sin cuenta que comparar se corre bcrypt igual: si no, el tiempo de respuesta + # diría qué emails existen aunque el resultado sea el mismo nil. + it 'still runs bcrypt when the email does not exist' do + allow(Devise::Encryptor).to receive(:compare).and_call_original + + described_class.new(email: 'ghost@test.com', password: 'password123', company: company).call + + expect(Devise::Encryptor).to have_received(:compare).with(User, described_class.dummy_digest, 'password123') + end + + it 'still runs bcrypt when the tenant could not be resolved' do + allow(Devise::Encryptor).to receive(:compare).and_call_original + + described_class.new(email: 'log@test.com', password: 'password123', company: nil).call + + expect(Devise::Encryptor).to have_received(:compare).once + end + # User incluye CompanyScoped: si el default scope se colara acá, un Current # heredado de otro request decidiría el tenant en vez del slug. it 'ignores a leftover Current.company_id and honours the given company' do diff --git a/spec/poros/auth/register_user_spec.rb b/spec/poros/auth/register_user_spec.rb index 76505266..1affb260 100644 --- a/spec/poros/auth/register_user_spec.rb +++ b/spec/poros/auth/register_user_spec.rb @@ -9,13 +9,47 @@ def register(extra_params = {}, into: company) described_class.new(params: params, company: into).call end - it 'creates a user under the given company', :aggregate_failures do + it 'creates the account under the given company', :aggregate_failures do user = register expect(user).to be_persisted expect(user.company).to eq(company) end + # Registrarse es pedir acceso: la cuenta no entra hasta que la aprueben. + it 'creates the account pending approval' do + expect(register.approved).to be(false) + end + + context 'when the email already has an account' do + before { User.create!(email: 'a@test.com', password: 'password123', company: otra) } + + it 'creates nothing and returns nil', :aggregate_failures do + result = nil + + expect { result = register }.not_to change(User, :count) + expect(result).to be_nil + end + + # El error de formato se informa igual que con un email libre, y sin + # mencionar que el email ya existe. + it 'still raises on invalid params, without saying the email is taken' do + expect { register({ password: '123' }) } + .to raise_error(ActiveRecord::RecordInvalid, + 'Validation failed: Password is too short (minimum is 6 characters)') + end + end + + # Dos registros simultáneos del mismo email pasan los dos la validación y el + # segundo choca contra el índice único: es un email que ya tenía cuenta. + it 'treats a duplicate caught by the unique index as an existing email' do + allow(User).to receive(:new).and_wrap_original do |original, *args| + original.call(*args).tap { |user| allow(user).to receive(:save!).and_raise(ActiveRecord::RecordNotUnique) } + end + + expect(register).to be_nil + end + # El company_id del body ya no llega hasta acá (el controller no lo permitea), # pero si llegara tampoco debe ganarle a la company resuelta por slug. it 'ignores a company_id passed in the params', :aggregate_failures do diff --git a/spec/requests/api/v1/auth_attempt_limit_spec.rb b/spec/requests/api/v1/auth_attempt_limit_spec.rb new file mode 100644 index 00000000..ab1d4c88 --- /dev/null +++ b/spec/requests/api/v1/auth_attempt_limit_spec.rb @@ -0,0 +1,96 @@ +# frozen_string_literal: true + +require 'rails_helper' + +# Límite de intentos de login y registro. Lo pidió la QA de TESIS-82: después +# de 30 passwords incorrectas seguidas, la correcta entraba igual. +# +# En test la cache es :null_store y el límite nunca se alcanza, así los specs +# que loguean muchas veces no chocan con él. Acá el contador usa una cache de +# verdad, sólo durante cada ejemplo. +RSpec.describe 'Auth attempt limit', type: :request do + include ActiveSupport::Testing::TimeHelpers + + let(:company) { Company.create!(name: 'Acme', tax_id: '20-11111111-1', slug: 'acme') } + let(:counter) { ActiveSupport::Cache::MemoryStore.new } + let(:max_attempts) { Api::V1::Auth::AttemptLimit::MAX_ATTEMPTS } + + before do + User.create!(email: 'log@test.com', password: 'password123', company: company) + store = Api::V1::Auth::SessionsController.cache_store + allow(store).to receive(:increment) { |*args, **options| counter.increment(*args, **options) } + allow(store).to receive(:read) { |*args, **options| counter.read(*args, **options) } + end + + def login(password: 'wrong', ip: '203.0.113.7') + post '/api/v1/auth/login', params: { email: 'log@test.com', password: password }, + headers: { 'X-Tenant-Slug' => company.slug, 'REMOTE_ADDR' => ip } + response + end + + def use_up_attempts(ip: '203.0.113.7') + max_attempts.times { login(ip: ip) } + end + + it 'lets every attempt of the window through' do + (max_attempts - 1).times { login } + + expect(login(password: 'password123')).to have_http_status(:ok) + end + + # Lo que se frena es al que prueba contraseñas, no al que entra: un depósito + # con varios operarios detrás del mismo NAT loguea muchas veces al empezar el + # turno. + it 'does not count the logins that succeed' do + (max_attempts + 2).times { login(password: 'password123') } + + expect(response).to have_http_status(:ok) + end + + it 'answers 429 once the attempts run out, even with the right password', :aggregate_failures do + use_up_attempts + login(password: 'password123') + + expect(response).to have_http_status(:too_many_requests) + expect(response.parsed_body['error']).to eq('Too many attempts, try again later') + end + + it 'tells the client when it can try again' do + use_up_attempts + + expect(login.headers['Retry-After']).to eq('180') + end + + it 'counts each IP on its own' do + use_up_attempts(ip: '203.0.113.7') + + expect(login(password: 'password123', ip: '198.51.100.9')).to have_http_status(:ok) + end + + it 'lets the IP in again once the window is over' do + use_up_attempts + + travel(Api::V1::Auth::AttemptLimit::WINDOW + 1.second) do + expect(login(password: 'password123')).to have_http_status(:ok) + end + end + + it 'limits the registration too' do + (max_attempts + 1).times do |n| + post '/api/v1/auth/register', params: { email: "nuevo#{n}@test.com", password: 'password123' }, + headers: { 'X-Tenant-Slug' => company.slug } + end + + expect(response).to have_http_status(:too_many_requests) + end + + # El logout no es un intento de adivinar nada: no cuenta ni se frena. + it 'does not limit the logout' do + token = login(password: 'password123').parsed_body['token'] + use_up_attempts + + delete '/api/v1/auth/logout', headers: { 'Authorization' => "Bearer #{token}", 'REMOTE_ADDR' => '203.0.113.7' } + + expect(response).to have_http_status(:no_content) + end +end diff --git a/spec/requests/api/v1/auth_spec.rb b/spec/requests/api/v1/auth_spec.rb index 34e27362..bcf5c3f0 100644 --- a/spec/requests/api/v1/auth_spec.rb +++ b/spec/requests/api/v1/auth_spec.rb @@ -21,23 +21,24 @@ def register(params = valid_params, slug: company.slug) [response.status, response.body] end - it 'creates a user', :aggregate_failures do + # Registrarse es pedir acceso (S02): la cuenta queda pendiente y la respuesta + # no devuelve nada que el que llama pueda usar. + it 'creates the account as a request pending approval', :aggregate_failures do expect { register }.to change(User, :count).by(1) - expect(response).to have_http_status(:created) + expect(response).to have_http_status(:accepted) + expect(User.last.approved).to be(false) end - it 'creates the user inside the tenant of the slug' do + it 'answers only that the request is pending' do register - expect(User.last.company).to eq(company) + expect(response.parsed_body).to eq('status' => 'pending_approval') end - it 'returns the user without exposing the password', :aggregate_failures do + it 'creates the account inside the tenant of the slug' do register - body = response.parsed_body - expect(body['email']).to eq('new@test.com') - expect(body.keys).not_to include('password', 'encrypted_password') + expect(User.last.company).to eq(company) end it 'stores the password hashed, never in plain text', :aggregate_failures do @@ -47,11 +48,68 @@ def register(params = valid_params, slug: company.slug) expect(User.last.encrypted_password).not_to eq('password123') end - it 'rejects a duplicate email' do - User.create!(email: 'dup@test.com', password: 'password123', company: company) - register(valid_params.merge(email: 'dup@test.com')) + it 'returns 422 for invalid input' do + expect(register(valid_params.merge(password: '123')).first).to eq(422) + end + + # El agujero que encontró la QA de TESIS-82: el registro daba acceso + # inmediato, y el slug es público (es el subdominio). Cualquiera se daba de + # alta en cualquier empresa y leía sus datos. + context 'when the account was just requested' do + def login_attempt(password: valid_params[:password]) + post '/api/v1/auth/login', params: valid_params.merge(password: password), headers: tenant_header + [response.status, response.body] + end + + before { register } + + it 'cannot log in, and the answer is the one of a wrong password', :aggregate_failures do + pending_login = login_attempt + + expect(pending_login.first).to eq(401) + expect(pending_login).to eq(login_attempt(password: 'wrong')) + end + + it 'logs in once it is approved' do + User.last.update!(approved: true) + + expect(login_attempt.first).to eq(200) + end + end + + # El email es único en toda la base. Si la respuesta cambiara cuando ya tiene + # cuenta, el registro diría qué emails existen, en esta empresa o en otra. + context 'when the email already has an account' do + let(:otra) { Company.create!(name: 'Otra', tax_id: '20-22222222-2', slug: 'otra') } + + before do + User.create!(email: 'ajeno@test.com', password: 'password123', company: otra) + User.create!(email: 'propio@test.com', password: 'password123', company: company) + end + + it 'does not create a second account' do + expect { register(valid_params.merge(email: 'ajeno@test.com')) }.not_to change(User, :count) + end + + it 'answers exactly like for a new email when the account is in another company' do + taken = register(valid_params.merge(email: 'ajeno@test.com')) + + expect(taken).to eq(register(valid_params.merge(email: 'nuevo@test.com'))) + end + + it 'answers exactly like for a new email when the account is in the same company' do + taken = register(valid_params.merge(email: 'propio@test.com')) + + expect(taken).to eq(register(valid_params.merge(email: 'nuevo@test.com'))) + end + + # Un error de formato se informa siempre. Si sólo se informara cuando el + # email está libre, la diferencia lo delataría igual. + it 'reports invalid input without telling whether the email exists' do + taken = register(valid_params.merge(email: 'ajeno@test.com', password: '123')) - expect(response).to have_http_status(:unprocessable_content) + expect(taken).to eq(register(valid_params.merge(email: 'nuevo@test.com', password: '123'))) + end end # El agujero que cierra TESIS-120: hasta acá `company_id` era un parámetro @@ -63,7 +121,7 @@ def register(params = valid_params, slug: company.slug) it 'ignores it and uses the tenant of the slug', :aggregate_failures do register(valid_params.merge(company_id: otra.id)) - expect(response).to have_http_status(:created) + expect(response).to have_http_status(:accepted) expect(User.last.company).to eq(company) expect(otra.users).to be_empty end @@ -113,6 +171,12 @@ def login(email: 'log@test.com', password: 'password123', slug: company.slug) expect(payload['company_id']).to eq(company.id) end + # Hallazgo de la QA de TESIS-82: Devise guarda el email en minúsculas, y + # quien lo tipeaba con alguna mayúscula recibía 401 con la password correcta. + it 'accepts the email in another case and with surrounding spaces' do + expect(login(email: ' Log@TEST.com ').first).to eq(200) + end + it 'returns 401 on wrong password' do expect(login(password: 'wrong').first).to eq(401) end @@ -170,6 +234,66 @@ def cross_tenant = login(slug: sur.slug) end end + # El JWT sigue siendo válido hasta vencer aunque la cuenta deje de estar + # habilitada. Dar de baja la empresa, o quitarle la aprobación a la cuenta, + # tiene que cortar también las sesiones que ya estaban abiertas, no sólo los + # logins nuevos. + describe 'a session that stops being allowed' do + let(:user) { User.create!(email: 'baja@test.com', password: 'password123', company: company) } + let(:auth) do + post '/api/v1/auth/login', params: { email: user.email, password: 'password123' }, headers: tenant_header + { 'Authorization' => "Bearer #{response.parsed_body['token']}" } + end + + # Se abre la sesión, pasa el cambio y recién ahí se usa el token. + def request_after_change + headers = auth + yield + get '/api/v1/warehouses', headers: headers + end + + it 'ends when the company is deactivated', :aggregate_failures do + request_after_change { company.update!(is_active: false) } + + expect(response).to have_http_status(:unauthorized) + expect(response.parsed_body['error']).to eq('The company of this account is not active') + end + + it 'ends when the account loses its approval', :aggregate_failures do + request_after_change { user.update!(approved: false) } + + expect(response).to have_http_status(:unauthorized) + expect(response.parsed_body['error']).to eq('This account is pending approval') + end + + # El logout es lo único que quien quedó afuera todavía puede querer hacer. Si + # se le respondiera 401, el token no entraría a la denylist y volvería a + # servir al revertirse el cambio dentro de sus 24 h. + def logout_and_revert(change, revert) + headers = auth + change.call + delete '/api/v1/auth/logout', headers: headers + status = response.status + revert.call + get '/api/v1/warehouses', headers: headers + [status, response.status] + end + + it 'can still log out while the company is inactive, for good' do + statuses = logout_and_revert(-> { company.update!(is_active: false) }, + -> { company.update!(is_active: true) }) + + expect(statuses).to eq([204, 401]) + end + + it 'can still log out while the account is pending approval, for good' do + statuses = logout_and_revert(-> { user.update!(approved: false) }, + -> { user.update!(approved: true) }) + + expect(statuses).to eq([204, 401]) + end + end + describe 'DELETE /api/v1/auth/logout' do let(:user) { User.create!(email: 'out@test.com', password: 'password123', company: company) } let(:token) do diff --git a/spec/requests/api/v1/integrations_spec.rb b/spec/requests/api/v1/integrations_spec.rb index 0bd1ae20..e5bb5cb3 100644 --- a/spec/requests/api/v1/integrations_spec.rb +++ b/spec/requests/api/v1/integrations_spec.rb @@ -3,7 +3,9 @@ require 'rails_helper' RSpec.describe 'Integrations API', type: :request do - let(:company) { Company.create!(name: 'Tenant A', tax_id: '30-11111111-1') } + let(:company) do + Company.create!(name: 'Tenant A', tax_id: '30-11111111-1', features: { 'integrations' => true }) + end let(:user) { User.create!(email: 'a@example.com', password: 'password123', company: company) } let(:headers) { auth_headers(user) } let!(:service) do @@ -103,6 +105,27 @@ end end + # La QA de TESIS-82 encontró que el flag sólo lo aplicaba el front: una + # empresa sin la feature configuraba integraciones llamando al endpoint. + context 'when the company does not have the integrations feature' do + before { company.update!(features: { 'integrations' => false }) } + + it 'refuses to configure the integration', :aggregate_failures do + expect do + put "/api/v1/integrations/#{service.id}", params: payload, headers: headers, as: :json + end.not_to change(CompanyIntegration, :count) + expect(response).to have_http_status(:forbidden) + end + + # El widget de nodos del panel lo pide igual, y sólo lista las plantillas + # globales con el estado de la propia empresa. + it 'still lists the services' do + get '/api/v1/integrations', headers: headers + + expect(response).to have_http_status(:ok) + end + end + context 'when another company already configured the same service' do let!(:other_integration) do CompanyIntegration.create!( diff --git a/spec/requests/api/v1/warehouses_spec.rb b/spec/requests/api/v1/warehouses_spec.rb index 4944f967..6e9594a2 100644 --- a/spec/requests/api/v1/warehouses_spec.rb +++ b/spec/requests/api/v1/warehouses_spec.rb @@ -202,6 +202,18 @@ def stock_queries_while(&) post '/api/v1/warehouses', params: {}, headers: headers, as: :json expect(response).to have_http_status(:bad_request) end + + # Hallazgo de la QA de TESIS-82: `require` devolvía lo que llegara en la + # clave y `permit` reventaba sobre un String, con un 500. + it 'returns 400 when warehouse is not an object' do + post '/api/v1/warehouses', params: { warehouse: 'Central' }, headers: headers, as: :json + expect(response).to have_http_status(:bad_request) + end + + it 'returns 400 when warehouse is a list' do + post '/api/v1/warehouses', params: { warehouse: [warehouse_attrs] }, headers: headers, as: :json + expect(response).to have_http_status(:bad_request) + end end describe 'PUT /api/v1/warehouses/:id' do @@ -232,6 +244,14 @@ def stock_queries_while(&) expect(response).to have_http_status(:not_found) end + it 'returns 400 and changes nothing when warehouse is not an object', :aggregate_failures do + put "/api/v1/warehouses/#{warehouse.id}", params: { warehouse: 'Actualizado' }, + headers: headers, as: :json + + expect(response).to have_http_status(:bad_request) + expect(warehouse.reload.name).to eq('Central') + end + it 'updates via PATCH as well', :aggregate_failures do patch "/api/v1/warehouses/#{warehouse.id}", params: { warehouse: { name: 'Actualizado' } }, @@ -312,6 +332,38 @@ def stock_queries_while(&) .to eq('Cannot delete warehouse with order lines taken from it') end end + + # Hallazgo de la QA de TESIS-82: la FK de stock_transfers es restrict y el + # modelo no la adelantaba, así que este borrado respondía 500. El destino de + # una transferencia en tránsito no tiene stock propio todavía: las unidades + # se le suman recién al recibirla. + context 'when a stock transfer references it and it has no stock of its own' do + it 'returns 409 and keeps it as the destination of a transfer in transit', :aggregate_failures do + create_transfer_touching_warehouse(as: :destination) + + delete "/api/v1/warehouses/#{warehouse.id}", headers: headers + + expect(response).to have_http_status(:conflict) + expect(Warehouse.find_by(id: warehouse.id)).to be_present + end + + it 'returns 409 as the origin of a transfer too' do + create_transfer_touching_warehouse(as: :origin) + + delete "/api/v1/warehouses/#{warehouse.id}", headers: headers + + expect(response).to have_http_status(:conflict) + end + + it 'says the transfers are what block it' do + create_transfer_touching_warehouse(as: :destination) + + delete "/api/v1/warehouses/#{warehouse.id}", headers: headers + + expect(response.parsed_body['error']) + .to eq('Cannot delete warehouse with stock transfers from or to it') + end + end end def create_warehouse_with_stock @@ -325,4 +377,14 @@ def create_order_line_from_warehouse OrderItem.create!(order: order, product: product, warehouse: warehouse, quantity: 1, unit_price: 100) end + + # Una transferencia en tránsito entre `warehouse` y otro depósito, sin stock en + # `warehouse`. `as:` dice qué papel cumple en ella. + def create_transfer_touching_warehouse(as:) + other = Warehouse.create!(company: company, name: 'Sucursal', zip_code: '1900', address: 'Calle 2') + product = Product.create!(company: company, sku: 'SKU-3', name: 'En viaje') + origin, destination = as == :origin ? [warehouse, other] : [other, warehouse] + StockTransfer.create!(company: company, product: product, origin_warehouse: origin, + destination_warehouse: destination, quantity: 1, dispatched_at: Time.current) + end end