Android-SingleSignOn icon indicating copy to clipboard operation
Android-SingleSignOn copied to clipboard

Break on minified app

Open p1gp1g opened this issue 3 years ago • 3 comments

When minify is turned on, the SSO doesn't work anymore (cf. https://github.com/UP-NextPush/android/commit/e3ff269a1f85556732f976f5b606b1a545a06b30)

> adb logcat --pid=(adb shell pidof -s com.nextcloud.client)


10-05 20:39:26.040  7489  7515 E InputStreamBinder: Error during Nextcloud request
10-05 20:39:26.040  7489  7515 E InputStreamBinder: java.lang.ClassNotFoundException: h3.c
10-05 20:39:26.040  7489  7515 E InputStreamBinder:     at java.lang.Class.classForName(Native Method)
10-05 20:39:26.040  7489  7515 E InputStreamBinder:     at java.lang.Class.forName(Class.java:454)
10-05 20:39:26.040  7489  7515 E InputStreamBinder:     at java.io.ObjectInputStream.resolveClass(ObjectInputStream.java:683)
10-05 20:39:26.040  7489  7515 E InputStreamBinder:     at java.io.ObjectInputStream.readNonProxyDesc(ObjectInputStream.java:1731)
10-05 20:39:26.040  7489  7515 E InputStreamBinder:     at java.io.ObjectInputStream.readClassDesc(ObjectInputStream.java:1622)
10-05 20:39:26.040  7489  7515 E InputStreamBinder:     at java.io.ObjectInputStream.readOrdinaryObject(ObjectInputStream.java:1900)
10-05 20:39:26.040  7489  7515 E InputStreamBinder:     at java.io.ObjectInputStream.readObject0(ObjectInputStream.java:1440)
10-05 20:39:26.040  7489  7515 E InputStreamBinder:     at java.io.ObjectInputStream.readObject(ObjectInputStream.java:428)
10-05 20:39:26.040  7489  7515 E InputStreamBinder:     at com.nextcloud.android.sso.InputStreamBinder.deserializeObjectAndCloseStream(InputStreamBinder.java:226)
10-05 20:39:26.040  7489  7515 E InputStreamBinder:     at com.nextcloud.android.sso.InputStreamBinder.performNextcloudRequestAndBodyStreamV2(InputStreamBinder.java:127)
10-05 20:39:26.040  7489  7515 E InputStreamBinder:     at com.nextcloud.android.sso.InputStreamBinder.performNextcloudRequestV2(InputStreamBinder.java:111)
10-05 20:39:26.040  7489  7515 E InputStreamBinder:     at com.nextcloud.android.sso.aidl.IInputStreamService$Stub.onTransact(IInputStreamService.java:158)
10-05 20:39:26.040  7489  7515 E InputStreamBinder:     at android.os.Binder.execTransactInternal(Binder.java:1302)
10-05 20:39:26.040  7489  7515 E InputStreamBinder:     at android.os.Binder.execTransact(Binder.java:1265)
10-05 20:39:26.040  7489  7515 E InputStreamBinder: Caused by: java.lang.ClassNotFoundException: h3.c
10-05 20:39:26.040  7489  7515 E InputStreamBinder:     ... 14 more
10-05 20:39:26.057  7489  7553 E com.nextcloud.android.sso.aidl.ParcelFileDescriptorUtil.TransferThread: writing failed: write failed: EPIPE (Broken pipe)
10-05 20:39:26.058  7489  7553 D InputStreamBinder: Done sending result
> adb logcat --pid=(adb shell pidof -s org.unifiedpush.distributor.nextpush)

10-05 20:37:33.584  7094  7094 I RegisterBroadcastReceiver: REGISTER
10-05 20:37:33.585  7094  7094 D i3.h    : NextcloudRetrofitServiceMethod() called with: apiEndpoint = [/index.php/apps/uppush], method = [public abstract n3.c r5.m.b(java.util.Map)]
10-05 20:37:33.585  7094  7094 D i3.h    : invoke call to api using observable class r5.a
10-05 20:37:33.585  7094  7094 D ApiUtils: Subscribed to createApp.
10-05 20:37:33.586  7094  7363 D i3.b    : copy data from service finished
10-05 20:37:33.591  7094  7365 W System.err: j3.j: h3.c
10-05 20:37:33.591  7094  7365 W System.err:    at i3.b.d(SourceFile:113)
10-05 20:37:33.591  7094  7365 W System.err:    at w3.c.c(SourceFile:18)
10-05 20:37:33.591  7094  7365 W System.err:    at n3.c.b(Unknown Source:2)
10-05 20:37:33.591  7094  7365 W System.err:    at w3.d.c(Unknown Source:9)
10-05 20:37:33.591  7094  7365 W System.err:    at n3.c.b(Unknown Source:2)
10-05 20:37:33.591  7094  7365 W System.err:    at w3.f$b.run(Unknown Source:6)
10-05 20:37:33.591  7094  7365 W System.err:    at n3.e$a.run(Unknown Source:9)
10-05 20:37:33.591  7094  7365 W System.err:    at y3.g.run(Unknown Source:13)
10-05 20:37:33.591  7094  7365 W System.err:    at y3.g.call(Unknown Source:0)
10-05 20:37:33.591  7094  7365 W System.err:    at java.util.concurrent.FutureTask.run(FutureTask.java:264)
10-05 20:37:33.591  7094  7365 W System.err:    at java.util.concurrent.ScheduledThreadPoolExecutor$ScheduledFutureTask.run(ScheduledThreadPoolExecutor.java:307)
10-05 20:37:33.591  7094  7365 W System.err:    at java.util.concurrent.ThreadPoolExecutor.runWorker(ThreadPoolExecutor.java:1137)
10-05 20:37:33.591  7094  7365 W System.err:    at java.util.concurrent.ThreadPoolExecutor$Worker.run(ThreadPoolExecutor.java:637)
10-05 20:37:33.591  7094  7365 W System.err:    at java.lang.Thread.run(Thread.java:1012)

p1gp1g avatar Oct 05 '22 19:10 p1gp1g

I assume because the classes have to have the exact same name on both sides (sso / 3rd party app and Nextcloud).

We had similar issues with the Notes app once and stopped our minifying experiments because of it, but i assume if you define exception rules for the sso package, it should work. Feedback welcome 😉

stefan-niedermann avatar Oct 05 '22 19:10 stefan-niedermann

At least I'm not the only one who have seen this issue :smile:

I will probably experiment a bit around it soon.

Nevertheless that information should probably be somewhere in the readme ?

p1gp1g avatar Oct 06 '22 07:10 p1gp1g

Actually, it makes sense in an architectural way - the method and class names are part of an API (between the apps) - minifying one end changes the method and class names to random stuff - a breaking change.

However, I absolutely agree with you, and I propose to add the documentation as soon as

I will probably experiment a bit around it soon.

we know some rules which work :wink: if you share the result with us.

stefan-niedermann avatar Oct 06 '22 08:10 stefan-niedermann

Using the following works

postprocessing {
    removeUnusedCode false
    removeUnusedResources true
    obfuscate false
    optimizeCode true
}

Instead of

minifyEnabled true
shrinkResources true

Note that it might be possible to set removeUnusedCode to true depending on the application.

In that case, using @SerializedName() for all parameters of the models (and the nested classes) - for instance here - will make that library work with obfuscated applications (== minifyEnabled true).

p1gp1g avatar Dec 21 '22 21:12 p1gp1g

Awesome, thanks for the update! I'll test the proposed configuration.

stefan-niedermann avatar Dec 22 '22 07:12 stefan-niedermann

You will test on Notes ? Let me know if removeUnusedCode to true works :)

p1gp1g avatar Dec 22 '22 08:12 p1gp1g

Guy's just to let you know Bookmarks also faild with the similer error.

dasbiswajit avatar Feb 16 '23 12:02 dasbiswajit

This branch https://github.com/Unpublished/Android-SingleSignOn/tree/proguardRules contains proguard consumer rules.

The provided rules work quite good together with -dontobfuscate. If you additionally use android.r8.fullMode=true you need to keep your retrofit and gson related classes, as following for UP-NextPush:

# retrofit
-keep class org.unifiedpush.distributor.nextpush.api.ApiResponse
# gson
-keep class org.unifiedpush.distributor.nextpush.api.SSEResponse { <fields>; }

Unpublished avatar Feb 24 '23 18:02 Unpublished

@Unpublished I don't have much experience with shrinking and / or obfuscating apps (due to issues with Nextcloud SSO I just disabled it and did not think about it again).

Could you please elaborate a bit more what a 3rd party app needs to do? As I understand you, we should

  • Merge your branch into the official SSO lib (would you mind opening a PR for that?)
  • The 3rd party apps should still need to add the flag -dontobfuscate? Where exactly?
  • Maybe we could add a section in the README.md that explains this and your instructions for android.r8.fullMode before merging your branch?

stefan-niedermann avatar Apr 13 '23 19:04 stefan-niedermann

Shrinking apps can definitely get troublesome, let me expand further:

Could you please elaborate a bit more what a 3rd party app needs to do? As I understand you, we should

* Merge your branch into the official SSO lib (would you mind opening a PR for that?)

Yes by declaring them as consumer rules R8/ProGuard automatically picks them up.

Ideally there would be separate modules for sso-rxjava2/3 like in retrofit. Regardless of minification, apps end up with deps they don't use as they probably won't use rxjava version 2 and 3 at same time. Just merging my branch would feel a bit dirty, though it would be better than now. Splitting into modules can always happen afterwards.

* The 3rd party apps should still need to add the flag `-dontobfuscate`? Where exactly?

This goes into the proguard-rules.pro of a 3rd party app like news-android for example. Enabling obfuscation always breaks SSO in my experiments and isn't needed in OSS in my view.

* Maybe we could add a section in the `README.md` that explains this and your instructions for `android.r8.fullMode` before merging your branch?

Sure, I can add a R8/ProGuard section with some references to the README.

Unpublished avatar Apr 13 '23 20:04 Unpublished

@p1gp1g would you mind testing the changes from #545 and do a review? 🙂 🚀

stefan-niedermann avatar Apr 20 '23 05:04 stefan-niedermann

I think this issue can be closed (see https://github.com/nextcloud/Android-SingleSignOn/pull/545).

Feel free to reopen.

stefan-niedermann avatar May 16 '23 11:05 stefan-niedermann