apostrophecms / passport-bridge

MIT License
1 stars 0 forks source link

Compatibility issues with passport-auth0 #12

Open aschlumpf opened 1 year ago

aschlumpf commented 1 year ago

To Reproduce

Step by step instructions to reproduce the behavior:

  1. Install passport-auth0 and to use with this package.
  2. Attempt to authenticate a user by navigating to /auth/auth0/login.
  3. Get a JS error, "callback is not a function" when the verify function is executed.

Expected behavior

Compatibility with passport-auth0, out-of-the-box or via configuration to get around any unexpected blockers.

Describe the bug

It seems that the package is not compatible with passport-auth0. Auth0 is the identity provider our team is required to use, and it seems like their verify function has a different number of arguments, for the purpose of "ID token validation." You can see their code here: https://github.com/auth0/passport-auth0/blob/master/lib/verifyWrapper.js#L15

Notice that the verifyWrapper takes 5-6 arguments, depending on configuration, while the verify function in this package is expecting 4-5 (reference: https://github.com/apostrophecms/passport-bridge/blob/main/index.js#L167). The difference here is that passport-auth0 has an extraParams parameter, which seems to map to the decoded JWT for the use of ID token validation.

Details

Version of Apostrophe @apostrophecms-pro/multisite v3.9.4

Version of Node.js: Node.js v16.16.0

Server Operating System: macOS 12.5.1

Additional context:

passport-auth0 is not the "official" Node.js plugin for auth0 anymore, but it is still receiving support for fixes. Perhaps the gap here is because of that, I am not sure. If this is an issue I should take up with the passport-auth0 package, let me know, however this is a blocker for my project.

Screenshots I can provide screenshots, just please contact me directly.

aschlumpf commented 1 year ago

I have verified that the argument discrepancy for the verify function is the issue by forking passport-auth0 and running that fork within my CMS.

You can see what I did here: https://github.com/aschlumpf/passport-auth0/pull/1/commits/56ccb3d590b1ac308c2b2772578dc650175af7c3

I can try to PR this change into the passport-auth0 project, but it seems like the arguments in the verify function may not be a bug from reading their README: https://github.com/auth0/passport-auth0#customization

boutell commented 1 year ago

Yes, it is difficult to be compatible with 100% of passport backends because of the (somewhat maddening) lack of standardization on this issue in particular. Modules that follow the same general patterns as the common oauth-based modules written by passport's author will fare best "out of the box."

The best resolution is probably for us to provide a new per-strategy option allowing you to supply a custom function to remap the parameters for success in these situations.

aschlumpf commented 1 year ago

That makes sense that there are no standards for this issue, I was unsure if there was a standard that passport-auth0 was breaking here. The custom function approach sounds like a good solution for these cases, since I am guessing the most used passport libraries are not breaking this unwritten convention.