Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat(android): use device serial for deviceName #4180

Merged
merged 5 commits into from
Mar 21, 2024
Merged

Conversation

jasonboukheir
Copy link
Contributor

@jasonboukheir jasonboukheir commented Mar 17, 2024

Fixes #4042

The serial number of the device is blocked behind a permission. There's a couple ways we can go about this:


(1) Ask the user to (optionally) grant the permission

When we show the grant VPN permission activity, we also mention the optional READ_PRIVILEGED_PHONE_STATE permission. Here, the user can decide to grant it or not, and if they decide not to, they can grant it in the future in the app settings. When the permission is not granted, the deviceName falls back to the Build.MODEL

(2) Force the user to grant the permission

We keep asking them to grant the permission in the splash view. deviceName is always the serial number of the device.

(3) Let MDM grant the permission

We don't provide a UI to grant the permission in the application. Instead, the deviceName is the Build.MODEL by default, unless advanced users or admins using MDM set the permission, in which case it's the serial number of the device.

(4) Let MDM set a custom/override device name

This could be an alternative to (3) if it is easier for customers using MDM software to manage it this way. Though I doubt it...


Going with option (3) is safe, and the other options can be added incrementally in the future. However, it requires communicating to the customer that they should set this permission for the deviceName to be the serial of the device. That's not a problem yet, since the relevant customer is using MDM to manage the app; it's trivial to set this permission via that UI.

If we did want to show this permission to the user, I think option (1) is most likely going to be better than option (2) in most cases.

@jasonboukheir jasonboukheir self-assigned this Mar 17, 2024
Copy link

vercel bot commented Mar 17, 2024

The latest updates on your projects. Learn more about Vercel for Git ↗︎

1 Ignored Deployment
Name Status Preview Comments Updated (UTC)
firezone ⬜️ Ignored (Inspect) Visit Preview Mar 21, 2024 0:28am

Copy link

github-actions bot commented Mar 17, 2024

Terraform Cloud Plan Output

Plan: 9 to add, 8 to change, 9 to destroy.

Terraform Cloud Plan

Copy link

github-actions bot commented Mar 17, 2024

Performance Test Results

TCP

Test Name Received/s Sent/s Retransmits
direct-tcp-client2server 223.3 MiB (+1%) 224.9 MiB (+0%) 315 (-12%)
direct-tcp-server2client 230.5 MiB (+1%) 231.4 MiB (+0%) 464 (+203%)
relayed-tcp-client2server 150.1 MiB (+4%) 151.1 MiB (+5%) 182 (+41%)
relayed-tcp-server2client 149.3 MiB (+0%) 149.7 MiB (+0%) 141 (-24%)

UDP

Test Name Total/s Jitter Lost
direct-udp-client2server 50.0 MiB (-0%) 0.31ms (+675%) 0.00% (NaN%)
direct-udp-server2client 50.0 MiB (-0%) 0.01ms (+20%) 0.00% (NaN%)
relayed-udp-client2server 50.0 MiB (+0%) 0.13ms (-22%) 0.00% (NaN%)
relayed-udp-server2client 50.0 MiB (-0%) 0.06ms (+3%) 0.00% (NaN%)

@jamilbk
Copy link
Member

jamilbk commented Mar 17, 2024

@jasonboukheir Thanks for summarizing our options, that makes things much more clear! I'll link the customer to this PR for input on which method is best. To avoid privacy and data concerns that affect all customers, I'm thinking 3 or 4.

@AndrewDryga
Copy link
Collaborator

I like the option 3. And I think we should not reuse Device Name for it but better we add a separate optional field for it that will be displayed in the portal.

@jamilbk
Copy link
Member

jamilbk commented Mar 17, 2024

I like the option 3. And I think we should not reuse Device Name for it but better we add a separate optional field for it that will be displayed in the portal.

Option 4 would be more consistent with how we allow users to set their own FIREZONE_NAME which we allow for the gateway and headless client. If the MDM solution supports setting that en-masse then that would work best since it wouldn't require any portal changes and give them control over what they see in the portal.

The underlying use case here is being able to quickly filter thousands of Clients in the table view by some more unique identifier than the device's model number. It could be a serial in this case, but for other Clients it could be a MAC address, Firebase.installationId, etc.

Keeping FIREZONE_NAME agnostic to what it represents may be more flexible and prevent us from having to add a specific metadata field for each piece of information we may or may not be able to retrieve from a certain platform.

@jamilbk
Copy link
Member

jamilbk commented Mar 17, 2024

Option 4 would also allow us from having to declare this information as "collected" in our privacy statements for the Google Play Store, since it's provided by the customer.

Edit: On second thought, we're still saving it, so I'll need to think about the privacy implications.

@jamilbk
Copy link
Member

jamilbk commented Mar 19, 2024

@jasonboukheir Customer input is that they can set the field to a variable, so they can override to a value of their choosing. Let's go with Option (4) which prevents us from needing to request additional permissions.

Keep in mind if the MDM config field is not set it will be null and not the empty string, so you need to check both of those before deciding to fallback to the build.MODEL

@tobiasmcnulty
Copy link

tobiasmcnulty commented Mar 19, 2024

Option 4 is a great fit for our use case; TinyMDM at least supports variables in the managed configuration interface, so we can populate the name with the serial number or whatever other variables me might want to in the future.

If this isn't too complicated, Option 3 would work well as a default value, i.e., if no name is provided via the MDM configuration, you could call back to the serial number or at worst the model. TinyMDM indeed gives admins the option to pre-approve permissions that would otherwise be requested by the application.

Thanks so much for the detailed write-up and options!

@jasonboukheir
Copy link
Contributor Author

Thank you for the feedback, everyone! Combo of (3) and (4) it is :)

@jasonboukheir jasonboukheir marked this pull request as ready for review March 21, 2024 05:16
@jasonboukheir
Copy link
Contributor Author

jasonboukheir commented Mar 21, 2024

Screenshot 2024-03-20 at 10 15 30 PM

Testing it out with MDM. Looks like it works!

There is another issue that is related to this #4181. We should make sure the tunnel is restarted when this mdm property changes as well. Merge this first, then I'll make sure to add the other property in that PR.

Signed-off-by: Jamil <jamilbk@users.noreply.github.com>
@jamilbk jamilbk enabled auto-merge March 21, 2024 12:28
@jamilbk jamilbk added this pull request to the merge queue Mar 21, 2024
Merged via the queue into main with commit c94b2de Mar 21, 2024
138 checks passed
@jamilbk jamilbk deleted the jasonboukheir/issue/4042 branch March 21, 2024 12:49
github-merge-queue bot pushed a commit that referenced this pull request Mar 22, 2024
Fixes #4040

Depends on #4180

---------

Signed-off-by: Jason Elie Bou Kheir <5115126+jasonboukheir@users.noreply.github.com>
Co-authored-by: Jamil <jamilbk@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Use hardware serial for Android deviceName
4 participants