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

useUser(): somente bater no endpoint se existir cookie indicando sessão #336

Closed
filipedeschamps opened this issue May 19, 2022 · 16 comments
Labels
back Envolve modificações no backend front Envolve modificações no frontend

Comments

@filipedeschamps
Copy link
Owner

Contexto

O hook useUser é usado para coisas como, no cabeçalho do TabNews mostrar os links de Login/Cadastro se a pessoa não estiver autenticada, ou mostrar o dropdown com o username do usuário.

Ele também é usado para mostrar ou não aquele botão adicional de editar o seu próprio conteúdo no componente Content.

Ainda nesse componente, ele também é usado para mostrar uma mensagem caso a pessoa não esteja logada.

O problema é que hoje, independente da condição do usuário estar logado ou não, ele bate no endpoint /api/v1/user e isso está causando bastante ruído nos logs, pois todas as pessoas que estão visitando o site e não estão logadas, por trás dos panos estão tomando um 403... e esses 403 estão se acumulando. Precisamos limpar isso, o que também irá parar de "flickar" o cabeçalho durante essa verificação (faça um refresh no TabNews e note que agonia, não aparece nada enquanto o endpoint não retorna).

Execução

Ao fazer Login (criar uma sessão), o backend devolve um cookie session_id mas que por questões de segurança ele não pode ser lido pelo client-side, então hoje não dá para saber se a pessoa está logada ou não sem bater na rota citada acima.

Dito isso, a ideia geral é na linha de: se não houver algum outro identificado estático de que sinalize que a pessoa possa estar logada, não fazer a request citada acima, porque com certeza vai tomar um 403. Se a pessoa tiver esse identificador, isso também não é garantia que ela continua logada, então nesse caso deve ser feita a request... que caso tome um 403, daí deve se remover esse identificador.

Mas agora as dúvidas são: esse identificador deve ser o que? Um cookie? Um item no localStorage? Quem deve setar ele, o client ou server? Em que momento? No momento do Login? Mas e as pessoas que já passaram pelo estágio de Login e se mantém logadas, como fazer uma feature que seja compatível com elas também (porque elas não terão de início o identificador)? Então será que nesse caso a rota /api/v1/user deveria retornar um cookie adicional que seja acessível pelo client-side?

Talvez o /api/v1/user sempre deva retornar um cookie com o username atual do usuário (porque ele pode mudar isso) e essa informação pode ser usada como o identificador. Se não tiver esse cookie, a pessoa está deslogada. Das pessoas que já estão logadas, mas também não tem esse cookie, na próxima chamada do /api/v1/user elas vão receber o cookie (o problema é como fazer elas chamarem o endpoint, dado que nosso objetivo agora é parar de chamar o endpoint se não tiver esse identificador). Talvez fazer uma chamada manual temporária, e remover isso depois de 30 dias. Enfim, se a pessoa tiver esse cookie, já instantaneamente usar isso como o username que aparece no cabeçalho para parar o flicker.

@filipedeschamps filipedeschamps added front Envolve modificações no frontend back Envolve modificações no backend labels May 19, 2022
@filipedeschamps filipedeschamps added this to the Milestone 4: TabCoins milestone May 19, 2022
This was referenced May 19, 2022
@aprendendofelipe
Copy link
Collaborator

Talvez o /api/v1/user sempre deva retornar um cookie com o username atual do usuário (porque ele pode mudar isso) e essa informação pode ser usada como o identificador. Se não tiver esse cookie, a pessoa está deslogada. Das pessoas que já estão logadas, mas também não tem esse cookie, na próxima chamada do /api/v1/user elas vão receber o cookie (o problema é como fazer elas chamarem o endpoint, dado que nosso objetivo agora é parar de chamar o endpoint se não tiver esse identificador). Talvez fazer uma chamada manual temporária, e remover isso depois de 30 dias. Enfim, se a pessoa tiver esse cookie, já instantaneamente usar isso como o username que aparece no cabeçalho para parar o flicker.

Isso! E essa chamada temporária pode ser só na tela de login, para já eliminar o 403 desnecessário das outras páginas.

Assim, se o usuário ainda não tem o cookie com o username, as páginas vão renderizar como se ele estivesse deslogado. Mas ao tentar postar algo já vai direcionar para o login, fazer a chamada para /api/v1/user automaticamente e voltar para onde estava sem precisar digitar a senha novamente (caso ele já esteja autenticado).

Lembrando que isso de aparecer como deslogado só vai acontecer para quem já está previamente autentitcado, mas ainda não acessou a versão nova para obter o cookie.

@tembra
Copy link
Contributor

tembra commented May 28, 2022

Estou querendo pegar essa issue amanhã se tudo der certo! hehehe
Mas fiquem a vontade :)

Comentário apenas pro @filipedeschamps trabalhar em outra frente se for o caso.

@filipedeschamps
Copy link
Owner Author

Ta pensando aqui, essa feature tem 3 pontos de interação:

  1. Login
  2. Requisições no meio da existência da sessão
  3. Logout

Talvez para cobrir tudo de forma centralizada, a gente pode trabalhar dentro do model session, pois ele é o responsável por renovar o cookie de sessão session_id... mais especificamente no método setSessionIdCookieInResponse() como a gente vai ver um pouco mais pra frente.

Tudo começa aqui nesse middleware que é declarado em basicamente todas as rotas:

async function injectAnonymousOrUser(request, response, next) {

E se o usuário está autenticado (que é o caso que queremos), a chamada entra aqui:

async function injectAuthenticatedUser(request, response) {

E se a sessão existe no banco de dados (e ela ainda está válida) e o usuário existe e possui todas as permissões, a sessão é renovada aqui:

const sessionRenewed = await session.renew(sessionObject.id, response);

E renovar significa duas coisas:

  1. Renovar a data de expiração da sessão lá no banco de dados (que não importa para essa implementação).
  2. Setar de novo o cookie session_id com uma nova data de expiração utilizando o método que comentei setSessionIdCookieInResponse().

const sessionObjectRenewed = await renewObjectInDatabase(sessionId);
setSessionIdCookieInResponse(sessionObjectRenewed.token, response);

Hoje esse método não recebe o user, recebe somente o token da sessão e o objeto de response do Next.js, mas poderia aceitar o user ou um username para aproveitar e "renovar" isso nos cookies também. Então poderia se chamar de uma forma mais genérica, como setAuthenticationCookiesInResponse() porque daí quando precisarmos implementar o /logout, podemos ter um método par chamado removeAuthenticationCookiesInResponse().

Fora isso, na hora de fazer o login (que é criar uma sessão), existe essa chamada:

const sessionObject = await authentication.createSessionAndSetCookies(storedUser.id, response);

E esse método também usa o session.setSessionIdCookieInResponse:

session.setSessionIdCookieInResponse(sessionObject.token, response);

Que novamente, poderia ser renomeado para algo mais genérico e que faça comportar também username.


Assim, todas as rotas que injetam o usuário vão ganhar esse recurso de graça e teremos mais elasticidade para adicionar ou remover cookies de autenticação de uma forma centralizada.

O que acham?


Como último ponto de observação, quando um controller recebe um erro de Autorização, ele chama esse método que limpa o session_id:

session.clearSessionIdCookie(response);

Ele também deveria ser algo mais genérico e gente poderia renomear o método clearSessionIdCookie() para removeAuthenticationCookiesInResponse(), pois isso casaria 100% com a sugestão do /logout ali de cima.

@filipedeschamps
Copy link
Owner Author

Em paralelo, me veio agora uma outra idéia que não pensei muito a fundo, mas acho que vai ajudar muito na performance da interface:

  1. Salvar o retorno do /user em localStorage.
  2. Se há essa informação em localStorage, usar ela e assumir que ela está válida.
  3. Isso vai fazer com tudo que precise do useUser() apareça instantaneamente na tela, seja o username lá em cima no menu, ou a opção de editar o seu próprio conteúdo.
  4. Em paralelo (e eu não sei como para ser sincero), fazer uma chamada no backend para buscar as informações atualizadas do /user.

Em resumo, seria uma estratégia de stale-while-revalidate, mas local (dado que não podemos fazer o cache dessa rota na CDN).

@filipedeschamps
Copy link
Owner Author

Estava pensando em usar algo como isso, olha que legal: https://github.com/leoafarias/use-state-persist

Alguém conhece algo mais atualizado?

@aprendendofelipe
Copy link
Collaborator

Opa, são dois problemas...

  1. Para a questão de menos acessos desnecessários para a API, acho boa ideia usar o localStorage, nem que seja só como solução temporária, pois assim daria pra resolver mexendo só no front. Provavelmente só no useUser e em pages/login.

  2. Já o problema do render, acho que a estratégia pode ser outra, pois mesmo buscando o username no localStorage, sempre irá renderizar o html estático primeiro e depois o do client, então eles precisam ser mais compatíveis em qualquer situação, seja logado ou não. Talvez algo assim:

  • Header - Não enviar mais as opções Login e Cadastrar no html estático. Após inicializar o useUser, aí sim renderizar o que fizer sentido (username ou Entrar), mas com uma transição mais suave e tentando evitar deslocamentos.
  • Content - Sempre renderizar os 3 pontinhos, só mudando as opções que aparecem ao clicar neles. Sem estar logado pode aparecer só a opção de denunciar, por exemplo.

@filipedeschamps
Copy link
Owner Author

Show de bola!! E não acho que é uma solução temporária, ter o user local (mesmo que stale), vejo que é uma ótima solução.

  • Header: Sobre renderizar só quando souber o que renderizar, eu vi que a Vercel faz isso na Dashboard deles. E não precisa ser suave, acho que quanto mais rápido melhor, o problem é que hoje o dropdown tem um height que faz o menu dar um pulinho pra baixo, ta feio.
  • Content: Por enquanto eu utilizaria a mesma estratégia do Header, que é só renderizar quando souber o que renderizar (que é o que está hoje).

Em paralelo, eu vi que o SWR tem um suporte para local cache, olha só:

Achei bem legal a solução 2 ali.

Em paralelo, quando o useUser() recebe um 403, precisamos fazer o objeto de erro ser retornado na propriedade de erro do swr e do nosso hook, e não do user. Acho que sem isso não dá para chamar o callback de erro ali do segundo artigo, e também vamos parar de mandar um objeto de erro no lugar do objeto de user (e o front vai se virar melhor com isso).

Bom, não vou mexer nisso agora, quero me focar nas issues de voltar o número de comentários pela API 🤝

@aprendendofelipe
Copy link
Collaborator

Opa, são dois problemas...

Dos dois problemas, o primeiro é resolvido no PR #440.

Já o segundo, como mexe com o design, acho que dá pra ficar para uma nova Issue.

@tembra
Copy link
Contributor

tembra commented Jun 7, 2022

@filipedeschamps

Salvar o retorno do /user em localStorage

Eu estava pensando em implementar exatamente assim! hehehe

Infelizmente meus dias (e finais de semana 😅) estão concorridos e ainda não consegui por a mão nessa issue.

@aprendendofelipe
Copy link
Collaborator

aprendendofelipe commented Jun 7, 2022

Edit. Eu tinha invertido direita e esquerda 😅

o problem é que hoje o dropdown tem um height que faz o menu dar um pulinho pra baixo, ta feio.

Não percebo esse pulinho pra baixo. O que me incomoda mais é o deslocamento do botão de Status que faz coisas piscarem... Se esse botão permanecer na direita e os componentes que só surgem no segundo render ficarem mais para a esqurda, já fica uma transição "suave".

Se acharem que compensa ver como fica (eu já testei aqui e curti), posso colocar isso no PR #440 ou, melhor ainda, no #425 (que, entre outras coisas, fixa o Header no topo).

Ou testem por si, movendo o Status lá para o final do componente Header:

...
      )}

      <Header.Item>
        <Header.Link href="/status" fontSize={2}>
          Status
        </Header.Link>
      </Header.Item>
    </Header>
  );
}

@filipedeschamps
Copy link
Owner Author

Issue fechada por dois PRs, dado que ela foi atacada em duas Fases:

Todas lideradas por @aprendendofelipe

@filipedeschamps
Copy link
Owner Author

Fora a UX muito melhor de usar o localStorage para mostrar o menu lá em cima, essa é a situação de erros do TabNews antes desse deploy, os erros do endpoint /api/v1/user esmagam os outros erros. Agora finalmente podemos criar um alerta para tentar pegar algum desvio de erros com números que façam sentido 🎉

image

@filipedeschamps
Copy link
Owner Author

@aprendendofelipe post comemorativo 😍 https://www.tabnews.com.br/filipedeschamps/nova-melhoria-aumento-de-performance-para-usuarios-logados-ultima-tarefa-antes-das-tabcoins 🎉

@filipedeschamps
Copy link
Owner Author

Olha como está o gráfico de erro do /api/v1/user, tenta adivinhar só por ele quando que foi feito o deploy:

image

@rodrigoKulb
Copy link
Contributor

@filipedeschamps a maioria dos acessos são de usuários anônimos?

@filipedeschamps
Copy link
Owner Author

@rodrigoKulb ótima pergunta! Eu não saberia dizer, preciso pensar em como montar uma query que agrupe isso 🤝 se eu conseguir mando o print em algum momento 👍

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
back Envolve modificações no backend front Envolve modificações no frontend
Projects
None yet
Development

No branches or pull requests

4 participants