Implement GitHub Enterprise LDAP API #1099
Conversation
|
wow, cant believe the builds passed straight up. not even one mismatched xmldoc comment 😀
|
|
😂 |
…ter turns into ldap_d_n instead of ldap_dn) Get rid of LdapTeam and LdapUser, the regular Team and User objects contain ldap_dn field if LDAP sync is enabled
…eplaced with generic values as we cant reveal our own domain details
ryangribble
force-pushed
the
enterprise-ldap-api
branch
from
4e3d243 to
8be8c41
Compare
|
@ryangribble thanks for those notes, very informative!
Well that's certainly an interesting undocumented feature! Hooray for JSON. Yeah, this seems fair given how the server is behaving. cc @haacked for objections (if any)
👍
I'll have a chat with the team and see if this is supposed to be the behaviour here.
👍
This seems plausible.
Yeah, I appreciate the complexities here. Will have a chat with the team and see if they have any thoughts on this so I can also test out LDAP behaviour. |
| /// </summary> | ||
| [SuppressMessage("Microsoft.Naming", "CA1704:IdentifiersShouldBeSpelledCorrectly", MessageId = "Dn")] | ||
| [SuppressMessage("Microsoft.Naming", "CA1709:IdentifiersShouldBeCasedCorrectly", MessageId = "Dn")] | ||
| public string LdapDn { get; protected set; } |
There was a problem hiding this comment.
The suppressed messages here and my OCD feelings about abbreviated names makes me wonder if this is something we could do better. Or do you think LdapDn more broadly acceptable than LdapDistinguishedNames?
There was a problem hiding this comment.
Alternatively, I think this could be added to the CustomDictionary.xml at the root of the repository.
There was a problem hiding this comment.
This could just be my inexperience with the way these things are working.
The Microsoft Code Analysis stuff wanted me to put LdapDN but when you do that, the json serialiser tried to look for field called ldap_d_n so in order to get the API field ldap_dn deserialized I had to use ldapDn on the c# side, and suppress the warnings about "Dn" not being spelled or cased correctly.
I didnt realise we could "control" what words are allowed. Does that apply to the Microsoft Code Analysis stuff or the API field formatting stuff, or both? There seems to be heaps of Suppress rules int he codebase for spelling and abbreviations, perhaps there needs to be a concerted effort to get rid of as many as possible, in favour of adding recognized words to this custom dictionary?
I also wasnt sure if I could set what the "api name" of a given property is (ive seen it done on Enum entries, but then also seen mention that that method didnt work on both serialisation and deserialization). If that is indeed possible then I could rename the field LdapDinstinguishedName and just "tag" it as ldap_dn for API purposes. If this is posible, can you point me at a good example?
There was a problem hiding this comment.
I also wasnt sure if I could set what the "api name" of a given property is
Yes, it's possible via the Parameter attribute. Here's one example.
There was a problem hiding this comment.
I didnt realise we could "control" what words are allowed.
That's done via the CustomDictionary.xml file (it's in the "Build" solution folder). I'd rather not add DN as a custom word though and use the full name for the C# API and use the ParameterAttribute to control the JSON name.
…_dn")] to set the API field name
|
OK I have renamed to |
And that worked? |
yep! Had a bit of a delay due to not getting a chance to rerun the integration tests against an LDAP enabled GHE server, but that's all done now and the renamed fields have worked fine 😀 Ive alsomerged latest master in... builds have passed for all platforms I think we should be good to hit the button on this one @haacked @shiftkey |
|
|
||
| var endpoint = ApiUrls.EnterpriseLdapTeamMapping(teamId); | ||
|
|
||
| return await ApiConnection.Patch<Team>(endpoint, newLdapMapping); |
There was a problem hiding this comment.
I think we can get away with dropping the async/await pair here, right?
|
@ryangribble just calling out a couple of cleanup opportunities with |
|
ok I can remove those... I've actually done that on alot of the API methods ive added in the last few pull requests. Want me to raise another PR to clean them all up? eg: |
Let's come back to this after we've got some of these existing PRs closed out. It's not urgent. |
Implement GitHub Enterprise LDAP API
|
|
Status
🚀 Ready to merge
This is built on top of #1073 and #1095 which are not yet merged.Sorry for the difficulty this poses to reviewing changes... only the top 5 commits are actually relating to this PR. I will rebase this as soon as my other ones are merged. I don't want to slow down even if the other PR's arent merged yet! 😀In the meantime it's still useful to put this up for any review/comments that aren't scared off by all the unrelated commits... plus I can make sure AppVeyor/Travis are happyHere's a diff link showing the consolidated changes for what will be in just this PR:TattsGroup/octokit.net@6d9b5f4...4e3d243Implement Enterprise LDAP Client
https://developer.github.com/v3/enterprise/ldap/
Fixes #1022
Includes normal and reactive implementations plus unit and integration tests for both
Notes
I feel an explanation of some of these changes is necessary.
ldap_dn property on User and Team
Originally I had implemented
LdapUser : UserandLdapTeam : Teamclasses inheriting from the base objects, and adding the one extraldap_dnfield, and using these in the response to the LDAP mapping api commands. However I later found that when a GitHub Enterprise instance has LDAP enabled, all requests that return aUserorTeameven the general GET calls, include aldap_dnfield, so I have addedLdapDnfield to theUserandTeamresponse objects, the xml doc notes it is GitHub Enterprise only. This seemed preferable than having to have special types when you wanted the ldap field and it shouldnt hurt any non enterprise users having this null property on those objects.Added
Post<T>(uri)override toApiConnectionandConnectionclassesThe LDAP Queue Sync job commands are a POST but do not contain a body/object. However they do provide a (admittedly trivial) response object that needs to be deserialized. Currently all the overrides of
Post<T>()require a body to be passed in and assert it is not null. There is aPost()that doesnt need a body, but that returns aHttpStatusCodeand doesn't handle deserializing to a<T>response object. So I added overrides toApiConnectionandConnectionclasses to suit this situation (POST with no body, that returns a<T>response object).Added
CreateEnterpriseTeamContexthelperSimilar to the
RepositoryContexthelper, I added a helper that allows the integration tests to create a team to run the tests, then clean it up afterwards.A note about LDAP call behaviour
This was a bit quirky so I thought it worth mentioning.
UpdateUserMapping- User must exist (obviously) and the specified LDAP distinguished name must exist (GitHub appears to validate this against the LDAP source)UpdateTeamMapping- Team must exist (obviously) and specified LDAP distinguished name is NOT validated. It can be set to something that doesnt exist and is regarded as a succesful API call (and the new LDAP binding is visible in the team settings on the gui etc).(The team will then be disabled on the next LDAP sync when it realises the group is invalid)
Tests
The integration tests require a GitHub enterprise instance so wont run as part of Octokit builds currently. Futhermore this API requires the GHE instance has LDAP binding enabled. Also note that I have had to censor the "distinguished name" properties used in the integration tests so they arent company specific. The integration tests also do NOT run against the "current user" because you dont want to mess with changing the LDAP binding on the user running the tests and potenitally disabling the account or otherwise losing access... So the integration tests have a
_testUserproperty declared at the top of the class (at the moment that user has to exist, as the method to add users is not implemented... though even when it was, in an LDAP enabled server any user you create would have to have a valid LDAP binding in the first place i suspect!)So with all those caveats aside, the integration tests are passing on my test GHE instance with LDAP binding to a Windows AD domain, after I set real/correct values for
_testUser,_distinguishedNameUserand_distinguishedNameTeamproperties. I didn't feel like for this specific functionality, it was worth adding yet more environment variables to the integration test helper stuff. Even if Octokit CI builds end up having an Enterprise server to run against, if you really wanted to run the LDAP tests you would need a SECOND enterprise server with LDAP enabled. It seems fairly low on the priority list IMO. But at least this API is now implemented and anyone who wants/needs to use it can. We just potentially wont have frictionless integration test execution...