Skip to content

Switched herokaiOnlyHandler to a general-purpose authCallback.#21

Open
jacobian wants to merge 1 commit intomasterfrom
authCallback
Open

Switched herokaiOnlyHandler to a general-purpose authCallback.#21
jacobian wants to merge 1 commit intomasterfrom
authCallback

Conversation

@jacobian
Copy link

No description provided.

@jacobian
Copy link
Author

Fix for #20

Copy link
Contributor

Choose a reason for hiding this comment

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

This seems right to me. reauthenticate is a bit of a misnomer, as this actually 401s for a JSON request or redirects to /auth/heroku for a non-JSON request.

I believe you do need to return here, though. I haven't looked at this code in ages :(

Copy link
Contributor

Choose a reason for hiding this comment

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

Also, you might be able to clean this stuff up by setting options.authCallback to a function() { return true; } by default and options.authCallbackFailedHandler = reauthenticate by default.

Then all you need is

options.authCallback(userSession.user) || return options.authCallbackFailedHandler(req, res, next);

@joeyjmorales
Copy link

@jclem I see you are no longer maintaining this? Can you please grant admin rights to me and @mmowris ? Working on node-heroku SSO apps and would like to pick this up. @jacobian - not sure if you ever used above PR, but have done some initial testing on the branch and had success. (I know it's been a while but fYI)

Thx!

@jclem
Copy link
Contributor

jclem commented Aug 1, 2017

@joeyjmorales I'm no longer at Heroku and don't have admin rights on this repo any longer. I'm not sure who a current maintainer is.

@jclem
Copy link
Contributor

jclem commented Aug 1, 2017

I think @dickeyxxx may be able to help you out here, though.

@jdx
Copy link

jdx commented Aug 1, 2017 via email

@jdx
Copy link

jdx commented Aug 1, 2017

Ooops I thought this was for node-heroku-client, nevermind

@joeyjmorales
Copy link

Thanks @dickeyxxx . Are you current maintainer? Wondering if we can push this PR through and/or, if it needs a maintainer, I'd be happy to chip in on this as we are using it for CSA Portal app.

Let me know. Thanks!
Joe

@jdx
Copy link

jdx commented Aug 1, 2017

yeah I can probably take ownership of it, though not until next week, I'm busy this week in Denver

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.

4 participants