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

[JENKINS-26375] dont use domain when creating user #73

Closed
wants to merge 1 commit into from

Conversation

elordahl
Copy link

Domain user accounts are created without the domain in jenkins e.g domain\user = user.
When TFS attempts to map authors to Jenkins users it creates a user by keeping the domain and replacing the '' with a '_'. e.g. domain\user = domain_user.

This is a problem because changes are never mapped to actual jenkins users, and the associated email address with the bad accounts are always invalid e.g. domain_user@domain.

Resolving https://issues.jenkins-ci.org/browse/JENKINS-26375

Note: I could only validate this against Active Directory and LDAP configurations.

@jenkinsadmin
Copy link
Member

Thank you for this pull request! Please check this document for how the Jenkins project handles pull requests.

@mosabua
Copy link
Member

mosabua commented Jul 13, 2016

This looks good imho. +1

wdyt @olivierdagenais and @davidstaheli

@olivierdagenais
Copy link
Member

I originally wrote the code that used the domain\user to domain_user mapping because, at the time, I had configured Jenkins with IIS such that IIS used Windows Authentication and added a header before forwarding the request to Tomcat. This made it easy to match the TFVC commits to Jenkins accounts that were created by the users simply accessing Jenkins (since they were mapped as domain_user).

Another reason for preserving the mapping would be to support users from more than one domain if you can have the same user ID represent different people in the different domains, for example JENKINS\leeroy and HUDSON\leeroy are not the same person. I don't know Active Directory that well, but I do remember you can have child domains, etc.

Since there is at least one use-case for preserving this mapping, it makes me think we might want to put this change behind a global configuration option. Something like a radio button group that looks like:

User account mapping

  • Convert DOMAIN\user to DOMAIN_user
  • Convert DOMAIN\user to user

What do you think?

Thanks!

  • Oli

.replace('>','_'); // 4 replace() still faster than regex
// end stolen portion

//should put this in a utility/getter function somewhere
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great idea! Please make it so. I recommend a static method on TfsUserLookup.

@olivierdagenais
Copy link
Member

Oh and lastly, we may want to test with Visual Studio Team Services to make sure the account names from there (which I think are e-mail addresses) also map in a sane manner.

@elordahl
Copy link
Author

elordahl commented Aug 31, 2016

@olivierdagenais My employer has migrated all products out of TFS, and I am no longer in a position where i can thoroughly test this. We were using this code, via custom build, but as we are no longer, I'll let you decide what to do with it. Feel free to close this PR!

Best of luck!

@smoyen
Copy link

smoyen commented Sep 7, 2016

@olivierdagenais - the solution that would give users a global configuration option is something I would be extremely interested in. We have branched off of this plugin to ensure that the domain is striped off for our Jenkins instance (we do not have a use case where we need to use domains), but would love to begin using the official plugin again so that we can get the other updates you create!
Are there any plans to implement this change?

@olivierdagenais
Copy link
Member

Superseded by #140. Thank you for the inspiration and your contribution!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants