search

parse-community / Parse-SDK-Android

The Android SDK for Parse Platform
https://parseplatform.org/
Other
1.88k stars 739 forks source link

Users are logged out after SDK upgrade #1158

Closed PLR2388 closed 2 years ago

PLR2388 commented 2 years ago

New Issue Checklist

Issue Description

ParseUser.getCurrentUser() becomes null after an update from 1.25.0 to 3.0.0.

Steps to reproduce

Actual Outcome

I'm logout

Expected Outcome

I should be login as before

Environment

Parse Android SDK

Server

Database

Logs

parse-github-assistant[bot] commented 2 years ago

Thanks for opening this issue!

qmetzler-luna commented 2 years ago

After some more testing, the issue seems to happen when updating the lib from 2.1.0 to 3.0.0

mtrezza commented 2 years ago

If you look at the commits, are you able to tell which commit may cause this?

PLR2388 commented 2 years ago

I think the commit that may cause this is this one as the others before 3.0.0 are just renaming or adding documentation 🤔

azlekov commented 2 years ago

In version 3.0.0 a breaking changes was introduced:

The following deprecated methods are removed: Parse.getParseDir() (use getParseCacheDir(String) or getParseFilesDir(String) instead)

Is very likely that internally the user was saved in the old place and after the upgrade the new one is used where the user cannot be found. If that is correct we can think about implementing a proper migration of the user or/and installation. From what I remember, there was actually some kind of migration, but I need to double-check what covers.

Nice catch @qmetzler-luna

mtrezza commented 2 years ago

@L3K0V if you could verify this, we may retroactively add a warning note to the changelog even before we publish a fix; this could be quite a severe issue if all users who update their app get logged out. I'll at least pin this for now.

mman commented 2 years ago

I have done quick debugging on this and it is indeed a serious bug.

This commit https://github.com/parse-community/Parse-SDK-Android/commit/7d0faa38bf3ccc85b715dc61965d5b82103c19a4#diff-c26dc69a408628b0da844de58b3d793a777cc908e5dde1777e2b54744de5d2bdL131 actually changed where the SDK looks up currentUser, currentInstallation, and installationId files (and maybe others, but these three are that my app uses and I can verify).

Previously these were stored under com.your.app.id/app_Parse/.

Newly they are looked up under com.your.app.id/files/com.parse/.

currentUser

currentInstallation

installationId

The not so trivial fix would be to check whether these files exist at their old locations and move them over to new locations before the SDK has a chance to look them up.

mtrezza commented 2 years ago

I agree that this is a serious bug as it effectively logs out every user. Sounds like a rather trivial fix to me. Would you want to open a PR for this?

mman commented 2 years ago

@mtrezza What's your idea of a trivial fix?

If we just revert back to loading at least currentUser, currentInstallation, and installationId from old location, then all apps out in the wild on 3.0.0 - that were forced to re-login will break as they now contain old data in old location and new data in new location. The old data are potentially/very probably not in sync. And I believe that there was some hidden motivation behind shuffling the files around in the app data directory. Right?

So the fix should be more elaborate, essentially trying to see if the file is present at new location, and use that (assuming login happened on 3.0.0), and if it is not present, trying to move previous file from old location to a new location and use that (assuming user was logged in on 1.x or 2.x).

In any case, looking that 3.0.0 was released Nov 21, it's potentially now out in the wild for several months ideal fix would not break anything again.

Your thoughts? Martin

mman commented 2 years ago

@mtrezza More over, currentUser is being now loaded from Parse.getParseFilesDir, whereas currentConfig, currentInstallation, and installationId are loaded from ParsePlugins.getFilesDir. They all if I read the code correctly end up in the same location in the end, but any reason why these are not aligned?

thanks, Martin

azlekov commented 2 years ago

@mman about

And I believe that there was some hidden motivation behind shuffling the files around in the app data directory. Right?

I did the changes because the old Parse.getParseFilesDir was deprecated for a long time back then. This massive update was the perfect place to introduce some breaking changes. Maybe I missed something to align all of them, because I getting just started with the SDK structure, architecture and decisions.

mtrezza commented 2 years ago

First of all, I'm glad we identified this issue. It actually sounds more like a bug than a breaking change, because there is virtually nothing the developer can do except for writing their own migration routine, which is such an internal matter that it should be done by the SDK.

A trivial fix could be:

if oldPath exists {
  if newPath doesn't exist {
    move all content from old path to new path
  }
  delete old path
}

If someone has both paths already then I wouldn't do any migration and just delete the old path.

In any case, we should add a warning note to the affected versions in the changelog (and GitHub releases page) ASAP, every day more may cause more damage. Anyone wants to open a PR?

azlekov commented 2 years ago

I will have no availability this and next 2-3 weeks. Sorry. Only have time to follow discussions and doing some reviews if necessary.

mtrezza commented 2 years ago

@L3K0V If you could do a review that would already help a lot.

@mman Would you be up to write a fix? We'll add a bounty to this for urgency so it's compensated as well.

I've added a note to the GH release & changelog:

⚠️ Warning

This version contains a bug that ignores SDK-internal data that is already stored locally on the client side. This includes for example the Parse SDK session token, so an already logged-in user will be required to log in again. If you are not starting with a new app but are considering upgrading an existing app you may want to skip this version and wait for a fix in a future version. (#1158)

Is that description accurate?

mman commented 2 years ago

@mtrezza I will not be able to look into it in the next few days, perhaps next week depending on what pops up :(

mtrezza commented 2 years ago

Thanks to @rommansabbir who submitted the PR for cache migration to fix this issue. Could anyone try out https://github.com/parse-community/Parse-SDK-Android/pull/1168 to verify that it works?

azlekov commented 2 years ago

https://github.com/parse-community/Parse-SDK-Android/commit/7d0faa38bf3ccc85b715dc61965d5b82103c19a4#diff-6cb14853da236c30db3f14e4005ea40c5c45cb8fcb9fc49541693186620de1dcR60

@mtrezza I remember that there was some migration functionality, but for the push notifications. I started wondering if this should be reverted? But I assumed that who is using the 1.26 and early versions which was introduced for a long long time are already migrated. For me this was a very old legacy migration.

Still, similar but very slim and simplified version of such migration should be used to migrate the user, installation etc. I already posted into the PR

mtrezza commented 2 years ago

Yes, I would remove old migration routines only after a very long time, if they don't hurt being there. Or if we can reasonably assume that the migration should have happened already, because previous SDK versions are not usable anymore (targetSDK version not accepted anymore on Google Play, etc)

mtrezza commented 2 years ago

Note: conceptual conversation about migration is happening in https://github.com/parse-community/Parse-SDK-Android/pull/1168

PLR2388 commented 2 years ago

Btw thank you all :)

mtrezza commented 2 years ago

Thank you for kicking it off by opening the issue ;-)