mozilla-mobile / firefoxdata-android Goto Github PK

For our unit tests!

I am getting a crash on Android O (I can add more info as to the exact version).
This is after pulling master and running the default config sample app.
Is there more investigation I could do here to help?

FATAL EXCEPTION: main
                                                                        Process: org.mozilla.sync.example, PID: 4879
                                                                        java.lang.SecurityException: Signature check failed for org.mozilla.sync.example
                                                                            at android.os.Parcel.readException(Parcel.java:1915)
                                                                            at android.os.Parcel.readException(Parcel.java:1861)
                                                                            at com.google.android.gms.common.internal.zzv$zza$zza.zza(Unknown Source:41)
                                                                            at com.google.android.gms.common.internal.zzf.zza(Unknown Source:0)
                                                                            at com.google.android.gms.internal.zzzf$zzc.zzxQ(Unknown Source:0)
                                                                            at com.google.android.gms.internal.zzzf$zzf.run(Unknown Source:0)
                                                                            at java.util.concurrent.Executors$RunnableAdapter.call(Executors.java:457)
                                                                            at java.util.concurrent.FutureTask.run(FutureTask.java:266)
                                                                            at java.util.concurrent.ThreadPoolExecutor.runWorker(ThreadPoolExecutor.java:1162)
                                                                            at java.util.concurrent.ThreadPoolExecutor$Worker.run(ThreadPoolExecutor.java:636)
                                                                            at com.google.android.gms.internal.zzabx.run(Unknown Source:7)
                                                                            at java.lang.Thread.run(Thread.java:764)

Through-out the login process, we request various credentials every time the library user tries to access a sync account:

• Crypto keys
• Collection info
• Sync Token

We could start caching these values, saving the user's time, battery, & data caps.

Note: this could get complicated if we can't determine when some credential expires. For example, we'll have to make a request with an out-of-date cached credential, which will fail, so we'll have to re-retrieve the keys and try the request again and, if there's a failure again, then report to the library user.

Currently, we're caching the email from when the user first created their account.

Two approaches:

1. we have getEmail block until the network request can complete (or fail, in which case we return the cached result)
2. We add a request each time we generate a FirefoxSyncClient (from the login manager).

I think you'll need to make an account/profile request to get the latest email.

Users don't change their email addresses very often so I'm okay leaving this to v2.

When we log out of an account, we have to hit the server to tell it to delete the device so the user won't see it as being signed in & to invalidate the tokens. If this request fails when signOut is called, we don't want the library user to have to keep track of the "am I signed out yet?" state in order to make this call – instead, we should delete local sign-in state (so the user can sign in again) and periodically retry the deletion request in the background until it completes.

This is a bad, overly general bug but we should consider adding more tests once this library is released (I think the tests are good enough to start the release - better done than perfect!). Here are some ideas:

• Ensure static resource calls return expected results from server results -
  hard, have to decrypt results
• Ensure network calls for static calls (e.g. CryptoKeys) gets appropriate result
  from response
• LoginManager.loadStoredSyncClient
• FirefoxSyncClient.getEmail
• FirefoxLoginManager.signOut/isSignedIn
• Additional tests via FirefoxSyncWebViewLoginManagerTest javadoc

In order to use this library, users must 1) enter a username & login and 2) verify their account. I'm concerned that users will cancel the login after they've entered their credentials but before we verify: it'd be great if we could explain to users why this is important.

I filed an issue on the fxa-content-server to update the copy within the web login flow itself but it seems to have stalled. If that doesn't move forward, we should consider implementing our own solution. Ideas:

1. If the user tries to cancel a while we're waiting for verification, we display a dialog: "If you don't verify, you'll miss out on X, Y, & Z! Continue?"
2. Before we display the web page, we write some text explaining what verification can get you

I renamed fxa_data -> download and now I get failures with

Project 'fxa_data' not found in root project 'workspace'.

With the invocation:

Using Command Line: gradle --no-daemon --continue ":fxa_data:testDebugUnitTest" ":download:testDebugUnitTest"

But the fxa_data module isn't displayed in the unit test screen so I can't disable these tests & we permafail.

Solution: I'm going to rename it back, disable the tests, and rename it back back.

Bookmark records can be orphaned from corruption or because the user retrieved a partial list. In the current implementation (FirefoxSyncBookmarks.createRootBookmarksFolder), we add the orphaned bookmarks to the root folder. This is problematic because it does not accurately represent the user's bookmarks and it can change across invocations but it was the simplest to implement to start and requires no intervention from library users, so it's easy to change later.

I think the proper solution (which this issue is for) is to download the specified number of records and then make additional requests to download any missing parent folders (and to do so until all folders are found or some unreasonable number of requests is made). This solves the problems above: the user's bookmarks are accurately represented (to the specified number) and the results will not change for that bookmark set. However, it introduces new problems: there are additional requests and thus more failure points, it can get significantly more records than specified by the library user, and it's more complicated.

Specifically, I'm concerned with:

• DataCollectionResult
• HistoryRecord & friends

Because they're not name-spaced. I'm currently talking to @liuche

I made the request from bintray and am awaiting approval.

I spoke to ckarlof – he mentioned that if you have 3+ consumers of an API and understand their needs, you're in an optimal position to create an API to solve whatever problems they have. I saw a similar quote in a presentation on API by Joshua Bloch where with 1 or 2 consumers, you fit your API solution too closely to their problems, rather than the general problem.

So I think when this library is used more heavily, we should re-evaluate how well we solve everyone's problems.

One concrete idea Chris shared about API building is that you should try to make the API as stateless as possible because it helps maintainability, debugging, & makes your code more easily testable.

One way you can do this is to return the state of the world to the API consumer in some Object (call it a "token") that they may not understand but pass back into future calls with the API – this will make it easier to test (you just mock an Object of the state you desire) rather than trying to advance the state of the system by making a series of method calls (that can change over time). Additionally, you can see the whole state object to know what's going wrong, rather than having to stop in the debugger in each component of the code to evaluate its memory state.

Currently, we have the user download the primary module and its dependencies (something like):

    compile(group: 'org.mozilla.sync', name: 'accounts', version: '0.0.1', ext: 'aar', classifier: '')
    compile(group: 'org.mozilla.sync', name: 'fennec', version: '0.0.1', ext: 'aar', classifier: '')
    compile(group: 'org.mozilla.sync', name: 'thirdparty', version: '0.0.1', ext: 'aar', classifier: '')

This is a side-effect to the way we publish resources because I couldn't figure out the right way (for more details, see ./publish.gradle). Instead, we want:

    compile 'org.mozilla.sync:accounts:0.0.1'

Ideally, we also ensure the dependencies are not visible to the application using our library. This works when the dependency is external (e.g. adding picasso to the accounts/ module will not expose it to the application) so it should be possible.

It was in there by default and now the style for our LoginActivity relies on it. It'd be great to not force the implementing application to have more methods than they really need.

At the very least, we should make the version required more lenient so the application won't have to download two versions of the support library.

With this change, we may also need to drop the use of our support library annotations.

We're waiting on mozilla/fxa-auth-server#1889 which we expect to land in ~3 weeks (7/13?).

We might be able to test this on Nightly (see the bug).

See title.

httpclientandroidlib is undesirable because it is:

• out-dated: has verbose, unintuitive APIs
• imported directly into our code (rather than being used as a gradle library)
• Wasn't written with Android in mind (e.g. has high method count)

But we're using it because the existing sync code wraps around it. By removing it, we win:

• simpler request code
• reduced method count

We should use the Android built-in (HttpURLConnection) or another library (OkHttp?).

Note: the existing sync code forces uses unnecessary delegates for blocking calls, convolution the code - we sure be sure to remove the delegates when we rewrite the requests (see also #3).

It'd be great if we could publish & link to our javadocs. Note that we have some invalid javadoc (e.g. I used & instead of the HTML equivalent) so this might be a bit a work.

We forced portrait so we don't have to handle state restoration because it's non-trivial:

• The WebView does not restore the text in input fields (and that's all this WebView is: a form)
• We can't back to the "needs verification screen", if we were in that state.

However, if the Activity is destroyed, e.g. from low memory which can easily happen when the user switches apps to verify their email, we do lose the web view state, which sucks for users - this issue is to restore that state. It may require some page content/server changes.

As of January 1 2019, Mozilla requires that all GitHub projects include this CODE_OF_CONDUCT.md file in the project root. The file has two parts:

1. Required Text - All text under the headings Community Participation Guidelines and How to Report, are required, and should not be altered.
2. Optional Text - The Project Specific Etiquette heading provides a space to speak more specifically about ways people can work effectively and inclusively together. Some examples of those can be found on the Firefox Debugger project, and Common Voice. (The optional part is commented out in the raw template file, and will not be visible until you modify and uncomment that part.)

If you have any questions about this file, or Code of Conduct policies and procedures, please see Mozilla-GitHub-Standards or email [email protected].

(Message COC001)

We're waiting on mozilla/fxa-content-server#5029 which should land around 7/5.

This data is the user's email, uid, and FxA keys (i.e. not sync - those need to be requested for). These keys will allow a user to modify FxA account data & request Sync credentials (to then access/modify that data).

Right now, we store the data in SharedPreferences, which is unencrypted. This is acceptable (and is what Fennec on Android does) because each app's data storage is protected from one another - each app runs as its own user and an app's files are saved with the user the app runs as. However, Android storage is vulnerable to the following:

• afaik, apps on rooted devices run as root and can read any files
• An unencrypted device can have its storage mounted on any computer and read from

These could be protected against if we encrypted the data ourselves. On iOS, we use the keychain (which uses the user's pin as the decryption key) but there is no easy equivalent on Android (since it has no common key) so I opted not to implement it for v1. That being said, if we can find a key (maybe the server would provide it?), we'd probably use the KeyStore class.

The HAWK auth header that we generate requires a timestamp: if this timestamp is too out-of-sync with the server, the request fails! Given my emulator's time is wildly inaccurate (-15 hours & 48 minutes) and I can still make requests, I think this skew may be relative to the time we initially created the account (as opposed to the absolute time the server has) so it's not necessary to handle for v1.

The server will return a timestamp header on certain requests so we can take that value to adjust our current clock skew. There is an class from the existing Sync code, SkewHandler, that can help with this.

JCenter is used by default in Android applications but Maven Central is the old default – we should try to support both.

I'm currently engaged in email conversations to resolve this.

We include a lot of third-party code and maybe some code that uses reflection (due to old sync being in a separate repository) – we should make sure we know the required proguard config for this code.

It'd provide a better user experience than forcing the user to hit back.

Due to the need to handle Activity restoration from Android, this is non-trivial: you'll need to post a message to a thread (presumably, the UiThread so we don't have to waste resources creating another one) but what happens to that Runnable/message when the Activity is recreated on rotation? How do you handle the Runnable in onPause/OnResume (or is it onStop/onStart?)? Do you remove it and re-add it?

In any case, I'd recommend encapsulating the timeout state in some composable Object rather than trying to embed it directly in the Activity code.

For some reason, some of the sync code uses "delegates", making you think they're async, but they're actually just synchronous calls that call out to an object. These delegates unnecessarily make the code really hard to read (and make you think it's async!).

Instead, these methods should add some code to steal the return value from the delegate to.

The methods (which might get renamed by the time this is implemented) that need work are:

• FirefoxSyncCryptoKeysAccessor.getBlocking
• FirefoxSyncCollectionInfoAccessor.getBlocking
• FirefoxSyncTokenAccessor.getBlocking - this is super fucked up: It blocks for the request and calls the callback on a separate thread
• All the get collection calls:
  • FirefoxSyncHistory.getBlocking
  • FirefoxSyncPasswords.getBlocking
  • FirefoxSyncBookmarks.getBlocking

The reason the delegates might exist are:

1. To provide additional input arguments (e.g. collectionTimeout()). But this can be done with normal function arguments
2. To call different methods on different error cases: but this can be done by throwing different types of checked exceptions (e.g. RequestTransportException for the onHandleTransportException call).

Note that when grabbing the various exceptions from the delegate, we should be sure to preserve the type of the exceptions when we throw them.

A good time to rewrite this code would be when implementing #4.

Currently, the code does not intervene when back is pressed: I assumed that this would set Activity.RESULT_CANCELED in the returned intent: instead, a null Intent is given to onActivityResult, which we determine is not ours (it's null!) and return. Either we need to:

1. Set RESULT_CANCELED explicitly to create the intent
2. Return an intent with a different result code (e.g. if RESULT_CANCELED automatically makes the intent null).

On second thought, we may need to explicitly create the intent so we can set the appropriate request codes too (if that's not done automatically).