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

Implement ValidateSessionState for GitHubProvider #385

Merged
merged 2 commits into from Feb 15, 2020
Merged

Implement ValidateSessionState for GitHubProvider #385

merged 2 commits into from Feb 15, 2020

Conversation

iain-buclaw-sociomantic
Copy link
Contributor

Refactors the setting of the Authorization header into getGitHubHeader.

Refs #382

@JoelSpeed - at the risk of doing something you are already looking at, this change is untested, but looks like it should fix the referenced problem.

Copy link
Member

@JoelSpeed JoelSpeed left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this is ok, will need a changelog entry and verifying before we merge though

Thanks @iain-buclaw-sociomantic for a quick turnaround on this

loshz
loshz previously approved these changes Feb 7, 2020
Copy link
Member

@loshz loshz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM once a changelog entry is added.

Thank you and congrats on your first contribution! 🎉

Refactors the setting of the Authorization header into getGitHubHeader.

Refs #382
@iain-buclaw-sociomantic
Copy link
Contributor Author

iain-buclaw-sociomantic commented Feb 7, 2020

Added changelog entry, due to the nature of Github's alerting system, we'll have to wait at least three days for confirmation of fix.

Running this patch on one node, I see no problems with the functionality of validateToken however.

@iain-buclaw-sociomantic
Copy link
Contributor Author

I have received no further emails from GitHub so far, so knock wood it will stay that way.

@matiasgarciaisaia
Copy link

Would this be enough to release a v5.0.1?

Looks good to me, by the way.

Really thankful, @iain-buclaw-sociomantic 👍

@cpanato
Copy link

cpanato commented Feb 14, 2020

This will be released as 5.0.1? any plans to merge and release this fix? thanks!

Copy link
Member

@JoelSpeed JoelSpeed left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants