fix: [TESIS-82] close the auth and multi-tenancy gaps found by the module QA - #89
Conversation
The JWT of a company that was deactivated stayed valid until it expired, so its users kept operating for up to 24 hours after the company was turned off. is_active was only checked on login, registration and tenant-config. authenticate_user! now refuses a session whose company is not active. The check sits there and not in Devise's active_for_authentication? on purpose: that hook answers 401 to any request carrying the token, including the ones that do not require a session (login, registration, tenant-config). Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
POST /auth/register created an account that could log in right away, inside
whatever company the X-Tenant-Slug header named. The slug is public (it is the
subdomain), so anyone could join any company and read all of its data.
Registering is now asking for access, which is what the S02 screen says
("Solicitá acceso al espacio de operación de tu organización"). The account is
created with approved = false and cannot log in until someone approves it from
the backoffice, where User gains the approved field. The column defaults to
true, so existing accounts and the ones created by the backoffice, the seeds
or the console keep working.
The endpoint also stops telling which emails exist. The email is unique across
the whole database, so "Email has already been taken" revealed accounts of any
company. It now answers 202 with the same body whether the email was new or
not, and it reports invalid input before looking at the email, so a short
password cannot be used to tell the two cases apart either.
A session whose account loses its approval ends on the next request, like the
one of a deactivated company.
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…email Devise stores the email downcased and stripped (case_insensitive_keys and strip_whitespace_keys), but it only normalizes on save and inside its own finders. AuthenticateUser looks the account up with its own find_by and passed the email as typed, so someone who registered as Ana@Norte.com got a 401 with the right password when typing it the same way. The frontend does not normalize it either. The email is now normalized the same way before the lookup. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Passwords could be guessed without limit: after 30 wrong ones in a row, the right one still logged in. Login and registration now allow 10 attempts every 3 minutes per IP and then answer 429 with a Retry-After header. The limit is Rails' rate_limit and not Devise's :lockable on purpose: locking the account would let anyone lock someone else out by mistyping their password. The counter lives in the app cache, Solid Cache in production. The test cache is :null_store, so the rest of the suite never reaches the limit; the new spec gives the counter a real store for each example. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
The integrations flag of a company was only enforced by the frontend, which hides the section. A company without the feature still configured integrations by calling PUT /integrations directly. The update now answers 403 when the flag is off. The listing stays open: the dashboard's integration nodes widget requests it for every company, and it only shows the global service templates with the company's own status. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…erence Deleting the destination of a transfer in transit answered 500. That warehouse has no stock of its own yet (the units are added when the transfer is received) and no order lines, so neither restrict_with_error let it through, and the delete reached the foreign keys of stock_transfers, which are restrict too: ActiveRecord::InvalidForeignKey. Warehouse now declares its outgoing and incoming transfers with restrict_with_error, so the delete answers 409 and the message says the transfers are what block it. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
POST and PUT /warehouses with {"warehouse": "x"} answered 500. require
returns whatever the key holds, and permit on a String raises NoMethodError.
warehouse_params now uses params.expect, which answers 400 for anything that
is not an object. The comment it replaces said expect would also answer 400 to
a company_id in the body; in Rails 8.1 it drops unpermitted keys silently, and
the company_id specs of TESIS-97 still pass.
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Registration as an access request, the per-request check of the company and the approval, the attempt limit per IP and the email normalization of the login, with the alternatives that were discarded and what the deployed environment needs for the attempt limit to count. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
LauAubert
left a comment
LauAubert
left a comment
There was a problem hiding this comment.
Buen PR: cada hallazgo en su commit, con specs, y las decisiones en el ADR. Dejo comentarios en línea. Los dos primeros (el logout que no revoca con la empresa inactiva y el límite de intentos que cuenta los logins exitosos) me parece que conviene resolverlos antes de mergear. El resto son menores o pueden ir en una card aparte.
| # 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!(*) |
There was a problem hiding this comment.
Este chequeo también corre en sessions#destroy, así que con la empresa inactiva (o la cuenta desaprobada) el logout responde 401 antes de llegar a RevokeToken, y el token nunca entra en la denylist.
El flujo: dan de baja a Norte → el próximo request da 401 → el front hace clearSession() → revokeSession() manda DELETE /auth/logout → 401 acá (el front ignora ese 401). Si Norte se reactiva dentro de las 24 h, cualquier copia de ese token (otro dispositivo, uno filtrado) vuelve a andar. Lo mismo si se quita la aprobación y después se vuelve a dar.
Propuesta: saltear el rechazo en destroy, o revocar antes de rechazar.
| WINDOW = 3.minutes | ||
|
|
||
| included do | ||
| rate_limit to: MAX_ATTEMPTS, within: WINDOW, only: :create, |
There was a problem hiding this comment.
rate_limit cuenta todos los POST /auth/login, también los exitosos, y la clave es solo la IP. Un depósito con 12 operarios detrás del mismo NAT que entran al empezar el turno: el 11.º recibe 429 aunque ponga la contraseña correcta. Y hasta que se resuelva TESIS-130 (remote_ip detrás del proxy), toda la app comparte un único contador: 10 logins cada 3 minutos para todos.
¿Qué tal contar solo los intentos fallidos, o usar IP+email como clave?
|
|
||
| user = find_user | ||
| return nil unless user&.valid_password?(@password) | ||
| return nil unless user&.valid_password?(@password) && user.approved? |
There was a problem hiding this comment.
Esto ya pasaba antes del PR, pero como se toca esta línea y el comentario repite la garantía: si el email no existe en el tenant, user&.valid_password? corta antes y no se corre bcrypt. El 401 vuelve mucho más rápido que para un email existente, así que el tiempo de respuesta permite enumerar usuarios aunque el cuerpo sea idéntico. En RegisterUser pasa lo mismo, más chico: el camino del email tomado se saltea el INSERT.
La mitigación habitual es comparar contra un hash dummy cuando no se encuentra el usuario. Puede ir en otra card, pero entonces ajustaría el comentario para no prometer algo que no se cumple.
| # endpoint directo. Se corta el alta y la modificación. El listado queda | ||
| # abierto porque lo usa el widget de nodos del panel, y sólo muestra las | ||
| # plantillas globales con el estado de la propia empresa. | ||
| before_action :require_integrations_feature, only: :update |
There was a problem hiding this comment.
Menor: es una regla de autorización y el CLAUDE.md pide «autorizar con Pundit, delegar a PORO». Con una IntegrationPolicy#update? + authorize, verify_authorized obligaría al próximo endpoint de integraciones a pasar por la misma regla; con el before_action hay que acordarse de repetirlo.
| # 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]) |
There was a problem hiding this comment.
El mismo require + permit que daba 500 sigue en products_controller.rb:123, stock_transfers_controller.rb:69, product_mappings_controller.rb:74 y orders_controller.rb:172 (por ejemplo, POST /products con {"product": "x"} → 500). Ya está en las limitaciones conocidas; propongo abrir una card para migrarlos todos a params.expect, así el hallazgo 7 no queda cerrado a medias.
| end | ||
|
|
||
| def session_refusal | ||
| return 'The company of this account is not active' unless current_company.is_active? |
There was a problem hiding this comment.
Nit: ahora cada request autenticado hace un SELECT a companies para leer is_active, también en los endpoints que no usan current_company. Es barato, pero está en el camino de toda la API. No bloquea.
TomasMartin2004
left a comment
TomasMartin2004
left a comment
There was a problem hiding this comment.
Revisión — TESIS-82 (PR #89) · Cierre de los hallazgos de Auth y Multi-tenancy
Revisado sobre TESIS-82-auth-multitenancy-qa-fixes (87ea8f9) contra origin/master, con el ADR-002 actualizado y el lado del front (web#51) al lado.
| Check | Resultado |
|---|---|
| Suite completa (local) | 1285 ejemplos, 0 fallas |
| RuboCop · Brakeman (local) | 258 archivos sin ofensas · 0 warnings |
db/schema.rb |
Sólo la columna nueva. Sin ruido de la versión local de PostgreSQL, que es la trampa habitual |
✅ El hallazgo grave está cerrado, y lo verifiqué por HTTP
Sondas propias sobre la rama, sin mirar tus specs:
registro con el email de OTRA empresa -> 202 {"status":"pending_approval"}, usuarios creados: 0
registro de un email nuevo -> 202, la cuenta nace con approved=false
login de esa cuenta pendiente -> 401 {"error":"Invalid email or password"} (idéntico al de password incorrecta)
El registro dejó de ser una puerta de entrada a cualquier empresa y no delata qué emails existen, ni en esta empresa ni en otra. Que los errores de formato se levanten antes de mirar el email es el detalle que cierra la enumeración de verdad: si no, una password corta respondería distinto según el email estuviera tomado.
El corte de las sesiones abiertas también funciona, que es la mitad que un cambio en el login no puede resolver:
empresa dada de baja, token vivo -> 401 "The company of this account is not active"
cuenta que pierde la aprobación -> 401 "This account is pending approval"
tenant-config (público) -> 200, no lo toca
Y la decisión de ponerlo en authenticate_user! y no en active_for_authentication? es la correcta: ahí el hook de Devise cortaría también el login y el registro, que son justo los que no tienen que exigir sesión.
✅ El resto de los hallazgos, verificados uno por uno
login con "ANA@NORTE.TEST" y con " Ana@Norte.Test " -> 200 con token
PUT /integrations sin la feature -> 403
GET /integrations sin la feature -> 200 (el widget del panel sigue andando)
PUT /integrations con features {"integrations": false} -> 403
DELETE de un depósito con una transferencia -> 409 "Cannot delete warehouse with stock transfers from or to it"
POST /warehouses con un body que no es objeto -> 400
Dos cosas que me gustaron:
feature_enabled?compara con== true, igual queisFeatureEnableddel front (features?.[feature] === true). Lo verifiqué enshared/api/tenant.ts: un"true"string da apagado en los dos lados. Que el back y el front decidan igual es lo que evita una UI que ofrece lo que la API rechaza.- El límite es por IP y no por cuenta. Coincido con el motivo del ADR, y es el tipo de decisión que se toma mal por default.
El spec del límite tiene dientes: saqué el rate_limit del concern y fallan 3 ejemplos, incluido el del registro. Darle una cache de verdad sólo a ese spec, dejando :null_store para el resto, está bien resuelto — el resto de la suite loguea decenas de veces y no choca.
🟡 Dar de baja una empresa suspende las sesiones, pero no las cierra
El corte es reversible, y el usuario no puede cerrar la suya mientras dura. Secuencia, verificada:
1. login, GET /warehouses -> 200
2. la empresa se da de baja, mismo token -> 401
3. el usuario intenta DELETE /auth/logout -> 401 ← el token NO entra a la denylist
4. la empresa se reactiva, MISMO token de nuevo -> 200
El paso 3 es el que me hace ruido: el logout es la única acción que un usuario bloqueado igual querría poder hacer, y hoy le responde 401. Como nunca se revoca, un token emitido antes de la baja vuelve a servir si la empresa se reactiva dentro de sus 24 h — incluido el de una sesión que la persona creyó cerrar.
Con la cuenta pendiente de aprobación pasa lo mismo (approved: false → true y el token viejo vuelve).
No lo bloqueo: el impacto es bajo y en la práctica el front borra la sesión al primer 401 (tu web#51 hace justo eso). Pero son dos salidas baratas, cualquiera sirve:
- Dejar pasar
sessions#destroypor el chequeo (revocar y devolver 204 igual), o - Revocar los tokens de la empresa al desactivarla.
Si te parece que el corte debe ser reversible —una suspensión no es una baja—, alcanza con que el ADR lo diga, y el punto queda cerrado.
🟡 Choca con TESIS-107 en el cuerpo de error del registro
registrations_controller.rb sigue respondiendo { "errors": [...] }, plural y array. api#86 (TESIS-107) lo cambia a { "error": "..." }, que es la forma única que fija el ADR-015, y lo agrega a api_contract_spec.rb.
No es un problema de este PR —tu rama salió de master, donde todavía está así—, pero los dos tocan el mismo archivo y el que entre segundo tiene conflicto. Aviso para coordinarlo: si entra este primero, en #86 lo reescribo yo sobre tu versión nueva.
Las limitaciones que declarás
Las dos están bien elegidas y bien separadas: no poder crear usuarios desde el backoffice (queda en TESIS-129) y el require + permit que sigue en los otros módulos. La segunda es exactamente el tipo de cosa que conviene que quede escrita en vez de arreglarse a medias acá.
Veredicto
APPROVE.
Cierra el agujero más serio que teníamos —registro abierto a cualquier empresa por un slug público— y lo hace sin dejar que la respuesta delate nada. El resto de los hallazgos están cubiertos, con specs que fallan si se deshacen. Lo del logout lo dejo a tu criterio: si preferís el corte reversible, que lo diga el ADR; si no, es una línea.
🤖 Generated with Claude Code
|
Confirmo los dos que Lautaro pide resolver antes de mergear: los reproduje sobre la rama. 1. El límite cuenta también los logins exitosos. Importa más de lo que parece en esta etapa: en la demo los dos portales se prueban desde la misma IP, y entre 2. El logout con la empresa inactiva es el 🟡 de mi review: el token nunca llega a la denylist y vuelve a servir si la empresa se reactiva dentro de sus 24 h. Mi aprobación queda en pie —el fondo del PR está bien y cierra el agujero grave—, pero no lo mergeo con estos dos abiertos: el primero se nota en la demo y el segundo deja un token vivo que alguien creyó cerrar. Avisá cuando estén y lo mergeo. Mergeé 🤖 Generated with Claude Code |
|
Aviso aparte, porque afecta a este PR aunque no sea suyo: Con los dos mergeados, Rails no arranca: Ninguna de las dos se aplicó en ningún lado todavía, así que renumerar una es gratis. En la review de #90 propuse que renumere esa —es la que abrió después— a 🤖 Generated with Claude Code |
The session check answered 401 to DELETE /auth/logout when the company was inactive or the account unapproved, so the token never reached the denylist and worked again if the company was reactivated within its 24 h. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
rate_limit counted every POST /auth/login, so the eleventh correct login from the same IP (operators behind one NAT, the demo) got 429. The login now counts only the 401s; the registration keeps counting every request. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
An unknown email or tenant skipped bcrypt and answered the 401 much faster than a wrong password, so the response time enumerated users. Compare against a throwaway digest with the same cost instead. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Move the integrations feature check from a before_action to CompanyIntegrationPolicy#update?, so verify_authorized enforces it. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
|
Verifiqué los dos que quedaban pendientes, sobre Logout de una sesión suspendida — resuelto, y revoca de verdad: Que el paso 3 dé 401 es lo que importa: el token no revive con la empresa. El límite cuenta sólo los fallidos — resuelto: Me gustó que el rechazo no evalúe la password: si la evaluara, el que prueba contraseñas seguiría probando y la correcta le daría el token igual. Y que el registro siga contando todos los pedidos, porque ahí no hay un «pedido exitoso» que se repita en el uso normal, está bien distinguido. Los otros dos commits que sumaste —bcrypt en los logins sin cuenta, y la autorización de integraciones por Pundit— van en la misma dirección y no cambian nada de lo que ya había revisado. Verificación completa sobre la rama: Mergeado. Queda pendiente lo de la migración duplicada, pero ahora es de 🤖 Generated with Claude Code |
3 participants
Ticket de Jira
https://proyectofinalfrlp.atlassian.net/browse/TESIS-82
Descripción
Corrige los hallazgos de backend de la validación del módulo de Auth y Multi-tenancy (TESIS-82). El más grave:
POST /auth/registercreaba una cuenta con acceso inmediato en la empresa que nombraba el headerX-Tenant-Slug. Como el slug es el subdominio, cualquiera podía darse de alta en cualquier empresa y leer todos sus datos. Además, una empresa dada de baja seguía operando con sus tokens hasta 24 h, el login no tenía límite de intentos ni encontraba la cuenta si el email venía con mayúsculas, el flagintegrationssolo lo aplicaba el front, y dos casos del CRUD de depósitos respondían 500.Cada hallazgo va en su propio commit, con sus specs. Las decisiones quedaron en la actualización de
docs/adr/ADR-002-authentication.md:rate_limitde Rails y no con:lockablede Devise: bloquear la cuenta permitiría dejar afuera a otro usuario tipeando mal su contraseña a propósito.authenticate_user!y no enactive_for_authentication?: el hook de Devise corta con 401 cualquier request que traiga el token, incluidos el login, el registro y tenant-config.Limitaciones conocidas:
require+permitque causaba el 500 en depósitos sigue en productos, transferencias, órdenes y mappings, que son de otros módulos.Cambios:
POST /api/v1/auth/registeren una solicitud de acceso: la cuenta nace conapproved: false, no obtiene token hasta que se aprueba, y el endpoint responde202 {"status":"pending_approval"}exista o no el email. Los errores de formato se informan antes de mirar el email, para que no lo delaten (hallazgos 1 y 3)users.approved(boolean, defaulttrue) y el campoapproveden el recurso User del backoffice para aprobar cuentasApplicationController#authenticate_user!para responder 401 cuando la empresa está inactiva o la cuenta perdió la aprobación; así se cortan también las sesiones abiertas (hallazgo 2)Api::V1::Auth::AttemptLimit: login y registro aceptan 10 intentos cada 3 minutos por IP y después responden 429 conRetry-After(hallazgo 5)Auth::AuthenticateUserpara normalizar el email (minúsculas, sin espacios) antes de buscar la cuenta (hallazgo 10)Company#feature_enabled?y hace quePUT /api/v1/integrations/:service_idresponda 403 si la empresa no tiene la featureintegrations. El GET sigue abierto porque lo usa el widget de nodos del panel, y solo lista las plantillas globales (hallazgo 4)Warehouselas transferencias de origen y de destino conrestrict_with_error: borrar un depósito al que referencia una transferencia responde 409 con el motivo, en vez de 500 (hallazgo 6)require+permitporparams.expectenWarehousesController: siwarehouseno es un objeto responde 400 en vez de 500. También corrige el comentario que afirmaba queexpectrechazaba elcompany_iddel body (hallazgo 7)Evidencia visual
N/A. Es un cambio de API; en el backoffice solo aparece el campo
approveden el recurso User.Cómo probar
Precondición: API local con los seeds (
nortetiene integraciones ysurno; contraseñapassword123). Todos los requests llevan el headerX-Tenant-Slugcon el slug de la empresa.Caso 1: el registro es una solicitud
POST /api/v1/auth/registerconX-Tenant-Slug: nortey{ "email": "nuevo@test.com", "password": "password123" }→ 202
{"status":"pending_approval"}POST /api/v1/auth/logincon esas credenciales→ 401, con la misma respuesta que una contraseña incorrecta
/admin→ Users, tildarApproveden la cuenta nueva y repetir el login→ 200 con token
Caso 2: el registro no delata emails de otras empresas
POST /api/v1/auth/registerconX-Tenant-Slug: nortey el emailadmin@sur.com→ 202 con el mismo cuerpo del caso 1, y no se crea ninguna cuenta
123→ 422 con el error de la contraseña, igual que con un email nuevo
Caso 3: una empresa dada de baja pierde las sesiones abiertas
admin@norte.comy hacerGET /api/v1/warehousescon el token→ 200
/admin→ Companies, destildarIs activeen Distribuidora Norte y repetir el GET→ 401
{"error":"The company of this account is not active"}Caso 4: límite de intentos
POST /api/v1/auth/login10 veces seguidas con una contraseña incorrecta→ 429
{"error":"Too many attempts, try again later"}con el headerRetry-After: 180→ 200
Caso 5: email con mayúsculas
POST /api/v1/auth/loginconX-Tenant-Slug: nortey el emailAdmin@Norte.com→ 200
Caso 6: el flag de integraciones se aplica en la API
Precondición: token de
admin@sur.com.PUT /api/v1/integrations/1con{ "credentials": { "access_token": "x" } }→ 403
{"error":"The integrations feature is not enabled for this company"}GET /api/v1/integrations→ 200, porque el panel lo sigue usando
Caso 7: bordes del CRUD de depósitos
POST /api/v1/warehousesy despacharle una transferencia conPOST /api/v1/stock-transfers({ "stock_transfer": { "product_id", "origin_warehouse_id", "destination_warehouse_id", "quantity" } })DELETE /api/v1/warehouses/:iddel depósito nuevo→ 409
{"error":"Cannot delete warehouse with stock transfers from or to it"}POST /api/v1/warehousescon{ "warehouse": "x" }→ 400
bundle exec rspecpasa los 1285 ejemplos ybundle exec rubocopno marca nada.Impacto y consideraciones
¿Introduce breaking changes?
Sí, uno acotado:
POST /api/v1/auth/registerresponde202 {"status":"pending_approval"}en lugar de 201 con el usuario, y la cuenta no entra hasta que la aprueben. El front actual no usa el registro (S02 no está implementada), así que no rompe nada en uso. El login ahora puede responder 429; el PR de TESIS-82 en proyecto-web le agrega su mensaje.¿Requiere nuevas variables de entorno?
No. Pero hay que correr la migración
AddApprovedToUsers(defaulttrue, sin backfill: las cuentas existentes siguen entrando). En el entorno desplegado, el límite de intentos necesita la base de Solid Cache (proyecto_api_production_cache), y querequest.remote_ipsea la IP del cliente y no la del proxy; si no, todos comparten un solo contador. Queda para TESIS-130.¿Afecta la arquitectura o genera un nuevo patrón?
Sí. Se actualizó
docs/adr/ADR-002-authentication.mdcon el registro como solicitud, el chequeo de la sesión en cada request, el límite de intentos (nuevo concernApi::V1::Auth::AttemptLimit) y la normalización del email en el login.🤖 Generated with Claude Code