Adicionando tipos de usuário e tela de acesso negado #120
Conversation
KozielGPC
force-pushed
the
feat/add-admin-user
branch
from
00fe7da to
4d17772
Compare
There was a problem hiding this comment.
Acho que a implementação ficou boa mas algumas mudanças são necessárias pra manter o padrão do projeto (uso de decorators e enums).
Também não tenho certeza quanto ao uso dos roles numa coluna JSON, parece que vai deixar algumas consultas mais complexas no futuro.
flask_backend/templates/base.html
Outdated
| <a class="nav-link" href="{{ url_for("screening.create") }}">Nova sessão</a> | ||
| </li> | ||
| <div class="vr mx-3"></div> | ||
| {% if g.user and "ADMINs" in g.user.roles %} |
There was a problem hiding this comment.
aqui tem um typo (ADMINs ==> ADMIN).
Eu acho que isso é um bom exemplo do porque seria legal usar um ENUM pros diferentes roles do sistema.
Dá uma olhada em flask_backend/utils/enums/environment.py , lá tem um enum pros valores válidos de ambiente.
Só não tenho certeza de como usaríamos o enum aqui no template. Mas me parece meio errado digitar manualmente o role em todos os lugares
scrapers/paulo_amorim.py
Outdated
| @@ -164,7 +164,9 @@ def _get_todays_features(self): | |||
| strong_tag = p_tag.find("strong") | |||
| if strong_tag is None: | |||
| continue | |||
| strong_text = unicodedata.normalize("NFKC", strong_tag.text) | |||
| strong_text = unicodedata.normalize( | |||
| "NFKC", strong_tag.text.replace("º", "") | |||
There was a problem hiding this comment.
Acho que isso é do outro PR, não?
flask_backend/routes/screening.py
Outdated
| if "ADMIN" in g.user.roles: | ||
| return render_template("screening/import.html", suggestions=suggestions) | ||
| else: | ||
| return render_template( | ||
| "auth/forbidden.html", | ||
| ) |
There was a problem hiding this comment.
O problema de fazer a validação só na hora de retornar o template é que caso a requisição seja POST, a gente vai rodar toda a lógica de importação, e no fim mostrar a tela de forbidden. Mas a importação vai ter acontecido.
O flask tem um padrão de usar decorators pra esse tipo de validação. Aqui tem mais info: https://flask.palletsprojects.com/en/stable/patterns/viewdecorators/
Dá uma olhada em flask_backend/routes/auth.py , onde eu defino o @login_required. Eu testei aqui na minha máquina o seguinte decorator, e parece ter funcionado bem:
# criar um rota específica pra tela de erro
@bp.route("/forbidden")
def forbidden():
return render_template("auth/forbidden.html")
def admin_only(view):
@functools.wraps(view)
def wrapped_view(**kwargs):
if "ADMIN" not in g.user.roles:
if request.method == "POST":
abort(403)
return redirect(url_for("auth.forbidden"))
return view(**kwargs)
return wrapped_viewDaí onde for só pra admin, a gente só passa lá em cima:
@bp.route("/screening/import", methods=("GET", "POST"))
@login_required
@admin_only
def import_screenings():
... # toda a lógica da rota
# não precisa mais de if aqui
return render_template("screening/import.html", suggestions=suggestions)Isso tem alguns benefícios, tipo: quando for um POST a gente consegue dar o status de erro (403), e também não precisa ficar colocando ifs em todas as rotas
flask_backend/models.py
Outdated
| id: Mapped[int] = Column(Integer, primary_key=True) | ||
| username: Mapped[str] = Column(String(20), unique=True, nullable=False) | ||
| password: Mapped[str] = Column(String, nullable=False) | ||
| roles: Mapped[List[str]] = Column(JSON, nullable=False) |
There was a problem hiding this comment.
Achei maneira a ideia de permitir diferentes roles, mas não tenho certeza se usar um campo JSON é a melhor escolha. No SQL geralmente usamos uma estrutura de tabelas many-to-many pra permitir múltiplos valores.
Exemplo seria criar uma tabela roles:
| id | role |
|---|---|
| 1 | user |
| 2 | admin |
E depois uma tabela user_roles pra relacionar qual usuário tem quais roles:
| user_id | role_id |
|---|---|
| 1 | 1 |
| 1 | 2 |
| 2 | 1 |
Aí temos a mesma liberdade, por ex ali o usuário com id 1 é tanto ADMIN quanto USER, mas o usuário com id 2 é apenas USER.
Acredito que o maior benefício desse método é na hora de buscar dados. Para buscar no campo JSON, acredito que o campo roles de cada usuário teria que ser carregado em memória pra que depois seja filtrado.
No caso do uso das tabelas, podemos usar funcionalidades padrão do SQL como joins.
Na implementação atual, como eu buscaria todos os usuários que não são ADMIN, ou apenas os que são ADMIN?
Dps que eu escrevi esse comentário dei uma pesquisada aqui e achei alguns métodos que precisa passar o SQL "cru", por ex
from sqlalchemy import select, create_engine
from sqlalchemy.orm import declarative_base, scoped_session, sessionmaker
DATABASE_URL = "sqlite:///./flask_backend.sqlite"
engine = create_engine(DATABASE_URL)
db_session = scoped_session(
sessionmaker(autocommit=False, autoflush=False, bind=engine)
)
admin_users = db_session.execute(
select(User).from_statement(
text("SELECT * FROM users WHERE EXISTS (SELECT 1 FROM json_each(roles) WHERE value = 'ADMIN') ORDER BY id")
)
).scalars().all()Outro ponto importante é saber se isso seria compatível com outros bancos de dados, como o postgres, caso no futuro a gente queira migrar
|
@guites Certo, pelo que eu entendi os pontos principais de mudanças então são:
Assim que possível já trabalharei nas atualizações e aviso aqui de novo. |
|
Fala @guites, tranquilo?! Fiz as alterações requisitadas aqui. Quando puder, revise de novo por favor! Fico no aguardo |
|
@guites Consegue aprovar esse PR fazendo um favor? |
| def admin_only(view): | ||
| @functools.wraps(view) | ||
| def wrapped_view(**kwargs): | ||
| if RoleEnum.ADMIN.role not in [role.role for role in g.user.roles]: |
There was a problem hiding this comment.
Acho que com a última alteração não precisa desse .role aqui, pq o RoleEnum.Admin é uma string. Acho que por isso tá dando esse erro:
Ao acessar por ex. a página /screening/new
| @@ -0,0 +1,6 @@ | |||
| from enum import StrEnum | |||
There was a problem hiding this comment.
Vendo aqui, o StrEnum entrou no Python 3.11 (https://docs.python.org/3/library/enum.html#enum.StrEnum).
Não tem problema, só precisamos atualizar a versão mínima no README (lá na linha 22) e também no Dockerfile.prod (mudar FROM python:3.10-slim pra FROM python:3.11-slim já resolve).
|
@KozielGPC mals o atraso =P mas consegui refazer os testes aqui. Se tu não tiver como mexer no código, me avisa que eu faço as últimas alterações necessárias pra fechar o PR. Valeu!! |
|
@guites Relaxa aahhaa, sem problemas. Cara, se puder alterar pra mim eu fico agradecido. Tava com o projeto configurado num outro computador, e ai troquei recentemente e to com ele zerado ainda kkkk |
Descrição
Baseado no que foi pedido na issue #40, foi adicionado o atributo
roles. para a entidadeUser, contendo as roles do usuário. Escolhi adicionar um atributorolesem vez de apenas umisAdminjá pensando que poderão existir mais roles no futuro além deUSEReADMIN. Além disso, foi adicionado a renderização condicional dos itensImportar sessõeseNova sessãodo menu, sendo renderizados apenas se o usuário tiver a roleADMIN. Também foi adicionado uma tela para indicar ao usuário que ele não tem as permissões necessárias, caso acesse uma página via URL que não poderia.Prints
Usuário com permissão de ADMIN
Menu
Menu mobile
Acesso à
screening/newAcesso à
screening/importUsuário sem permissão de ADMIN
Menu
Menu mobile
Acesso à
screening/newAcesso à
screening/importUsuário não autenticado
Menu
Menu mobile
closes #40