Proper multi-user support #1349

infinity0 · 2023-08-28T00:05:58Z

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 · 2023-08-28T00:32:45Z

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 · 2023-08-29T12:54:45Z

@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 · 2023-08-29T13:19:02Z

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 · 2023-08-29T13:25:09Z

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 · 2023-08-29T13:26:51Z

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 · 2023-08-29T13:30:44Z

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 · 2023-08-29T13:39:29Z

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 · 2023-08-29T13:42:42Z

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 · 2023-08-29T14:39:21Z

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 · 2023-08-29T15:16:03Z

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 · 2023-08-29T19:46:18Z

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 · 2023-09-01T01:49:14Z

Thanks for the explanation. I will go through it

infinity0 · 2023-09-04T11:44:35Z

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 · 2023-09-04T12:51:37Z

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 · 2023-09-04T21:48:05Z

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 · 2023-09-18T19:09:01Z

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

ukanth · 2023-09-19T23:01:03Z

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

amaa-99 · 2023-09-22T11:31:19Z

@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 · 2023-09-22T12:43:43Z

@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 · 2023-09-22T15:26:02Z

@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 · 2023-09-22T19:19:08Z

@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 · 2023-09-25T09:40:02Z

@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 · 2024-03-04T16:25:05Z

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

ukanth · 2024-03-04T16:26:31Z

app/src/main/java/dev/ukanth/ufirewall/MainActivity.java

@@ -176,6 +177,7 @@ public void setDirty(boolean dirty) {
    @Override
    public void onCreate(Bundle savedInstanceState) {
        super.onCreate(savedInstanceState);
+        MultiUser.setup();


This should be called after the root check is done ?

Fixed in c75f211 hopefully, lemme know if I did it wrong.

ukanth · 2024-03-04T16:27:27Z

app/src/main/res/values-ar/strings.xml

@@ -293,7 +293,7 @@
    <string name="multi_apply_rules">تطبيق القواعد على تبديل الأوضاع</string>
    <string name="multi_apply_rules_info">تحميل وتطبيق القواعد عند التبديل بين ملفات التعريف</string>
    <string name="multi_user_title">تفعيل دعم تعدد المستخدمين</string>
-    <string name="multi_user_summary">دعم تعدد المستخدمين للنظام (يعمل فقط في الوضع حيث يتم حظر العناصر المحددة)</string>


Could you please remove all the language related changes from this PR ? This is managed separately using external service ?

OK, but shall I keep the change to app/src/main/res/values/strings.xml since this is the "main" string?

Based on other changes to that file I'll assume yes for now. I've squashed this change to app/src/main/res/values/strings.xml into c75f211 and moved the other language-specific changes into dbe1d3e temporarily for the record. I can delete the latter commit once you're happy with the rest of this PR.

infinity0 · 2024-03-04T21:10:06Z

@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 · 2024-03-04T21:27:59Z

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

infinity0 · 2024-04-17T15:29:51Z

Working on this again now.

infinity0 · 2024-04-17T15:42:43Z

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

…SERS

infinity0 · 2024-04-17T16:44:23Z

@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 · 2024-04-17T16:46:48Z

@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 · 2024-04-28T00:41:07Z

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

ukanth · 2024-04-28T14:59:55Z

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

You may have to resync base with beta branch. some other pull request has been merged.

infinity0 · 2024-04-28T19:17:20Z

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

You may have to resync base with beta branch. some other pull request has been merged.

I intentionally did not resync with the beta branch because it is currently already failing: beta files, beta CI failing.

But in fact the CI above forced merged my commits in this PR with the beta branch anyway, it's testing f1a234d the merge commit rather than my branch tip dbe1d3e.

Anyway the problem is pre-existing on the current beta branch, it has nothing to do with this PR, you'll need to ask the previous committer to unbreak it. Something to do with 68ed4d0, and native-built artifacts:

/home/runner/work/afwall/afwall/app/src/main/java/dev/ukanth/ufirewall/Api.java:1863: error: cannot find symbol
        if (!installBinary(ctx, R.raw.busybox_arm64, "busybox")) return false;
                                     ^
  symbol:   variable busybox_arm64
  location: class raw
/home/runner/work/afwall/afwall/app/src/main/java/dev/ukanth/ufirewall/Api.java:1864: error: cannot find symbol
        if (!installBinary(ctx, R.raw.iptables_arm64, "iptables")) return false;
                                     ^
  symbol:   variable iptables_arm64
  location: class raw
/home/runner/work/afwall/afwall/app/src/main/java/dev/ukanth/ufirewall/Api.java:1865: error: cannot find symbol
        if (!installBinary(ctx, R.raw.ip6tables_arm64, "ip6tables")) return false;
                                     ^
  symbol:   variable ip6tables_arm64
  location: class raw
/home/runner/work/afwall/afwall/app/src/main/java/dev/ukanth/ufirewall/Api.java:1866: error: cannot find symbol
        if (!installBinary(ctx, R.raw.nflog_arm64, "nflog")) return false;
                                     ^
  symbol:   variable nflog_arm64
  location: class raw

infinity0 · 2024-06-09T19:13:41Z

Ping @ukanth - any luck fixing the issues on the beta branch?

infinity0 force-pushed the beta branch from 1d8fe53 to 32e72dd Compare August 29, 2023 12:53

infinity0 marked this pull request as ready for review August 29, 2023 12:53

infinity0 changed the title ~~WIP: proper multi-user support. works, but missing some app names and icons~~ Proper multi-user support Aug 29, 2023

infinity0 force-pushed the beta branch 2 times, most recently from 753cb0a to 291a878 Compare August 29, 2023 13:17

infinity0 force-pushed the beta branch from 07e606f to fef8e83 Compare August 29, 2023 19:52

infinity0 force-pushed the beta branch from fef8e83 to 0036f86 Compare September 4, 2023 12:49

ukanth reviewed Mar 4, 2024

View reviewed changes

ukanth changed the title ~~Proper multi-user support~~ Proper multi-user support (WIP) Mar 5, 2024

infinity0 added 8 commits April 17, 2024 16:52

Full support for multi-user systems

c75f211

Properly clear the label cache when changing isMultiUser setting

6d14938

Don't check for hidden apps, since it is pointless and needs MANAGE_U…

953b621

…SERS

filter: fix uniqueness logic

8afc9cc

Partial revert of aa400d0 which broke the export logic

aa91a99

Make import-export work for multi-user too

f12b301

OPTIONAL: Delete unnecessary hiddenapi files

387f384

OPTIONAL: Update UI strings for multi-user

dbe1d3e

infinity0 force-pushed the beta branch from 0036f86 to dbe1d3e Compare April 17, 2024 16:40

infinity0 changed the base branch from beta to main April 17, 2024 16:40

infinity0 changed the title ~~Proper multi-user support (WIP)~~ Proper multi-user support Apr 17, 2024

Proper multi-user support #1349

Are you sure you want to change the base?

Proper multi-user support #1349

Conversation

infinity0 commented Aug 28, 2023 • edited Loading

infinity0 commented Aug 28, 2023

infinity0 commented Aug 29, 2023

ukanth commented Aug 29, 2023

infinity0 commented Aug 29, 2023

infinity0 commented Aug 29, 2023

ukanth commented Aug 29, 2023

infinity0 commented Aug 29, 2023

infinity0 commented Aug 29, 2023

ukanth commented Aug 29, 2023

infinity0 commented Aug 29, 2023

infinity0 commented Aug 29, 2023 • edited Loading

ukanth commented Sep 1, 2023

infinity0 commented Sep 4, 2023

infinity0 commented Sep 4, 2023

infinity0 commented Sep 4, 2023

infinity0 commented Sep 18, 2023

ukanth commented Sep 19, 2023

amaa-99 commented Sep 22, 2023

infinity0 commented Sep 22, 2023 • edited Loading

amaa-99 commented Sep 22, 2023 • edited Loading

amaa-99 commented Sep 22, 2023

infinity0 commented Sep 25, 2023 • edited Loading

ukanth commented Mar 4, 2024

ukanth Mar 4, 2024

Choose a reason for hiding this comment

infinity0 Apr 17, 2024

Choose a reason for hiding this comment

ukanth Mar 4, 2024

Choose a reason for hiding this comment

infinity0 Apr 17, 2024

Choose a reason for hiding this comment

infinity0 Apr 17, 2024

Choose a reason for hiding this comment

infinity0 commented Mar 4, 2024

infinity0 commented Mar 4, 2024

infinity0 commented Apr 17, 2024

infinity0 commented Apr 17, 2024

infinity0 commented Apr 17, 2024 • edited Loading

infinity0 commented Apr 17, 2024

infinity0 commented Apr 28, 2024

ukanth commented Apr 28, 2024

infinity0 commented Apr 28, 2024 • edited Loading

infinity0 commented Jun 9, 2024

infinity0 commented Aug 28, 2023 •

edited

Loading

infinity0 commented Aug 29, 2023 •

edited

Loading

infinity0 commented Sep 22, 2023 •

edited

Loading

amaa-99 commented Sep 22, 2023 •

edited

Loading

infinity0 commented Sep 25, 2023 •

edited

Loading

infinity0 commented Apr 17, 2024 •

edited

Loading

infinity0 commented Apr 28, 2024 •

edited

Loading