search

strongloop / loopback-component-passport

LoopBack passport integration to support third party logins and account linking
Other
139 stars 228 forks source link

support nameID as saml profile id field #259

Closed peterwillis closed 6 years ago

peterwillis commented 6 years ago

I am not sure what saml strategies are supported here, but passport-saml uses a nameID field for the profile id as per the SAML specification.

Description

Related issues

Checklist

slnode commented 6 years ago

Can one of the admins verify this patch? To accept patch and trigger a build add comment ".ok\W+to\W+test."

virkt25 commented 6 years ago

ok to test

virkt25 commented 6 years ago

@peterwillis Can you please rebase the PR on the latest master. I've dropped support for Node 4 so CI should go all green and we can get this landed and released. Thanks for your hard work :)

shimks commented 6 years ago

@peterwillis Could you let us know if we need a test for the change you've made here? If so, please attach the test with this PR

peterwillis commented 6 years ago

passport is the whole package, passport-saml is a strategy. I assume passport-saml is the strategy that most people would configure as the module for using saml authentication in loopback-component-passport as it's the main strategy for saml authentication in passport. In that strategy, the id field is nameID. Is there a different strategy used generally used with the saml configuration? I had an alternative approach but I think the approach in this PR is better since nameID is the standard field shown in the saml specification. In the alternate approach, there wouldn't need to be a hardcoded field name just for the microsoft provider, but I didn't break backward compatibility in that commit. I would like to add some tests, I noticed that there are only tests for the ldap section of the configurator. I will try and come up with something appropriate. I think there should be tests for the other auth options too.

If anyone is interested in having a further discussion about this module, please let me know because I have some other thoughts.

peterwillis commented 6 years ago

I can't see an easy way to add tests for the part I changed. Any testing for the passport side of things would require work on passport-configurator so there is a way to test the passport strategies. It's not just the saml configuration that is affected by this, it's all the strategies. If you would like to integrate testing here then I would suggest doing it as a separate task, perhaps using this in some way? https://github.com/jaredhanson/chai-passport-strategy

virkt25 commented 6 years ago

@peterwillis I agree that this change doesn't need tests and is something that can be added as a follow up task. Thanks for your contribution. Expect a release containing the changes shortly.