exlinc/keycloak-passport

Callback not firing

garmeeh opened this issue · 12 comments

garmeeh commented

Hi, I am currently trying to use this Strategy with next-auth. I logged an issue on next-auth because the callback didn't seem to be firing for keycloak-passport. Wondering would you maybe have any insight on why it wouldn't be firing?

svarlamov commented

Hi @garmeeh, I just read through the issue that you posted in next-auth. I'm not familiar with that project directly, however, I vaguely remember dealing with the req argument issue when I was building keycloak-passport... You can get a sample of how we implement the strategy over here

We use the signature req, accessToken, refreshToken, profile, done which requires this line in the config. Although as discussed in the next-auth issue, it seems as though you're also setting that...

Beyond that, I'm not exactly certain what the cleanest solution is, given that this is largely handled by the oauth2 boilerplate.

garmeeh commented

No worries @svarlamov, thanks for the looking into it for me. The auth server is great for reference 👍

svarlamov commented

Sure, let me know if there's anything that I can do in the mainline keycloak-passport to support next-auth. Would be happy to do that as I'm sure it would benefit other users

  • 👍2
svarlamov commented

@garmeeh Were you able to find the solution to this issue? Also, any luck re: #2? Thanks!

garmeeh commented

Hi @svarlamov sorry about the delay in replying.

I'm working with @woppers to try resolve #2.

In regards to my original issue, the only chagne we had to make to keycloak-passport was to change this line

this.name = 'Keycloak';

to

this.name = 'keycloak';

I'm pretty sure this is something we could open a PR for on next-auth to add an option to not .toLowerCase() the provider name option. If we get the flow working I will add comment back here for anyone finding the issue later. I would be happy to consider this issue closed if that suits you.

svarlamov commented

I see... I'm concerned that mainlining a name change might break other implementations. Is there a particular reason why it needs to be lowercase for next auth?

garmeeh commented

I agree, making that change would probably break other implementations. I am not sure if there was a particular reason for the lowercase, but I would hope that I could open a PR for next-auth and add an option to disable the lowercase.

svarlamov commented

What is that name actually used for in next-auth?

aol-nnov commented

@svarlamov seems they pick a strategy by name and they get it from url /auth/oauth/<strategyName>

not sure why they need .toLowerCase() though...

svarlamov commented

@aol-nnov Thanks for clarifying the use case in next-auth. I'm not sure what specifically I can add here, but if we get clarity on the requirements for compatibility with next-auth I would be happy to mainline the changes so long as they don't break other impls

aol-nnov commented

Anyway, I've opened a PR on that, because no rules enforcing strategy name to be lower case were found.

nextauthjs/next-auth#51