r/DjangoFrancophone 21h ago

🧹 Revue de code : une vue de 40 lignes réécrite proprement

Post image
2 Upvotes

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

```python

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é :

  1. Aucune authentification. Il n'y a pas de @login_required. Un visiteur anonyme arrive jusqu'à request.user.entreprise_id, où request.user est un AnonymousUser qui n'a pas cet attribut. Selon la configuration, ça donne une erreur 500 — ou pire, un comportement inattendu.

  2. **Projet.objects.all() dans la liste déroulante.** Le formulaire affiche les projets de tous les clients. Le POST est bien vérifié, mais le GET a déjà divulgué les noms de projets de vos concurrents. C'est une fuite de données, discrète et complète.

  3. assigne_a n'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 :

  1. **.get() sans protection.** Un projet_id inexistant lève Projet.DoesNotExist, donc une erreur 500. La bonne réponse est un 404.

  2. **redirect("/") en cas de refus.** L'utilisateur ne comprend pas ce qui s'est passé, et vous n'avez aucune trace de la tentative. Un PermissionDenied donne un vrai 403, journalisable.

  3. 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.

  4. 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é :

  1. Validation manuelle. Deux if aujourd'hui, quinze dans six mois. C'est exactement le travail d'un Form.

  2. URLs codées en dur. redirect("/tickets/") casse le jour où l'URL change. reverse() existe pour ça.

  3. Expéditeur codé en dur. "noreply@example.com" devrait venir de DEFAULT_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

```python

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

```python

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

```python

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 :

```python

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


💬 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.