HTTP 500 error from GitHub when user is not logged in and 2FA is enabled

[TimothyJones]

When I am not logged in github in another window (session) I get 500 (Looks like something went wrong!) page instead of Authorization / Login page.

Originally posted by @lolejar in #23 (comment)

There's an issue with GitHub where it rejects the state sent by Cognito when 2FA is enabled and the user is not logged in. We did a fair bit of testing and couldn't figure out exactly what the issue was - but we think it is to do with the length of the state parameter provided by Cognito.

You can work around this by generating your own (shorter) state token and sending them on to GitHub - but if you do this, the shim needs to maintain state, and there's an extra step where GitHub redirects back to the shim, which redirects back to Cognito.

A colleague has a private fork of this repo which maintains a mapping of the Cognito states, using a dynamo DB table to do this. I'd like to look at merging it in, but I'm in two minds, because it's adding a fair bit of extra resources (and therefore impacts cost and scalability) in order to work around a GitHub bug.

Discussion welcome

[lolejar]

well the best solution if possible would be to make it a feature switch. either create dynamo db and map states or allow the old authentication. anyway me myself don't mind setting up extra resources as I have 2fa enabled and it makes life a bit harder not to be able to login straight but many people don't and for them it would not make sense to set up extra resources.

[DannyDouglass]

hi folks - moving my input here after realizing i missed this issue in #27. imo, this idp option is incomplete without supporting 2fa, regardless of who is at fault. it's a tough pill to swallow to tell users you have to login to github externally first to allow for 2fa scenarios just to access a product leveraging this well-designed approach.

in my research, this is one of the only real resources that shares a path to enabling github sso via a cogntio openid connector. we've hit a few issues with the latest cognito features including tenant claims necessary to support appsync and graphql. my company is based in sf and has some access to aws resources. we'd be delighted to help introduce the support for github 2fa.

@TimothyJones any chance i can get access to that private fork in the meantime?

[TimothyJones]

I no longer have access myself (I've moved positions since then), but I've sent the team a message. I know it includes other proprietary changes, so I'm not sure they're able to share it directly.

I agree that it would be great if this wrapper "just works" with 2FA- although it's a lot of extra infrastructure to add for what is essentially a workaround.

we'd be delighted to help introduce the support for github 2fa.

This would be extremely welcome!

If it would be helpful, I can spend some time this weekend creating a diagram of the flow for the workaround.

[DannyDouglass]

that would be killer @TimothyJones - i shot over a note with some thoughts as well.

[TimothyJones]

Update: We can get the relevant parts up on a branch here for you to experiment with. I'll try to make this happen this weekend.

I don't have the time to test it and make sure it's ready to merge in, but if you can do that work, I'd be very happy to merge whatever you come up with.

[DannyDouglass]

perfect - i'll take care of that. looking forward to contributing to this project on that note and moving fwd.

[DannyDouglass]

i've already made changes to the existing repo to better support stages - i'll put up another branch w/ those changes tomorrow

[DannyDouglass]

that SAM stage bug was annoying ;)

[DannyDouglass]

@TimothyJones can you provide me push right to the repo for a branch?

[TimothyJones]

I can't issue granular permissions without moving the repo to an organisation. Would you be able to fork and make a PR instead?