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.

Sign up for GitHub

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

Fix logout/invalidate bug #158

Open
wants to merge 3 commits into
base: main
Choose a base branch
from
Open

Fix logout/invalidate bug #158

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
5fc25f0
remove show_black_list_exception from check token validation
Gemui Jun 20, 2022
a0699b6
add changelog
Gemui Jun 20, 2022
b5ee9ef
Update CHANGELOG.md
Gemui Jun 20, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions CHANGELOG.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,7 @@ You can find and compare releases at the GitHub release page.

### Fixed
- Auth header not ignoring other auth schemes
- Logout/invalidate not working

## [1.4.2]

Expand Down
1 change: 0 additions & 1 deletion src/Manager.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -108,7 +108,6 @@ public function decode(Token $token, $checkBlacklist = true)
if (
$checkBlacklist &&
$this->blacklistEnabled &&
$this->getBlackListExceptionEnabled() &&
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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.

$this->blacklist->has($payload)
) {
throw new TokenBlacklistedException('The token has been blacklisted');
Expand Down