Conversation
|
Can you rebase this? |
8eadd43 to
e584d3d
Compare
done, thanks for taking a look. looks to be passing the build now. |
|
the paired auditee list is not explicitly refreshed when a new auditee is added, but also, my screenshot just shows the same fingerprint multiple times because i only have one auditable device for testing. |
| final TextView auditeeListLabel = findViewById(R.id.auditee_list_label); | ||
| final LinearLayout auditeesContainer = findViewById(R.id.auditee_list_values); | ||
| if (auditees.isEmpty()) { | ||
| auditeeListLabel.setText(R.string.paired_auditees_list_empty); | ||
| auditeesContainer.removeAllViews(); | ||
| return; | ||
| } |
There was a problem hiding this comment.
I think it's better to use a RecyclerView here, since the app doesn't have a limit on the number of auditees
There was a problem hiding this comment.
updated in 734502c - used androidx.core.widget.NestedScrollView to make the whole page scroll correctly.
i didn't implement this initially because i incorrectly thought this wouldn't provide value without the asynchronous file iterator - though i now realize we still get value from the recycled views.
There was a problem hiding this comment.
though now, i'm realizing that the nested scroll view might negate the benefits of the recycler view...i think i may have to make the existing attestation content a header within the recycler view.
| } | ||
|
|
||
| static List<String> getAuditorFingerprints(final Context context) { | ||
| final File dir = new File(context.getFilesDir().getParent() + "/shared_prefs/"); |
There was a problem hiding this comment.
might be better to use the same way how AOSP gets the preference directory:
| final File dir = new File(context.getFilesDir().getParent() + "/shared_prefs/"); | |
| final File dir = new File(context.getDataDir(), "shared_prefs"); |
but I did notice this was the code used elsewhere in the app already
There was a problem hiding this comment.
given that this PR already has some significant changes, i think it would be good to limit the blast radius to make debugging / bisecting easier - could we make this change in a future PR?
There was a problem hiding this comment.
though, i guess we may have to change this anyway to achieve the ask in #318 (comment)
There was a problem hiding this comment.
Sure, I think issues like this could be handled later
There was a problem hiding this comment.
are TODO comments acceptable? or shall i open an issue?
There was a problem hiding this comment.
I think we should just open an issue for SharedPreference usage in general, and maybe make a TODO note that links to the issue
| } | ||
| }); | ||
|
|
||
| final List<String> auditees = AttestationProtocol.getAuditorFingerprints(this); |
There was a problem hiding this comment.
This seems to do file system operations (listing files in a directory) on the main thread
There was a problem hiding this comment.
good callout - fixed in 9396ebf, included an initial "loading paired auditees..." text for the label, & tested with a Thread.sleep(5000).
| static List<String> getAuditorFingerprints(final Context context) { | ||
| final File dir = new File(context.getFilesDir().getParent() + "/shared_prefs/"); | ||
| ArrayList<String> auditees = new ArrayList<>(); | ||
| final String[] sharedPreferenceFiles = dir.list(); |
There was a problem hiding this comment.
Using a directory stream might be more efficient here, since all we need here is iteration over the files. Doc comments for java.io.File#list says:
Note that the java.nio.file.Files class defines the newDirectoryStream method to open a directory and iterate over the names of the files in the directory. This may use less resources when working with very large directories, and may be more responsive when working with remote directories.
Though again, I did notice this was how the app was doing this initially
There was a problem hiding this comment.
this was my initial thought - to return an iterator from AttestationProtocol rather than a list. There were a few reasons why I didn't implement this initially:
-
keep in line with the existing code, reduce blast radius of this PR
-
inconsistent ordering of auditee pairings - currently I have the auditee list sorted consistently based on lexicographic comparison of hex representation of their fingerprints. however by using the iterator, the list ordering is not guaranteed to be the same every time the list is loaded - it is unclear what ordering the OS will use and whether it will be the same when called twice.
-
using an iterator does not provide random access to the filenames - e.g. if someone scrolls up, we can't go back to ask the iterator "hey could you remind me what the 5th filename was?" - meaning we will have to keep the entire list in memory as it loads. the iterator is still an efficiency win, but consumes the same amount of memory in the worst case scenario (when someone scrolls to the bottom of the list)
-
we are loading only a very small amount of data per pairing - each fingerprint string takes up only ~64 bytes iiuc (+ a pointer or whatever java does internally)
so i think using the iterator definitely does provide greater efficiency, but might not be worth these drawbacks + increased complexity. what do you think? i'm happy to implement this either way.
There was a problem hiding this comment.
I see the reasoning and I agree, this should be fine then
There was a problem hiding this comment.
re: the discussion on matrix - using sqlite solves problems 2 & 3 + also allows fancier ordering (like ordering by first or last verified time)
| final String deviceName = decapitatedFilename.substring(0, decapitatedFilename.length() - ".xml".length()); | ||
| if (deviceName.isBlank()) { | ||
| continue; | ||
| } | ||
| auditees.add(deviceName); |
There was a problem hiding this comment.
deviceName variable seems to be a fingerprint
| <com.google.android.material.appbar.MaterialToolbar | ||
| android:id="@+id/toolbar" | ||
| android:layout_width="match_parent" | ||
| android:layout_height="?attr/actionBarSize" /> |
There was a problem hiding this comment.
I think the toolbar in the new activity should show a back button, and the title in the toolbar should be something descriptive like "Viewing auditee"
There was a problem hiding this comment.
added back button in 13975e1 - will update screenshots after other changes are done
| if (fingerprint_hex == null || fingerprint_hex.isBlank()) { | ||
| Log.e(TAG, "auditee fingerprint not provided"); | ||
| return; | ||
| } |
There was a problem hiding this comment.
Activity should finish and/or show some error message
There was a problem hiding this comment.
given that we don't expect this case to happen regularly (if at all) - i think using the snackbar might be overkill - do you think a toast is reasonable?
There was a problem hiding this comment.
Toast seems fine, and yeah, I think this would only happen through programming error anyway
There was a problem hiding this comment.
there is a different issue, where any random string can be passed as the fingerprint, which will cause the activity to display default values.
i think it is technically possible for a user to click the "clear auditor pairings" menu button, then click on an auditee before the clearing code is completely run.
we could temporarily disable the auditees list or do some locking to prevent this - but someone using adb or system settings to clear the app data while the app is running could still cause this.
There was a problem hiding this comment.
added toast & activity finishing behavior in f5a9eca
| addSummaryField("fingerprint", fingerprint_hex); | ||
| addSummaryField("first verified", new Date(summary.verifiedTimeFirst()).toString()); | ||
| addSummaryField("last verified", new Date(summary.verifiedTimeLast()).toString()); | ||
| addSummaryField("verified boot key", summary.verifiedBootKey()); | ||
| addSummaryField("pinned OS version", String.valueOf(summary.pinnedOsVersion())); | ||
| addSummaryField("pinned OS patch level", String.valueOf(summary.pinnedOsPatchLevel())); | ||
| addSummaryField("pinned vendor patch level", String.valueOf(summary.pinnedVendorPatchLevel())); | ||
| addSummaryField("pinned boot patch level", String.valueOf(summary.pinnedBootPatchLevel())); | ||
| addSummaryField("pinned app version", String.valueOf(summary.pinnedAppVersion())); | ||
| addSummaryField("pinned app variant", String.valueOf(summary.pinnedAppVariant())); | ||
| addSummaryField("pinned security level", String.valueOf(summary.pinnedSecurityLevel())); |
There was a problem hiding this comment.
- I think we should use the strings for these labels in
res/values/strings.xml, though they might need to be adapted here, since they have new line chars in them - The "Information provided by the verified OS" section should probably be here, but I don't think the app is currently storing that info (unlike the server), so we can leave that for now
- For consistency, I think we should have sections here like the web app. The "first verified" and "last verified" should be under an "Attestation history" section, and the other fields should be under a "Hardware verified information" section.
There was a problem hiding this comment.
for 1, the existing strings in strings.xml have control characters + format placeholders in them, so i don't think it is viable to use them. i can add these to strings.xml though.
There was a problem hiding this comment.
fixed 2 & 3 in 4aac0f3 based on the web app + updated the screenshot in the PR description
realized this was from the |
| <androidx.recyclerview.widget.RecyclerView | ||
| android:id="@+id/auditee_list_recycler" | ||
| android:layout_width="match_parent" | ||
| android:layout_height="0dp" | ||
| android:layout_weight="1" | ||
| android:clipToPadding="false" | ||
| android:scrollbars="vertical" | ||
| android:nestedScrollingEnabled="true"/> |
There was a problem hiding this comment.
After thinking about it, I believe we should apply the same left and right margins as everything else (buttons, auditee_list_label, introduction text, etc.)
app/src/main/res/values/strings.xml
Outdated
| <string name="inspect_auditee_title">inspect auditee</string> | ||
| <string name="inspect_auditee_os_provided_info">information from the verified OS</string> | ||
| <string name="inspect_auditee_hardware_verified">hardware verified information</string> | ||
| <string name="inspect_auditee_attestation_history">attestation history</string> | ||
| <string name="inspect_auditee_error_message_missing_fingerprint">auditee fingerprint was not provided</string> | ||
|
|
||
| <string name="inspect_auditee_field_name_fingerprint">fingerprint</string> | ||
| <string name="inspect_auditee_field_name_pinned_security_level">security level</string> | ||
| <string name="inspect_auditee_field_name_pinned_os_version">OS version</string> | ||
| <string name="inspect_auditee_field_name_pinned_os_patch_level">OS patch level</string> | ||
| <string name="inspect_auditee_field_name_pinned_vendor_patch_level">vendor patch level</string> | ||
| <string name="inspect_auditee_field_name_pinned_boot_patch_level">boot patch level</string> | ||
| <string name="inspect_auditee_field_name_verified_boot_key">verified boot key</string> | ||
| <string name="inspect_auditee_field_name_pinned_app_version">app version</string> | ||
| <string name="inspect_auditee_field_name_pinned_app_variant">app variant</string> | ||
| <string name="inspect_auditee_field_name_first_verified">first verified</string> | ||
| <string name="inspect_auditee_field_name_last_verified">last verified</string> |
There was a problem hiding this comment.
Think we should capitalize the first words for consistency
There was a problem hiding this comment.
do you mean consistency with the web app or the rest of this app?
i personally feel that non-capitalized strings look better in the UI, especially in a table - are there project wide design guidelines for this kind of thing? happy to update it either way.
There was a problem hiding this comment.
I don't personally have an issue with non-capitalized strings, but I'd say we want to have consistency with the web app/rest of the app
I don't think we have explicit guidelines for design (we do loosely follow Material Design when possible), but the rest of our standalone apps also keep things capitalized. e.g., PdfViewer also has a "table" (constrained by dialog window) of properties:
| runOnUiThread(() -> { | ||
| Toast.makeText( | ||
| this, | ||
| R.string.inspect_auditee_error_message_missing_fingerprint, | ||
| Toast.LENGTH_SHORT | ||
| ).show(); |
There was a problem hiding this comment.
I believe this should be unnecessary; onCreate already runs on the UI/main thread
| addSummaryFieldToTable(R.id.summary_hardware, R.string.inspect_auditee_field_name_fingerprint, fingerprint_hex); | ||
| addSummaryFieldToTable(R.id.summary_hardware, R.string.inspect_auditee_field_name_pinned_security_level, String.valueOf(summary.pinnedSecurityLevel())); | ||
| addSummaryFieldToTable(R.id.summary_hardware, R.string.inspect_auditee_field_name_pinned_os_version, String.valueOf(summary.pinnedOsVersion())); | ||
| addSummaryFieldToTable(R.id.summary_hardware, R.string.inspect_auditee_field_name_pinned_os_patch_level, String.valueOf(summary.pinnedOsPatchLevel())); | ||
| addSummaryFieldToTable(R.id.summary_hardware, R.string.inspect_auditee_field_name_pinned_vendor_patch_level, String.valueOf(summary.pinnedVendorPatchLevel())); | ||
| addSummaryFieldToTable(R.id.summary_hardware, R.string.inspect_auditee_field_name_pinned_boot_patch_level, String.valueOf(summary.pinnedBootPatchLevel())); | ||
| addSummaryFieldToTable(R.id.summary_hardware, R.string.inspect_auditee_field_name_verified_boot_key, summary.verifiedBootKey()); | ||
|
|
||
| addSummaryFieldToTable(R.id.summary_os, R.string.inspect_auditee_field_name_pinned_app_version, String.valueOf(summary.pinnedAppVersion())); | ||
| addSummaryFieldToTable(R.id.summary_os, R.string.inspect_auditee_field_name_pinned_app_variant, String.valueOf(summary.pinnedAppVariant())); | ||
|
|
||
| addSummaryFieldToTable(R.id.summary_history, R.string.inspect_auditee_field_name_first_verified, new Date(summary.verifiedTimeFirst()).toString()); | ||
| addSummaryFieldToTable(R.id.summary_history, R.string.inspect_auditee_field_name_last_verified, new Date(summary.verifiedTimeLast()).toString()); |
There was a problem hiding this comment.
I think these fields should have user-readable strings where possible, e.g. security level has various strings handled at appendVerifiedInformation (
Auditor/app/src/main/java/app/attestation/auditor/AttestationProtocol.java
Lines 845 to 900 in 6483efc
formatPatchLevel, etc.
Some of these methods for user-readable strings fortunately have static methods for use, but others are embedded in the AttestationProtocol methods and could be refactored for use here
There was a problem hiding this comment.
agree that it is ideal to show the human readable strings - however given that:
- we have many other existing display differences with the web app (different fingerprint display format, missing fields not stored in mobile, display of certificate info, etc.)
- the current numbers are still useful despite not being fully human readable
- we have to fix the history strings to remove the control characters & format specifiers before they can be easily repurposed
is this something we can implement in a future PR? happy to implement it either way.
There was a problem hiding this comment.
Yes, I agree that cleaning up the rest of the UI code to make it reusable elsewhere/making it consistent can be done in a future PR
| final String fingerprint_hex = getIntent().getStringExtra(INTENT_KEY_FINGERPRINT); | ||
| if (fingerprint_hex == null || fingerprint_hex.isBlank()) { |
| protected void onCreate(Bundle savedInstanceState) { | ||
| super.onCreate(savedInstanceState); | ||
| setContentView(R.layout.activity_inspectauditee); |
There was a problem hiding this comment.
There was a problem hiding this comment.
color issue is fixed with EdgeToEdge.enable(this); in d33414d
There was a problem hiding this comment.
umm for some reason my phone fails to take a screenshot in landscape mode now...
There was a problem hiding this comment.
the auto inset fix method in general seems a little hacky but is probably unavoidable to get good backwards compatibility i guess. i can't deny that it works effectively.
There was a problem hiding this comment.
updated the screenshots in the PR description
There was a problem hiding this comment.
do you mind verifying the fix on your side?
Looks good now
f0bdc04 to
b5cdf9b
Compare
|
cc @inthewaves can we merge this? |




background
the web app (https://attestation.app) allows users to view all the paired auditees - this PR adds the same functionality to the mobile app
this is a prerequisite for fixing #51 as the pairings need to be identifiable & enumerable before they can be individually deleted or renamed.
changes
InspectAuditeeActivitythat allows a paired auditee to be viewed in detail (with selectable TextViews)AttestationProtocol.javato allow retrieving information about paired auditeesAttestationActivityto include a list of fingerprints below the buttonsscreenshots
cc @muhomorr @inthewaves