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.

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

[connect] Login fails on mobile #300

Closed
friedger opened this issue Mar 24, 2020 · 22 comments · Fixed by #540
Closed

[connect] Login fails on mobile #300

friedger opened this issue Mar 24, 2020 · 22 comments · Fixed by #540
Assignees
Labels
bug Functionality broken effort:small Expected to take up to 1 day of integration work enhancement-p1 Critical functionality needed by many users, with no clear alternatives
Milestone

Comments

@friedger
Copy link
Contributor

Using connect library on mobile (chrome and firefox on Android) fails to handle the pending sign in.

A tab with the auth response is opened, however, the onFinish function is not called. This works on desktop e.g. for http://coronatrackerv1.s3-website-us-east-1.amazonaws.com/

Possible work around, check manually for userSession.isPendingSignin() and call userSession.handlePendingSignIn()

@hstove
Copy link
Contributor

hstove commented Mar 30, 2020

Thanks for the report @friedger !

I need to do some investigating. I wouldn't be surprised if cross-origin postMessage has issues on mobile. Does the redirectURL param still work if the callback isn't fired? If not, then that's definitely a bug.

@friedger
Copy link
Contributor Author

Does the redirectURL param still work if the callback isn't fired?

Not sure what you mean here. How could I check that?

@hstove
Copy link
Contributor

hstove commented Apr 2, 2020

See these docs: https://github.com/blockstack/ux/tree/master/packages/connect#authoptions

You can include a redirectTo option. If the callback fails, the user should be redirected to redirectTo with an authResponse param, just like how current blockstack.js works.

@markmhendrickson
Copy link
Collaborator

@hstove is this a matter of providing additional migration guidance rather than fixing a bug per se?

@ryanarndtcm
Copy link

This is coming in from a user in support who added to it above, but has not been given a response on next steps. this is impacting the coronatracker app.

Hi Blockstack support,

I was sent your way to see if we could resolve an issue we're having developing CoronaTracker (https://coronatracker.me/). A few days back, we filed #300 this issue, as we were having trouble signing in on mobile.

On Chrome iOS - we were unable to log in at all
On Safari iOS - we were only able to log in after several attempts

We don't believe this is something on our end, but if it is or you have any ideas of what it could be, please let me know. If this issue has been resolved - could you send the steps necessary to solve it?

@hstove
Copy link
Contributor

hstove commented Apr 17, 2020

Yes, the fix here is that you'll need to implement the usual userSession.handlePendingSignIn() flow inside of your app, on page load. It's possible we could have Connect do this automatically, but I'm not 100% sure that's the best idea. I would appreciate feedback on that idea, but in the meantime, I'd suggest implementing that manually in your app to get it fixed ASAP.

@SomeMoosery
Copy link

SomeMoosery commented Apr 17, 2020

So I'm still relatively new to Blockstack - are we saying that on mobile we'll have to handle Blockstack auth the "traditional" way where it opens an entirely new window etc? Or, is there a way to use both Connect and handlePendingSignIn() flow in tandem for mobile so that the users have the same experience on mobile or desktop?

@njordhov
Copy link

njordhov commented Apr 18, 2020

the fix here is that you'll need to implement the usual userSession.handlePendingSignIn() flow inside of your app, on page load.

Still got feedback that the issue remained with Connect even in an app that explicitly called handlePendingSignIn at loading time. Can this issue possibly be explained by handlePendingSignIn being called more than once?

@markmhendrickson markmhendrickson assigned hstove and unassigned hstove Apr 20, 2020
@hstove
Copy link
Contributor

hstove commented Apr 27, 2020

So I'm still relatively new to Blockstack - are we saying that on mobile we'll have to handle Blockstack auth the "traditional" way where it opens an entirely new window etc? Or, is there a way to use both Connect and handlePendingSignIn() flow in tandem for mobile so that the users have the same experience on mobile or desktop?

@SomeMoosery, yes, you'll still have to handle it the 'traditional way' on most mobile browsers. This is because mobile browsers don't support the cross-origin messaging that we do between the original app and the popup. We still have a fallback that supports redirect-based auth, like how it used to work.

Still got feedback that the issue remained with Connect even in an app that explicitly called handlePendingSignIn at loading time. Can this issue possibly be explained by handlePendingSignIn being called more than once?

@njordhov do you have an example?

@njordhov
Copy link

njordhov commented Apr 28, 2020

Can this issue possibly be explained by handlePendingSignIn being called more than once?

@njordhov do you have an example?

The mobile login failure is NOT explained by an app calling handlePendingSignIn more than once.

The CoronaTracker app uses my react-blockstack package, which sets up a handlePendingSignIn callback before Connect is initialized. I suspected this could cause a conflict, but the CoronaTracker devs tested out this hypothesis by disabling the react-blockstack initialization, and it made no difference, Connect login still failed on mobile without react-blockstack initialization, just as it does in apps that doesn't use the react-blockstack package.

BTW: Is handlePendingSignIn actually supporting more than one callback at a time? If not, perhaps it should, so there can be multiple subscription to the authentication state change.

@njordhov
Copy link

njordhov commented Apr 28, 2020

Has the Connect team been able to replicate the problem of login failing on mobile? I hear login still fails on mobile when using Banter.

@muneebm
Copy link

muneebm commented Apr 30, 2020

Mobile login is not working even if you have redirectTo param in AuthOptions.
I think it's because didSendMessageBack in finalizeAuthResponse below could be set to true even if the source.postMessage is not successful.
https://github.com/blockstack/ux/blob/f1921a7b109afb6c149e5f67df33ef52867cab96/packages/app/src/common/utils.ts#L53-L78

@hstove
Copy link
Contributor

hstove commented Apr 30, 2020

Good catch @muneebm . I have an idea for how I could fix that.

@timstackblock
Copy link
Contributor

@friedger @muneebm @njordhov @SomeMoosery

Hey guys I ran through mobile connect login on Android Chrome and it seems that the issue has been resolved.

Please see the video in the link below and let me know if there is anything else needed or can we close this issue out?

http://somup.com/cYhIoXjrET

@SomeMoosery
Copy link

Interesting, especially since the link you followed is from an old build of the CoronaTracker app so wouldn't have been due to anything on our end. Unsure the status within Blockstack though :)

@muneebm
Copy link

muneebm commented May 14, 2020

@hstove hstove closed this as completed Jun 16, 2020
@ryanarndtcm
Copy link

@hstove @timstackblock sounds like its not working again

image

@markmhendrickson markmhendrickson added this to the 2020 W27-W29 milestone Jun 30, 2020
@timstackblock
Copy link
Contributor

Let me take a look

@BlockSurvey
Copy link

BlockSurvey commented Jul 7, 2020

Hi @markmhx and @hstove , Login is not working on mobile. I am attaching the authOptions of BlockSurvey below, take a look. Let me know, if anything I need to change. Thanks.

let authOptions = {
  redirectTo: '/dashboard',
  manifestPath: '/manifest.json',      
  sendToSignIn: false,
  userSession: this.userSession,
  appDetails: {
    name: 'BlockSurvey',
    icon: 'https://blocksurvey.io/assets/images/logo/blocksurvey-logo-login.svg'
  },
  finished: ({ userSession }) => {        
    window.location = "https://blocksurvey.io/dashboard";
  }
};

@hstove
Copy link
Contributor

hstove commented Jul 8, 2020

@BlockSurvey @muneebm We have newer docs for supporting the redirect fallback here: https://docs.blockstack.org/develop/connect/overview.html

@BlockSurvey
Copy link

@hstove , Thanks for your reply.

We have fallback also, attaching the code below. We observed that, if we use finished callback, redirect will not be considered. So redirect is not even happening, since below code is not getting executed. Thanks.

// BS Login Module
if (this.userSession.isSignInPending()) {

  // If it is in progress
  this.userSession.handlePendingSignIn()
    .then((userData) => {
      // Always redirect to dashboard
      window.location = <any>(Constants.DOMAIN_URL + "/dashboard");
    });
}

@dantrevino
Copy link

dantrevino commented Jul 10, 2020

I'm redirecting after my handlePendingSignIn() and run into the same issue. My bigger issue is that on mobile, the app stays in the new popup window, rather than closing and going back to the original calling window.

site:
https://webby-daily.runkodapps.com/

video/demo:
https://photos.app.goo.gl/YmBtn3RaV6NfivqD9

    if (userSession.isUserSignedIn()) {
      this.setProfile();
    } else if (userSession.isSignInPending()) {
      userSession.handlePendingSignIn().then(() => {
        this.setProfile();
        // window.location = window.location.href.split("?")[0];
      });
    }
  ...

@hstove hstove assigned timstackblock and unassigned hstove Jul 29, 2020
@markmhendrickson markmhendrickson added this to Ready for design in UserX Sprint (2020 W33-W35) via automation Aug 12, 2020
@markmhendrickson markmhendrickson moved this from Ready for design to In development in UserX Sprint (2020 W33-W35) Aug 12, 2020
@markmhendrickson markmhendrickson linked a pull request Aug 12, 2020 that will close this issue
@hstove hstove added enhancement-p1 Critical functionality needed by many users, with no clear alternatives effort:small Expected to take up to 1 day of integration work labels Aug 12, 2020
@markmhendrickson markmhendrickson moved this from In development to Ready for or in QA in UserX Sprint (2020 W33-W35) Aug 12, 2020
@hstove hstove self-assigned this Aug 19, 2020
@hstove hstove moved this from Ready for or in QA to In development in UserX Sprint (2020 W33-W35) Aug 19, 2020
@markmhendrickson markmhendrickson moved this from In development to Code approved in UserX Sprint (2020 W33-W35) Aug 19, 2020
@markmhendrickson markmhendrickson moved this from Code approved to Ready for or in QA in UserX Sprint (2020 W33-W35) Aug 19, 2020
@hstove hstove moved this from Ready for or in QA to In development in UserX Sprint (2020 W33-W35) Aug 19, 2020
UserX Sprint (2020 W33-W35) automation moved this from In development to Done Aug 20, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Functionality broken effort:small Expected to take up to 1 day of integration work enhancement-p1 Critical functionality needed by many users, with no clear alternatives
Projects
None yet
Development

Successfully merging a pull request may close this issue.

10 participants