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

fix(auth): revert to using decode instead verify for jwt #227

Merged
merged 1 commit into from
Dec 30, 2022

Conversation

dpopp07
Copy link
Member

@dpopp07 dpopp07 commented Dec 30, 2022

The move to verify was a bit shortsighted. It requires a public key or secret as an argument to verify the signed token. Typical usage of IBM's IAM service doesn't lend itself to this flow and the tokens returned don't have valid JWT signatures. This led to a runtime error everytime the core made an IAM request - a bit of a showstopping bug.

Additionally, we never had a goal of performing client-side validation of these tokens, we only decode them to determine the expiration time for usage in our refresh logic. The decode method is perfectly sufficient for that and indeed is called within the verify method anyways. Perhaps this flow will change in the future but this is all we need for now.

This reverts the logic to what we were using before but still using the safe version of the dependency. The new version makes the decode function read-only so I had to adjust our approach to mocking in the unit tests.

The move to `verify` was a bit shortsighted. It requires a public key or secret as an
argument to verify the signed token. Typical usage of IBM's IAM service doesn't lend
itself to this flow and the tokens returned don't have valid JWT signatures. This led
to a runtime error everytime the core made an IAM request - a bit of a showstopping bug.

Additionally, we never had a goal of performing client-side validation of these tokens,
we only decode them to determine the expiration time for usage in our refresh logic.
The `decode` method is perfectly sufficient for that and indeed is called within the
`verify` method anyways. Perhaps this flow will change in the future but this is all
we need for now.

This reverts the logic to what we were using before but still using the safe version
of the dependency. The new version makes the `decode` function read-only so I had to
adjust our approach to mocking in the unit tests.

Signed-off-by: Dustin Popp <dpopp07@gmail.com>
Copy link
Member

@apaparazzi0329 apaparazzi0329 left a comment

Choose a reason for hiding this comment

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

Sorry this is also on me. I pulled in the wrong changes when testing this with the node-sdk and I can confirm that I get the aforementioned error. This reversion should go in asap

@dpopp07 dpopp07 merged commit cf3d641 into main Dec 30, 2022
@dpopp07 dpopp07 deleted the dp/fix-regressions branch December 30, 2022 18:12
ibm-devx-sdk pushed a commit that referenced this pull request Dec 30, 2022
## [4.0.2](v4.0.1...v4.0.2) (2022-12-30)

### Bug Fixes

* **auth:** revert to using decode instead verify for jwt ([#227](#227)) ([cf3d641](cf3d641))
@ibm-devx-sdk
Copy link

🎉 This PR is included in version 4.0.2 🎉

The release is available on:

Your semantic-release bot 📦🚀

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants