Serializer responsability

[sergiowalls]

I can see that serializers in this library are authenticating users. Does this break the Single Responsability Principle? In my project I want to pass the request object to the authentication method but as it is coupled with serialization I can not do it without rewriting the entire TokenObtainSerializer.

[davesque]

Maybe it does. Although the single responsibility principle is more of a guideline than a rule. In practice, it's not possible to rigorously adhere to that principle. In my experience, form and serializer classes usually end up doing more than just serialization tasks and form stuff and it's awkward to avoid that.

You're welcome to try and figure out a (hopefully) simple modification to the library that will enable you to do what you need and submit it as a PR.

[sergiowalls]

Okay, I will make a propose. Thanks for you attention! 😄

[sergiowalls]

#27 I try to make the minimum changes but I add some test to this new "feature"

[davesque]

I'll try and find some time in the next few days to look over this. Mostly looks good at a glance.

[sergiowalls]

Yeep, it's all okay? @davesque

[davesque]

I think it may be possible to use the self.context property on serializer instances which automatically includes references to things like the request and view instances. See here:

http://www.django-rest-framework.org/api-guide/serializers/#including-extra-context

That article talks about including "extra" context. However, I'm pretty sure that a fair amount of information is included in this context attribute by default. Of course, I think you'll still have to create and use a custom serializer.

[liampauling]

I am having issues with 'django axes' due to it requiring the request to authenticate.

@davesque looking through the code is there any reason why the authentication cannot be moved to the JWTAuthentication class? This seems to be standard with the examples in restframework and I would have thought make things cleaner.

[sergiowalls]

@davesque I think that using self.context implicitly is a bad idea as it could imply unexpected behavior for customs requests and views. Simple JWT users may spent some time looking for this behavior if we make it implicit. As said in the Zen of Python

Explicit is better than implicit.

[davesque]

Honestly, this just seems too specific to your particular use case. I suggest you just use this pattern in your project and that we keep it separate from the main fork of simple jwt for now.

[davesque]

As per a discussion in another issue, I'm starting to see more how the way our serializers work is kind of problematic. I'll have to revisit this at a later time because it seems like a bit of work. Ideally, if we were to change this, I think it would involve moving all token logic outside of serializers and into views.

I think I had made the serializers work the way they do because I saw them as performing the task of validating data provided in the request and also producing the object that will be serialized in the response. This sometimes might involve accessing contextual information such as the request object. I think the DRF author Tom Christie also shares this view since he made that kind of information available through the self.context property. Otherwise, it wouldn't be there.

Anyway, we'll have to take another look at this.

[sergiowalls]

Good! I will try to find some time this week to think about it...

[loicgasser]

one way this could be solved is by removing the authentication from the serializer and let users configure what authentication backend they want to use to sign in. adapt the views accordingly and initialize the serializer with the user object instead.

[Andrew-Chen-Wang]

We currently do something with import_module from Django's utils, but that may be inefficient if we're not doing it module level (in case the user wants to do some dynamic authorization). This was done in #279

But I agree with @loicgasser . I don't think we need to stick with how DRF does the validation; it's more like how this package can evolve to be a little more flexible for developers.