Fix logout/invalidate bug

[Gemui]

Description

Fix logout/invalidate bug #157 - #84

Token has been added to black list but the problem with decode method

    public function decode(Token $token, $checkBlacklist = true)
    {
        $payloadArray = $this->provider->decode($token->get());

        $payload = $this->payloadFactory
            ->setRefreshFlow($this->refreshFlow)
            ->customClaims($payloadArray)
            ->make();

        if (
            $checkBlacklist &&
            $this->blacklistEnabled &&
            $this->getBlackListExceptionEnabled() &&
            $this->blacklist->has($payload)
        ) {
            throw new TokenBlacklistedException('The token has been blacklisted');
        }

        return $payload;
    }

It call getBlackListExceptionEnabled method that should not be in the first level of blacklist check

After remove this method I can now use logout and invalidate successfully.
The correct using of env key JWT_BLACKLIST_ENABLED is to save in blacklist, and also check if the coming token is in blacklist or not

Also the description about JWT_SHOW_BLACKLIST_EXCEPTION meaning it responsible to put in laravel logs only, not validate blacklist or not

So that Env key should not be in the check of blacklist

Checklist:

• I've added tests for my changes or tests are not applicable
• I've changed documentations or changes are not required
• I've added my changes to CHANGELOG.md

[mfn]

This came from #7 , @Messhias what do you think?

[Messhias]

This came from #7, @Messhias what do you think?

In this case, him turns it on by config file, or the unit tests there's something wrong. But at least here it seems fine. What do you think? Did you get something on your side?

[Gemui]

@Messhias After merge the current pull request, you will not need to enable JWT_SHOW_BLACKLIST_EXCEPTION env key for using invalidate/logout methods .
Just enable JWT_BLACKLIST_ENABLED key that responsible for catch the blacklisted token

[Messhias]

But that's the thing, the config it's for this to be configurable, for my example, I don't want this exception being thrown all the time just because I'm logging out and invalidating the token.

[eschricker]

@Messhias I don't understand why the exception is shown in your logs. If JWT_SHOW_BLACKLIST_EXCEPTION is set to false, the decode function should always return the payload of the token, even if it's blacklisted. My understanding is that this exception is catched in JWT::check() to check if your credentials are valid. If JWT::checkOrFail is used, the exception will not be catched and could be shown in your logs. So probably there is an error in application?

[Messhias]

@Messhias I don't understand why the exception is shown in your logs. If JWT_SHOW_BLACKLIST_EXCEPTION is set to false, the decode function should always return the payload of the token, even if it's blacklisted. My understanding is that this exceptcaughtcatched in JWT::check() to check if your credentials are valid. If JWT::checkOrFail is used, the exception will caughtcatched and could be shown in your logs. So probably there is an erthe ror in application?

No error, it's set to false, if you enter in the forked repository this is a very old issue that was left behind, foreasonreasons sometimes the configuwasn't gotn't get by env file (there wasn't even a test for that).

But if this is giving trouble for aend-usersd users don't worry and let's remove it and I can handle this annoying error by my side without issues.

But anyway the PRneedsll needs changes because it'll be necessary remove the unit tests too otherwise will keep breaking the CI.

[Messhias]

What's the decision about this subject?

[felipefrancioni]

Hi! I have the same problems... when will this fix be merged?

[Messhias]

Hi! I have the same problems... when will this fix be merged?

I'm waiting for @eschricker and @mfn decision about this subject, in my decision my vote is no.

Let's wait for the principal maintainers to give their opinion.

[eschricker]

Hi! I have the same problems... when will this fix be merged?

I'm waiting for @eschricker and @mfn decision about this subject, in my decision my vote is no.

Let's wait for the principal maintainers to give their opinion.

Wouldn't it be easier to set the default value of JWT_SHOW_BLACKLIST_EXCEPTION to true?
Then the unit-tests don't need to be changed and it should work as intended for the majority. What do you think @mfn @Messhias

[Messhias]

Hi! I have the same problems... when will this fix be merged?

I'm waiting for @eschricker and @mfn decision about this subject, in my decision my vote is no.
Let's wait for the principal maintainers to give their opinion.

Wouldn't it be easier to set the default value of JWT_SHOW_BLACKLIST_EXCEPTION to true? Then the unit-tests don't need to be changed and it should work as intended for the majority. What do you think @mfn @Messhias

For me sounds good.

[mfn]

I abstain from specific vote feedback here, I don't feel to understand these parts well enough!

[hms5232]

Merely remove $this->getBlackListExceptionEnabled() will make show_black_list_exception in config/jwt.php meaningless. IMO, If it can return other value or do something else, it will be better and flexible.

public function decode(Token $token, $checkBlacklist = true)
{
    $payloadArray = $this->provider->decode($token->get());

    $payload = $this->payloadFactory
        ->setRefreshFlow($this->refreshFlow)
        ->customClaims($payloadArray)
        ->make();

    if (
        $checkBlacklist &&
        $this->blacklistEnabled &&
        $this->blacklist->has($payload)
    ) {
        // like this
        if ($this->getBlackListExceptionEnabled())
            throw new TokenBlacklistedException('The token has been blacklisted')

        return $something;  // return something difference or do something
    }

    return $payload;
}

[eschricker]

JWT_SHOW_BLACKLIST_EXCEPTION

I will create a separate PR which changes the default value.

@Messhias it would be great, if you could enable the blacklisting exception and investigate where the exception is thrown. Maybe you call checkOrFail instead of check. My understanding of the code is that the blacklisting does not work when the exception is disabled, so you probably have a security issue in your application. Please double check this. If the result is that there is an issue in your application, it would be great if we removed the option the disable the throwing of the blacklist exception.

[eschricker]

JWT_SHOW_BLACKLIST_EXCEPTION

I will create a separate PR which changes the default value.

@Messhias it would be great, if you could enable the blacklisting exception and investigate where the exception is thrown. Maybe you call checkOrFail instead of check. My understanding of the code is that the blacklisting does not work when the exception is disabled, so you probably have a security issue in your application. Please double check this. If the result is that there is an issue in your application, it would be great if we removed the option the disable the throwing of the blacklist exception.

#180

[Messhias]

@eschricker the #180 seems to solve this issue as well, should we close this MR? Or merge it?

[eschricker]

@eschricker the #180 seems to solve this issue as well, should we close this MR? Or merge it?

@Messhias As I said, it would great if you would double check your system, why this exception is thrown. If it is a usage error, then we can use the PR to remove the functionality. If the functionality is needed within the library we can close this PR.

Therefore I would keep the PR open, til you give feedback about your investigation.

[Messhias]

@eschricker the #180 seems to solve this issue as well, should we close this MR? Or merge it?

@Messhias As I said, it would great if you would double check your system, why this exception is thrown. If it is a usage error, then we can use the PR to remove the functionality. If the functionality is needed within the library we can close this PR.

Therefore I would keep the PR open, til you give feedback about your investigation.

We can merge it without issues, I can handle it here on my side.

[eschricker]

Before we merge this, I think more things need to be changed: This is the only place where this is used, so we can remove all the code that affects the Show Blacklist Exception function.

[Messhias]

Before we merge this, I think more things need to be changed: This is the only place where this is used, so we can remove all the code that affects the Show Blacklist Exception function.

Like unit tests for example?

[eschricker]

Before we merge this, I think more things need to be changed: This is the only place where this is used, so we can remove all the code that affects the Show Blacklist Exception function.

Like unit tests for example?

Yes, and stuff like the entry in the config file.

[Messhias]

@Gemui can you take a look at this, please?

[Messhias]

If anyone don't take a look on this for us I'll close this PR in my next round.