Possible Bug: Output fields from DjangoFormMutation

[oliviarves]

In the initialization of the Meta class of a DjangoFormMutation, the output fields are declared similar to the input fields of the mutation, like:

input_fields = fields_for_form(form, only_fields, exclude_fields)
output_fields = fields_for_form(form, only_fields, exclude_fields)

For example, if we have a form for authentication, like the one provided by django:

class AuthenticationForm(forms.Form):
    """
    Base class for authenticating users. Extend this to get a form that accepts
    username/password logins.
    """
    username = UsernameField(
        max_length=254,
        widget=forms.TextInput(attrs={'autofocus': True}),
    )
    password = forms.CharField(
        label=_("Password"),
        strip=False,
        widget=forms.PasswordInput,
    )
    ...

And we link it to a mutation:

class AuthMutation(DjangoFormMutation):
    """
    Mutation to login a user
    """
    class Meta:
        form_class = AuthenticationForm
    ...

generates a mutation that requires a username and a password on the response.

AuthMutationPayload{
    username: String!
    password: String!
    clientMutationId: String
}

Is this right? Is sending back the password to the user secure? I think the output fields should be initialized as an OrderedDict().

[maarcingebala]

I think by default all fields are serialized in the output. If I'm correct you should be able to use exclude_fields in mutation's Meta to exclude the unwanted fields.

[oliviarves]

@maarcingebala Thank you for the answer. When I use exclude_fields I am able to exclude the unwanted fields, but my question is: is it correct to force the output fields to include the input fields?

[maarcingebala]

In your case input and output should probably be different. Input fields should include only username and password as you have it now. In the output of this mutation I would expect data of the authenticated user (in case of succesful authentication) or an error otherwise. Returning the password back sounds rather risky and probably there is no need in the application to do that.

[maarcingebala]

I havent used those FormMutations that are currently in Graphene but I assume that the problem is that the output by default mirrors the input?

[oliviarves]

Exactly, the output fields mirrors the input, which I think it could be fine, but when it comes to cases like this authentication example, it makes me think if is necessary to declare the excluded fields all the time. I just feel like it is not right. That is why I proposing to initialize the output fields as an OrderedDict, but I realize that sometimes it could be useful to automatically include the input fields to the output ones. So, I suposse it is convenient someway.
Anyway, it is just a thought, and I wanted to share it, thinking that maybe it was a typing error or something like that. Please, excuse me if my English is not good.

[oliviarves]

Actually, exclude_fields does remove the fields, but in both input and output fields.

[maarcingebala]

I think in case of authentication you'll be better off implementing the mutation yourself. You could use the AuthenticationForm in the mutate method, populate it with data from the input, do the authentication logic and manually return the output.

To return the same fields in the output as in the input is a fair idea, but in my opinion, it may work well only for simple, CRUD-like mutations such as creating or updating models. To make it more versatile, those form-based mutations should provide easy ways to override both the input and the output.

[maarcingebala]

We tried those form-based mutations in our project before it was merged to master in graphene-django, but it turned out that some parts of our logic had to placed inside the form classes and some other parts in mutate functions. Also, when we had to include or exclude particular fields, we had to do it either at the form level or the mutation Meta-class level. Everything started to become a bit messy and we eventually gave up this approach and came up with our solution - model based mutations. We use it for CRUD-like mutations based on models and for all other cases such as authentication, upload etc we have simple BaseMutations that unify the way we return user errors. Although we reimplemented some logic of model forms, we find this approach more convenient so far.

[jarcoal]

It would be nice to be able to specify output fields as there are many cases where values going out will not match values going in. Even just exposing separate methods like get_input_fields() and get_output_fields() would make it easy to do this.

[stale]

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

[jtratner]

see #933 and #934 where I posted up a fix to split out input and output field management :)