hCaptcha / hcaptcha-android-sdk

Android SDK for hCaptcha
https://www.hcaptcha.com
MIT License
71 stars 35 forks source link

HCaptcha dialog recieves click events when not showing #107

Closed Syrou closed 1 year ago

Syrou commented 1 year ago

Currently the code allows for the dialog to receive click events even when it is not showing. Our setup is as follows:

    .builder()
    .siteKey(siteKey)
    .diagnosticLog(false)
    .loading(false)
    .hideDialog(false)
    .build()

Even though the dialog is not showing, it still takes and listens to click events on the location. This currently gives an error in the callback chain, and more to the problem, it sits on top of our existing login button. If a user clicks on our apps login button while hcaptcha is verifying, hcaptcha will trigger addOnFailureListener because the invisible dialog sitting on top of our apps login button. Could this be fixed?

amusejfo commented 1 year ago

@CAMOBAP Any chance this could be looked into? Let me know if you need more details

CAMOBAP commented 1 year ago

@amusejfo thanks for the support

@Syrou to be on the same page you are passing .loading(false), so you have some custom loader or an another mechanism to 'block' UI, right? If so could you please explain a bit when you show/hide loader?

Thanks

Syrou commented 1 year ago

Yes, that is correct. We show the loader directly when the button is pressed, until either a success/failure callback is triggered from hcaptcha. This loader is shown in the button you pressed. Since we want the user from keep spamming the button, we set it to disabled during the duration. I noticed the reported error, because when I was testing clicking the disabled button, I instead triggered a failure event in the hcaptcha-sdk

CAMOBAP commented 1 year ago

@Syrou thanks for the reply

We show the loader directly when the button is pressed until either a success/failure callback is triggered

To be on the same page, does the loader's UI cover the whole screen or just a part of it (or not cover it at all)?

SDK takes some time to load content and prepare a captcha (till SDK not loaded clicks will not lead to cancel), so it makes sense to show some loading dialog on top to the user to prevent occasional clicks that lead to cancel

Syrou commented 1 year ago

The loader's UI is only shown in our login button. The button gets disabled after press, as to limit spamming. This is not the problem. The problem is that the fragment manager that hcaptcha uses to display the captcha webview is clickable when hidden.

The clickable hidden fragment/webview from hcaptcha sits on top of our login button which displays the loading spinner and is during loading disabled. It should not, because it triggers an error callback when clicked. This covers roughly the same area as when the challenge webview is shown, and is also in the center of the screen.

Syrou commented 1 year ago

https://github.com/hCaptcha/hcaptcha-android-sdk/blob/main/sdk/src/main/java/com/hcaptcha/sdk/HCaptchaDialogFragment.java#L110 This line needs to be configured to be disabled while the dialog is not shown. We already handle timeout/retry and cancel ourselves with our own UI.

CAMOBAP commented 1 year ago

@Syrou thanks for your cooperation, PR is on the way.

To be on the same page are you expecting that user input (touches) should reach the original activity where hCaptcha loads (while hCaptcha is loading)?

Syrou commented 1 year ago

@CAMOBAP Yeah, all input events needs to be able to go through to the original source.

CAMOBAP commented 1 year ago

@CAMOBAP Yeah, all input events needs to be able to go through to the original source.

Thanks for the reply, I am still researching this topic, because of DialogFragment is in "another" view hierarchy, I'm still searching for reliable solution with this

Karahanli16 commented 1 year ago

Şu anda kod, iletişim kutusunun gösterilmediğinde bile tıklama olaylarını almasına izin veriyor. Kurulumumuz aşağıdaki gibidir:

    .builder()
    .siteKey(siteKey)
    .diagnosticLog(false)
    .loading(false)
    .hideDialog(false)
    .build()

İletişim kutusu gösterilmese de, konumdaki tıklama olaylarını alır ve dinler. Bu şu anda geri arama zincirinde bir hata veriyor ve sorundan daha fazlası, mevcut oturum açma düğmemizin üstünde oturuyor. Bir kullanıcı, hcaptcha doğrulama yaparken uygulamalarımızın oturum açma düğmesini tıklarsa, hcaptcha, uygulamalarımızın oturum açma düğmesinin üzerinde oturan görünmez iletişim kutusu nedeniyle addOnFailureListener'ı tetikler. Bu düzeltilebilir mi?

Syrou commented 1 year ago

@CAMOBAP Is the https://github.com/hCaptcha/hcaptcha-android-sdk/pull/113 PR the proposed solution?

CAMOBAP commented 1 year ago

@Syrou yep, in case you have a chance to try it, it will be nice to hear your feedback

Syrou commented 1 year ago

@CAMOBAP Can you publish a snapshot on maven or here for it?

Syrou commented 1 year ago

Using "com.github.hcaptcha:hcaptcha-android-sdk:PR113-SNAPSHOT", or "com.github.hcaptcha:hcaptcha-android-sdk:bugfix/keep-webview-invisible-till-loaded-SNAPSHOT" does not work so I can't confirm it easily unless you publish it

CAMOBAP commented 1 year ago

@Syrou could you please doublecheck "com.github.hcaptcha:hcaptcha-android-sdk:PR113-SNAPSHOT" one more time?

Looks like it took some time to build

Syrou commented 1 year ago

Still seems like the click events go through. I click on screen and get "hCaptcha failed: Challenge Closed 30" with the same config I originally posted.public static final String VERSION_NAME = "3.8.2_34"; is the slug found in the SNAPSHOT, in case it is not the correct version published.

Syrou commented 1 year ago

I believe the latest code also needs to handle the code on https://github.com/hCaptcha/hcaptcha-android-sdk/blob/main/sdk/src/main/java/com/hcaptcha/sdk/HCaptchaDialogFragment.java#L166

Because right now, the dialog dismiss event will also trigger even if the dialog fragment is invisible. It should also not trigger unless we are actually seeing the dialog fragment.

CAMOBAP commented 1 year ago

@Syrou what is the usual delay on your side for loading?

Also could you please confirm that after HCaptcha.setup you call HCaptcha.verifyWithHCaptcha without arguments?

Syrou commented 1 year ago

It is depending on network connection, but on slow net 2-4 seconds.

Also could you please confirm that after HCaptcha.setup you call HCaptcha.verifyWithHCaptcha without arguments?

Confirmed.

I am pretty sure the reason for this still happening now is because of the fragment dismiss.

CAMOBAP commented 1 year ago

Still seems like the click events go through.

As far as I understand this is expected behavior, I just tested:

Could you please confirm that it works as expected?

I click on screen and get "hCaptcha failed: Challenge Closed 30" with the same config I originally posted

SDK didn't intercept touches till WebView is not loaded. SDK cannot block interception at all because there is a chance that a visual challenge will be presented

I am pretty sure the reason for this still happening now is because of the fragment dismiss.

Dismissing can be started from two points:

According to my measurements delay between web view load and visual challenge / token response is tens of ms Probably on your side, it's significantly bigger if you are able to reproduce it stably

Because right now, the dialog dismiss event will also trigger even if the dialog fragment is invisible.

I think I know how to make it more precise, I will prepare another PR

Syrou commented 1 year ago

I don't think this has anything to do with the visual challenge tho? This is for when nothing is shown at all.

CAMOBAP commented 1 year ago

I don't think this has anything to do with the visual challenge tho? This is for when nothing is shown at all.

When nothing is shown and SDK calls a success callback with a token, so the user should not be able to cancel it anyway

CAMOBAP commented 1 year ago

@Syrou may I ask you to have a look at com.github.hcaptcha:hcaptcha-android-sdk:PR126-SNAPSHOT

Syrou commented 1 year ago

With that version I can no longer build because of: " inferred type is Context but FragmentActivity was expected" at the line HCaptcha.getClient(context) where context is of type android.content.Context

CAMOBAP commented 1 year ago

@Syrou in fact SDK always requested FragmentActivity, starting from 3.9.0 we just make it explicit (instead unsafe cast)

Syrou commented 1 year ago

Yeah... we are not using Fragments so, that seems like an odd approach, but we can cast it to FragmentActivity for now since AppCompatActivity extends it. And a word of caution, you probably should not bind the FragmentActivity to whatever you doing, because that will leak whenever that activity gets destroyed.

Update This PR seems to indeed fix the problem

CAMOBAP commented 1 year ago

Yeah... we are not using Fragments so, that seems like an odd approach, but we can cast it to FragmentActivity for now since AppCompatActivity extends it. And a word of caution, you probably should not bind the FragmentActivity to whatever you doing, because that will leak whenever that activity gets destroyed.

Good point, thanks for the feedback, we are always trying to improve our API. I think there is a potential to make API less dependent on FragmentActivity https://github.com/hCaptcha/hcaptcha-android-sdk/issues/127

Update This PR seems to indeed fix the problem

Thanks for confirmation