Proper multi-user support

[infinity0]

Fixes #1192.

We copy some code from LSPosed to expose the IPackageManager hidden android API, which in combination with pm grant $pkg INTERACT_ACROSS_USERS allows us to directly query all packages of all users through the Android API, which unlike pm list packages also allows us to get the icons and the application labels (common names) as well.

(previous draft description deleted in favour of this up-to-date description)

[infinity0]

Looks like LSPosed uses a different approach https://github.com/LSPosed/LSPosed/blob/master/daemon/src/main/java/org/lsposed/lspd/service/PackageService.java#L142 by using the internal IPackageManager instead of the regular Android API PackageManager. Will take a more detailed look in the upcoming days.

[infinity0]

@ukanth This is ready. I've commented out the parts that test for G.supportDual as I'm not sure how it interacts with multi-user behaviour, but it's a simple case of uncommenting them if that's preferred.

[ukanth]

Thanks for the PR. I will review it since it has many changes (actually lot of them in strings.xml and hiddenapi)

May I know the reason if using LSPosed related code here ? If we include that, I may not be able to publish on playstore.

[infinity0]

it has many changes (actually lot of them in strings.xml and hiddenapi)

The first commit is the important one and it is not too big. The strings.xml stuff is just editing some translations.

May I know the reason if using LSPosed related code here ?

Using LSPosed code (hiddenapi) is necessary to get the icons and labels (common names) of apps that are not installed on the current user. This is not easily available even if we use pm list packages with su/libsu. It is technically possible with su/libsu if you read the apks directly from the filesystem, but that would be incredibly tedious.

hiddenapi can be reduced in size, we're not using all of it. To keep things simple I just cp -a the whole thing for now.

Not sure about the play store policy though, that would indeed be annoying...

[infinity0]

Example screenshot here, click to expand. The visual duplication is a bit ugly but I don't have any better ideas, and at least the functionality works...

[ukanth]

Thank for the reference. Can we make LSPosed related code as lib rather than integrate directly into AFWall+ ? I may have to create separate flavor of the application for github/opensource version. Because I doubt it can be integrated and published on playstore.

Also on the visual part, we can group each user applications as filters (same logic of separating user/system/core apps) and profiles.

[infinity0]

Can we make LSPosed related code as lib rather than integrate directly into AFWall+

Most of the code is already external to the AFWall app:

• it links in a public LSPosed library which is already published as an external gradle dependency "org.lsposed.hiddenapibypass:hiddenapibypass:4.3"
• it copies in a private LSPosed library which they don't publish outside of that repo

The only thing that is inside the AFWall app is:

• I added an additional class dev.ukanth.ufirewall.MultiUser, that uses the 2 libraries above. It is only used in the following places:

app/src/main/java/dev/ukanth/ufirewall/Api.java
1423:                installed = MultiUser.getInstalledPackagesFromAllUsers(MultiUser.MATCH_ALL_METADATA);
1452:                int user_id = MultiUser.applicationUserId(apinfo);

app/src/main/java/dev/ukanth/ufirewall/MainActivity.java
189:        MultiUser.setupPermissions();

This file could in theory be moved into a separate library. However, whatever mechanism we use for conditional compilation ("separate flavour"), could just also ignore the whole MultiUser.java file, this would perhaps be simpler than moving it out of the AFWall app into a separate library just for this one file.

Also on the visual part, we can group each user applications as filters (same logic of separating user/system/core apps) and profiles.

I'm not quite sure what you mean here, an example would be good. In any case I suggest this be done later in another PR.

[infinity0]

Because I doubt it can be integrated and published on playstore.

Are you able to check for sure? From what I understand the LSposed stuff I added is just ordinary java glue code that exposes some internal Android APIs, and doesn't modify the system. It doesn't break anything in terms of security policies or make the device more insecure - if you use those internal APIs without root you just get SecurityException thrown. (This is why in AFWall we also need pm grant via su.) So Play Store might be OK with it.

The LSposed stuff that does modify your system in the low-level security sense, is not part of this PR, and we don't need that.

[ukanth]

Can we make LSPosed related code as lib rather than integrate directly into AFWall+

Most of the code is already external to the AFWall app:

* it links in a public [LSPosed library](https://github.com/LSPosed/AndroidHiddenApiBypass) which is already published as an external gradle dependency "org.lsposed.hiddenapibypass:hiddenapibypass:4.3"

* it copies in a private [LSPosed library](https://github.com/LSPosed/LSPosed/tree/master/hiddenapi/stubs) which they don't publish outside of that repo

The only thing that is inside the AFWall app is:

* I added an additional class `dev.ukanth.ufirewall.MultiUser`, that uses the 2 libraries above. It is only used in the following places:

app/src/main/java/dev/ukanth/ufirewall/Api.java
1423:                installed = MultiUser.getInstalledPackagesFromAllUsers(MultiUser.MATCH_ALL_METADATA);
1452:                int user_id = MultiUser.applicationUserId(apinfo);

app/src/main/java/dev/ukanth/ufirewall/MainActivity.java
189:        MultiUser.setupPermissions();

This file could in theory be moved into a separate library. However, whatever mechanism we use for conditional compilation ("separate flavour"), could just also ignore the whole MultiUser.java file, this would perhaps be simpler than moving it out of the AFWall app into a separate library just for this one file.

Also on the visual part, we can group each user applications as filters (same logic of separating user/system/core apps) and profiles.

I'm not quite sure what you mean here, an example would be good. In any case I suggest this be done later in another PR.

I am talking about this code inside AFWall source

[infinity0]

Yes, that's already a separate private library. It's separate from the AFWall app. It's not a published library in the sense of a jar file that can be downloaded like the other dependencies, it can only be used if it's somewhere on your filesystem.

The reason it's not a published library is because it's LSPosed's own subset snapshot of the internal Android APIs. It doesn't make sense to publish them, because they are hidden android APIs. In general this is just how private libraries work, you include them in your git repo but don't publish them.

I'm not exactly sure specifically why you think this is bad, so I'm not sure what sort of alternatives to suggest to you to fix it. It is what LSposed themselves do. Could you explain why you think it's bad in more detail?

One possible option is to instead include LSPosed as a git submodule and include it that way if you prefer, but then you will still have it as a path inside the working tree. Or we can put it into another repo under your username, and include that as a git submodule.

[infinity0]

I added another few commits:

• 26da19c moves all the extra multi-user/lsposed functionality into MultiUser.java. (Just a few lines; most of it was there already.) This makes it easier to build a version without multiuser support and without the LSPosed code, by simply doing:

  1. Deleting or ignoring MultiUser.java
  2. Deleting or ignoring all references to it from the rest of the AFWall app
  3. Dropping the dependencies on hiddenapi in app/build.gradle

  Please note my comment here that it may not be necessary do this and distribute two versions of AFWall - the Play Store may be happy to accept the LSPosed code I included as it is just plain Java code with no security consequences, as I explained in that comment.

• fef8e83 is optional. The upside is it reduces the size footprint of the hiddenapi-stubs private LSPosed library in this repository, but the downside is that it makes it harder to maintain in future if Android/LSPosed add extra imports to those files. cp -a is easier than going through all the files and seeing what's actually the minimum needed.

[ukanth]

Thanks for the explanation. I will go through it

[infinity0]

I tested this on OnePlus 8T and unfortunately it breaks due to getInstalledPackagesFromAllUsers requiring MANAGE_USERS. Earlier I tested with OnePlus 5T and it only required INTERACT_ACROSS_USERS. It seems there's a difference in behaviour, even though I am running "the same version" i.e. LineageOS 20 on both devices.

I'll have to figure out some other way around this, or it might be impossible...

[infinity0]

I tested this on OnePlus 8T and unfortunately it breaks due to getInstalledPackagesFromAllUsers requiring MANAGE_USERS. Earlier I tested with OnePlus 5T and it only required INTERACT_ACROSS_USERS. It seems there's a difference in behaviour, even though I am running "the same version" i.e. LineageOS 20 on both devices.

I'll have to figure out some other way around this, or it might be impossible...

This is fixed in 84eccdf, explanation in the comments.

I also rebased on top of 3.6.0.

I also fixed an existing bug in the filter logic 76c66a1. Now you can search "user 11" in the search box for a basic way to make it look a bit nicer, as we discussed above.

[infinity0]

I just realised, import / export functionality doesn't combine well with this, since it doesn't export the UIDs but only the package names. I'll have a go at fixing that in a few days.

[infinity0]

@ukanth any chance to take a look? Would be good to get some feedback before I continue to sink more time into this.

[ukanth]

Not yet. I will check when I get some time. Please go ahead and update it. I will merge it.

[amaa-99]

@ukanth any chance to take a look? Would be good to get some feedback before I continue to sink more time into this.

Hi infinity0. I've also had a go at fixing the pkg manager issues. Currently my changes are kept on a branch of my fork only. I think there's some useful changes there which could be integrated together with this, in particular regarding the application icons code...

[infinity0]

@amaa-99 The issue with the application icons code was solved a while ago. I don't experience any issues with the latest commit 0036f86 on my tester phones. What issues are you experiencing?

[amaa-99]

@amaa-99 The issue with the application icons code was solved a while ago. I don't experience any issues with the latest commit 0036f86 on my tester phones. What issues are you experiencing?

Hi. You're right, I just noticed that a bunch of those changes have already been merged with the pull #1338. My original title to that was "Bug in the implementation of Api.getpackagesForUser() leading to empty package list".

The main changes related to that where to use the packageManager.getInstalledApplications API instead of executing 'pm list packages' on the shell (huge speed difference, plus other compatibility issues) and inconsistencies on the setting of the icon bitmaps in various places (I've created the public static Drawable getApplicationIcon(Context context, int appUid) method (with an internal cache) to simplify/consolidate that.

p.s. Many of those changes are centered on the onBindViewHolder() methods... Additionally to it I've got an open pull request #1337 which fixes the crashes on various Android versions due to the (pretty) date/time format there.

[amaa-99]

@amaa-99 The issue with the application icons code was solved a while ago. I don't experience any issues with the latest commit 0036f86 on my tester phones. What issues are you experiencing?

p.s. At least on some (rooted) devices the default user has the ID 0 (zero). This leads to that user to not get added to the list of users:

            for (UserHandle user : list) {
                Matcher m = p.matcher(user.toString());
                if (m.find() && m.groupCount() > 0) {
                    int id = Integer.parseInt(m.group(1));
                    if (id > 0) {
                        listOfUids.add(id);
                    }
                }
            }

and respectively the packages for that user won't get listed:

            HashMap<Integer, String> packagesForUser = new HashMap<>();
            if(G.supportDual()) {
                packagesForUser  = getPackagesForUser(listOfUids);
            }

This seemed to lead to empty app lists on some of my tests. Have you observed anything similar? (Refer to pull #1339 for details)

[infinity0]

@amaa-99 What you are talking about is nothing to do with this PR. We grab the list of users via a different method. All the code inside the if (G.supportDual()) stuff is irrelevant to this PR. In fact I personally think it's cleaner to just delete it.

[ukanth]

@infinity0, What is the current status of this PR ? Shall I start going through it ?

[infinity0]

@infinity0, What is the current status of this PR ? Shall I start going through it ?

Yes please. I think I also need to rebase and fix some merge conflicts.

[infinity0]

Oh, I still need to fix the import/export functionality to deal with the new uids. I'll get around to that soon.

[infinity0]

Working on this again now.

[infinity0]

@ukanth the beta branch is broken atm, both the NDK build and the gradle build fail, so I'll rebase this against the main branch instead, is that OK?

[infinity0]

@ukanth should be good to go. Import/export works by saving the package name of non-owner users as "${pkgName}/${user_id}" so e.g. "dev.ukanth.ufirewall/10" in the JSON. I don't think this clashes with anything else.

[infinity0]

@ukanth BTW lemme know if I should uncomment the if (G.supportDual()) { stuff. That has nothing to do with this PR, but I commented it out for now because I wasn't sure if it will interact badly with the isMultiUser stuff here.

[infinity0]

The current CI build failure is due to something already existing on the main branch, not an issue with my PR.