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.

Sign up for GitHub

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

Jump to bottom

Add X.509 Certificate chains viewer component. #842

Merged
merged 1 commit into from
Jan 24, 2025
Merged

Add X.509 Certificate chains viewer component. #842

merged 1 commit into from
Jan 24, 2025

Conversation

koukarine
Copy link
Contributor

  • Reusable Compose components for the chain display.
  • Item in Testapp module for the demo, using the chain from tests.

Tested manually with multiple certificates chain (previously defined in tests), as well as with local tests.

  • Tests pass

@koukarine koukarine requested review from davidz25 and kdeus January 14, 2025 00:36
@koukarine koukarine force-pushed the cert_tree_viewer branch 2 times, most recently from b87c1ec to ec9c164 Compare January 14, 2025 01:40
@davidz25
Copy link
Contributor

Would it be possible to post some screenshots too? Thanks!

davidz25
davidz25 reviewed Jan 14, 2025
View reviewed changes
Copy link
Contributor

@davidz25 davidz25 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks like a great start, comments inline!

...upport/src/commonMain/kotlin/com/android/identity/appsupport/ui/x509chain/X509ChainViewer.kt Outdated Show resolved Hide resolved
...upport/src/commonMain/kotlin/com/android/identity/appsupport/ui/x509chain/X509ChainViewer.kt Outdated Show resolved Hide resolved
...upport/src/commonMain/kotlin/com/android/identity/appsupport/ui/x509chain/X509ChainViewer.kt Outdated Show resolved Hide resolved
...appsupport/src/commonMain/kotlin/com/android/identity/appsupport/ui/x509chain/X509VewData.kt Outdated Show resolved Hide resolved
...upport/src/commonMain/kotlin/com/android/identity/appsupport/ui/x509chain/X509ChainViewer.kt Outdated Show resolved Hide resolved
...ty-appsupport/src/commonMain/kotlin/com/android/identity/appsupport/ui/x509chain/X509View.kt Outdated Show resolved Hide resolved
identity/src/commonTest/kotlin/com/android/identity/util/X509ViewDataKtTest.kt Outdated Show resolved Hide resolved
identity/src/commonMain/kotlin/com/android/identity/util/DecimalUtil.kt Outdated Show resolved Hide resolved
identity/src/commonMain/kotlin/com/android/identity/util/DecimalUtil.kt Show resolved Hide resolved
samples/testapp/build.gradle.kts Outdated Show resolved Hide resolved
@koukarine
Copy link
Contributor Author

Would it be possible to post some screenshots too? Thanks!

Just attached a couple to the ticket.

@koukarine koukarine force-pushed the cert_tree_viewer branch 5 times, most recently from 13211e4 to ed8a02c Compare January 16, 2025 00:58
davidz25
davidz25 reviewed Jan 17, 2025
View reviewed changes
Copy link
Contributor

@davidz25 davidz25 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks like a great start, I left some comments/suggestions! Please post screenshots on the PR in the future instead of in the ticket!

...upport/src/commonMain/kotlin/com/android/identity/appsupport/ui/x509chain/X509ChainViewer.kt Outdated Show resolved Hide resolved
...appsupport/src/commonMain/kotlin/com/android/identity/appsupport/ui/x509chain/X509VewData.kt Outdated Show resolved Hide resolved
...ty-appsupport/src/commonMain/kotlin/com/android/identity/appsupport/ui/x509chain/X509View.kt Outdated Show resolved Hide resolved
...upport/src/commonMain/kotlin/com/android/identity/appsupport/ui/x509chain/X509ChainViewer.kt Outdated Show resolved Hide resolved
samples/testapp/src/commonMain/kotlin/com/android/identity/testapp/Destinations.kt Outdated Show resolved Hide resolved
davidz25
davidz25 reviewed Jan 22, 2025
View reviewed changes
...rt/src/commonMain/kotlin/com/android/identity/appsupport/ui/x509chain/X509CertChainViewer.kt Outdated Show resolved Hide resolved
...appsupport/src/commonMain/kotlin/com/android/identity/appsupport/ui/x509chain/X509VewData.kt Outdated Show resolved Hide resolved
...ty-appsupport/src/commonMain/kotlin/com/android/identity/appsupport/ui/x509chain/X509View.kt Outdated Show resolved Hide resolved
identity/src/commonMain/kotlin/com/android/identity/util/DecimalUtil.kt Outdated Show resolved Hide resolved
samples/testapp/src/commonMain/kotlin/com/android/identity/testapp/Destinations.kt Outdated Show resolved Hide resolved
samples/testapp/src/commonMain/kotlin/com/android/identity/testapp/ui/X509ChainScreen.kt Outdated Show resolved Hide resolved
kdeus
kdeus reviewed Jan 23, 2025
View reviewed changes
Copy link
Contributor

@kdeus kdeus left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Mostly small stuff. I'll make a final pass once these are addressed.

identity-appsupport/src/commonMain/composeResources/values/strings.xml Outdated Show resolved Hide resolved
...upport/src/commonMain/kotlin/com/android/identity/appsupport/ui/x509chain/CertificateView.kt Outdated Show resolved Hide resolved
...upport/src/commonMain/kotlin/com/android/identity/appsupport/ui/x509chain/CertificateView.kt Outdated Show resolved Hide resolved
...rt/src/commonMain/kotlin/com/android/identity/appsupport/ui/x509chain/CertificateViewData.kt Outdated Show resolved Hide resolved
...port/src/commonMain/kotlin/com/android/identity/appsupport/ui/x509chain/CertificateViewer.kt Outdated Show resolved Hide resolved
identity/src/commonMain/kotlin/com/android/identity/crypto/ICert.kt Outdated Show resolved Hide resolved
identity/src/commonTest/kotlin/com/android/identity/util/X509ViewDataKtTest.kt Outdated Show resolved Hide resolved
identity/src/commonMain/kotlin/com/android/identity/util/DecimalUtil.kt Outdated Show resolved Hide resolved
samples/testapp/src/commonMain/kotlin/com/android/identity/testapp/App.kt Outdated Show resolved Hide resolved
samples/testapp/src/commonMain/kotlin/com/android/identity/testapp/TestAppUtils.kt Outdated Show resolved Hide resolved
@koukarine koukarine force-pushed the cert_tree_viewer branch 4 times, most recently from 3a4ff88 to 04f29e1 Compare January 23, 2025 17:12
davidz25
davidz25 reviewed Jan 23, 2025
View reviewed changes
Copy link
Contributor

@davidz25 davidz25 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Some nits - I'm a stickler for naming, sorry! - and also some more substantial issues.

...rt/src/commonMain/kotlin/com/android/identity/appsupport/ui/x509chain/CertificateViewData.kt Outdated Show resolved Hide resolved
...port/src/commonMain/kotlin/com/android/identity/appsupport/ui/x509chain/CertificateViewer.kt Outdated Show resolved Hide resolved
...rt/src/commonMain/kotlin/com/android/identity/appsupport/ui/x509chain/CertificateViewData.kt Outdated Show resolved Hide resolved
identity/src/commonMain/kotlin/com/android/identity/crypto/ICert.kt Outdated Show resolved Hide resolved
...androidMain/kotlin/com/android/identity/testapp/ui/AndroidKeystoreSecureAreaScreenAndroid.kt Outdated Show resolved Hide resolved
samples/testapp/src/commonMain/kotlin/com/android/identity/testapp/Destinations.kt Show resolved Hide resolved
samples/testapp/src/commonMain/kotlin/com/android/identity/testapp/Destinations.kt Show resolved Hide resolved
...es/testapp/src/commonMain/kotlin/com/android/identity/testapp/ui/CertificatesViewerScreen.kt Outdated Show resolved Hide resolved
samples/testapp/src/commonMain/kotlin/com/android/identity/testapp/TestAppUtils.kt Outdated Show resolved Hide resolved
...rt/src/commonMain/kotlin/com/android/identity/appsupport/ui/x509chain/CertificateViewData.kt Outdated Show resolved Hide resolved
@koukarine koukarine force-pushed the cert_tree_viewer branch 2 times, most recently from 9e4a4ef to 8bd5402 Compare January 24, 2025 16:29
davidz25
davidz25 reviewed Jan 24, 2025
View reviewed changes
Copy link
Contributor

@davidz25 davidz25 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks great to me, thanks! Mostly just nits/naming to be addressed and then we can land this. I'll leave it to @kdeus to Approve it.

identity-appsupport/src/commonMain/composeResources/values/strings.xml Outdated Show resolved Hide resolved
samples/testapp/src/commonMain/kotlin/com/android/identity/testapp/App.kt Outdated Show resolved Hide resolved
samples/testapp/src/commonMain/kotlin/com/android/identity/testapp/App.kt Outdated Show resolved Hide resolved
samples/testapp/src/commonMain/kotlin/com/android/identity/testapp/TestAppUtils.kt Outdated Show resolved Hide resolved
...app/src/commonMain/kotlin/com/android/identity/testapp/ui/CertificateViewerExamplesScreen.kt Show resolved Hide resolved
samples/testapp/src/commonMain/kotlin/com/android/identity/testapp/ui/CertificateScreen.kt Outdated Show resolved Hide resolved
samples/testapp/src/commonMain/composeResources/values/strings.xml Outdated Show resolved Hide resolved
identity-appsupport/src/commonMain/composeResources/values/strings.xml Outdated Show resolved Hide resolved
.../commonMain/kotlin/com/android/identity/appsupport/ui/certificateviewer/CertificateViewer.kt Show resolved Hide resolved
.../commonMain/kotlin/com/android/identity/appsupport/ui/certificateviewer/CertificateViewer.kt Outdated Show resolved Hide resolved
@kdeus kdeus self-requested a review January 24, 2025 19:27
@koukarine koukarine force-pushed the cert_tree_viewer branch 2 times, most recently from ccd0a4d to da89f93 Compare January 24, 2025 19:39
kdeus
kdeus approved these changes Jan 24, 2025
View reviewed changes
.../commonMain/kotlin/com/android/identity/appsupport/ui/certificateviewer/CertificateViewer.kt Outdated Show resolved Hide resolved
.../commonMain/kotlin/com/android/identity/appsupport/ui/certificateviewer/CertificateViewer.kt Outdated Show resolved Hide resolved
.../commonMain/kotlin/com/android/identity/appsupport/ui/certificateviewer/CertificateViewer.kt Show resolved Hide resolved
identity-appsupport/src/commonMain/composeResources/values/strings.xml Outdated Show resolved Hide resolved
.../commonMain/kotlin/com/android/identity/appsupport/ui/certificateviewer/CertificateViewer.kt Outdated Show resolved Hide resolved
samples/testapp/src/commonMain/kotlin/com/android/identity/testapp/ui/CertificateScreen.kt Outdated Show resolved Hide resolved
@koukarine
Add X.509 Certificate chains viewer to the testapp using reusable Com…
df46441 
…pose components.

Tested manually with multiple certificates chain (previously defined in tests), as well as with local tests.

Signed-off-by: koukarine <[email protected]>
@koukarine koukarine merged commit a83f15b into openwallet-foundation-labs:main Jan 24, 2025
3 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@davidz25 davidz25 davidz25 left review comments

@kdeus kdeus kdeus approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@koukarine @davidz25 @kdeus