Users are logged out after SDK upgrade

[PLR2388]

New Issue Checklist

• I am not disclosing a vulnerability.
• I am not just asking a question.
• I have searched through existing issues.
• I can reproduce the issue with the latest version of Parse Server and the Parse Android SDK.

Issue Description

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

Steps to reproduce

• Log in to an app with 1.25.0
• Update your app with parse 3.0.0
• See that you're logout

Actual Outcome

I'm logout

Expected Outcome

I should be login as before

Environment

Parse Android SDK

• SDK version: 3.0.0
• Operating system version: API 30

Server

• Parse Server version: 4.10.4 (I tried in a staging environment with 5.2.0 too)
• Operating system: Ubuntu 20.04.2 LTS
• Local or remote host (AWS, Azure, Google Cloud, Heroku, Digital Ocean, etc): Remote host

Database

• System (MongoDB or Postgres): MongoDB
• Database version: 4.4.13
• Local or remote host (MongoDB Atlas, mLab, AWS, Azure, Google Cloud, etc): MongoDB Atlas

Logs

[parse-github-assistant]

Thanks for opening this issue!

• 🚀 You can help us to fix this issue faster by opening a pull request with a failing test. See our Contribution Guide for how to make a pull request, or read our New Contributor's Guide if this is your first time contributing.

[qmetzler-luna]

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

[mtrezza]

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

[PLR2388]

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]

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]

@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]

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

This commit 7d0faa3#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

• 2.1.0:

  Parse-SDK-Android/parse/src/main/java/com/parse/ParseCorePlugins.java

  Line 131 in b065eff

  File file = new File(Parse.getParseDir(), FILENAME_CURRENT_USER);

• 3.0.0:

  Parse-SDK-Android/parse/src/main/java/com/parse/ParseCorePlugins.java

  Line 131 in fd1d1da

  File file = new File(Parse.getParseFilesDir(), FILENAME_CURRENT_USER);

currentInstallation

• 2.1.0:

  Parse-SDK-Android/parse/src/main/java/com/parse/ParseCorePlugins.java

  Line 280 in b065eff

  File file = new File(ParsePlugins.get().getParseDir(), FILENAME_CURRENT_INSTALLATION);

• 3.0.0:

  Parse-SDK-Android/parse/src/main/java/com/parse/ParseCorePlugins.java

  Line 280 in fd1d1da

  File file = new File(ParsePlugins.get().getFilesDir(), FILENAME_CURRENT_INSTALLATION);

installationId

• 2.1.0:

  Parse-SDK-Android/parse/src/main/java/com/parse/ParsePlugins.java

  Line 173 in b065eff

  new InstallationId(new File(getParseDir(), INSTALLATION_ID_LOCATION));

• 3.0.0:

  Parse-SDK-Android/parse/src/main/java/com/parse/ParsePlugins.java

  Line 172 in fd1d1da

  new InstallationId(new File(getFilesDir(), INSTALLATION_ID_LOCATION));

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]

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]

@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]

@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]

@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]

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]

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]

@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]

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

[mtrezza]

Thanks to @rommansabbir who submitted the PR for cache migration to fix this issue. Could anyone try out #1168 to verify that it works?

[azlekov]

7d0faa3#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]

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]

Note: conceptual conversation about migration is happening in #1168

[PLR2388]

Btw thank you all :)

[mtrezza]

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