Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Expirar todas as sessões válidas ao resetar a senha do usuário #615

Merged
merged 3 commits into from
Aug 3, 2022

Conversation

filipedeschamps
Copy link
Owner

@filipedeschamps filipedeschamps commented Aug 3, 2022

Continuação do PR #603 feito por @eletroswing

Antes de tudo, fiz o commit b388a20 com o seguinte:

  • Aumentei um pouco a cobertura do teste quando é invalidada a sessão com o DELETE. Isto não está relacionado ao PR original mas aproveitei o embalo no assunto sessions.
  • Então agora é testado que antes a sessão funciona ao usar ela para buscar por /user, e depois de invalidar a sessão, garante que a mesma busca por /user com a mesma sessão retorna um 401.
  • Esse tipo de estratégia vai ser aproveitada no próximo commit.

Então depois disso, no commit cfdde6d trabalhei de fato no que o PR #603 propõe, que é garantir que ao resetar a senha de um usuário, todas as sessões são invalidadas.

  • Então criei um teste que cria duas sessões ativas, e ao resetar a senha, me certifico que as duas sessões são expiradas.
  • E na query do método que expira todas as sessões, adicionei uma condicional AND expires_at >= now() para só mexer nas sessões que estão para vencer, e não em todas as sessões históricas do usuário.
  • Fora isso removi um potencial de SQL Injection dessa linha, mas sem preocupações, pois essa linha era do PR que estava em Work In Progress.

@vercel
Copy link

vercel bot commented Aug 3, 2022

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

Name Status Preview Updated
tabnews ✅ Ready (Inspect) Visit Preview Aug 3, 2022 at 9:33PM (UTC)

@filipedeschamps
Copy link
Owner Author

@eletroswing peço que veja a implementação dos testes e caso tenha ficado alguma dúvida, me avise para ajudar a esclarecer 🤝

Em paralelo fica um desafio para todos nós: quais outros cenários a gente poderia garantir o funcionamento neste caso?

@filipedeschamps
Copy link
Owner Author

Fiz uma pequena alteração: na query do método que expira todas as sessões, adicionei uma condicional AND expires_at >= now() para só mexer nas sessões que estão para vencer, e não em todas as sessões históricas do usuário.

@kaique-soares
Copy link
Contributor

kaique-soares commented Aug 3, 2022

@filipedeschamps no commit 207b2df me surgiu uma dúvida.

No arquivo models/session.js a função expireAllFromUserId de fato cumpre sua intensão de expirar todas as sessions do user. No entanto, tem um detalhe na issue #384 me gerou uma certo desconforto, e por consequência uma dúvida.

A issue:

O PR 374 trouxe a feature de reset de senha, e agora pouco me veio em mente que uma medida de segurança interessante seria invalidar todas as sessões ativas o user após esse reset de senha.

A dúvida:
A parte que diz "sessões ativas" implicaria em mudar a implementação para: expirar as sessions do user, adicionando o filtro expires_at > now() na query?

...
WHERE user_id = $1 AND expires_at > now()
...

[Edit] o tempo que demorei para estruturar minha dúvida foi suficiente para o @filipedeschamps implementar e de quebra responder, sensacionaaal!

@filipedeschamps
Copy link
Owner Author

Sensacionaaaaal @kaique-soares muito obrigado pela contribuição e tenho certeza absoluta que o ato de você escrever esse comentário mandou ondas gravitacionais que meu cérebro captou, implementou e acabamos enviando tudo ao mesmo tempo 🤝

Então obrigado por pensar nisso, e obrigado ainda mais por escrever algo detalhado assim!

@filipedeschamps filipedeschamps changed the title Expire session Expirar todas as sessões válidas ao resetar a senha do usuário Aug 3, 2022
@filipedeschamps filipedeschamps merged commit 49abfc0 into main Aug 3, 2022
@filipedeschamps filipedeschamps deleted the expire-session branch August 3, 2022 22:02
@filipedeschamps
Copy link
Owner Author

Merged! Let's goooooo!!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants