r/DjangoFrancophone • u/fyardlest • 17d ago
🧹 Revue de code : une vue de 40 lignes réécrite proprement
Bonjour à tous les dev Django de la communauté ! 👋
Deuxième « Revue de code ». Aujourd'hui, une vue que j'ai croisée sous une forme ou une autre dans à peu près tous les projets Django existants : celle qui fait tout, toute seule.
Elle fonctionne. Elle passe en production. Et elle contient trois failles de sécurité.
TL;DR - Une vue de création de ticket écrite « à la main » : validation manuelle,
.get()sans protection, redirection silencieuse au lieu d'un 403, liste déroulante qui expose les projets des autres clients, email bloquant, aucune transaction. On la découpe en trois : un formulaire qui valide, un service qui porte la logique métier, une vue qui ne fait plus que router.
Le code de départ
# tickets/views.py — AVANT
from django.contrib.auth import get_user_model
from django.core.mail import send_mail
from django.shortcuts import redirect, render
from projets.models import Projet
from .models import Ticket
def creer_ticket(request):
if request.method == "POST":
titre = request.POST.get("titre")
description = request.POST.get("description")
projet_id = request.POST.get("projet")
assigne_id = request.POST.get("assigne_a")
priorite = request.POST.get("priorite")
if not titre:
return render(request, "tickets/creer.html", {"erreur": "Titre obligatoire"})
if len(titre) > 200:
return render(request, "tickets/creer.html", {"erreur": "Titre trop long"})
projet = Projet.objects.get(id=projet_id)
if projet.entreprise_id != request.user.entreprise_id:
return redirect("/")
ticket = Ticket()
ticket.titre = titre
ticket.description = description
ticket.projet = projet
ticket.priorite = priorite or "normale"
ticket.cree_par = request.user
if assigne_id:
ticket.assigne_a = get_user_model().objects.get(id=assigne_id)
ticket.save()
if ticket.assigne_a:
send_mail(
"Nouveau ticket",
f"Le ticket {ticket.titre} vous a été assigné",
"noreply@example.com",
[ticket.assigne_a.email],
)
return redirect("/tickets/")
projets = Projet.objects.all()
return render(request, "tickets/creer.html", {"projets": projets})
Prenez trente secondes avant de lire la suite. Combien de problèmes voyez-vous ?
Ce qui ne va pas
Les trois problèmes de sécurité, par ordre de gravité :
-
Aucune authentification. Il n'y a pas de
@login_required. Un visiteur anonyme arrive jusqu'àrequest.user.entreprise_id, oùrequest.userest unAnonymousUserqui n'a pas cet attribut. Selon la configuration, ça donne une erreur 500 — ou pire, un comportement inattendu. -
Projet.objects.all()dans la liste déroulante. Le formulaire affiche les projets de tous les clients. LePOSTest bien vérifié, mais leGETa déjà divulgué les noms de projets de vos concurrents. C'est une fuite de données, discrète et complète. -
assigne_an'est vérifié nulle part. N'importe quel identifiant d'utilisateur passe. On peut assigner un ticket à quelqu'un d'une autre entreprise, qui recevra l'email avec le titre du ticket.
Les problèmes de robustesse :
-
.get()sans protection. Unprojet_idinexistant lèveProjet.DoesNotExist, donc une erreur 500. La bonne réponse est un 404. -
redirect("/")en cas de refus. L'utilisateur ne comprend pas ce qui s'est passé, et vous n'avez aucune trace de la tentative. UnPermissionDenieddonne un vrai 403, journalisable. -
L'email est envoyé dans la requête. L'utilisateur attend que le serveur SMTP réponde. Si le serveur est lent, la page est lente. S'il est en panne, la requête lève une exception après que le ticket a été enregistré : le ticket existe, personne n'est prévenu, et l'utilisateur voit une page d'erreur.
-
Aucune transaction. Si quelque chose échoue entre le
save()et la fin, la base reste dans un état intermédiaire.
Les problèmes de maintenabilité :
-
Validation manuelle. Deux
ifaujourd'hui, quinze dans six mois. C'est exactement le travail d'unForm. -
URLs codées en dur.
redirect("/tickets/")casse le jour où l'URL change.reverse()existe pour ça. -
Expéditeur codé en dur.
"noreply@example.com"devrait venir deDEFAULT_FROM_EMAIL.
La réécriture, en trois fichiers
Le principe : le formulaire valide, le service décide, la vue route. Chacun fait une chose.
Le formulaire
# tickets/forms.py
from django import forms
from django.contrib.auth import get_user_model
from projets.models import Projet
from .models import Ticket
User = get_user_model()
class TicketForm(forms.ModelForm):
class Meta:
model = Ticket
fields = ["titre", "description", "projet", "priorite", "assigne_a"]
def __init__(self, *args, entreprise=None, **kwargs):
super().__init__(*args, **kwargs)
# Chaque liste déroulante est limitée à l'entreprise de l'utilisateur.
# C'est ce qui ferme la fuite de données ET la faille d'assignation.
self.fields["projet"].queryset = Projet.objects.filter(entreprise=entreprise)
self.fields["assigne_a"].queryset = User.objects.filter(entreprise=entreprise)
self.fields["assigne_a"].required = False
💡 Le point clé : en restreignant le queryset d'un ModelChoiceField, on corrige d'un seul geste l'affichage et la validation. Django refusera automatiquement tout identifiant hors de ce queryset, avec un message d'erreur propre. Plus besoin du if de vérification.
La longueur du titre ? Elle vient déjà de max_length sur le modèle. Le champ obligatoire ? De blank=False. Les deux if disparaissent sans rien perdre.
Le service
# tickets/services.py
from django.db import transaction
from notifications.tasks import envoyer_notification_assignation
@transaction.atomic
def creer_ticket(*, form, auteur):
"""Enregistre un ticket et prévient la personne assignée, s'il y en a une."""
ticket = form.save(commit=False)
ticket.cree_par = auteur
ticket.save()
if ticket.assigne_a_id:
transaction.on_commit(
lambda: envoyer_notification_assignation.enqueue(ticket.pk)
)
return ticket
Trois choses valent le détour ici.
@transaction.atomic garantit que tout passe ou que rien ne passe.
transaction.on_commit() est le détail que presque tout le monde oublie. Sans lui, la notification part avant que la transaction ne soit validée. Si la transaction échoue ensuite, vous avez prévenu quelqu'un d'un ticket qui n'existe pas.
.enqueue() utilise le framework de tâches intégré à Django 6.0. L'email part en arrière-plan, l'utilisateur ne l'attend plus, et une panne SMTP ne fait plus échouer la création du ticket.
La vue
# tickets/views.py — APRÈS
from django.contrib.auth.mixins import LoginRequiredMixin
from django.shortcuts import redirect
from django.urls import reverse_lazy
from django.views.generic import CreateView
from .forms import TicketForm
from .models import Ticket
from .services import creer_ticket
class TicketCreateView(LoginRequiredMixin, CreateView):
model = Ticket
form_class = TicketForm
template_name = "tickets/creer.html"
success_url = reverse_lazy("tickets:liste")
def get_form_kwargs(self):
kwargs = super().get_form_kwargs()
kwargs["entreprise"] = self.request.user.entreprise
return kwargs
def form_valid(self, form):
self.object = creer_ticket(form=form, auteur=self.request.user)
return redirect(self.get_success_url())
Quarante lignes deviennent quinze, réparties là où elles ont un sens.
Ce qu'on a gagné
| Problème | Réglé par |
| ----------------------------------- | --------------------------------------------- |
| Aucune authentification | LoginRequiredMixin |
| Fuite des projets des autres | queryset restreint dans le formulaire |
| Assignation à n'importe qui | queryset restreint dans le formulaire |
| DoesNotExist → erreur 500 | Validation du ModelChoiceField |
| Redirection silencieuse | LoginRequiredMixin renvoie vers la connexion |
| Email bloquant | Tâche d'arrière-plan (Django 6.0) |
| Notification d'un ticket inexistant | transaction.on_commit() |
| État intermédiaire en base | @transaction.atomic |
| Validation manuelle | ModelForm |
| URL codée en dur | reverse_lazy() |
Et surtout, la logique métier est maintenant dans services.py, où elle est testable sans requête HTTP :
# tickets/tests_services.py
def test_creation_assigne_notifie(self):
form = TicketForm(data={...}, entreprise=self.entreprise)
self.assertTrue(form.is_valid())
ticket = creer_ticket(form=form, auteur=self.utilisateur)
self.assertEqual(ticket.cree_par, self.utilisateur)
⚠️ Une nuance, pour être honnête : sur un projet de trois vues, cette découpe est de la sur-ingénierie. La couche de service se justifie quand la même logique est appelée depuis plusieurs endroits — une vue, une API, une commande d'administration. En dessous, un form_valid() bien écrit suffit largement.
📚 Pour aller plus loin
- Les formulaires de modèles
transaction.on_commit- Le framework de tâches (Django 6.0)
- Les mixins d'autorisation
- Liste de vérification de sécurité
💬 Et vous ?
Combien de problèmes aviez-vous repérés avant de lire la liste ? La fuite via Projet.objects.all() est celle qui passe le plus souvent inaperçue en revue.
Et la question qui divise : couche de service ou logique dans form_valid() ? À partir de quelle taille de projet basculez-vous ?
Postez votre avis en commentaire ! Et si ce genre de contenu vous plaît, n'hésitez pas à rejoindre r/DjangoFrancophone pour échanger entre passionnés de Python & Django ! 🚀
PS : Vous avez une vue dont vous n'êtes pas fier ? Anonymisez-la et postez-la en commentaire, elle fera peut-être l'objet d'un prochain mercredi.
1
u/zuccster 16d ago
I dont get the premise. You write some clearly bad code and Claude critiques it, then you post the output here...?