-
Notifications
You must be signed in to change notification settings - Fork 10
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
Shortcomings and issues that prevent easy migration from Firebase #53
Comments
Thanks for the detailed feedback @coderaper. I will address them below:
For HTTP requests, you can use our interceptors. For non http request, you can fetch the access token using the
Fair point. We will work on this when we have the time.
You have two options here:
|
The Firebase based applications I ported were built around a globally accessible, automatically updated User (account) object. If this user object was in a signed-in state, then all the tokens it contained were valid and could be passed to the backends. If the User object was not in a sign-in state, for example due to an initiated sign-out, a communication failure with the authentication server, etc., the application user was automatically redirected from its current location to the sign-in page. This User object was also visualised on the profile management pages, meaning that it acted as a cache and therefore no calls were made to the authentication backend unless account referesn or some account management operations were initiated. I have found that many other applications follow a similar architecture. I wanted to keep this architecture to minimise the changes when porting to Supertokens and even to make the authentication backends interchangeable. That's why my initial thought was to replace the access to the access tokens of the User object with a call to getAccessToken() which is exactly what you suggest. After analyzing what getAccessToken() does, I have decided not to go down this road. There are several reasons for this:
That's why I finally came up with the solution of not using getAccessToken(), but instead implementing a wrapper that takes care of providing and maintaining an automatically updated user (account) object. This also motivated me to give you my feedback. There may be other Supertoken users who use non-HTTP requests to their backends and would benefit from a more flexible API. |
Appreciate the detailed feedback here. Here are my thoughts:
If you are using cookie based auth (which is default for web), there is no way that the frontend has access to the access token, which is why it returns
Well, in this case, you can extend the access token lifetime. Or, if you want, you can have your own setInterval somewhere which calls
I believe that a failure in refreshing the session cause of network issues should cause the getAccessToken function call to throw an error. Are you observing some of other behaviour?
Im glad you got a solution working. Do you mind sharing the relevant code snippet so I can see how we can improve our SDK? Thanks. |
I assume you have a typo in your answer and you mean SuperTokens.doesSessionExist() and SuperTokens.attemptRefreshingSession() instead of Session.doesSessionExist() and Session.attemptRefreshingSession(). I do not see much sense in calling SuperTokens.doesSessionExist() right before calling getAccessToken(), first because I can still get null due to race conditioning, and second because it does not solve 2, 3, and 4. What I am currently doing is to periodically call SuperTokens.attemptRefreshingSession() just before the access token expires and then call SuperTokens.getAccessToken() to get the refreshed token. What I am missing are public API functions that can give me information and control over the session state. What I mean here is that if I call SuperTokens.doesSessionExist() after I restarting an application with an active session, it would return false even if there was a session at the moment the application was closed and there is a refresh token stored in the Shared Preferences. In fact, if I call SuperTokens.attemptRefreshingSession() anyway, the session would be restored. So I think that doesSessionExist() should probably be renamed to doesActiveSessionExist() to better reflect it's purpose. We also need an additional function which I'll tentatively call doesPersistedRefreshTokenExist(), which should return true if there is a refresh token stored in the Shared Preferences that can be used to resume/refresh the session and false otherwise. I also think that unconditionally storing the refresh token in the Shared Preferences is a bad idea from a security point of view, especially considering that this storage is not encrypted. It should only be done if the user explicitly agrees. In most of the applications including ours there is an option in the user profile typically called "Keep me signed in on this device". What we expect is that if keepMeSignedIn is off than the app would not persist the refresh tokens and when restarted it would prompt the user to sign in. Of course, this does not imply that the app would not refresh the session unless the app was not closed. Whether the application would refresh the session or let it expire (if the user does not respond to the notification) depends on the particular use case. The current SuperTokens implementation does not have a keepMeSignedIn like input argument, and it always stores both the refresh and access tokens on the device. Although I have managed to implement keepMeSignedIn so that it works as expected from the point of view of the unsuspecting observer, in reality this creates a potential vulnerability. This is a huge step back from the Firebase version where I had control over whether and where to store the refresh token and where the access token was never persisted on the device. Is it there really any reason to store the access token in the Shared Preferences? It would be nice if you stopped persisting it at all and just kept it only in the memory. Another session control function we need is a one that deletes the currently persisted refresh token from the Shared Preferences. I'll call it deletePersistedRefreshToken() for now. The need for this function arises, for example, when you change the value of keepMeSignedIn from On to Off, or select a different SuperTokens server. Such use case arises when we have a development, staging and production implementation of our servers, each with its own dedicated authentication server. If we summarise my main wishes, they are:
Meanwhile you can only call SuperTokens.init() once. This is a problem if we want to implement a dialogue in the application that allows us to change the endpoint of the SuperTokens server without restarting. I have managed to overcome this problem by abusing the existing API -- my init() wrapper resets the SuperTokens.isInitCalled flag before calling SuperTokens.init(). Of course, there is no guarantee that my solution will work after the next update of the supertokens_flutter package. It could be that someone decides to increase the encapsulation and changes SuperTokens.isInitCalled from a public variable to a getter, or hides it altogether.
No, because I always refresh the session just before it expires. If the refresh fails, then the user enters Unauthenticated or Reauthenticable state and he is redirected to the corresponding pages, therefore he is not able to call the backend API and he does not need to prepare access token. |
What's the race condition?
I don't think that's true. This function returns true if the front-token exists in storage, and that exists as long as the refresh token exists.
I agree. We can consider allowing users to add a storage plugin for now which will allow them to store the tokens wherever they like.
It makes sense. I believe this would be solved when we allow users to specify their own storage for tokens. You could, for example, keep the session tokens in memory and that way on app close, the user would be logged out.
How would this be different from the current signOut function we have? That calls an API to delete the session from the server, as well as from the device storage.
Right. But if you did not do this background refresh and let our sdk do a refresh if needed when doesSessionExist is called, does it throw in case the refresh API fails? If not, this is a bug and we will fix it. |
What I mean here is this. Suppose I have already lost the connection to the authentication server. I first call doesSessionExist(), which fetches the cached FrontToken, checks that it has not expired and returns true. Encouraged, I call getAccessToken(), which internally calls doesSessionExist() again. Now there is a non-zero probability that this time the session expiration check ...
if (accessTokenExpiry != null && accessTokenExpiry < now) {
...
}
... will fail (because some time has elapsed), but due to the lost connection the session cannot be renewed. In the end, I may still get null from the getAccessToken() call, even though I have just checked that a session exists.
My bad here. I have already forgotten what the actual reason was that made me decide to stop using the doesSessionExist() function. It was actually the other way round. From the name, I initially assumed that this was a purely informational function that would return true if there was an active session or a persisted refresh token that could be used to resume the session. What I didn't expect is that this function has a side effect, and that side effect is to restore a non-active but persisted session. This turned out to be quite an obstacle because I lost control over when and on which page I wanted to resume the inactive but persisted session. That's because I want to show the user what's going on, display appropriate text, show a busy indicator, etc. I hope this explains why I need a pure getter function that has no side effects. In my previous comment I tentatively called this function doesPersistedRefreshTokenExist(). From my point of view, doesSessionExist() is actually equivalent to calling attemptRefreshingSession() while catching any exceptions and finally returning a simple boolean result indicating success or failure.
I agree. If the application developer has control on this whether and where to store the tokens than there is no need to provide any support for keepMeSignedIn on the SuperTokens side.
The difference is that in the case of sign out, the user is immediately kicked out of the program. In the case of setting keepMeSignedIn to Off (i.e. do not persist the refresh tokens anymore) and deleting the last persisted refresh token, the user can continue working until he closes the program. At least this was the behavior of our applications when using Firebase based authentication. Now I am looking for a way to achieve equivalent behavior with SuperTokens based authentication, which seems difficult if I as a developer do not have control over whether the tokens are persisted. My current workaround is to maintain additional flags in the application's local DB that would indicate whether or not to ignore the always-persisted refresh tokens. I am not happy about this, because it gives the users the false impression that their tokens are not stored on the particular device, when in fact they are stored in a plain text format. (Of course, the tokens are deleted if the user signs out before closing the application.)
My test showed that doesSessionExist() it doesn't throw when there is no connection to the authentication server, but this doesn't matter to me because, as I have already wrote above, I have stopped using it because of the side effect it exhibits. |
Thanks for the feedback. We will investigate some of the issues you pointed out when we get the time :) |
I am porting our Flutter multiplatform applications (linux, web, android) from firebase to supertokens based authentication. During the process of porting I found several problems and was forced to sacrifice some of the existing user account management functionality. Here I have tried to summarize the difficulties I encountered in the hope that some of them will be addressed by the supertokens_flutter team in the future.
In order for the supertokens_flutter package to be useful for use cases similar to ours, the current API needs to be extended to allow subscribing to authentication state changes as well as user profile and access token changes, similar to what the Firebase Auth API for Flutter provides.
The supertokens_flutter package uses shared preferences to persist some sensitive data, but our applications use a different storage implementation. It would be nice if a storage API was defined so that people could interface their own storage implementation. That way, their applications can store their preferences in a single store, which could eventually implement ecryption.
The timeJoined field is only included in responses to signup or signin calls. If the applications just resume the authentication session on startup instead of forcing the user to re-signin, then there seems to be no way to display this value on the user profile page. The result of the getAccessTokenPayloadSecurely() function call does not contain timeJoined, and I have not been able to find any other Supertokens API call besides signup and signin that would allow me to get the user's timeJoined property.
Unlike Firebase authentication, there is no out-of-the-box support for the displayName of the user account. see firebase get user data API call
Unlike Firebase authentication, there is no out-of-the-box support for the passwordUpdatedAt of the user account. see firebase get user data API call
Unlike Firebase authentication, there is no out-of-the-box support for the lastLoginAt of the user account. see firebase get user data API call
Unlike Firebase authentication, there is no out-of-the-box support for the photoUrl of the user account. see firebase get user data API call
As you can see from the screenshot of the Firebase powered user profile page of our apps, most of the information and functionality cannot be easily implemented with Supertokens.
The text was updated successfully, but these errors were encountered: