Libera o nome de usuário e email caso o token de ativação da conta expire

[Rafatcb]

Mudanças realizadas

Conforme mencionado em #957 (comment), agora ao realizar um cadastro, caso o username ou email do usuário já esteja cadastrado, mas o token de ativação não tenha sido utilizado e já expirou, então é possível realizar um novo cadastro com esse username ou email.

Resolve #957

Tipo de mudança

• Correção de bug

Checklist:

• As modificações não geram novos logs de erro ou aviso (warning).
• Eu adicionei testes que provam que a correção ou novo recurso funciona conforme esperado.
• Tanto os novos testes quanto os antigos estão passando localmente.

[vercel]

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name	Status	Preview	Comments	Updated (UTC)
tabnews	✅ Ready (Inspect)	Visit Preview	💬 Add feedback	Sep 4, 2024 0:56am

[aprendendofelipe]

Massa @Rafatcb! 💪

Mas tenho algumas de preocupações...

Capitalização de username

A criação de um novo usuário com diferente capitalização (mudando entre maiúsculas e minúsculas) com relação a outro usuário não ativo vai acabar inserindo uma nova linha ao invés de atualizar a existente, duplicando o username na tabela users, com variação apenas na capitalização.

Isso porque a diferença de capitalização não vai fazer entrar na cláusula ON CONFLICT adicionada nesse PR, já que ainda não temos uma restrição do tipo:

CREATE UNIQUE INDEX unique_username_ci ON users (LOWER(username));

Independentemente da funcionalidade desse PR, acho importante adicionar no banco de dados uma restrição alinhada como essa acima. Isso porque hoje a restrição só existe na função validateUniqueUser, mas esse PR acaba removendo parcialmente a restrição em casos bem específicos (token expirado ou usuário banido), o que não permitiu pegar a regressão com os testes atuais.

Risco de sobrescrever usuário existente

Na nova inserção estamos sobrescrevendo os dados de usuários existentes quando há conflito de username ou email, sem nenhuma outra verificação, apenas confiando que essa verificação ocorreu anteriormente em validateUniqueUser.

Como visto no tópico anterior, isso deixa margem para a criação de problemas graves que podem passar despercebidos pelos testes. Por isso, acredito que seja preciso adicionar na cláusula ON CONFLICT as mesmas restrições presentes em validateUniqueUser. Algo assim:

      INSERT INTO
        users (username, email, password, features)
      VALUES
        ($1, $2, $3, $4)
      ON CONFLICT (${conflictField}) DO UPDATE SET
        username = $1,
        email = $2,
        password = $3,
        features = $4,
        created_at = NOW(),
        updated_at = NOW(),
        rewarded_at = NOW()
+      WHERE
+      (
+        SELECT
+          COUNT(*)
+        FROM
+          users u
+        LEFT JOIN
+          activate_account_tokens aat ON u.id = aat.user_id
+        WHERE
+          (
+            LOWER(u.username) = LOWER($1) OR
+            LOWER(u.email) = LOWER($2)
+          )
+            AND
+          (
+            COALESCE(aat.used, true) OR
+            aat.expires_at > NOW() OR
+            'nuked' = ANY(u.features)
+          )
+      ) = 0
      RETURNING
        *
      ;

Caso de username e email já existentes

Esse caso em que os dois campos já estão presentes em usuários não ativos não está sendo tratado pelos testes. Parece que o comportamento atual está adequado, mas, sem testes, não dá para garantir. Acho que precisamos testar tanto para username e email repetidos em um único usuário, assim como se for o username de um e o email de outro usuário, os dois já existentes e ainda não ativados (ou banidos).

Concluindo

Acho que é uma ótima melhoria. Só que é preciso resolver os pontos acima. 👍

[Rafatcb]

Excelente comentário, @aprendendofelipe . Acredito ter resolvido os pontos levantados, seguindo suas sugestões.

Se preferir o primeiro commit num PR separado para realizar as migrações antes de mesclar a verificação, pode avisar que eu passo o commit para outro PR 👍

[aprendendofelipe]

Acredito ter resolvido os pontos levantados, seguindo suas sugestões.

O teste de email e username coincidindo com dois usuários diferentes precisa ser do caso em que nenhum dos usuários já existentes será retornado na query de validateUniqueUser, ou seja, os dois precisam estar com tokens expirados, sem nenhum estar banido ou com token utilizado.

Num primeiro momento, eu achei que só faltava o teste, mas, olhando melhor, esse caso não está sendo tratado, e vai retornar um erro do database, já que as duas variações da inserção (variando a constraint) não serão possíveis.

Como o objetivo é lidar com os expirados da mesma maneira que um username ou email nunca utilizados, acho que não vai ter como fugir de excluir entradas do banco de dados (ou editar para algum dado aleatório, o que não vejo vantagem). Então uma opção seria excluir uma das entradas duplicadas e atualizar a outra.

Mas acredito que seja melhor adicionar uma etapa após a validateUniqueUser, que pode ser algo como deleteExpiredUsers. Se validateUniqueUser não retornar erro, então chamamos deleteExpiredUsers que pode excluir quaisquer entradas que coincidirem com os novos dados que estão sendo inseridos. O bom é que desvincula totalmente do usuário anterior, pois será criado um novo ID. Isso de chamar a deleteExpiredUsers pode valer também no patch dos usuários, apenas tomando o cuidado de excluir apenas os novos dados, e não excluir o próprio usuário que está sendo editado. Com isso, nem precisaria alterar query de inserção, podendo permanecer como já é antes desse PR.

A deleteExpiredUsers até pode ser melhorada futuramente, pegando outros casos que possam ser excluídos, como usuários criados e ativados, mas nunca utilizados de fato, por exemplo.

Não sei se consegui explicar direito o que pensei como solução, mas o mais importante é ficar claro qual é o problema a ser resolvido, que é permitir a criação (ou edição) de um usuário utilizando um username e um email que ambos foram utilizados em dois outros usuários, mas que nunca chegaram a ser ativados.

Tirando esse teste específico, os demais do método POST permaneceriam do mesmo jeito. Bom, na verdade, tenho uma dúvida sobre os testes. Foi de propósito colocar eles fora do describe Anonymous user, diretamente em POST /api/v1/users? Talvez compense colocar em um describe específico dos duplicados, ou mover para dentro de Anonymous user.

Se preferir o primeiro commit num PR separado para realizar as migrações antes de mesclar a verificação, pode avisar que eu passo o commit para outro PR 👍

Acho que separar a migration é uma boa ideia, até porque é uma melhoria por si só, e evita preocupações como em mesclar esse PR em horário tranquilo e/ou rodar a migration na correria assim que concluir o deploy.

[Rafatcb]

Mas acredito que seja melhor adicionar uma etapa após a validateUniqueUser, que pode ser algo como deleteExpiredUsers. Se validateUniqueUser não retornar erro, então chamamos deleteExpiredUsers que pode excluir quaisquer entradas que coincidirem com os novos dados que estão sendo inseridos. (...) Com isso, nem precisaria alterar query de inserção, podendo permanecer como já é antes desse PR.

Se entendi direito, não funcionaria neste cenário:

1. Tentativa de cadastro: user, [email protected]
2. Token expira
3. Tentativa de cadastro: user [email protected]

Como não teve nenhuma requisição que passou com sucesso por validateUniqueUser entre 1 e 3, então 3 daria erro porque o user+[email protected] ainda existiria no banco. O deleteExpiredUsers não deveria ser antes dovalidateUniqueUser, então?

Outra dúvida, o deleteExpiredUsers apagaria também os tokens desses usuários em activate_account_tokens para não ficarem referenciando um user_id que não existe, certo?

Foi de propósito colocar eles fora do describe Anonymous user, diretamente em POST /api/v1/users?

Não foi de propósito 😅

Talvez compense colocar em um describe específico dos duplicados, ou mover para dentro de Anonymous user.

Posso criar um describe específico, dentro do Anonymous user, e mover os outros testes que já existem sobre isso, como With "username" duplicated exactly (same uppercase letters). É isso que você pensou?