ctm / mb2-doc

Mb2, poker software
https://devctm.com
7 stars 2 forks source link

GitHub login doesn't work #1381

Closed ctm closed 2 months ago

ctm commented 2 months ago

Fix the warning about the nick field of GitHubLogin not being used and make sure that people are able to do a GitHubLogin with and without a nick.

In theory, this is trivial, although I think to test it I have to manually go in and monkey with the local database.

FWIW, I've known about this for a while; the warning sticks out like a sore thumb.

ctm commented 2 months ago

This is due to the ReconnectingWebSocket not being connected on the /github?code=… route. Fixing it is basically just a matter of looking at what our pre-factor code did to set up the ReconnectingWebSocket for that route. My guess is it didn't do anything explicit (since I don't remember removing anything explicit), but the old structure "just fell through" into a connected state.

ctm commented 2 months ago

Some of the problem is due to my fix for people getting logged out on a redeploy (#1379). When I fixed that regression, I also fixed a bug that was causing mb2 to reload itself even when it didn't need to after a server restart. That fix delays setting up the ReconnectingWebSocket until after a fetch is done to check the etag so that we'll know the etag in the future. That works fine in the normal case but gets in the way of the GitHub redirect, which expects the web socket to already have been set up.

I'll have to think about this, but the two obvious solutions are either to defer the use of the websocket until the fetch finishes or to do them both in parallel. The latter makes me nervous though, because it may introduce a race condition that will never fail with the current deploy model but could fail if we change how we do hot reloading. However, if we change how we do hot reloading, I'm not convinced that deferring the websocket use is safe, either. I think the only safe thing would be if we were handed the etag of what we're running and I don't think we are. Ugh.

The reason the pre-factor code worked is we never picked up the etag initially, so when it came to check whether we had a new version or not, we'd always think that we had a new version, because we'd be comparing Some(new_version_etag) to None.

Anyway, if I revert the check_for_update parameter to false in the ReconnectingWebSocket::new invocation, logging in via GitHub without a nick works, and I haven't yet checked to see if creating a new account via having a nick specified works.

ctm commented 2 months ago

Here's my fix:

        // Choosing to not check for updates if there's an action is a
        // bit of a hack.  There used to be a bug where we simply
        // wouldn't fetch the etag soon enough to make it useful to
        // avoid a reload when the wasm file *didn't* change.  The fix
        // for that broke logging in via GitHub.  This hack fixes that
        // regression.
        let check_for_update = ctx.props().action.is_none();

It's a compromise, but I may be redoing the OAuth workflow before too long, so it's easily good enough for now, since it's better than what we had before I introduced the regression.

Deploying now.