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

[IDOR] sécurisation des urls de consultation des PASS IAE - part 1 [GEN-2405] #5446

Merged
merged 2 commits into from
Jan 22, 2025

Conversation

vincentporte
Copy link
Contributor

@vincentporte vincentporte commented Jan 22, 2025

🤔 Pourquoi ?

les approval sont consultables à partir de leur id incrémental

Catégories changelog

PASS IAE

🍰 Comment ?

🧑‍🏭 ajout du champ public_id, uuid, unique, sur les modèles Approval
🧑‍🏭 hydratation du champ, puis passage en default uuid4

⚠️ public_id reste en null=True pour éviter un déphasage entre l'état de la base de données et django pendant les migrations.

⏭️ PR préparatoire à la #5431

🚨

  • Mettre à jour le CHANGELOG_breaking_changes.md > Non

Copy link
Contributor

@xavfernandez xavfernandez left a comment

Choose a reason for hiding this comment

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

Dans l'idée, il faut que les tests passent sur tous les commits pour faciliter un éventuel git bisect. Donc la mise à jour des snapshots devraient être squashés avec l'ajout du champs.

name="public_id",
field=uuid_field(default=None),
),
migrations.RunPython(fill_public_id_approval, migrations.RunPython.noop, elidable=True),
Copy link
Contributor

Choose a reason for hiding this comment

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

Il vaut mieux mettre l'ajout de la colonne dans une migration dédiée, sans atomic=False. Sinon, si le déploiement est interrompu, ça va être pénible.

field=uuid_field(default=None),
),
migrations.RunPython(fill_public_id_approval, migrations.RunPython.noop, elidable=True),
migrations.AlterField(model_name="approval", name="public_id", field=uuid_field(default=uuid.uuid4)),
Copy link
Contributor

Choose a reason for hiding this comment

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

Pourquoi faire un AlterField ? Tu peux directement ajouter le champs avec son default=uuid.uuid4.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

je vérifie, mais lors des premiers essais, avec un default=uuid.uuid4, la migration appliquait la même valeur d'uuid sur toutes les lignes, et cassait la contrainte d'unicité.

Copy link
Contributor

@xavfernandez xavfernandez Jan 22, 2025

Choose a reason for hiding this comment

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

Effectivement, il faut bien faire un AddField(..., default=None, ...) suivi d'un AlterField(..., default=uuid.uuid4, ...) 😞

@vincentporte vincentporte force-pushed the vp/idor_approvals_part1 branch from 20c311e to df34895 Compare January 22, 2025 15:09
@@ -560,7 +561,7 @@ class Approval(PENotificationMixin, CommonApprovalMixin):
public_id = models.UUIDField(
verbose_name="identifiant public",
help_text="identifiant opaque, pour les API et les URLs publiques",
default=None,
default=uuid.uuid4,
Copy link
Contributor

Choose a reason for hiding this comment

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

J'aurais mis cette modif dans le 1er commit (avec une migration enchaînant AddField & AlterField), mais ça marche aussi comme cela :)

@vincentporte vincentporte added this pull request to the merge queue Jan 22, 2025
Merged via the queue into master with commit 3bbf119 Jan 22, 2025
9 checks passed
@vincentporte vincentporte deleted the vp/idor_approvals_part1 branch January 22, 2025 15:35
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants