Local stored refresh token not cleared when refresh token has expired about nhost-dart HOT 15 CLOSED

Another point to note is that this error/exception is not propagated to the client. So the client has no way of knowing something like this has happened under the hood and can take any action

from nhost-dart.

Thanks for the report @pjindal91, and sorry for the wait.

I'll be looking at this shortly.

from nhost-dart.

No problem at all @shyndman. I actually made the fix locally but that logic didn't have any unit tests so I wasn't comfortable pushing it since couldn't tell if it would break some assumptions.

from nhost-dart.

It appears that you are quite correct. This is a bug. Thank you. :)

Another point to note is that this error/exception is not propagated to the client. So the client has no way of knowing something like this has happened under the hood and can take any action

The mechanism that this library uses to notify changes to authentication state is via callbacks (Auth.addAuthStateChangedCallback). Would that suffice, or is there some reason you'd specifically need to know that a refresh token has expired during auto-login?

from nhost-dart.

Yeah notification via callback is fine. Right now major problem is this

from nhost-dart.

Comment about error propagation is so that things like these can be somehow handled by the client. Right now since the exception is swallowed by the library the client doesn't get to know that something wrong has happened. For example if in this case, refresh token expiry error was somehow propagated then the client could have manually cleared the token from the storage/log user out. Since that doesn't exist and combined with this bug, it becomes difficult for client to do anything

from nhost-dart.

Yup, got it.

I see what you're saying about the exception, but technically, this isn't an exceptional case for the library (unless, you know...there's a bug in it). It'd feel weird to be throwing exceptions during expected use cases just so people can work around bugs.

Better to just fix it (and again, sorry about the reply time)

from nhost-dart.

Yeah in this case i agree, however i still feel slightly awkward knowing that there could be an underlying issue which i wouldn't be informed about and can take action. One simple example i can think of is that somehow the server is down/ or internet connection is lost while logging in and in this case, the library would silently fail

Now from application point of view, i would imagine getting to know about it might be useful, maybe the application wants to have some retry logic on its end with backoff. Or maybe the library wants to let the user give more information.

What are your thoughts? Are there other ways library helps handling such issues?

from nhost-dart.

You've convinced me. This is a design flaw. Going over the code, seeing that // Silent failure comment cinches it.

And thinking out loud, calling out to an async method in the Auth constructor is a mistake because it detaches from the context of the caller's stack. Still, there are options, but I need to think about it.

Let me first deal with the simpler issue of auth state and the clearing of the refresh token.

from nhost-dart.

Yeah i think it might be worth opening a separate issue for error management since thats not technically a bug and might even require breaking changes. We can focus on the immediate bug for this issue

from nhost-dart.

Hey there, any ETA on releasing the updated version of the SDK including the fix? Planning a next release currently and would be nice to know if it's worth waiting. Many thanks!

from nhost-dart.

I believe @shyndman has published a new package version with the fix.

from nhost-dart.

Was the fix in there @pjindal91 ? I just saw dependency updates in the changelogs. Happy to be proved wrong:)

from nhost-dart.

@MaxSchilling yeah in 2.0.1. It was after my pr was merged, so it should have that. Try updating your packages and you should get it

from nhost-dart.

Hey @MaxSchilling, just a small clarification here.

We use melos for changelog generation.

When the nhost_sdk package changes, its changelog will include specifics based on git commit subject lines, while all of the dependent packages will get that "Update a dependency..." entry.

Which, admittedly, isn't the best. Maybe some of the updates should trickle down.

from nhost-dart.