jazzband / djangorestframework-simplejwt

A JSON Web Token authentication plugin for the Django REST Framework.
https://django-rest-framework-simplejwt.readthedocs.io/
MIT License
3.95k stars 658 forks source link

Refresh token rotation not storing the valid one on Outstanding token table #25

Open jesusjiib opened 6 years ago

jesusjiib commented 6 years ago

Hi!

When _'ROTATE_REFRESHTOKENS': True, and _'BLACKLIST_AFTERROTATION': True, the old refresh token is stored on the Blacklist table, but the new one delivered is not stored on the outstanding token table.

Is that the desired behaviour? Because by that way the new delivered refreshtoken cannot be manually blacklisted since we don't have it anywhere

Thank you for the lib!!

sshishov commented 5 years ago

Also noticed this. It should not be like this.

Igreh commented 5 years ago

Any updates here? bug or feature?

sshishov commented 5 years ago

I fixed it by myself. I am not sure that it will be fixed soon... As the PR is a half of the year old

Igreh commented 5 years ago

I fixed it by myself. I am not sure that it will be fixed soon... As the PR is a half of the year old

Could you provide a patch? I suppose it will speed up issue resolving

boris-savic commented 4 years ago

Hi,

i've also came across this issue and before I try to make pull request can active devs please check if this is the right way to address this issue.

class TokenRefreshPatchedSerializer(serializers.Serializer):
    # Patched TokenRefresh serializer so it
    # stores the new refresh token to the list of Outstanding tokens
    refresh = serializers.CharField()

    def validate(self, attrs):
        refresh = RefreshToken(attrs['refresh'])

        data = {'access': str(refresh.access_token)}

        if api_settings.ROTATE_REFRESH_TOKENS:
            blacklisted_token = None
            if api_settings.BLACKLIST_AFTER_ROTATION:
                try:
                    # Attempt to blacklist the given refresh token
                    blacklisted_token = refresh.blacklist()
                except AttributeError:
                    # If blacklist app not installed, `blacklist` method will
                    # not be present
                    pass

            refresh.set_jti()
            refresh.set_exp()

            # PATCHED - Create Outstanding Token in the db
            if blacklisted_token:
                OutstandingToken.objects.create(
                    user=blacklisted_token.user,
                    jti=refresh.payload['jti'],
                    token=str(refresh),
                    created_at=refresh.current_time,
                    expires_at=datetime_from_epoch(refresh['exp']),
                )

            data['refresh'] = str(refresh)

        return data

So basically in the RefreshSerializer add the needed condition checking and store the new refresh token in the database.

Andrew-Chen-Wang commented 4 years ago

I'm not aware of a PR for this. Additionally, refresh.blacklist() seems to already do this???

@boris-savic Can you put that patch inside the try/except? In that case, you wouldn't need the if blacklisted_token if you just put it underneath the setting of that name. But really... it seems like it's already done.

I've lost a bit of time to properly debug this issue since aps are coming around, so do you mind putting a couple of print statements here: https://github.com/SimpleJWT/django-rest-framework-simplejwt/blob/80c6e7e2f4a40c9bfb8e9b5bc2c6ce895575f157/rest_framework_simplejwt/tokens.py#L194

Just test out if the method is even invoked. I'd advise putting an assertion to make sure the token is found after the OutstandingToken.object.get_or_create.

boris-savic commented 4 years ago

Hi @Andrew-Chen-Wang - when refresh.blacklist() is called it creates new outstanding token if it doesn't exist yet, and blacklists it immediately. While that is correct, the outstanding token gets created only at the moment it gets blacklisted as well.

Ideally a newly issued refresh token (if rotation is enabled) should be placed in the list of outstanding tokens immediately.