feat: Implementing encrypted local storage for user sessions #1191

DrMimik · 2023-03-17T22:37:16Z

New Pull Request Checklist

I am not disclosing a vulnerability.
I am creating this PR in reference to an issue.

Issue Description

User data can be copied on rooted devices.

Closes: #1192

Approach

Encrypting local user session using Jetpack security features to ensure better security for rooted devices.

TODOs before merging

Add tests
Add changes to documentation (guides, repository pages, in-code descriptions)

parse-github-assistant · 2023-03-17T22:37:19Z

Thanks for opening this pull request!

🎉 We are excited about your hands-on contribution!

azlekov

Wow @DrMimik! Many thanks for this contribution. It's highly appreciated! Looks very clean and nice!

I have the feeling that this is something working for a long time somewhere else, correct me if I'm wrong. I'm guessing that because of the copyright comments.

I left just few questions and small javadoc suggestions. I'm wondering @mtrezza if the migration mechanism should be here forever or there is some policy about that. But if @DrMimik say that already migrated stores are covered and there's no performance issue maybe can stay for a while.

azlekov · 2023-03-18T19:39:57Z

parse/src/main/java/com/parse/ParseObjectStoreMigrator.java

+
+import java.util.Arrays;
+
+public class ParseObjectStoreMigrator<T extends ParseObject> implements ParseObjectStore<T> {


You could add some Javadoc with comments.

Does this migrator needs to be here forever or just for about some migration period?

Wow @DrMimik! Many thanks for this contribution. It's highly appreciated! Looks very clean and nice!

I have the feeling that this is something working for a long time somewhere else, correct me if I'm wrong. I'm guessing that because of the copyright comments.

I left just few questions and small javadoc suggestions. I'm wondering @mtrezza if the migration mechanism should be here forever or there is some policy about that. But if @DrMimik say that already migrated stores are covered and there's no performance issue maybe can stay for a while.

Thank you guys for creating this awesome platform, I actuallty just copied the ParseObjectStore's logic for creating a new ObjectStore and used Jetpack's crypto library.

Since Robolectric doesn't mock Android's java.security.KeyStore I copied a couple files from this repo (with their copyright XD ) to create tests using Robolectric.

I am actually in a learning process, so please don't hesitate to edit or comment on anything in the code.

We don't have a specific policy for the duration of providing migration mechanisms. It depends on the type of change. In this case I'd see the mechanism staying for several years, so indefinite at this point. The reason is that it can be a years long process to migrate clientes once they are released to end-users.

I think the migrator should be available for old users until they migrate, or remove it on a major release.

azlekov · 2023-03-18T19:42:21Z

parse/src/main/java/com/parse/EncryptedFileObjectStore.java

+import java.security.GeneralSecurityException;
+import java.util.concurrent.Callable;
+
+public class EncryptedFileObjectStore<T extends ParseObject> implements ParseObjectStore<T> {


Can you add Javadoc on the public classes and their public methods you have introduced?

azlekov · 2023-03-18T19:43:59Z

parse/src/main/java/com/parse/ParseObjectStoreMigrator.java

+        this.legacy = legacy;
+    }
+
+    private static <T extends ParseObject> Task<T> migrate(


Is there a way to check whenever migrate is needed?

If from is deleted and next time you call migrate the onSuccessTask what will happen? It will call again onSuccessTask? Is there chance onFailedTask or something similar to be called in such case?

Thanks for pointing that out, that is actually a bug which is going to be calling from.getAsync() recursively, I'll fix it ASAP.

azlekov · 2023-03-18T19:48:31Z

parse/src/test/java/com/parse/AndroidKeyStoreProvider.kt

+
+        override fun generateKeyPair(): KeyPair = wrapped.generateKeyPair().also { keyPair ->
+            null
+//            AndroidKeyStore.storedKeys[lastSpec!!.keystoreAlias] = KeyStore.PrivateKeyEntry(keyPair.private, arrayOf(keyPair.toCertificate()))


Let's not have commented code.

If this is a TODO or a FIXME or something important for reference later on, please consider adding a descriptive comment instead of deleting it.

azlekov · 2023-03-18T19:50:18Z

parse/src/test/java/com/parse/EncryptedFileObjectStoreTest.java

+    }
+
+    @Test
+    public void testGetAsync() throws Exception {


Can we have a test covering the migrate? Pardon me if is covered already. If it is can you point it to me?

mtrezza · 2023-03-18T21:30:38Z

parse/src/test/java/com/parse/AlgorithmParameterSpecExt.kt

+package com.parse
+
+/*
+ * Copyright 2020 Appmattus Limited


We need to make sure to consider the license implications. This repository is planned to be migrated to Apache 2, we should do that before merging this, otherwise we need to add this as a second license.

mtrezza · 2023-03-18T21:36:13Z

We don't have a specific policy for the duration of providing migration mechanisms. It depends on the type of change. In this case I'd see the mechanism staying for several years, so indefinite at this point. The reason is that it can be a years long process to migrate clientes once they are released to end-users.

Fix: migrate method was called recursively in EncryptedFileObjectStore

azlekov · 2023-05-16T07:52:40Z

@mtrezza @DrMimik I believe here we are on the finish line, what do you think can we get this merged?

rommansabbir · 2023-05-16T09:28:14Z

Hi, pardon me if my question is out of the context. Why we need to Encrypt the user session for Rooted Device? If a device is rooted, the device is compromised. And to decrypt the Session Token, someone have to decrypt the token by using decode method to get JSON payload. If so, then session token is already Encrypted, so why do we need to add an extra security layer for the Session Token?

feat: Implementing encrypted local storage for user sessions

c84466e

mtrezza requested a review from a team March 18, 2023 18:03

azlekov reviewed Mar 18, 2023

View reviewed changes

mtrezza requested changes Mar 18, 2023

View reviewed changes

Add: javadoc to public methods

b446664

Fix: migrate method was called recursively in EncryptedFileObjectStore

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: Implementing encrypted local storage for user sessions #1191

feat: Implementing encrypted local storage for user sessions #1191

DrMimik commented Mar 17, 2023 •

edited

parse-github-assistant bot commented Mar 17, 2023 •

edited

azlekov left a comment •

edited

azlekov Mar 18, 2023

DrMimik Mar 19, 2023 •

edited

DrMimik Mar 19, 2023 •

edited

azlekov Mar 18, 2023

azlekov Mar 18, 2023

azlekov Mar 18, 2023

DrMimik Mar 19, 2023 •

edited

azlekov Mar 18, 2023

azlekov Mar 18, 2023

mtrezza Mar 18, 2023 •

edited

mtrezza commented Mar 18, 2023 •

edited

azlekov commented May 16, 2023

rommansabbir commented May 16, 2023


		import java.util.Arrays;

		public class ParseObjectStoreMigrator<T extends ParseObject> implements ParseObjectStore<T> {

feat: Implementing encrypted local storage for user sessions #1191

Are you sure you want to change the base?

feat: Implementing encrypted local storage for user sessions #1191

Conversation

DrMimik commented Mar 17, 2023 • edited

New Pull Request Checklist

Issue Description

Approach

TODOs before merging

parse-github-assistant bot commented Mar 17, 2023 • edited

Thanks for opening this pull request!

azlekov left a comment • edited

Choose a reason for hiding this comment

azlekov Mar 18, 2023

Choose a reason for hiding this comment

DrMimik Mar 19, 2023 • edited

Choose a reason for hiding this comment

DrMimik Mar 19, 2023 • edited

Choose a reason for hiding this comment

azlekov Mar 18, 2023

Choose a reason for hiding this comment

azlekov Mar 18, 2023

Choose a reason for hiding this comment

azlekov Mar 18, 2023

Choose a reason for hiding this comment

DrMimik Mar 19, 2023 • edited

Choose a reason for hiding this comment

azlekov Mar 18, 2023

Choose a reason for hiding this comment

azlekov Mar 18, 2023

Choose a reason for hiding this comment

mtrezza Mar 18, 2023 • edited

Choose a reason for hiding this comment

mtrezza commented Mar 18, 2023 • edited

azlekov commented May 16, 2023

rommansabbir commented May 16, 2023

DrMimik commented Mar 17, 2023 •

edited

parse-github-assistant bot commented Mar 17, 2023 •

edited

azlekov left a comment •

edited

DrMimik Mar 19, 2023 •

edited

DrMimik Mar 19, 2023 •

edited

DrMimik Mar 19, 2023 •

edited

mtrezza Mar 18, 2023 •

edited

mtrezza commented Mar 18, 2023 •

edited