Skip to content

Added search method and improved user creation method #113

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

Open
wants to merge 5 commits into
base: master
Choose a base branch
from

Conversation

seanwlk
Copy link

@seanwlk seanwlk commented Jun 5, 2019

Check commits description for further information.

seanwlk added 3 commits June 5, 2019 18:34
I'm unsure of what happened here, but if I try this in our domain
it always returns a void list because it cannot get the forest.
As soon as i changed it to default domain it worked.
In a normal AD, the object gets created with the CN (Common Name) and
sAMAccountName different from each other. With the current process,
the function creates the user with the given sAMAccountName and puts it
as CN too, which makes it not editable later in the script since AD blocks
you from editing the CN of a "opened" object.

Example:
CN = "Walker Sean"
sAMAccountName = "seanwlk"
@zakird
Copy link
Owner

zakird commented Jun 5, 2019

In the vast majority of situations, CN is going to be the same as samAccountName. In order to not break API, I'd suggest making one of these fields optional and defaulting to the same value.

@seanwlk
Copy link
Author

seanwlk commented Jun 5, 2019

In the vast majority of situations, CN is going to be the same as samAccountName. In order to not break API, I'd suggest making one of these fields optional and defaulting to the same value.

I work for an helthcare agency and we have a total of 12000+ accounts in one shared domain and other 3 domains with the same amount of users but split between them (long story).

All these got installed by Microsoft officials and with default schema configuration the CN and the samaccount are different because in our case it's "Surname Name" and samaccount it's the tax code for the shared domain with 12k+ users, and for the other 3, it's a variation of the name+surname, for example name.surname or initial of the name + full surname. This is due to the fact that if you later search for a user having only his name it would be unpractical for both us IT and the HR.

Having spaces in the UPN/sAMAaccount it's a bad idea so couldn't use your function to make our "clean" look in the OU browser.

@zakird
Copy link
Owner

zakird commented Jun 5, 2019

That makes sense. I don't think it's at odds with making the field have a default value though.

@seanwlk
Copy link
Author

seanwlk commented Jun 5, 2019

Made the CN optional, but kept the change of argument name in the commit name -> sAMAccountName because what you are doing there is actually creating the account with it's mandatory data (account name/upn) and it's not supposed to contain spaces because you should keep pre-2000 compatibility (yes sadly we still have some, even some servers with old applications we cannot get rid of).
From how i see it, calling that "name" it's not correct since it induces you into think it's the name of the person so "Surname space Name" which is what i actually did at the beginning and later found out about this.

At the end it's up to you :D Nice job anyway!

PS
can you take a look at #112 , lost whole day on it, I'm pretty sure it's on pywin32 side the problem but you might know better since you dealt with it more then me. After this i wont bother you anymore :D

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.

2 participants