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

feat(login/logout): adicionando a possibilidade de deslogar #484

Merged
merged 3 commits into from
Jul 27, 2022

Conversation

gabriel-ayusso
Copy link

Incluindo método (bem simples) para deslogar o usuário

Repository owner deleted a comment from vercel bot Jul 10, 2022
@ErickCReis
Copy link
Contributor

Acho que seria interessante mover essa lógica para o hook useUser, assim daria para aproveitar e atualizar o estado do usuário e a interface responderia instantaneamente, sem precisar de uma navegação (não sei se é a melhor opção).

Ainda deve ser necessário alguma implementação na api para expirar/apagar a sessão do usuário, de qualquer forma esse pr já me parece um ótimo começo.

gabriel-ayusso pushed a commit that referenced this pull request Jul 11, 2022
Alterada a responsabilidade de logoff para o contexto de usuário; incluida a funcionalidade de
encerrar a sessão no backend para eliminar o cookie

#484
@gabriel-ayusso
Copy link
Author

Acho que seria interessante mover essa lógica para o hook useUser, assim daria para aproveitar e atualizar o estado do usuário e a interface responderia instantaneamente, sem precisar de uma navegação (não sei se é a melhor opção).

Ainda deve ser necessário alguma implementação na api para expirar/apagar a sessão do usuário, de qualquer forma esse pr já me parece um ótimo começo.

Boa... Subi um PR com os ajustes sugeridos. Acho que agora está melhor. Obrigado!

Repository owner deleted a comment from vercel bot Jul 11, 2022
Repository owner deleted a comment from vercel bot Jul 11, 2022
gabriel-ayusso pushed a commit that referenced this pull request Jul 11, 2022
adicionando a dependencia router no useCallback de logout (removendo warning)

re #484
@vercel
Copy link

vercel bot commented Jul 11, 2022

@gabriel-alienlab is attempting to deploy a commit to the TabNews Team on Vercel.

To accomplish this, @gabriel-alienlab needs to request access to the Team.

Afterwards, an owner of the Team is required to accept their membership request.

If you're already a member of the respective Vercel Team, make sure that your Personal Vercel Account is connected to your GitHub account.

@gabriel-ayusso
Copy link
Author

Eu subi um commit agora, mas ele deu falha por conta de Lint Commits, agora não sei como corrigir :-(
Pelo que entendi foi a mensagem de commit de uma outra pessoa. Sorry :-\

@coffeeispower
Copy link
Contributor

coffeeispower commented Jul 11, 2022

Eu subi um commit agora, mas ele deu falha por conta de Lint Commits, agora não sei como corrigir :-(
Pelo que entendi foi a mensagem de commit de uma outra pessoa. Sorry :-\

Eu comitei pelo github e esqueci de usar a convençao das mensagens de commit, e so usar um git rebase -i HEAD~4 mudar pick pra reword no commit e dps mudar a mensagem de commit, salvar e sair do editor, depois e so dar force push

@coffeeispower
Copy link
Contributor

Vou arrumar aqui rapidinho

coffeeispower pushed a commit that referenced this pull request Jul 11, 2022
Alterada a responsabilidade de logoff para o contexto de usuário; incluida a funcionalidade de
encerrar a sessão no backend para eliminar o cookie

#484
coffeeispower pushed a commit that referenced this pull request Jul 11, 2022
adicionando a dependencia router no useCallback de logout (removendo warning)

re #484
@coffeeispower
Copy link
Contributor

Parece estar resolvido agora

@gabriel-ayusso
Copy link
Author

Parece estar resolvido agora

Valeuuu! 🖖

@vercel
Copy link

vercel bot commented Jul 11, 2022

@gabriel-ayusso is attempting to deploy a commit to the TabNews Team on Vercel.

To accomplish this, @gabriel-ayusso needs to request access to the Team.

Afterwards, an owner of the Team is required to accept their membership request.

If you're already a member of the respective Vercel Team, make sure that your Personal Vercel Account is connected to your GitHub account.

coffeeispower pushed a commit that referenced this pull request Jul 12, 2022
Alterada a responsabilidade de logoff para o contexto de usuário; incluida a funcionalidade de
encerrar a sessão no backend para eliminar o cookie

#484
coffeeispower pushed a commit that referenced this pull request Jul 12, 2022
adicionando a dependencia router no useCallback de logout (removendo warning)

re #484
@coffeeispower
Copy link
Contributor

coffeeispower commented Jul 12, 2022

Fiz um rebase da branch main pra atualizar

@aprendendofelipe
Copy link
Collaborator

Que bom ver vocês trabalhando nisso, mas tenho que pontuar algumas coisas que pensei aqui:

  1. O principal que falta implementar é a possibilidade de remoção da(s) sessão(ões) do usuário no BD.
  2. Remover a sessão somente no client não é nada seguro.
  3. Deletar o cookie e o localStorage já estava implementado e ocorrendo quando a sessão não estava válida.
  4. Nada impede que seja melhorada a maneira como ocorre o item 3 para eliminar a chamada ao endpoint user, mas sobre o useUser:
  • não acho legal ter o router.replace nesse hook, pois nem sempre será necessário mudar a rota ao fazer logout;
  • nem devemos setar o isLoading, pois isso irá causar efeitos colaterais que não fazem sentido durante o logout.

@gabriel-ayusso
Copy link
Author

Oi @aprendendofelipe . Realmente você tem razão. Acabei de subir um fix que corrige esses pontos.

Dúvida: Quando a sessão fica inválida, o browser não envia o token/cookie pro backend, certo? Se isso estiver certo, para manter a base limpa, seria interessante apagar todas as sessões já expiradas?

Outra dúvida: Devemos realmente excluir as sessões, ou manter elas para auditoria por um tempo?

@gabriel-ayusso
Copy link
Author

⚠️ Não entendi o fail de Lint Commits. Eu fiz o commit usando o npm run commit. Será que alguém poderia me ajudar a esclarecer o que fiz de errado?

Vlww

@coffeeispower
Copy link
Contributor

coffeeispower commented Jul 15, 2022

warning Não entendi o fail de Lint Commits. Eu fiz o commit usando o npm run commit. Será que alguém poderia me ajudar a esclarecer o que fiz de errado?

Vlww
@gabriel-ayusso
Por isso: https://imgur.com/u4QmXYb.png

Vc vai ter que fazer um rebase interativo pra mudar a mensagem de commit de merge

@aprendendofelipe
Copy link
Collaborator

Dúvida: Quando a sessão fica inválida, o browser não envia o token/cookie pro backend, certo? Se isso estiver certo, para manter a base limpa, seria interessante apagar todas as sessões já expiradas?

Para melhor clareza de informações, não vamos confundir "sessão inválida" com "cookie expirado".

O cookie expirado não é enviado, pois ele é excluído assim que expira.

A sessão inválida é quando ela não existir no BD ou, caso exista, estar expirada.

As sessões expiradas, se não me engano, ainda não estão sendo excluídas do BD. É preciso criar uma rotina para isso, mas não vejo necessidade nesse PR.


Outra dúvida: Devemos realmente excluir as sessões, ou manter elas para auditoria por um tempo?

Boa pergunta! Acho bom rolar uma conversa, incluindo o @filipedeschamps, para decidir sobre isso. Pois existe alternativas como revogar a sessão atribuindo, por exemplo, expires_at = updated_at = now() .


Não entendi o fail de Lint Commits. Eu fiz o commit usando o npm run commit. Será que alguém poderia me ajudar a esclarecer o que fiz de errado?

Você fez o merge devolvendo todos os commits antigos, e um deles está com problema (62b601b). É bom fazer um rebase com a main, deixando somente um commit.


No geral, acho que o que foi implementado até aqui 7936b1b é o que realmente precisava, mas a maneira que foi implementado ainda precisa de ajustes para organizar o código:

  1. Está estranho a função que remove a sessão no BD (deleteSession) ser chamada por dentro da função que limpa o cookie (clearSessionIdCookie);
  2. As duas poderiam ser chamadas dentro da deleteHandler, que é onde você pode obter o session_id para passar para a deleteSession;
  3. Assim você pode deixar a clearSessionIdCookie como estava originalmente e também não precisa da alteração no controller.

O useUser e o Header acho que estão OK.

@gabriel-ayusso
Copy link
Author

Dúvida: Quando a sessão fica inválida, o browser não envia o token/cookie pro backend, certo? Se isso estiver certo, para manter a base limpa, seria interessante apagar todas as sessões já expiradas?

Para melhor clareza de informações, não vamos confundir "sessão inválida" com "cookie expirado".

O cookie expirado não é enviado, pois ele é excluído assim que expira.

A sessão inválida é quando ela não existir no BD ou, caso exista, estar expirada.

As sessões expiradas, se não me engano, ainda não estão sendo excluídas do BD. É preciso criar uma rotina para isso, mas não vejo necessidade nesse PR.

Outra dúvida: Devemos realmente excluir as sessões, ou manter elas para auditoria por um tempo?

Boa pergunta! Acho bom rolar uma conversa, incluindo o @filipedeschamps, para decidir sobre isso. Pois existe alternativas como revogar a sessão atribuindo, por exemplo, expires_at = updated_at = now() .

Não entendi o fail de Lint Commits. Eu fiz o commit usando o npm run commit. Será que alguém poderia me ajudar a esclarecer o que fiz de errado?

Você fez o merge devolvendo todos os commits antigos, e um deles está com problema (62b601b). É bom fazer um rebase com a main, deixando somente um commit.

No geral, acho que o que foi implementado até aqui 7936b1b é o que realmente precisava, mas a maneira que foi implementado ainda precisa de ajustes para organizar o código:

  1. Está estranho a função que remove a sessão no BD (deleteSession) ser chamada por dentro da função que limpa o cookie (clearSessionIdCookie);
  2. As duas poderiam ser chamadas dentro da deleteHandler, que é onde você pode obter o session_id para passar para a deleteSession;
  3. Assim você pode deixar a clearSessionIdCookie como estava originalmente e também não precisa da alteração no controller.

O useUser e o Header acho que estão OK.

Opa, finalmente consegui entender o rebase... rsrs Obrigado pela paciencia e pelas dicas @aprendendofelipe e @coffee-is-power ;-)

@coffeeispower
Copy link
Contributor

coffeeispower commented Jul 16, 2022

O rebase e basicamente pegar duas branches e colocar uma em cima da outra, o que e util quando voce quer atualizar a sua branch por exemplo, ele pega os commits que vc fez na sua branch e coloca em cima dos commits da outra branch

@filipedeschamps
Copy link
Owner

Primeiro, @gabriel-ayusso parabéns pela coragem de fazer a implementação e digo isso porque geralmente as pessoas possuem medo de mexer das partes da aplicação que tocam na autenticação/autorização (e justíssimo).

Fiz alguns comentários na issue da milestone #570 e fazendo referência para cá. Posso mexer nessa branch aqui para dar continuidade na sua implementação? Eu vou alterar algumas coisas e implementar testes.

Em paralelo, @aprendendofelipe é bizarro como pensamos igual em vários pontos 😂 sugestões excelentes 👍

@gabriel-ayusso
Copy link
Author

Primeiro, @gabriel-ayusso parabéns pela coragem de fazer a implementação e digo isso porque geralmente as pessoas possuem medo de mexer das partes da aplicação que tocam na autenticação/autorização (e justíssimo).

Fiz alguns comentários na issue da milestone #570 e fazendo referência para cá. Posso mexer nessa branch aqui para dar continuidade na sua implementação? Eu vou alterar algumas coisas e implementar testes.

Em paralelo, @aprendendofelipe é bizarro como pensamos igual em vários pontos 😂 sugestões excelentes 👍

Oi @filipedeschamps!

Claro!! Será uma honra! ;-)

@aprendendofelipe
Copy link
Collaborator

é bizarro como pensamos igual em vários pontos

hahaha... é pior do que você pensa, pois já aconteceu diversas vezes de eu escrever uma resposta aqui no repositório e, antes que eu pudesse enviar, surgir uma resposta sua quase igual ao que eu estava escrevendo... Deve ser culpa do nome parecido 😜

@vercel
Copy link

vercel bot commented Jul 26, 2022

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

Name Status Preview Updated
tabnews ✅ Ready (Inspect) Visit Preview Jul 27, 2022 at 2:51AM (UTC)

@filipedeschamps
Copy link
Owner

Turma, as principais coisas que os últimos commits fazem:

  1. Squash dos dois commits do @gabriel-ayusso em um só, com mensagem em inglês, e rebased com a main.
  2. Renomeei método deleteSession para expireById.
  3. O método expireById atualiza expires_at para created_at - interval '1 day' e o updated_at para now().
  4. O método expireById agora retorna o objeto de sessão atualizado para retornarmos isso no response do DELETE.
  5. Para entrar no controller do DELETE, usuário precisa estar autenticado e possuir a feature read:session.
  6. Esta foi a cobertura de testes que coloquei para o DELETE:

image

  1. Para evitar clique errado no Logout, coloquei caixa de confirmação:

image

@filipedeschamps
Copy link
Owner

Funcionando no ambiente de homologação: https://tabnews-git-feature-deslogar-tabnews.vercel.app/

@coffeeispower
Copy link
Contributor

Massinha, pra mim ta pronto

@aprendendofelipe
Copy link
Collaborator

7. Para evitar clique errado no Logout, coloquei caixa de confirmação:

Só estou incomodado com essa confirmação para sair, mas se for só eu, manda ver, pois tudo está funcionando perfeitamente 🚀

@filipedeschamps
Copy link
Owner

Só estou incomodado com essa confirmação para sair

Você diz o texto ou o fato de precisar confirmar? Minha preocupação ao adicionar isso foi "missclick", principalmente em mobile 👍

@aprendendofelipe
Copy link
Collaborator

Você diz o texto ou o fato de precisar confirmar? Minha preocupação ao adicionar isso foi "missclick", principalmente em mobile 👍

O fato de precisar confirmar me incomoda, pois a probabilidade de ter clicado de propósito é muito maior do que por acidente. Se fosse uma ação complicada de ser revertida, aí sim acho que caberia a confirmação, mas é só um logout.

@filipedeschamps
Copy link
Owner

O fato de precisar confirmar me incomoda, pois a probabilidade de ter clicado de propósito é muito maior do que por acidente. Se fosse uma ação complicada de ser revertida, aí sim acho que caberia a confirmação, mas é só um logout.

Faz total sentido. Otimização prematura da minha parte! Vou remover 🤝

@filipedeschamps
Copy link
Owner

Merged! Let's gooooooo!!

@filipedeschamps
Copy link
Owner

Ta uma delícia deslogar e logar 😍 fiz várias vezes em produção 🤝

@filipedeschamps filipedeschamps mentioned this pull request Jul 27, 2022
2 tasks
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.

6 participants