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 NullPointerException when response is empty #19

Closed
wants to merge 1 commit into from

Conversation

Malachiasz
Copy link

Fixes the issue reported here: dmfs/oauth2-essentials#78

@dmfs
Copy link
Owner

dmfs commented Jul 8, 2020

Sorry for the delayed response. I've thought about this for quite some time and I've come to the conclusion that this is not the right fix for the original issue. This fix only defers the original crash you reported to a later point in the instruction flow. It doesn't fix it. So merging this change would still leave you with a crash, just with a different stack trace.

The original issue is that a null refresh token was passed to the OAuth2 refresh grant flow and null is never a valid refresh token. A better response would be to introduce an explicit null check and throw an IllegalArgumentException instead. I'm not a big fan of that either, though. If someone likes to use null as valid return types they should be prepared for the possibility of NullPointerExceptions. Also in cases like this it doesn't really make much of a difference.

I've talked to Sebastian and I believe he talked to you. I've submitted a PR which avoids passing null and fixes this crash. He told me you've implemented a similar fix. The real challenge is to find out where the null originated. In the original code there are two potential sources. One of them would be my responsibility and I'll double check whether there could be the reason.
If you doubt this decision I invite you to have a chat with me over this (for instance on Teams, Sebastian can tell you how to reach me via mail).

@dmfs dmfs closed this Jul 8, 2020
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.

2 participants