Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

#267 get all groups for ms-ad #272

Merged
merged 1 commit into from May 30, 2019

Conversation

Projects
None yet
4 participants
@Tyler-RCSD
Copy link
Contributor

commented Nov 28, 2018

Description

groupSearch results were based on the _groups ldap attribute and ignored the memberOf attribute which is traditionally used in Active Directory. This fix makes sure both work.

Related issues

Checklist

  • New tests added or existing tests modified to cover all changes
  • Code conforms with the style
    guide
@slnode

This comment has been minimized.

Copy link

commented Nov 28, 2018

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

@ermakovov

This comment has been minimized.

Copy link

commented May 24, 2019

@virkt25, @jannyHou, @b-admike, @dhmlau, hi!

Can you approve this pull request? This is very important for our current project!

@jannyHou
Copy link
Contributor

left a comment

@Tyler-RCSD Thank you for the contribution.
I am not familiar with the ldap config, is memberOf a common field in the ldap standard or a field defined by yourself?

If it's a field only used in your project, then the fix should be "honor custom ldapAttrName", you can specify it in the configuration file, then use if(ldapAttrName === '_groups' || config.ldapAttrName).

And can you also add a test case?

Let me know if you have other questions and I can help you land the PR.

@jannyHou

This comment has been minimized.

Copy link
Contributor

commented May 24, 2019

@slnode test please

@jannyHou jannyHou self-assigned this May 24, 2019

@Tyler-RCSD

This comment has been minimized.

Copy link
Contributor Author

commented May 24, 2019

memberOf is the standard attribute within Microsoft Active Directory for groups. I think anyone would expect that this should work out of the box with AD without having to configure a custom ldapAttrName for groups.

I will see if I can figure out a test case next week. Honestly, I'm not all that familiar with writing test cases so if someone has a suggestion please let me know.

@jannyHou

This comment has been minimized.

Copy link
Contributor

commented May 24, 2019

@Tyler-RCSD Got it. I quickly checked the test for configurator, adding a new one should be simple, you can follow this test case to add a similar one for memberOf 🙂

@Tyler-RCSD

This comment has been minimized.

Copy link
Contributor Author

commented May 27, 2019

@jannyHou Can you review and see if I've done this properly?

@jannyHou

This comment has been minimized.

Copy link
Contributor

commented May 30, 2019

@slnode test please

@jannyHou
Copy link
Contributor

left a comment

LGTM 🚢 Thank you for the contribution again 🎉 !

@jannyHou jannyHou merged commit 2ed93d8 into strongloop:master May 30, 2019

12 checks passed

Commit Linter commits are all properly formatted
Details
PR Linter PR is up to date
Details
[cis-jenkins] PR Builder Build finished.
Details
[cis-jenkins] x64 && linux && nvm,10 Success! (9610f17)
Details
[cis-jenkins] x64 && linux && nvm,6 Success! (9610f17)
Details
[cis-jenkins] x64 && linux && nvm,8 Success! (9610f17)
Details
clahub All contributors have signed the Contributor License Agreement.
Details
loopback-component-passport Success! (9610f17)
Details
loopback-component-passport/node=4.x,os=windows Success! (9610f17)
Details
loopback-component-passport/node=6.x,os=windows Success! (9610f17)
Details
pr-builder
Details
security/snyk - package.json (StrongLoop) No manifest changes detected
@Tyler-RCSD

This comment has been minimized.

Copy link
Contributor Author

commented May 30, 2019

Thanks @jannyHou . How often do versions get released and pushed to npm?

@jannyHou

This comment has been minimized.

Copy link
Contributor

commented May 30, 2019

@Tyler-RCSD Released 3.11.0 for it 🙂

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.