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

Jump to bottom

The current SESSION_EXPIRED mechanism is too rigid #67

Closed
zsoltboldizsar opened this issue Apr 22, 2022 · 2 comments · Fixed by #68
Closed

The current SESSION_EXPIRED mechanism is too rigid #67

zsoltboldizsar opened this issue Apr 22, 2022 · 2 comments · Fixed by #68
Assignees
@zsoltboldizsar
Labels
bug Something isn't working

Comments

@zsoltboldizsar
Copy link
Contributor

The current SESSION_EXPIRED mechanism can only be triggered if a retrofit2.HttpException is thrown, therefore when using this library in combination with Retrofit-Error-Parsing, which throws its own custom exception (NetworkException), the SESSION_EXPIRED mechanism simply gets bypassed, resulting in the consumer not being informed about the incident (instead of being logged out, users would remain logged in and being shown errors for any attempt of accessing remote resources).
@zsoltboldizsar zsoltboldizsar added the bug Something isn't working label Apr 22, 2022
@zsoltboldizsar zsoltboldizsar self-assigned this Apr 22, 2022
zsoltboldizsar added a commit that referenced this issue Apr 22, 2022
@zsoltboldizsar
Extend the range of exceptions IsSessionExpiredException accepts
e8f4a2c 
Also expose IsSessionExpiredException so consumers could define
their own evaluator which they in turn could use to set up
the OAuth module itself.
All exceptions are then passed to this evaluator so nothing falls
through the cracks.

#67
zsoltboldizsar added a commit that referenced this issue Apr 22, 2022
@zsoltboldizsar
Increase library version to v1.0.3
c74f0cf 
#67
This was referenced Apr 22, 2022
Merged
Closed
@fknives
Copy link
Contributor

fknives commented Apr 22, 2022

Hey,
Thanks for the report, this is indeed a design flaw and should have been reported and fixed a while back 👍
Even though it can be worked around with the current IsSessionExpiredException and ErrorResponseToExceptionConverter.Factory it makes sense to adjust this library, I completely agree.

However I think since it can be worked around it would be good to keep backward compatibility so existing workarounds don't break, they just become deprecated.
I am going to review the PR now, and see if it can be kept as backward compatible if you are agreeing with my reasoning.

@zsoltboldizsar
Copy link
Contributor Author

Yeah, I've had this on my todo list as well for a good while now, didn't have time to file a PR for it until now. 🙂

I wasn't aware of any workaround, but I'm pretty sure it'll break it as I changed the method signatures in IsSessionExpiredException. Anyway, thanks for taking the time to review it 👍🏼

@fknives fknives mentioned this issue Apr 23, 2022
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@zsoltboldizsar zsoltboldizsar

Labels
bug Something isn't working
Projects
None yet
Milestone
No milestone
2 participants
@zsoltboldizsar @fknives