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

Remove Logged output for rescued JWT exceptions #1610

Merged
merged 4 commits into from
Dec 15, 2022

Conversation

klenotiw
Copy link
Contributor

@klenotiw klenotiw commented Dec 13, 2022

What this PR does

fixes: #1567

We decided to remove this logged output. If we feel like we need it again we should consider putting it in a lower log level like :debug or :info so it would be easier for devs to turn off.

Things to focus on

  1. Do we like this approach?

Checklist

Before submitting the PR, please consider if any of the following are needed:

  • Update CHANGELOG.md if the changes would impact users
  • Update README.md, if appropriate.
  • Update any relevant pages in /docs, if necessary
  • For security fixes, the Disclosure Policy must be followed.

CHANGELOG.md Outdated Show resolved Hide resolved
@klenotiw klenotiw marked this pull request as ready for review December 13, 2022 17:51
Copy link
Contributor

@nelsonwittwer nelsonwittwer left a comment

Choose a reason for hiding this comment

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

Looks good, though do you mind explaining why we needed to remove those tests? I'm surprised they are impacted by removing the logger?

@klenotiw
Copy link
Contributor Author

klenotiw commented Dec 15, 2022

Looks good, though do you mind explaining why we needed to remove those tests? I'm surprised they are impacted by removing the logger?

Well Im not removing the whole test, Im just removing the expectation of an error to be logged.

Also all but two errors are being made by ::JWT which isn't something we raise. The two we do generate are in the validate_payload. I guess we could simplify that method and remove our custom errors?

@klenotiw klenotiw merged commit 613fef7 into main Dec 15, 2022
@klenotiw klenotiw deleted the klenotiw/remove-jwtmiddlewarn-warnings branch December 15, 2022 19:47
@shopify-shipit shopify-shipit bot temporarily deployed to rubygems January 5, 2023 21:04 Inactive
@zzooeeyy zzooeeyy mentioned this pull request Apr 24, 2024
4 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

JWTMiddleware warns/decodes tokens excessively
2 participants