Error is not displayed as per design when scanning previously used Sync QR code

VolodLytvynenko commented 1 year ago

It might be nice to fix it together with https://github.com/status-im/status-mobile/issues/15945

Steps to reproduce:

User_A Generate a valid sync QR code.
User_B Scan the sync QR code
User_C Scan the same sync QR code

Actual result:

When scanning a previously used sync QR code, 'Oops something went wrong' page is shown

Expected result:

When scanning a previously used sync QR code, toast with error message should be displayed https://www.figma.com/file/o4qG1bnFyuyFOvHQVGgeFY/Onboarding-for-Mobile?type=design&node-id=4477-328513&mode=design&t=qEoB2LPz5HkB3QXX-0

Logs:

geth.log Status.log

ENV:

[Nightly 10 Jul 2023]

Samyoul commented 1 year ago

I'd like to query the errors in the designs:

The designs show the following errors:

Oops! This is not a sync QR code
Oops! This sync QR code has expired
Oops! This sync QR code has been used before
Oops! Devices are not in the same network

The errors need a little tweaking to be feasible.

Oops! This is not a sync QR code > Oops! This is not a VALID sync QR code
- This distinction is important because the application only has a concept of validity, the underlying data in the QR either validates or it doesn't validate. The distinction between something that IS a sync / pairing QR code or not, is a much harder thing for a computer to do.
Oops! Devices are not in the same network > Oops! Cannot find your other device, are you sure you are on the same network?
- This is something that we can't know, because the OS network interface will not allow us access to the network ID in many mobile phone contexts.
Oops! This sync QR code has expired
- A pairing session has a concept of expiry, but the connection parameters and ConnectionString do not. This is something that could be introduced but it requires additional work. At the moment an attempt to access an expired connection is the same as attempting to access a device that is not available on the network.
- This functionality will need to be introduced at the start of a mobile and desktop release cycle because it will introduce a breaking change with older versions.
- See https://github.com/status-im/status-go/issues/3759
Oops! This sync QR code has been used before
- This is not something that we can know currently.
- I'd argue that the long term concept of Used before shouldn't be known. We don't need to be stateful here, the QR code either works or doesn't work. The focus should be on idempotence of the pairing function. So multiple concurrent calls with the same QR will only execute once
- See https://github.com/status-im/status-go/pull/3750

Oops! This sync QR code has been used before is something that we can know in the context of the current application session. So while the app is running but not after the application has closed. We can / should have temporary knowledge of the QR codes scanned. But this needs to be rolled into making local pairing functions idempotent, which they aren't strictly at the moment, but can be, though will require additional work.

Samyoul commented 1 year ago

This PR https://github.com/status-im/status-go/pull/3750 will give session based feedback that can tell the user Oops! This sync QR code has been used before.

But as discussed here https://github.com/status-im/status-mobile/issues/16548#issuecomment-1632708369 I don't think that persisting this data after the application has been closed is appropriate.

igor-sirotin commented 1 year ago

The designs show the following errors: ...

Just want to mention that we have a bit different error messages in desktop design.

This does not look like a sync QR code (@Samyoul, this one I think satisfies your comment)
This sync QR code has expired
This sync QR code has been used before
Device failed to sync

(and for inputing the code as text, sync QR code is replaced with sync code).

Not a big issue, but would be good to make it consistent everywhere 🙂

cc @benjthayer

Samyoul commented 1 year ago

With regards to Oops! This sync QR code has expired https://github.com/status-im/status-go/issues/3759 proposes a solution that will allow QR codes to be scanned and give immediate expiry validation. It also allows for better session timeframe synchronicity between Sender and Receiver local pairing devices.

Samyoul commented 1 year ago

I'm reopening this issue, because although I am confident that the issue is partially resolved, the functionality hasn't been confirmed by QA. I've merged https://github.com/status-im/status-go/pull/3750

VolodLytvynenko commented 1 year ago

I'm reopening this issue, because although I am confident that the issue is partially resolved, the functionality hasn't been confirmed by QA. I've merged status-im/status-go#3750

Hi @Samyoul . Thank you for the update. The issue is still reproducible for me for both IOS and Android. Steps are the same https://github.com/status-im/status-mobile/issues/16548#issue-1796951029

Logs:

geth.log Status.log

Env:

Nightly [23 Aug 2023]

Samyoul commented 1 year ago

Thank you @VolodLytvynenko hmm that is interesting, do you log out between step 2 and step 3?

qoqobolo commented 1 year ago

Hey @Samyoul!

do you log out between step 2 and step 3?

Do you mean logging out by User_A? No, this issue can be reproduced if User_A generates a QR only once (without logging out, User_A just keeps the QR open) and User_B and User_C scan this code (it's the same for them, not expired).

Although, if I understand correctly, one QR code can only be used once, that is, only by one user (by User_B in this particular case). And the problem is that User_C "can" use this code, i.e. User_C scans the code but does not see the Oops! This sync QR code has been used before error, but sync does not actually happen (and the user gets stuck on the Syncing devices... screen).

Please correct me if I misunderstood the logic behind this scenario (when two users scan the same QR code) or missed something in the discussions.

cammellos commented 1 year ago

@Samyoul could you give an update please?

Samyoul commented 1 year ago

OH!

User_C Scan the same sync QR code

I'm such an idiot! 🙈 Ok, so I've implemented a fix for the following scenario:

Device A creates a QR code
Device B scans the QR code
Device B scans the same QR code again
Device B shows error that the QR code has already been used.

I don't know why I thought that would fix this issue. So, I can implement a fix for when Device C, Device D etc scan a QR code that has been previously used.

We do have code that prevents the access of a QR code after it has been used previously by returning an https error, but I need to investigate why that error causes the application to hang.

The status-go geth.log is normal. The error below, from Status.log, is interesting. Looks like the UI is not handling the error returned by status-go:

INFO [status-im.signals.core:32] - local pairing signal received {:event {:type "connection-success", :action 1}}
INFO [status-im2.contexts.syncing.events:70] - Initiated local pairing {:response "{\"error\":\"[client] status not ok when getting challenge, received '403 Forbidden'\"}", :event :syncing/input-connection-string-for-bootstrapping}

[client] status not ok when getting challenge, received '403 Forbidden' this error means that Device A refused to accept the connection from Device C. If another device receives a 403 when getting a challenge, this means that Device A has locked the QR code session.

FYI, the reason we don't give more informative errors from Device A is because Device A is hosting a local but publicly local server. This means that attackers and legitimate actors both have access to the local paring endpoints, meaning that any error we return from these endpoints should be addressed to an attacker. Meaning we need to be as vague as possible while at the same time giving legitimate actors some level of information (which is achieved via context).

Samyoul commented 1 year ago

@cammellos Sorry for the late reply I only just spotted your comment. I don't think that this issue is related to status-go, I believe that the status-mobile implementation is not correctly handling errors returned from the :syncing/input-connection-string-for-bootstrapping method.

@siddarthkay @smohamedjavid do you know if there are certain errors that aren't handled when local pairing?

smohamedjavid commented 1 year ago

The mobile handles only one unhappy flow (listed below) for local pairing.

Oops! This is not a sync QR code (It's a simple check on whether the connection string starts with cv)

The other unhappy flows listed in https://github.com/status-im/status-mobile/issues/16548#issuecomment-1632708369 are yet to be done (by checking the error key in the InputConnectionStringForBootstrapping method's callback).

churik commented 9 months ago

@VolodLytvynenko is it still relevant?

VolodLytvynenko commented 9 months ago

@VolodLytvynenko is it still relevant?

@churik Yes, part of this issue is addressed in this PR which prevents the app from getting stuck after tapping the 'try again' button. The current PR includes showing the correct error for the scenario of scanning an already used sync QR code which is not implemented yet

ilmotta commented 2 months ago

FYI: There are ongoing efforts to make syncing more user-friendly, including how sync errors are handled until version 2.30. Depending on the outcome of the new UX, we may close this issue, but let's wait for further notice.

Samyoul commented 2 months ago

@ilmotta In case it makes an impact, I have a list of issues that may be worth considering https://github.com/status-im/status-go/labels/E%3ALocal%20Pairing

ilmotta commented 2 months ago

@ilmotta In case it makes an impact, I have a list of issues that may be worth considering https://github.com/status-im/status-go/labels/E%3ALocal%20Pairing

Thank you @Samyoul! I wasn't aware 👍🏼

status-im / status-mobile