AD Integration - ADHelper.GetAllDomainPossibilities fails if users' UPN suffixes don't refer to their AD domain

[rozniak]

The ADHelper.GetAllDomainPossibilities() method fails if a user is passed in with a UPN suffix that doesn't refer to their AD domain (or an AD domain in general). This causes the update methods in ADBackend to fail.

Example of an environment that causes this:

• The AD domain is chicago.contoso.com - this is set in the ActiveDirectoryDefaultDomain config
• A user exists with the UPN [email protected]
• User is in a security group that is mapped to a team (in this case Git_Team_WebApps)

When updating, you'll see output in the logs showing the problem:

[Information] Searching for group "Git_Team_WebApps" in domain "chicago.contoso.com"
[Information] Looking for user with guid "xxxx" in domain "chicago.contoso.com"
[Error] Failed to create Directory context for domain "contoso.com".
System.DirectoryServices.ActiveDirectory.ActiveDirectoryObjectNotFoundException: The specified domain does not exist or cannot be contacted.
   at System.DirectoryServices.ActiveDirectory.Domain.GetDomain(DirectoryContext context)
   at Bonobo.Git.Server.Helpers.ADHelper.GetDomain(String parsedDomainName)

---

For context, the UPN suffix is set like this because of Office 365 single sign-on (and perhaps other services). I did ask our sysadmins about it and apparently it's a valid config, so I suppose it should be accounted for.

[rozniak]

I did implement a 'fix' on my own fork for this, but I had a feeling it was a bit of a bodge and in retrospect it kind of is.

This commit: rozniak@cde09b2

I decided to implement a new configuration setting ActiveDirectoryUseDomain that would basically 'always use' the specified domain inside ADHelper.GetAllDomainPossibilities(). This works of course, because I could set chicago.contoso.com in there and it would work.

I had a thought that it is probably the wrong way to go about it - the method should try the UPN anyway, and if it fails, continue to the rest of the function instead of binning out. Perhaps that would be a better idea?

[willdean]

Yes, I think in general it would be a good idea to add an additional way of finding the user, rather than having something which overrides or replaces any of the existing ways. Is that merely a matter of removing the yield break? (We could debate what order that method returned domains in, I suppose...)

BTW if your Linked-In is to be believed we're at opposite ends of the same county...

[rozniak]

You know what... this might be a non-issue - I had a look at the original source and it doesn't really make sense why the exception would make the whole thing fail. It's dealt with correctly in GetDomain() by just returning null...

The only thing is that I imagine it'll waste a lot of time trying to create DirectoryContext objects that will fail. Perhaps it is worth caching failures for domain lookups so if it tried and failed to get a DirectoryContext for a particular domain within the last x minutes, don't bother again?

The only other thing is with the logs outputting the error every time... in this case it would be expected to fail so it seems a bit annoying to output the same error every time. Unless it's worth adding a param to suppress that particular problem? (like logging it to Verbose instead)

Could make a separate issue for all this if needed.

PS - I checked since I hadn't been on LinkedIn in a while and yeah it's roughly accurate, small world I suppose 😛

[willdean]

I'm not really familiar with what this looks like when it's running, but I agree that we shouldn't log things that look like errors if the system is running normally, because that just wastes everyone's time.

We probably shouldn't have exceptions happening in a normal non-exceptional path either, because that's very expensive and potentially confusing. Is this what's happening?

[rozniak]

That is pretty much that's what I think is happening... UpdateUsers() was failing overall but that must be unrelated or something, I might have to do more looking at it.

I'm currently digging into it a bit, really it kinda stems from whether this code path is even necessary at all. Here's the gist of UpdateUsers():

• Retrieve the Git users group
• Remove users from internal user list that aren't in AD anymore by their GUID
• Enumerate Git users group members, select the UPNs of users
• Get users by the UPN <-- seems totally redundant to me since we literally just enumerated UserPrincipal objects...
• Add/update the internal user list

Without that redundant bit, the call that triggers the exception would never get made since it purely comes from the fact that the UPN suffix is not an AD domain. It's a weird config and to be honest I don't know how Bonobo would get around that if the call needed to exist without trying to establish the DirectoryContext at least once.

[willdean]

It could well be that the code has suffered from that sort of 'wikipedia effect', where bits that made sense on their own have been assembled into something completely incomprehensible.

Like I said, the AD stuff has been endless trouble, and it could well be that something has been added to fix a problem which then rendered something else obsolete but it wasn't then removed.

[rozniak]

Potentially - probably more likely than not if the layout of the code is anything to go by...

To be honest a lot of the AD stuff in general could do with a bit of a refactor, at least to get it to adhere to some kind of coding standard - the amount of typos in the Intellisense was triggering me big time.

[willdean]

Since we're in a mood for purity, I'll point out that it's "number of typos", not "amount of typos" :-)

If you feel like a clean-up then go for it - all I'll ask is

• that you don't do it if you think you won't be around to help with the support if/when some AD stuff breaks on the next release
• Don't mix a load of clean-up in with new functional changes.

I don't like the separate home-brew file-based database/cache for AD anyway. #489 was about this, though nobody still working on the project knew why it was done like that. ISTR that a lot of the AD database is effectively just cache, which you can blow away if you need to and it rebuilds from AD. But also in there is the repository table, which you can't delete so casually.

[rozniak]

Since we're in a mood for purity, I'll point out that it's "number of typos", not "amount of typos" :-)

It's a good thing we have code reviews. 😛

that you don't do it if you think you won't be around to help with the support if/when some AD stuff breaks on the next release

I should always be around to be honest, I still reply to comments on things I posted 10 years ago on YT, even if it's just to say I no longer have files or something - I really hate this kind of situation: https://xkcd.com/979/

Don't mix a load of clean-up in with new functional changes.

Well I was mostly just thinking about neatening up the existing code - making the code conform to the corefx stuff, removing the redundant calls (but keeping the resultant functionality the same).

In regards to the storage, to be honest I actually figured that all it would do with AD was import things into the internal EF schema. I did see there is another EF schema for AD (I think?) and I guess I am just as confused as you as to why it has been done that way. 😵

Perhaps worth opening a separate issue about it to track what needs doing to simplify that.

[willdean]

The AD stuff is not stored in EF, it's done separately in a bunch of json files. Although it's not great, it's probably not the highest priority to fix it. (Upgrade Git is probably the highest priority)

[rozniak]

BTW update in regards to this issue - it's still a problem when the cache expiry time is quite long and no access to Bonobo has been attempted, it'll cause this exception to be thrown next time it syncs (though UpdateUsers() won't fail).