tabnews.com.br Reverter ganhos de TabCoins em conteúdos pegos pelo firewall interno e outros ajustes

Mudanças realizadas

Reversão de ganhos de TabCoins

A função creditOrDebitTabCoins foi chamada para reverter possíveis ganhos de TabCoins que o usuário teve com os conteúdos publicados que foram pegos pelo firewall interno.

Endpoints

GET /api/v1/events/firewall/[id]: obter dados sobre o evento do firewall e quais foram os conteúdos e usuários afetados.
POST /api/v1/moderations/review_firewall/[id]: confirmar ou desfazer um evento específico de firewall. A confirmação (confirm) adiciona a feature nuked ao usuário ou o status deleted aos conteúdos envolvidos. A reversão (undo) retorna os TabCoins removidos, restaura o status dos conteúdos e features dos usuários, a depender do tipo de firewall. Este endpoint retornará os dados atualizados contendo o evento original do firewall, o novo evento criado ao desfazer o firewall, os dados dos conteúdos e usuários afetados.

Resolve #1463

Tipo de mudança

[x] Melhorias no firewall interno
[x] Nova funcionalidade

Checklist:

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

Feb 26 '24 00:02 Rafatcb

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	Jul 30, 2024 0:39am

Feb 26 '24 00:02 vercel[bot]

Com a adição de undoContentsTabcoins, talvez seja melhor executar tudo dentro de uma transação, mas é preciso pensar melhor se isso faz sentido, pois, se der algum erro, não seria interessante reverter toda a transação.

Acho que criar uma transação para cada iteração do for pode fazer sentido, assim garantimos que toda a atualização do saldo referente ao conteúdo X foi feita corretamente, sem impactar a atualização do saldo dos outros conteúdos ou a atualização do status. O que acha?

Edit: Vendo melhor, a condição em que entrará na função creditOrDebitTabCoins só tem uma query que modifica o banco, então a transação não teria impacto, apenas um try / catch que poderia prevenir algo, mas não sei se tem necessidade ali. Vou subir as alterações sem try / catch e sem transaction.

Feb 26 '24 22:02 Rafatcb

@Rafatcb, aquele owner_id: context.user.id deixou visível que falta um teste que garanta que os conteúdos serão despublicados mesmo sendo de diferentes usuários. Já faltava um teste desse antes do PR.

E a questão de diferentes usuários me fez pensar na possível situação de usuários legítimos que publicarem algo pela mesma rede pública, ao mesmo tempo, e somente o terceiro saberá que alguma coisa errada aconteceu. Os dois primeiros não saberão o que aconteceu com suas publicações. Será que é interessante verificar se alguma das publicações é de um usuário diferente do terceiro e enviar uma notificação por email?

Um efeito colateral da implementação é que ficou mais complicado de reverter o efeito do firewall em casos de falso positivo. Antes, apesar de ter que fazer diretamente no banco, era só voltar as publicações de draft para published, mas agora seria necessário também desfazer o efeito de creditOrDebitTabCoins. Só que falta uma definição de como seria isso de forma a não causar inconsistências.

Feb 27 '24 22:02 aprendendofelipe

aquele owner_id: context.user.id deixou visível que falta um teste que garanta que os conteúdos serão despublicados mesmo sendo de diferentes usuários. Já faltava um teste desse antes do PR.

Criarei o teste :+1:

E a questão de diferentes usuários me fez pensar na possível situação de usuários legítimos que publicarem algo pela mesma rede pública, ao mesmo tempo, e somente o terceiro saberá que alguma coisa errada aconteceu. Os dois primeiros não saberão o que aconteceu com suas publicações. Será que é interessante verificar se alguma das publicações é de um usuário diferente do terceiro e enviar uma notificação por email?

Acho que isso pode ser interessante sim.

Um efeito colateral da implementação é que ficou mais complicado de reverter o efeito do firewall em casos de falso positivo. Antes, apesar de ter que fazer diretamente no banco, era só voltar as publicações de draft para published, mas agora seria necessário também desfazer o efeito de creditOrDebitTabCoins. Só que falta uma definição de como seria isso de forma a não causar inconsistências.

Ainda não pensei na melhor forma em fazer isso. Você já tem alguma ideia? Se seria criando uma função SQL ou uma feature e um endpoint para desfazer os efeitos de um evento do firewall específico?

Feb 27 '24 22:02 Rafatcb

Ainda não pensei na melhor forma em fazer isso. Você já tem alguma ideia? Se seria criando uma função SQL ou uma feature e um endpoint para desfazer os efeitos de um evento do firewall específico?

Também não pensei. Acho que o ponto mais importante é como ficaria no banco de dados, já que não podemos excluir os balanços criados pelo evento de firewall. Provavelmente teria que criar novos balanços revertendo os do evento de reversão 😅

Talvez uma undoAllFirewallSideEffect parecida com a undoAllRatingOperations.

Feb 27 '24 23:02 aprendendofelipe

Firewall

Percebi que a alteração que fiz removia o conteúdo, mas não atualizava o deleted_at, então aproveitei para corrigir isso. Também passei a atualizar o updated_at nas três procedures do firewall.

Email

A parte do e-mail acabou ficando bem "personalizada" de acordo com singular/plural e publicação/comentário. Para as publicações, consegui enviar o título, mas para os comentários não sei o que seria melhor para quem receber o e-mail ter uma noção, então passei apenas os IDs. Se você ter alguma sugestão de melhoria, pode falar.

Não sei se o texto "Identificamos que você está tentando..." é adequado, se deveria mencionar "Identificamos muitas novas publicações no seu IP" ou algo parecido.

Desfazer o efeito do firewall

Modifiquei o user.addFeatures e o balance.undo, e criei o content.undoDeleted para conseguir atualizar os dados de uma forma mais adequada para essa situação específica.

Algumas dúvidas:

Essa é uma ação sobre o "firewall", então criei o endpoint DELETE /api/v1/firewall/[event_id], mas na verdade a ação é orientada pela tabela events, tanto que recebe o event.id como parâmetro. O endpoint deveria ser DELETE /api/v1/events/[id]?
É mais adequado realizar o event.findOne e a validação no controller e passar o objeto foundEvent para o model (e nesse caso o model precisaria confiar que o objeto foundEvent está correto) ou deixar o findOne e as validações no model mesmo?
Acha que faz mais sentido o endpoint retornar algo específico, retornar {} ou null?

Mar 04 '24 22:03 Rafatcb

Sobre a refatoração em models/transacional, aprovo todas as mudanças. Até por isso acho que talvez compense separar em um PR prévio a esse e já matar essa etapa.

Ok, vou fazer isso. Eu fiz aqui porque estava precisando copiar os estilos e percebi a repetição.

Sobre a parte de email dentro de models/firewall, será que não são coisas que deveriam estar no models/notification?

Eu havia começado a fazer no models/notification, mas depois de inspecionar o código tinha ficado em dúvida se esse era o "padrão do projeto". Vou organizar e passar para o models/notification.

Mar 09 '24 20:03 Rafatcb

Agora está retornando os conteúdos e usuários afetados, com os valores atualizados.

Estou deixando o PR em draft porque ainda falta criar o endpoint GET, mas se quiser pode revisar o conteúdo atual porque fiz a ponto de "deixar pronto para o merge". O GET virá num commit separado.

Mar 12 '24 01:03 Rafatcb

Fiz o GET. Abaixo estão alguns pontos que refleti e fiquei indeciso sobre como proceder. O PR está funcional mas acredito que esses pontos (além dos já comentados) mereçam atenção antes de realizar o merge:

Não sei se a interface de retorno está ideal:
```
{
  affected: {
    contents?,
    users?
  },
  event,
  status: 'active' | 'reversed'
}
```
Eu escolhi adicionar um atributo affected (que pode ter outro nome) para não correr o risco de eventualmente retornarmos mais coisas e ficar incompatível com a interface atual (imagine que vamos retornar um event afetado, por exemplo). E a interface do affected está compatível com o retorno do endpoint do undo.
Não consegui entender como modificar o schemas no validator para conseguir usar no filteredOutputValues do GET. Comecei experimentando validar apenas o retorno de event, mas o schema está esperando que o objeto seja o event, e não que tenha uma tributo event, então já não soube como lidar com esses casos de "atributos com o mesmo nome". Outro exemplo é o status, que não deveria ter a mesma validação do status do content.

Além disso, precisei modificar o schemas.id para aceitar um Array para conseguir passar um array no where.id do content.findAll. Não me parece ser a solução ideal.

Mar 16 '24 01:03 Rafatcb

Não quis dizer que seria necessária uma ação para os conteúdos deixarem de ser exibidos. Com o status firewall já não devem ficar disponíveis publicamente. E é exatamente o que acontece na versão atual. 👍

A minha dúvida é sobre os casos que não vamos desfazer o firewall. Nesse casos, vamos deixar os conteúdos permanentemente com status firewall? Como um moderador vai saber que o caso já foi analisado por outro (ou por ele mesmo em outro momento), e que a conclusão foi de que o evento não deve ser revertido?

Ah, entendi. Posso adicionar essa funcionalidade. Crio um endpoint POST /moderations/confirm_firewall e crio uma funcionalidade confirm:firewall ou utilizo a undo:firewall e renomeio ela para review:firewall, por exemplo?

Outra opção é deixar apenas um endpoint POST /moderation/review_firewall que recebe no body um valor { action: 'confirm' | 'undo' }.

Edit: E o caso de contas de usuários, que não tem status, seria tratado como? Dá para saber por meio dos eventos, mas me parece bem custoso buscar todos os eventos firewall:block_users que não possuem um outro evento moderation:unblock_users ou moderation:block_users (ou algo assim, como "confirmação"), visto que um evento referencia o outro por meio do campo metadata.

Apr 20 '24 18:04 Rafatcb

No caso dos usuários, olhando pelos banidos, o status é definido pelas features. Então pode adicionar uma feature que confirme o banimento do usuário pego pelo firewall. Acho que pode ser até a mesma feature nuked.

O que acha?

Para mim, faz sentido adicionar nuked ao confirmar. Quando for realizar a alteração desse endpoint vejo se surgirá alguma dúvida.

Apr 22 '24 23:04 Rafatcb

Além de implementar o que foi discutido em um novo commit (https://github.com/filipedeschamps/tabnews.com.br/pull/1638/commits/bfdf5a2300bea9a8928974255951413fd8c310bc), mudei o nome de algumas funções que criei nos testes para ficar com o padrão ...ViaApi.

Apr 26 '24 10:04 Rafatcb

Realizei as alterações propostas. Separei o models/firewall em diferentes arquivos e acho que ficou bem organizado. Intencionalmente, não estou exportando o event-types pelo firewall/index.js pois não vi a necessidade de usá-lo fora dos modelos do firewall.

May 15 '24 01:05 Rafatcb

Podemos deixar os commits assim :+1:

Parece tudo certo, pode rodar as migrações.

May 15 '24 23:05 Rafatcb

Precisei alterar a migration para remover a versão antiga das funções antes de criar as novas, pois não foi possível atualizar as funções alterando o tipo de retorno delas.

Agora foi tudo normalmente e já podemos testar em homologação. Inclusive já adicionei as features para os moderadores 👍

May 15 '24 23:05 aprendendofelipe

Criei alguns comentários em homologação, caí no rate limit duas vezes, mas nenhuma no firewall.

Então, fui tentar criar contas, e consegui cair no firewall. Só não consegui continuar o teste porque não temos um GET /api/v1/events/firewall para listar os erros e pegar o id 😅

May 16 '24 00:05 Rafatcb

Vi aqui que se alguém é pego pelo firewall e continua realizando a ação, novos eventos do firewall são criados. Eventos com ids diferentes, mas tendo todas as outras informações iguais. Isso faz com que o teste abaixo passe:

test('With two "firewall:block_users" events from the same IP', async () => {
  const usersRequestBuilder = new RequestBuilder('/api/v1/users');
  const firewallRequestBuilder = new RequestBuilder(`/api/v1/events/firewall`);
  await firewallRequestBuilder.buildUser({ with: ['read:firewall'] });

  // Create users
  const { responseBody: user1 } = await usersRequestBuilder.post({
    username: 'firstUser',
    email: '[email protected]',
    password: 'password',
  });
  const { responseBody: user2 } = await usersRequestBuilder.post({
    username: 'secondUser',
    email: '[email protected]',
    password: 'password',
  });
  const { response: user3Response } = await usersRequestBuilder.post({
    username: 'thirdUser',
    email: '[email protected]',
    password: 'password',
  });

  expect(user3Response.status).toEqual(429);

  let allEvents = await event.findAll();
  const firstFirewallEvent = allEvents.at(-1);

  const { response: user4Response } = await usersRequestBuilder.post({
    username: 'fourthUser',
    email: '[email protected]',
    password: 'password',
  });

  expect(user4Response.status).toEqual(429);

  allEvents = await event.findAll();
  const secondFirewallEvent = allEvents.at(-1);

  expect(firstFirewallEvent.id).not.toEqual(secondFirewallEvent.id);

  // Get firewall side-effects
  const { response: firstFirewallResponse, responseBody: firstFirewallResponseBody } =
    await firewallRequestBuilder.get(`/${firstFirewallEvent.id}`);
  const { response: secondFirewallResponse, responseBody: secondFirewallResponseBody } =
    await firewallRequestBuilder.get(`/${secondFirewallEvent.id}`);

  expect(firstFirewallResponse.status).toEqual(200);
  expect(secondFirewallResponse.status).toEqual(200);

  await usersRequestBuilder.setUser(user1);
  const { responseBody: user1AfterFirewall } = await usersRequestBuilder.get(`/${user1.username}`);

  await usersRequestBuilder.setUser(user2);
  const { responseBody: user2AfterFirewall } = await usersRequestBuilder.get(`/${user2.username}`);

  expect(firstFirewallResponseBody).toEqual({
    affected: {
      users: [user1AfterFirewall, user2AfterFirewall],
    },
    events: [
      {
        created_at: firstFirewallResponseBody.events[0].created_at,
        id: firstFirewallEvent.id,
        metadata: {
          from_rule: 'create:user',
          users: [user1AfterFirewall.id, user2AfterFirewall.id],
        },
        originator_user_id: null,
        type: 'firewall:block_users',
      },
    ],
  });

  expect(Date.parse(firstFirewallResponseBody.events[0].created_at)).not.toEqual(NaN);

  expect(secondFirewallResponseBody).toEqual({
    affected: {
      users: [user1AfterFirewall, user2AfterFirewall],
    },
    events: [
      {
        created_at: secondFirewallResponseBody.events[0].created_at,
        id: secondFirewallEvent.id,
        metadata: {
          from_rule: 'create:user',
          users: [user1AfterFirewall.id, user2AfterFirewall.id],
        },
        originator_user_id: null,
        type: 'firewall:block_users',
      },
    ],
  });

  expect(Date.parse(secondFirewallResponseBody.events[0].created_at)).not.toEqual(NaN);
});

Esse não me parece o comportamento ideal, já que na hora de confirmar ou desfazer o evento, após um deles ser analisado, o outro não terá mais impacto.

Faz sentido mudar as funções create...RuleSideEffect para, antes de criar um novo evento, procurar se o evento já existe e enviá-lo pela notificação?

May 16 '24 11:05 Rafatcb

Faz sentido mudar as funções create...RuleSideEffect para, antes de criar um novo evento, procurar se o evento já existe e enviá-lo pela notificação?

Pode ser que eu não tenha entendido sua dúvida, mas acredito que não faça sentido procurar outros eventos no momento que alguém cair no firewall. Mas na ação de desfazer ou confirmar o evento de firewall, aí sim, acho que é necessário buscar todos. Quando eu sugeri de não colocar o limite de 2 eventos no validador, foi justamente pensando nisso. 👍

May 16 '24 11:05 aprendendofelipe

Criei a função findAllRelatedEvents para conseguir obter todos os eventos relacionados numa única ida ao banco. Não sei se dá para deixar a query melhor, imagino que ela ficará lenta num banco com muitos eventos. O lado positivo é que esses endpoints serão pouco utilizados, já que é exclusivo para os moderadores.

May 18 '24 17:05 Rafatcb

O comportamento ao desfazer os TabCoins na função creditOrDebitTabCoins ignora a operação caso o conteúdo esteja com o total de TabCoins negativo:

Se a quantidade de TabCoins do conteúdo for positiva, o usuário perde-as. Por exemplo: +10 -4, o usuário perderá 6 TabCoins (ficando com 0). Teste Spamming valid "child" contents with TabCoins earnings em tests/integration/api/v1/contents/firewall.post.test.js.
Se a quantidade de TabCoins do conteúdo for negativa, a quantidade de TabCoins do usuário não muda. Por exemplo: +1 -5, o usuário continuará com o saldo de -4 TabCoins. Teste Spamming valid "child" contents with one with negative TabCoins em tests/integration/api/v1/contents/firewall.post.test.js.

Para os conteúdos pegos pelo firewall, manteremos esse comportamento, certo?

Os testes para esse cenário em tests/integration/api/v1/moderations/review_firewall/[id]/post.test.js são os testes de nome With a "firewall:block_contents:text_root" event with a content with negative TabCoins (um para undo e outro para confirm).

Jul 28 '24 20:07 Rafatcb

Gostaria de ouvir opiniões sobre como podemos deixar mais claros quais são os requisitos que cada teste está garantindo.

Acredita que o problema seja o nome das funções? Eu acho que o comportamento esperado fica mais claro quando é usado it('should ....

Jul 30 '24 00:07 Rafatcb

Gostaria de ouvir opiniões sobre como podemos deixar mais claros quais são os requisitos que cada teste está garantindo.

Acredita que o problema seja o nome das funções? Eu acho que o comportamento esperado fica mais claro quando é usado it('should ....

Em alguns casos pode ser melhor adequar o nome, mas em outros é preciso ser mais objetivo nas asserções. Em muitos casos, diminuir a quantidade de expect já deixaria mais explícita a razão de existir de cada teste.

Algumas asserções extras ajudam no momento da criação do teste, pois são verificações se a etapa de preparação está correta antes de ir para o que realmente queremos testar. Essas asserções que ajudam apenas durante a criação dos testes podem ser removidas assim que tiver certeza de que o teste realmente testa o que se espera. Algumas faz sentido manter apenas por documentação, por facilitarem a leitura, por exemplo uma verificação do status (ou o valor de algum campo específico) retornado em uma requisição durante a etapa de preparação, mas isso serve apenas como uma alternativa aos comentários, então não é interessante afastar essas asserções do trecho que definiu cada resultado.

Casos onde seja obrigatório manter as asserções na etapa de preparação do teste para garantir que ele esteja funcionando podem indicar a falta de outros testes para essas etapas específicas.

Mas esses problemas estão presentes em muitos dos nossos testes, não é algo exclusivo do PR atual, então não vai impedir a mesclagem. Minha preocupação é por ser uma dívida técnica que está crescendo muito.

Bora pro merge, pois é muito importante reverter esses os ganhos 🚀

Jul 30 '24 12:07 aprendendofelipe

tabnews.com.br tabnews.com.br copied to clipboard

Reverter ganhos de TabCoins em conteúdos pegos pelo firewall interno e outros ajustes

Mudanças realizadas

Reversão de ganhos de TabCoins

Endpoints

Tipo de mudança

Checklist:

Firewall

Email

Desfazer o efeito do firewall

tabnews.com.br
tabnews.com.br copied to clipboard