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

Review recommendation for JWT Authentication #5838

Closed
dopry opened this Issue Feb 19, 2018 · 6 comments

Comments

Projects
None yet
4 participants
@dopry
Copy link

commented Feb 19, 2018

I tested out https://github.com/GetBlimp/django-rest-framework-jwt based on reading through http://www.django-rest-framework.org/api-guide/authentication/.

It doesn't seem to be well supported. It implements some sort of sliding token by default without blacklisting. It uses the 'access' token as the 'refresh' token. It's not a workflow I've found with tokens in any standards, which makes me fear that it hasn't been very well vetted for security.

To see some more typical flows for token/claim based auth read up on the specs for OpenID Connect, SAML, or Kerberos. They all use different types of tokens, but the procedures and workflows for handling tokens are well vetted.

I did a review of https://github.com/davesque/django-rest-framework-simplejwt. It's appears actively supported. The workflow uses separate access token and refresh tokens. It does have a sliding token option, and the docs mention it is a less secure albiet convenient approach. It also provides a blacklisting
app. It's a much better implementation.

I think it would be good to remove mentions of https://github.com/GetBlimp/django-rest-framework-jwt, until the maintainers can start addressing issues in the queue and improve their documentation. I think it presents a security risk to new developers who aren't really familiar with JWTs or token/claims based authentication workflows and the risks that come with them.

@carltongibson

This comment has been minimized.

Copy link
Collaborator

commented Feb 21, 2018

Hi @dopry. Could you put a PR together with your suggested changes? This will give us something concrete to review.

@Allan-Nava

This comment was marked as off-topic.

Copy link

commented Apr 19, 2018

Hi @carltongibson ,

I want to use:

class CareerViewSet(viewsets.ModelViewSet):
    #
    authentication_classes  = (JSONWebTokenAuthentication,)
    permission_classes      = ()
    queryset                = Career.objects.all()
    serializer_class        = CareerSerializer

This is my serializer:

class CareerSerializer(serializers.ModelSerializer):
    #
    position        = PositionSerializer(read_only=True)
    category        = GroupTypeSerializer(read_only=True)
    championship    = ChampionshipSerializer(read_only=True)
    id_team         = GroupSerializer(read_only=True)
    #
    class Meta:
        model   = Career
        fields  = ('__all__')
    #
#

But it doesn't work, how can I implemente the JSONWebTokenAuthentication with ModelViewSet?

@carltongibson

This comment has been minimized.

Copy link
Collaborator

commented Apr 19, 2018

The discussion group is the best place to take this discussion and other usage questions. Thanks!

@carltongibson

This comment has been minimized.

Copy link
Collaborator

commented Apr 19, 2018

Ei, sorry. There's an issue here too. Would be happy to review a PR on the docs resolving this.

@Allan-Nava

This comment was marked as off-topic.

Copy link

commented Apr 19, 2018

I am trying to build custom Jsonwebtoken based with django-rest-framework-jwt because I use keycloak.

from rest_framework_jwt.settings        import api_settings
from rest_framework_jwt.compat          import Serializer as JWTSerializer
from keycloak                           import KeycloakOpenID
from keycloak.exceptions                import KeycloakAuthenticationError

class KeyCloakJSONWebTokenSerializer(JWTSerializer):
    """
    Serializer class used to validate a username and password.
    'username' is identified by the custom UserModel.USERNAME_FIELD.
    """
    #
    def __init__(self, *args, **kwargs):
        """
        Dynamically add the USERNAME_FIELD to self.fields.
        """
        super(KeyCloakJSONWebTokenSerializer, self).__init__(*args, **kwargs)
        #
        self.fields[self.username_field]    = serializers.CharField()
        self.fields['password']             = PasswordField(write_only=True)
        #
    #
    @property
    def username_field(self):
        return get_username_field()
    #
    def validate(self, attrs):
        credentials = {
            self.username_field: attrs.get(self.username_field),
            'password': attrs.get('password')
        }
        if all(credentials.values()):
            try: 
                token       = keycloak_openid.token(credentials['username'], credentials['password']) # Get token
            except KeycloakAuthenticationError as e1:
                err = JSONParser().parse(BytesIO(e1.response_body))
                print(err)
                msg = _('Unable to log in with provided credentials.')
                raise serializers.ValidationError(msg)
            except Exception as e:
                print("exception: "+str(e))
                msg = _('Unable to log in with provided credentials.')
                raise serializers.ValidationError(msg)
            #
            userinfo    = keycloak_openid.userinfo(token['access_token']) # Get Userinfo
            user        = get_bossoidc_user(userinfo['sub'])
            #
            if user:
                if not user.is_active:
                    msg = _('User account is disabled.')
                    raise serializers.ValidationError(msg)
                #'payload': jwt_payload_handler(user, token),
                return {
                    'token': token,
                    'user': user
                }
            else:
                msg = _('Unable to log in with provided credentials.')
                raise serializers.ValidationError(msg)
        else:
            msg = _('Must include "{username_field}" and "password".')
            msg = msg.format(username_field=self.username_field)
            raise serializers.ValidationError(msg)
        #
    #
#

And I have the viewset:

class CareerViewSet(viewsets.ModelViewSet):
    #
    authentication_classes  = (KeyCloakJSONWebTokenSerializer,)
    permission_classes      = ()
    queryset                = Career.objects.all()
    serializer_class        = CareerSerializer
    #
    permission_classes_by_action = {
                                    'create': [],
                                    'list': []
                                    }
    #
    def create(self, request, *args, **kwargs):
        return super(CareerViewSet, self).create(request, *args, **kwargs)
    #
    def list(self, request, *args, **kwargs):
        return super(CareerViewSet, self).list(request, *args, **kwargs)
    #
@mscansian

This comment has been minimized.

Copy link
Contributor

commented Aug 22, 2018

We have also fallen in the trap of using GetBlimp/django-rest-framework-jwt because of the recommendation. It looks like the project is abandoned and also it does not work with django-axes since version 4.x.x :(

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.