Federation Auth: Unauthorized

[stupiduglyfool]

Bonobo Version: 5.0.1.0. Issue: Attempting to login with a federation setup takes me to an unauthorized page.

Details:

I already have an adfs environment setup and working, it is currently operational with other relying parties and works well with for example IdentityServer3.

I have created a relying party for bonobo, I presume it requires a passive setup, the federation guide does not specify what the Passive URL should be or which adfs profile to use.. these are the values I have used:

ADFS 2.0 Profile No token encryption certificate Enable support for WS-Fed Passive Protocol URL: https://git.fqdn/bonobo.git.server RelyingPartyIdentifier: urn:bonobo

I have these values in my web.config:

<add key="AuthenticationProvider" value="Federation" />           
<add key="MembershipService" value="ActiveDirectory" />
<add key="ActiveDirectoryDefaultDomain" value="mydomain.co.uk" />
<add key="ActiveDirectoryBackendPath" value="~\App_Data\ADBackend" />
<add key="ActiveDirectoryMemberGroupName" value="Developers" />
<add key="ActiveDirectoryTeamMapping" value="Team1=Developers" />
<add key="ActiveDirectoryRoleMapping" value="Administrator=Senior Developers" />

<add key="FederationMetadataAddress" value="https://adfs.fqdn/federationmetadata/2007-06/federationmetadata.xml" />
<add key="FederationRealm" value="urn:bonobo" />

When I goto https://git.fqdn/bonobo.git.server I am prompted by adfs for my credentials.. which I enter credentials of a user I know to be in the above groups, but unfortunately I get taken to an Unauthorized page. Additionally there doesn't seem to be any useful information in any logs.. but I admit I cannot see any info in the documentation about increasing log levels.

Any help or guidance would be appreciated.

[stupiduglyfool]

So I've grabbed the latest code and got digging.

During debugging it seems I am successfully logged in, I have an Identity, IsAuthenticated is true.. but I do not have any roles despite having the config above set up as per the documentation.

WebAuthorizationAttribute.OnAuthorization IsInRole(Definitions.Roles.Member)

Always returns false, additionally ADRoleProvider is never called, therefore there is nothing to relate my identity to the Member or Administrator roles and thus I get forwarded to unauthorized. Looking through the commit history of other Role providers I see this has had a bit of a makeover recently, so this maybe an oversight.

Although the federation documentation says that no special claims are required, I have discovered if I add the relevant claims in adfs. Upn/Email/Name, and Group Membership claims linking AD groups Developers, SeniorDevelopers to bonobo custom roles Member,Administrator respectively..

I can now log in via adfs and it is respecting the relevant Admins/Normal users as expected.

I had a good look through the code, it is looking in great shape..

One issue I did notice whilst I was trying out various configurations.. in ADBackend.cs approx line 153, will throw an exception if you have nested groups: memberGroup.Members.Cast

I have fixed it in my code locally: memberGroup.Members.OfType however I guess you may want to respect nested groups and recurively build your list up.

Hope this helps.

[larshg]

@stupiduglyfool Can't you create a PR with the changes you made, so it can be reviewed and properly merged into the master?

[stupiduglyfool]

Yeah for any changes I make I plan on submitting PR.

I do want to query whether the current federation behaviour I've noted, and the workaround via configuring adfs claims, which is against the documented approach, is expected...?

[larshg]

Ah, yes okay... Unfortunately I haven't tried to setup a ADFS and therefore cannot test it. But the part about nested AD groups could maybe go in a separate PR - that I can test, and @hmmwhatsthisdo might want to try it out too :)

[DerKamii]

Is it possible that this error fit's for "just" Active Directory Provider? I can't set up "Cookie" Auth with "AD" Provider. Whenever i try to clone something i get "Fatal: authentication failed." But i can log in into the site with my account@domain.local (where i my domain isn't called domain... Just to make sure i configured it right)

Cookie and Internal settings are working just fine. So "admin:admin" is working as expected.

Thanks for advise.

[RedX2501]

This seems to be related to #419 Have you had any chance to further investage this?

[stupiduglyfool]

Hi, I still aim to submit a PR, just been a bit busy over christmas.

[RedX2501]

@stupiduglyfool Sorry to bother you but what about that PR?

[stupiduglyfool]

Hi, I've taken a look at the recent code in order to produce a PR, I notice that there is a significant refactor occurred which means my code changes no longer cleanly applies to yours, I have compared my changes against the previous changeset and determined there was only minimal changes anyway, and it is possible to bring them to the latest code, I also see you already made the fix to the ADBackend cast.. I think this was the main bug.

In testing the new code changes against my adfs setup, I notice there are some new problems..

In UserExtensions.cs the Id method is now throwing a FormatException because it has been changed from a String to a guid ecf37339ecd807cac02de7d509939618e571db4f but the claim requested is still upn, isn't a UPN usually in the format username@domain?

FYI: I've attached two images showing my adfs setup, and bonobo config which I found critical to getting this working previously.. as I mentioned despite the documentation stating no specific claims were needed, I found stepping through the code with visual studio debugger, things do not work without these.

I am happy to continue fine tune things with my adfs setup, and hopefully produce a PR, if you could provide some guidance on how to move forward with the claims issues.

[willdean]

Is there a claim which would conventionally be used for a GUID-style ID? (Rather than upn?)

[stupiduglyfool]

Apparently the ldap attribute required is ObjectGuid.. http://serverfault.com/questions/564959/send-objectguid-as-an-ad-fs-2-0-claim

I will give this a try..

[stupiduglyfool]

I now have it working with the latest git code, without making any sourcecode changes, the recent changes you have made to the adfs, team and accounts pages seem to have cleared up any problems.

However I do have some guidance here for your documentation..

• How to convert objectguid
• How to setup a string transformation rule
• String processing in adfs claims
• How to install custom stores

Following the above links I've managed to develop a solution to allow me to configure an adfs relying party to automatically convert the ObjectGuid to a guid and send it back as UPN, this way I don't need to make any code changes in bonobo.. although maybe in the future you want to use the PPID claim type instead of UPN.

I've attached the code I used to produce the adfs claims rule, I also included the compiled dll, the last link above contains guidance on how to install it: AdfsStringProcessing.zip

I discovered with the latest code you require more claims to be configured in adfs, so I've got a screenshot of my full adfs setup here:

Finally, I found that I needed to fully delete the generated ADBackend, and bonobo database, restart the webserver, before the accounts/teams were successfully shown.

[willdean]

Well impressed...

Could we take the ObjectGuid as Base64 into Bonobo and convert it in there, so that people don't need to do it in the server set-up? (Can you tell I've never looked at the federation stuff at all...?)

[stupiduglyfool]

Thats an option, I didn't think you'd want to do that, because ADFS is only one source of claims..

[willdean]

I was just wondering if a claim called ObjectGuid turned up from an external source we could convert that internally.

I am concerned that we're misusing upn anyway, which may be a long-term source of confusion even if we can get away with it within the application.

[willdean]

After a quick google, it seems ClaimTypes.PPID (Private Personal ID) might be a better place to put the upn than in the upn claim.

Does that make any sense?

[stupiduglyfool]

I was typing a very similar message..

[stupiduglyfool]

In this case I would suggest ObjectGuid = PPID, rather than UPN.

[willdean]

What I'm struggling with now is the PPID is not defined in the version of ClaimTypes we're using. And we're using the now-recommend version, from System.Security.Claims, rather than the older one.

Obviously it's just a magic string, so it doesn't matter who defines it, but it makes me wonder if we're on the right track, or if that's just an accidental omission from the new ClaimTypes list.

[stupiduglyfool]

There is a different list, in another namespace:

https://msdn.microsoft.com/en-us/library/system.identitymodel.claims.claimtypes.ppid(v=vs.110).aspx

[willdean]

Yes, but that name space defines different Claims class and everything else and is apparently superseded.

Just found this, which makes some sense: http://stackoverflow.com/questions/24892222/using-claims-types-properly-in-owin-identity-and-asp-net-mvc

The answer there implies NameIdentifier for the guid, and Name for the username.

[willdean]

That SO question and the two answers leads me to:

                result.Add(new Claim(ClaimTypes.NameIdentifier, user.Id.ToString()));
                result.Add(new Claim(ClaimTypes.Name, user.Username));
                result.Add(new Claim(ClaimTypes.GivenName, user.GivenName));
                result.Add(new Claim(ClaimTypes.Surname, user.Surname));
                result.Add(new Claim(ClaimTypes.Email, user.Email));
                result.Add(new Claim(ClaimTypes.Role, Definitions.Roles.Member));

For the GetClaimsForUser call.

What do you think?

[larshg]

I also looked into it originally and tried to find the proper one for passing a (GU)ID - but since bonobo used the UPN claim for the ID - I kept using that.

It does seem fine to use the NameIdentifier :)

[stupiduglyfool]

It looks ok to me, however you are currently using Name, NameIdentifier for other purposes in UserExtensions, so that will need rewiring.

[willdean]

Here's another SO vote for NameIdentifier

http://stackoverflow.com/questions/5814017/what-is-the-purpose-of-nameidentifier-claim

Extremely hard to look up in the first place but seemingly easy to confirm once you've found it.

It looks ok to me, however you are currently using Name, NameIdentifier for other purposes in UserExtensions, so that will need rewiring.

Yes, I'll look at them all.

[stupiduglyfool]

I think we need to get consistency across the different providers.

[willdean]

@stupiduglyfool I've just pushed this change to a new branch on the main github repo, called NewClaims - would you mind trying that and seeing if it works more comfortably with ADFS? Obviously it screws-up everyone's cookies, which is something that will need to be dealt with.

You'll probably still need to do the conversion of the GUID, but into NameIdentifier now.

[stupiduglyfool]

@willdean seems to work ok

These are the current adfs claim rules to make everything work:

Passthrough rules: (LDAP attribute -> Outgoing type) Given-Name -> 'Given Name' Surname -> Surname E-Mail-Addresses -> E-Mail Address 'Token Groups - Unqualified Names' -> Role ObjectGuid -> http://temp.org/identity/claims/adObjectGuidBase64org User-Principal-Name -> Name

Role Rules: (Outgoing claim value -> Outgoing claim type) Member -> Role (Mapped to an AD group which determines if a user is a member) Administrator -> Role (Mapped to an AD group which determines if a user is an admin)

Transform Rules: 'http://temp.org/identity/claims/adObjectGuidBase64org' -> 'Name Identifier' (Looks for the outgoing claim type of http://temp.org/identity/claims/adObjectGuidBase64org and invokes the string processing for the value to convert base64 guid to string)

[willdean]

Well, does that seem like an improvement?

[stupiduglyfool]

Yes, now I need one less claim configured, it makes the config more succinct than the previous attempt. Its also improved because now we've put some thought into the correct usage of the claim types.. so should avoid confusion when people come to configure their relying party for bonobo.

[stupiduglyfool]

I think it would be beneficial to remove the dependency of the transform rule.

[stupiduglyfool]

I think there maybe a race condition between the population of the adbackend users + teams. If I purge the adbackend folder occaisionally the json file has an array of null users.

[willdean]

I think it would be beneficial to remove the dependency of the transform rule.

I am wondering if all we need to do there is to make the Id() extension method smart enough to return a Guid from either a string guid or from a base64 guid. Then the claim would have the untransformed guid in your setup, but the app would always just see a guid.

I am not clear enough about how the whole process works to know if that's sensible, or if there isn't some better point at which to do the transform.

[willdean]

I think there maybe a race condition between the population of the adbackend users + teams. If I purge the adbackend folder occaisionally the json file has an array of null users.

There is a Parallel.Invoke in ADBackend.cs which I think may have been a little ill-advised anyway.

Could you try replacing that with sequential set of three calls?

[stupiduglyfool]

I think conversion based on its format sounds like a nice solution, either that we could add an auth method claim, and use that to decide what the incoming id format is expected to be. Other than the solution I have now, I don't believe there is an earlier opportunity to make that translation.

I spotted the Parallel code and I agree it is the cause of this problem.

[willdean]

I'll remove that Parallel.() from master now - I've been giving it angry glances ever since I first looked at the Bonobo code...

[willdean]

@stupiduglyfool I've just committed code onto the NewClaims branch which should cope with the NameIdentifier claim containing a Base64 guid rather than a ToString guid.

Can you see if that lets you drop the translation at your end? (Obviously you'll still need to map ObjectGuid to NameIdentifier)

[stupiduglyfool]

Yeah thats working without the transformation.

[willdean]

@stupiduglyfool If you have a moment, could you please try the code in master - this now uses the claims we experimented with here, and the built-in Guid/base64 translation. In addition, we should be able to cope with the username arriving in UserPrincipleName instead of Name, so that's another mapping which might look a bit simpler.

[larshg]

@stupiduglyfool is this solved with the latest 6.1?

[stupiduglyfool]

@larshg Sorry for slow response, I will take a look at some point this week and let you know!

[larshg]

@stupiduglyfool no worries :) - thanks for responding :)