citusdata / django-multitenant

Python/Django support for distributed multi-tenant databases like Postgres+Citus
MIT License
721 stars 119 forks source link

Forms from django-simple-multitenant #35

Closed INovozhilov closed 4 years ago

INovozhilov commented 5 years ago

Hi. I use django form and have to filter my foreign key field by myself and it is not safe and secure. Are you going to make base classes and mixins for forms.ModelForm and maybe some solution for Built-in class-based generic views?

louiseGrandjonc commented 5 years ago

Hi @INovozhilov,

thank you for raising this issue. I think it's a great idea.

Could you show me what you do currently in your forms and views? It would help me in order to fix this issue :)

INovozhilov commented 5 years ago

Of course. Now I have very simple app.

Simple class-based view without form:

class SubscriptionUpdateView(UpdateView):
    model = SubscriptionsType
    fields = ['name', 'price',
              'duration', 'visit_limit']

View with form:

class ClientCreateView(CreateView):
    model = Client
    form_class = ClientForm

Form itself:

class ClientForm(forms.ModelForm):
    class Meta:
        model = Client

        fields = ['name', 'address',
                  'birthday', 'phone_number', 'email_address', 'vk_user_id']

All models have FK to other.

Now, my solution is declare a base class for form and use it instead of ModelForm. But I'm newby in Python and Django and I'm not shure in this solution.

class TenantModelForm(forms.ModelForm):
    def __init__(self, *args, **kwargs):
        super(TenantModelForm, self).__init__(*args, **kwargs)

        tenant = get_current_tenant()
        if tenant:
            for field in self.fields.values():
                if isinstance(field, (forms.ModelChoiceField, forms.ModelMultipleChoiceField,)):
                    # Check if the model being used for the ModelChoiceField has a tenant model field
                    if hasattr(field.queryset.model, 'tenant_id'):
                        # Add filter restricting queryset to values to this tenant only.
                        kwargs = {field.queryset.model.tenant_id: tenant}
                        field.queryset = field.queryset.filter(**kwargs)
rob101 commented 4 years ago

This problem arises because Django uses ._base_manager for form handling and not the default_manager which multitenant subclasses. What about applying a filter to base_manager?

christokritz commented 4 years ago

@rob101 In the Django dos the say "Don’t filter away any results in this type of manager subclass". Do you think there will be unintended side effects if a filter is applied to base_manager in this case?

https://docs.djangoproject.com/en/3.0/topics/db/managers/#don-t-filter-away-any-results-in-this-type-of-manager-subclass

rob101 commented 4 years ago

@christokritz Yes, most likely this would be problematic.

The other issue faced here is that logically any Tenant Model should have null=False applied to the Tenant FK. In the tenant model mixin the save() method is overridden to apply the current tenant, however, because forms call clean() first it will throw a validationError.

rob101 commented 4 years ago

@louiseGrandjonc I understand this project is forked from django-simple-multitenant which is no longer available. However, this fork: https://github.com/phugoid/django-simple-multitenant/blob/master/multitenant/forms.py has a solution; is there any reason you chose not to take it across?

louiseGrandjonc commented 4 years ago

Duplicated in #87