Skip to content

Conversation

@ItzTas
Copy link

@ItzTas ItzTas commented Aug 23, 2024

#5 middleware de autenticação de usuario

@ItzTas ItzTas requested a review from heyitsmepablo August 23, 2024 00:51
@ItzTas ItzTas requested review from ed-henrique and heyitsmepablo and removed request for ed-henrique and heyitsmepablo August 23, 2024 00:51
@heyitsmepablo heyitsmepablo requested review from ed-henrique and removed request for heyitsmepablo August 23, 2024 02:41
@heyitsmepablo heyitsmepablo added the enhancement New feature or request label Aug 23, 2024
@heyitsmepablo heyitsmepablo linked an issue Aug 23, 2024 that may be closed by this pull request
5 tasks
Copy link

@ed-henrique ed-henrique left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Acredito que aplicando as modificações propostas e melhorando as mensagens de erro retornadas ao usuário, esse middleware vai ficar pronto.

*/
import jwt from 'jsonwebtoken';
import moment from 'moment';
const JWT_SECRET = `${process.env['JWT_SECRET']}`;

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Deveria haver uma condição checando se o JWT_SECRET que foi obtido não é vazio, porque isto poderia causar inconsistências com os tokens já existentes para os usuários. Exemplo:

if (!JWT_SECRET) {
  // Abortar execução do servidor ou obter o valor de JWT_SECRET de outra fonte
}

* @throws {JsonWebTokenError}
* @throws {TokenExpiredError}
*/
function decodeJWT(token, options = undefined) {

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

O parâmetro options para decodificar o JWT não é interessante nesse caso, pois permite que haja mudanças por desenvolvedores futuros em padrões essenciais que invalidariam os tokens de usuário já existentes e funcionais, como:

  • algorithms
  • ignoreExpiration
  • maxAge
  • allowInvalidAsymmetricKeyTypes

No geral, se tratando de um middleware de autenticação, o recomendado é que essas opções sejam definidas como uma constante que não deve ser alterada, ou que a função decodeJWT seja encapsulada antes de ser exportada, para que não seja possível alterar as opções por outro módulo que não seja o de middleware. Exemplo:

const defaultOptions = {
  // Defina as opções padrão aqui
}

function decodeJWT(token) {
  try {
    return jwt.verify(token, JWT_SECRET, defaultOptions);
  } catch (err) {
    // Tratamento de erros
  }
}

ou

function decodeJWTPadrao(token) {
  return decodeJWT(token, {
    // Insira as opções desejadas aqui
  });
}

// ...

export { decodeJWTPadrao, ... }

throw new Error('sem token de autorização ou nome errado');
}
const headerDiv = bearerToken.split(' ');
if (headerDiv[0] != 'Bearer' || headerDiv.length === 0) {

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Essa condição poderia verificar se o tamanho de headerDiv é maior que 2, o que indicaria um token JWT corrupto, provavelmente. Isso poderia ser feito dessa forma:

if (headerDiv[0] != 'Bearer' || headerDiv.length !== 2) {
  // Lógica
}

* @param {jwt.SignOptions} [options={ expiresIn: defaultTokenExpiration }]
* @returns {String}
*/
function signJWT(payload, options = {}) {

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Mesma questão do decodeJWT. O parâmetro options deve ser uma constante ou a função deve ser encapsulada.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

fiz assim o sign pra eu poder colocar o tempo de expiração como 1ms no teste

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Talvez fazendo de uma forma parecida à descrita aqui. Basicamente, deixa o que é pra ser exportado para testes com um nome bem explícito que é para testes, tipo testSignJWT, não deixando brecha para que os desenvolvedores se confundam no futuro.

Sinceramente, eu não gosto muito dessa abordagem, até porque em Go eu só colocaria os testes dentro do mesmo pacote do código sem exportar o que é pra ser interno. Mas anyways, a gente lida com o que a gente tem.

*
*/
import jwt from 'jsonwebtoken';
import moment from 'moment';

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Moment.js é uma biblioteca legada, que apenas recebe atualizações corretivas. Não há necessidade para sua utilização, dado que é possível utilizar os métodos de Date para alcançar o mesmo resultado.

Consultar estado da biblioteca

* @returns {String}
*/
function formatObject(obj) {
return JSON.stringify(obj, null, 2);

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Essa linha fica muito obscura pra quem não mexe com JavaScript direto. Talvez fosse mais interessante fazer algo assim:

return JSON.stringify(obj, NULL_KEYS_VALUE, SPACE_IDENTATION);

Dessa forma, mesmo alguém que não tenha familiaridade com os parâmetros da função JSON.stringify entenderia rapidamente.

try {
// @ts-ignore
let id = decoded.sub;
let reuslt = await pool.query('SELECT *FROM users WHERE id = $1 ', [id]);

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Alterar para:

let result = await pool.query('SELECT * FROM users WHERE id = $1', [id]);

Cuidado com os nomes das variáveis para não confundir futuros desenvolvedores.

// @ts-ignore
let id = decoded.sub;
let reuslt = await pool.query('SELECT *FROM users WHERE id = $1 ', [id]);
if (reuslt.rows.length == 0) {

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

É interessante que haja uma condição checando se mais de um usuário foi encontrado com aquele ID, e em caso positivo, realizar um log e devolver um erro 5xx. Exemplo:

if (result.rows.lenght > 1) {
  console.log(`ERROR: Mais de um usuário foi encontrado com o id ${id}`);
  return res.status(500).json({ error: 'Houve um erro no banco de dados ao autenticar o usuário' });
}

É raro de acontecer esse tipo de situação, e ela geralmente é associada a modificação diretamente via banco de dados ao invés do uso da aplicação, mas é importante manter em mente que isso pode acontecer.


describe('AUTH TEST', () => {
afterAll(async () => {
await pool.query('TRUNCATE TABLE users CASCADE');

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Muito cuidado com esse tipo de limpeza após os testes. Apenas realize esse tipo de operação se houver certeza que o banco utilizado é o de testes. Ainda mais porque esse middleware não lida com a criação de novos usuários, apenas leitura dos já existentes.

import { signJWT } from '../src/helpers/auth.js';
import { formatObject } from '../src/helpers/general_helpers.js';
import pool from '../src/database/database.js';
import moment from 'moment';

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Remoção da biblioteca legado Moment.js.

@ItzTas ItzTas requested a review from ed-henrique August 23, 2024 23:15
Copy link

@ed-henrique ed-henrique left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok.

Só manter em mente que essa API vai servir algum usuário ou desenvolvedor frontend, geralmente. Isso significa que erros mais descritivos são preferíveis, como O token JWT passado foi inválido ao invés de invalido.

Além disso, o servidor não faz nada se o banco de dados não estiver conectado a ele. Por isso, ferramentas como as descritas aqui na documentação do pacote pg são úteis para saber se o servidor deve ser parado.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

enhancement New feature or request

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Historia 8: Autententicação do Usuario

5 participants