Skip to content

Conversation

@everpcpc
Copy link
Contributor

@everpcpc everpcpc commented May 10, 2016

see also: https://github.com/jaredhanson/passport-github/blob/master/lib/strategy.js#L53

passport-github allows passing these options to support a private github enterprise instance.

lib/config.js Outdated
clientSecret: "replace me with the real value",
authorizationURL: "leave me empty to https://github.com/login/oauth/authorize",
tokenURL: "leave me empty to https://github.com/login/oauth/access_token",
userProfileURL: "leave me empty to https://api.github.com/user",
Copy link
Owner

Choose a reason for hiding this comment

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

I think that for the default values, either you leave it really empty or you use those default URLs. The strings as they are, are a bit confusing, IMHO.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

moved the descriptions to readme

@everpcpc
Copy link
Contributor Author

everpcpc commented May 12, 2016

But there seems something wrong using the github enterprise auth. (While the default github.com works well.)
I ran an auth procedure, the request logs is as follows: (I disabled the anonRead)

GET / 302 3.722 ms - 64
GET /wiki/Home 302 2.544 ms - 56
GET /login 304 354.384 ms - -
GET /auth/github 302 4.086 ms - 0
GET /auth/github/callback?code=xxxxxxxxxxxxx 302 1170.624 ms - 64
GET /auth/done 302 5.527 ms - 46
GET / 302 1.916 ms - 64
GET /wiki/Home 302 3.384 ms - 56
GET /login 304 209.308 ms - -

It seems the /auth/github/callback got the correct user but did not set it in the request, so _getAuthDone() got an undefined 'res.locals.user' .
I believe it's a issue with passport-github or passport.

@everpcpc
Copy link
Contributor Author

everpcpc commented Feb 1, 2017

close for #186

@everpcpc everpcpc closed this Feb 1, 2017
@everpcpc everpcpc deleted the github branch March 22, 2017 07:59
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants