Sign in with QR code by jmartinesp · Pull Request #2793 · element-hq/element-x-android

jmartinesp · 2024-05-03T15:05:24Z

Type of change

Feature
Bugfix
Technical
Other :

Content

Bumped SDK version to be able to use new APIs.
Added changes to :libraries:matrix modules to map data from/to the Rust SDK and perform a QR code login.
Added the new screens for this feature.
Moved OnboardingConfig to :appconfig.
Added a FF for this feature, disabled by default in release, enabled by default in debug and nightly.
Replaced our Zxing library with Zxing-CPP which was re-built in native code and is faster and more accurate (Zxing, or at least the version we had to use was outdated and didn't work well with some generated QRs).
Made sure the login flow can only be used in portrait mode (the landscape one was broken in most devices anyway).
Fixed some issues (thanks @bmarty for the early review!).
Added tests for the new code.

Motivation and context

Closes #2632.

Screenshots / GIFs

Lots in the PR.

Tests

Create an account in https://pr12370--matrix-react-sdk.netlify.app/.
In that version of EW, go to settings -> labs -> scroll to the bottom and enabled native OIDC login.
On the EX app, tap on sign in with QR code and follow the instructions.
You should be able to login using the QR code. And if you have any issues you should be able to restart the login attempt.

Tested devices

Physical
Emulator
OS version(s): 14

Checklist

Changes have been tested on an Android device or Android emulator with API 23
UI change has been tested on both light and dark themes
Accessibility has been taken into account. See https://github.com/element-hq/element-x-android/blob/develop/CONTRIBUTING.md#accessibility
Pull request is based on the develop branch
Pull request includes a new file under ./changelog.d. See https://github.com/element-hq/element-x-android/blob/develop/CONTRIBUTING.md#changelog
Pull request includes screenshots or videos if containing UI changes
Pull request includes a sign off
You've made a self review of your PR

- Force portrait orientation on the login flow. - Create `NumberedList` UI components. - Improve camera permission dialog. - Improve how the QR code preview is rendered. - Freeze the last frame of the preview when a QR code is read. - Update `FlowStepPage` contents to fit the new designs. - Update strings in UI.

This needs `jme/qr-login-for-testing-with-clients` branch in the rust-sdk project

…lient and other SDK services

Rename `QrCodeLoginPresenter` to `QrCodeLoginManager`, since it's not really a presenter.

…nce we're done

…cessfully read

…ager`

…Exception.InvalidQrCode`

…WithQrCode

ganfra

Good work!
Have not tested yet, but I'll.
Some small remarks/questions.

ganfra · 2024-05-30T09:02:44Z

....screens.qrcode.scan_QrCodeScanView_null_QrCodeScanView-Day-10_11_null_3,NEXUS_5,1.0,en].png

Button is close to the CameraView?

I'll try to fix it.

6ff42bc, I thought it was going to be harder to fix.

ganfra · 2024-05-30T09:04:03Z

appnav/src/main/kotlin/io/element/android/appnav/NotLoggedInFlowNode.kt

@@ -75,6 +77,7 @@ class NotLoggedInFlowNode @AssistedInject constructor(
        @Parcelize
        data class LoginFlow(


Should not be an enum? I don't think we can have both to true, right?

ganfra · 2024-05-30T09:08:38Z

...login/impl/src/main/kotlin/io/element/android/features/login/impl/di/QrCodeLoginComponent.kt

+
+@SingleIn(QrCodeLoginScope::class)
+@MergeSubcomponent(QrCodeLoginScope::class)
+interface QrCodeLoginComponent : NodeFactoriesBindings {


Do we really need a separate component and scope for this? This is just to keep the StateFlow<QrCodeLoginStep>?

The problem here is QrCodeLoginManager had to be shared by several components (QrCodeLofingFlowNode, QrCodeScanPresenter) and for that I needed a singleton if I'm not mistaken. But having a singleton can be troublesome if you aren't super careful with cleaning up its state once you're done working with it, so I created a new component to hold it that will be created when the QR code flow starts and discarded once it's finished.

This should ensure the object is shared but can't be leaked in any way, although it's more complex than I intended it to be.

ganfra · 2024-05-30T09:09:37Z

...io/element/android/features/login/impl/screens/qrcode/confirmation/QrCodeConfirmationNode.kt

+import io.element.android.libraries.architecture.inputs
+import io.element.android.libraries.di.AppScope
+
+@ContributesNode(AppScope::class)


Use QrCodeLoginScope?

ganfra · 2024-05-30T09:10:02Z

...c/main/kotlin/io/element/android/features/login/impl/screens/qrcode/error/QrCodeErrorNode.kt

+import io.element.android.libraries.core.meta.BuildMeta
+import io.element.android.libraries.di.AppScope
+
+@ContributesNode(AppScope::class)


Use QrCodeLoginScope?

ganfra · 2024-05-30T09:10:22Z

...c/main/kotlin/io/element/android/features/login/impl/screens/qrcode/intro/QrCodeIntroNode.kt

+import io.element.android.anvilannotations.ContributesNode
+import io.element.android.libraries.di.AppScope
+
+@ContributesNode(AppScope::class)


Use QrCodeLoginScope?

ganfra · 2024-05-30T09:13:42Z

...l/src/main/kotlin/io/element/android/libraries/featureflag/impl/StaticFeatureFlagProvider.kt

                FeatureFlags.MarkAsUnread -> true
                FeatureFlags.RoomDirectorySearch -> false
                FeatureFlags.ShowBlockedUsersDetails -> false
+                FeatureFlags.QrCodeLogin -> OnBoardingConfig.CAN_LOGIN_WITH_QR_CODE


I'd prefer the value to be directly here...

We're probably going to remove the feature flag soon-ish and go back to the build config constant, so I made this compromise because I didn't want to remove the constant just to add it back later or have 1 value that actually does something (FF here) and another that does nothing at the same time.

…ope`

jmartinesp · 2024-05-31T06:42:48Z

Can we retest this @bmarty, @ganfra and merge it if it's fine?

…WithQrCode

bmarty

I guess it's fine since the feature is still disabled (CAN_LOGIN_WITH_QR_CODE = false)

sonarqubecloud · 2024-05-31T11:31:50Z

Quality Gate passed

Issues
0 New issues
0 Accepted issues

Measures
0 Security Hotspots
No data about Coverage
0.0% Duplication on New Code

See analysis details on SonarCloud

hughns · 2024-05-31T12:24:34Z

I've tested the latest build against synapse-oidc.lab.element.dev with no issues found.

bmarty and others added 30 commits April 10, 2024 09:30

QrCode login: enable and navigate to new Node

855647f

QrCode login: Intro screen with Camera permission

cb0287a

QrCode Scanner - WIP

188e5e1

to merge if it works.

8af1d52

Add new code confirmation and connection not secure screens

6f36885

WIP: start integrating Rust SDK APIs.

2a30842

This needs `jme/qr-login-for-testing-with-clients` branch in the rust-sdk project

Set up QR code login flow up to failure at the end

253ce12

Add localized strings

c9a2e29

Fix multiple ClientBuilder instances being created

c83e6c5

Use checkCodeString for the check code value instead

92ea84c

Finish the login flow on a successful QR code login

0542be7

WIP changes to try to fix verification state

a3ba41c

Reuse existing code and simplify the flow

4803864

Remove unnecessary 'link new device' option

8f35a7b

Actually cancel the QR code login and do a cleanup of any lingering c…

a580355

…lient and other SDK services

Add some tests to onboarding screen

2fc6499

Add some tests to QR code intro screen

f4b51f9

Fix generic error screen not being displayed

d6ef9be

More tests

5791d32

Add minimal tests to QrCodeLoginFlowNode.

7b2c4ad

Rename `QrCodeLoginPresenter` to `QrCodeLoginManager`, since it's not really a presenter.

Workaround QrCodeErrorView crash

dc435f8

Fix QR code login and verification

18a8fd8

Fix navigation issues in login flow by actually resetting the state o…

def8af2

…nce we're done

Actually QrCodeLoginManagerImpl doesn't need to be a singleton

f3b2aa8

Add and fix tests

14775d5

Display the 'establishing secure connection' after the QR code is suc…

bf87cfe

…cessfully read

Update strings

1ec15c0

Move OnboardingConfig to appconfig module

57e7379

More tests, rename QrCodeLoginManagerImpl to `DefaultQrCodeLoginMan…

e12c19e

…ager`

jmartinesp added 4 commits May 29, 2024 16:14

Add new QrLoginException.OtherDeviceNotSignedIn and remove `QrLogin…

53f5f09

…Exception.InvalidQrCode`

Bump SDK, fix tests and lint issues

bfd27ed

Merge remote-tracking branch 'origin/develop' into feature/bma/signIn…

13c5007

…WithQrCode

Fix more lint issues

b81556f

jmartinesp marked this pull request as ready for review May 29, 2024 16:33

jmartinesp requested a review from a team as a code owner May 29, 2024 16:33

jmartinesp requested review from ganfra and removed request for a team May 29, 2024 16:33

Update screenshots

fdc1f8f

ganfra approved these changes May 30, 2024

View reviewed changes

jmartinesp and others added 4 commits May 30, 2024 11:53

Use enum for login flow type

9f615c4

Make nodes in qrcode feature use QrCodeLoginScope instead of `AppSc…

0688064

…ope`

Add bottom padding to the camera preview in QrCodeScanView

6ff42bc

Update screenshots

13c97ca

jmartinesp added 2 commits May 31, 2024 12:12

Update strings

a2ac07a

Merge remote-tracking branch 'origin/develop' into feature/bma/signIn…

6b5e558

…WithQrCode

jmartinesp force-pushed the feature/bma/signInWithQrCode branch from 68efc1f to 6b5e558 Compare May 31, 2024 10:31

Update screenshots

6375be4

bmarty approved these changes May 31, 2024

View reviewed changes

Fix lint issues

eab9676

jmartinesp added the Run-Maestro Starts a Maestro Cloud session to run integration tests label May 31, 2024

github-actions bot removed the Run-Maestro Starts a Maestro Cloud session to run integration tests label May 31, 2024

Fix maestro tests

7fad1a6

jmartinesp added the Run-Maestro Starts a Maestro Cloud session to run integration tests label May 31, 2024

github-actions bot removed the Run-Maestro Starts a Maestro Cloud session to run integration tests label May 31, 2024

jmartinesp merged commit c8bd04c into develop May 31, 2024

jmartinesp deleted the feature/bma/signInWithQrCode branch May 31, 2024 12:38

		@@ -75,6 +77,7 @@ class NotLoggedInFlowNode @AssistedInject constructor(
		@Parcelize
		data class LoginFlow(

Conversation

jmartinesp commented May 3, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Type of change

Content

Motivation and context

Screenshots / GIFs

Tests

Tested devices

Checklist

Uh oh!

ganfra left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jmartinesp commented May 31, 2024

Uh oh!

bmarty left a comment

Choose a reason for hiding this comment

Uh oh!

sonarqubecloud bot commented May 31, 2024

Quality Gate passed

Uh oh!

hughns commented May 31, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

jmartinesp commented May 3, 2024 •

edited

Loading