Bug #63725
TenantFileSystemStorage vs django 2.2.26, suspiciousfileoperation
0%
Description
avant, la méthode save() de Storage se terminait ainsi :
name = self.get_available_name(name, max_length=max_length)
return self._save(name, content)
désormais elle ajoute une vérification du résultat,
name = self._save(name, content)
# Ensure that the name returned from the storage system is still valid.
validate_file_name(name, allow_relative_path=True)
return name
et ça regarde /var/lib/whatever/tenants/whatever/media/ vs default_storage.location qui est /usr/lib/python3/dist-packages/media et ça dit suspicious.
Fichiers
Révisions associées
trivial: remove support of django-tenant-schemas w/o TenantStorageMixin (#63725)
Historique
Mis à jour par Frédéric Péters il y a environ 2 ans
- Statut changé de Nouveau à En cours
- Assigné à mis à Frédéric Péters
Mis à jour par Frédéric Péters il y a environ 2 ans
- Fichier 0001-multitenant-add-save-method-to-storage-backend-to-ch.patch 0001-multitenant-add-save-method-to-storage-backend-to-ch.patch ajouté
- Fichier 0001-multitenant-redo-storage-backend-by-overriding-locat.patch 0001-multitenant-redo-storage-backend-by-overriding-locat.patch ajouté
- Statut changé de En cours à Solution proposée
- Patch proposed changé de Non à Oui
Approche 1 qui remplace la méthode save() pour y mettre notre propre détection de suspicious moment.
Approche 2 qui remplace la propriété location. (de base c'est une cached_property, ici une property simple, ça ne doit pas être significatif).
Je ne sais pas trop pourquoi on n'avait pas fait ça comme ça à la base.
Peut-être une piste dans la docstring "Lookup files first in $TENANT_BASE/<tenant.schema>/media/ then in default location", on aurait eu l'idée à un moment de gérer le fallback media, mais on ne l'aurait jamais implémenté ensuite ?
~~
À remonter je vois que le TenantStorageMixin dans django-tenant-schemas fait aussi sa modif sur la méthode path(); j'en viendrais donc à penser qu'il y a une raison, ne serait-ce que ne pas se trouver avec le path() de django-tenant-schemas ajouter le chemin de tenant par dessus un self.location qui l'aurait déjà. Sauf que c'est de toute façon le path() de FileSytemStorage qui est appelé, donc ça n'arrive pas.
~~
Bref je serais pour la seconde approche.
~~
et j'ajoute 0002 pour supprimer la prise en charge de compatibilité avec une ancienne version de django-tenant-schemas.
Mis à jour par Frédéric Péters il y a environ 2 ans
Mis à jour par Thomas Noël il y a environ 2 ans
Frédéric Péters a écrit :
À remonter je vois que le TenantStorageMixin dans django-tenant-schemas fait aussi sa modif sur la méthode path(); j'en viendrais donc à penser qu'il y a une raison, ne serait-ce que ne pas se trouver avec le path() de django-tenant-schemas ajouter le chemin de tenant par dessus un self.location qui l'aurait déjà.
Le prototype du code TenantStorageMixin vient de https://github.com/bernardopires/django-tenant-schemas/issues/182 et d'un certain bdauvergne, ça doit être sa faute :)
Sans doute que ça vient de la nécessité à l'époque de tourner encore sur Django 1.8 où location n'était pas une property, et on ne pouvait jouer que dans path().
Sauf que c'est de toute façon le path() de FileSytemStorage qui est appelé, donc ça n'arrive pas.
Et donc ce TenantStorageMixin ne sert à rien du tout pour nous, puisqu'il ne contient que cette méthode path() ... Ça serait mieux de ne pas l'utiliser parce qu'il met une confusion à la lecture, du moins pour quelqu'un comme moi ne se rappelle jamais du MRO Python.
et j'ajoute 0002 pour supprimer la prise en charge de compatibilité avec une ancienne version de django-tenant-schemas.
Deviendrait inutile donc.
(et ça mériterait presque une remontée upstream si django-tenant-schema ne semblait pas un petit peu à l'abandon)
Mis à jour par Frédéric Péters il y a environ 2 ans
Et donc ce TenantStorageMixin ne sert à rien du tout pour nous, puisqu'il ne contient que cette méthode path() ... Ça serait mieux de ne pas l'utiliser parce qu'il met une confusion à la lecture, du moins pour quelqu'un comme moi ne se rappelle jamais du MRO Python.
Il ne sert à rien mais s'il est pas là il y a warning,
if not isinstance(default_storage, TenantStorageMixin):
errors.append(Warning(
"Your default storage engine is not tenant aware.",
hint="Set settings.DEFAULT_FILE_STORAGE to "
"'tenant_schemas.storage.TenantFileSystemStorage'",
id="tenant_schemas.W003"
))
Mis à jour par Thomas Noël il y a environ 2 ans
- Statut changé de Solution proposée à Solution validée
Frédéric Péters a écrit :
Et donc ce TenantStorageMixin ne sert à rien du tout pour nous, puisqu'il ne contient que cette méthode path() ... Ça serait mieux de ne pas l'utiliser parce qu'il met une confusion à la lecture, du moins pour quelqu'un comme moi ne se rappelle jamais du MRO Python.
Il ne sert à rien mais s'il est pas là il y a warning,
Et donc je valide ainsi ! (la solution avec surcharge de self.location)
Mis à jour par Frédéric Péters il y a environ 2 ans
- Statut changé de Solution validée à Résolu (à déployer)
commit 656d02d9705f536e908752ec9236cc66fa577890
Author: Frédéric Péters <fpeters@entrouvert.com>
Date: Fri Apr 8 15:57:50 2022 +0200
trivial: remove support of django-tenant-schemas w/o TenantStorageMixin (#63725)
commit 46b334579f5a22b82353a7d766c6475978d842f1
Author: Frédéric Péters <fpeters@entrouvert.com>
Date: Fri Apr 8 14:41:26 2022 +0200
multitenant: redo storage backend by overriding location property (#63725)
Mis à jour par Thomas Noël il y a environ 2 ans
Et je me suis amusé à soumettre ça à l'upstream, sans trop de conviction https://github.com/bernardopires/django-tenant-schemas/pull/684
Mis à jour par Transition automatique il y a environ 2 ans
- Statut changé de Résolu (à déployer) à Solution déployée
Mis à jour par Benjamin Dauvergne il y a environ 2 ans
Thomas Noël a écrit :
Et je me suis amusé à soumettre ça à l'upstream, sans trop de conviction https://github.com/bernardopires/django-tenant-schemas/pull/684
Le fork activement maintenu est https://github.com/django-tenants/django-tenants
multitenant: redo storage backend by overriding location property (#63725)