Bug 9486 - samba-tool classicupgrade don't create userPrincipalNames
samba-tool classicupgrade don't create userPrincipalNames
Status: ASSIGNED
Product: Samba 4.1 and newer
Classification: Unclassified
Component: Tools
unspecified
All All
: P5 normal
: 4.3
Assigned To: Andrew Bartlett
Samba QA Contact
:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2012-12-09 22:03 UTC by Alejandro Escanero Blanco
Modified: 2014-11-29 10:15 UTC (History)
3 users (show)

See Also:


Attachments
Patch for samba4rc6 with options to manage UPN (25.78 KB, patch)
2012-12-09 22:03 UTC, Alejandro Escanero Blanco
no flags Details
Patch for samba4rc6 with options to manage UPN (rev1) (26.46 KB, patch)
2012-12-10 10:08 UTC, Alejandro Escanero Blanco
no flags Details
From master -> git format-patch -1 (26.71 KB, patch)
2012-12-10 11:26 UTC, Alejandro Escanero Blanco
no flags Details
From master -> git format-patch -1 (without trailing tabulars or spaces) (26.70 KB, text/plain)
2012-12-10 13:18 UTC, Alejandro Escanero Blanco
no flags Details
Patch for samba4rc6 with options to manage UPN (rev1)(without trailing tabulars or spaces) (26.01 KB, text/plain)
2012-12-10 13:20 UTC, Alejandro Escanero Blanco
no flags Details
Updated patch for userPrincipalName (git patch -1) (27.24 KB, patch)
2013-03-14 12:27 UTC, Alejandro Escanero Blanco
no flags Details
Updated patch for userPrincipalName (git patch -1) (27.43 KB, patch)
2013-03-15 10:39 UTC, Alejandro Escanero Blanco
no flags Details
Updated patch for userPrincipalName (git patch -1) (27.41 KB, patch)
2013-03-18 13:15 UTC, Alejandro Escanero Blanco
no flags Details
userPrincipalName changes for BUG 9486. It makes samba-tool behave more like ADUC (28.03 KB, patch)
2013-03-19 15:28 UTC, Alejandro Escanero Blanco
no flags Details
userPrincipalName changes for BUG 9486. It makes samba-tool behave more like ADUC (28.05 KB, patch)
2013-03-21 19:02 UTC, Alejandro Escanero Blanco
no flags Details
userPrincipalName changes for BUG 9486. It makes samba-tool behave more like ADUC (28.07 KB, patch)
2013-03-24 19:53 UTC, Alejandro Escanero Blanco
no flags Details
userPrincipalName changes for BUG 9486. It makes samba-tool behave more like ADUC (28.04 KB, patch)
2013-07-01 13:07 UTC, Alejandro Escanero Blanco
no flags Details
autobuild log (1.07 MB, text/x-log)
2013-07-01 13:08 UTC, Alejandro Escanero Blanco
no flags Details
userPrincipalName changes for BUG 9486. It makes samba-tool behave more like ADUC (29.20 KB, patch)
2013-07-03 11:00 UTC, Alejandro Escanero Blanco
no flags Details
serPrincipalName changes for BUG 9486. It makes samba-tool behave more like ADUC (30.81 KB, patch)
2013-08-07 17:49 UTC, Alejandro Escanero Blanco
no flags Details

Note You need to log in before you can comment on or make changes to this bug.
Description Alejandro Escanero Blanco 2012-12-09 22:03:11 UTC
Created attachment 8315 [details]
Patch for samba4rc6 with options to manage UPN

I use the samba-tool domain samba3upgrade to move from samba3 ldap to samba4. All was ok, but when I was triing to add the domain to a Ovirt 3.1 Engine I find that no user has a UPN (UserPrincipalName) attribute.

I create this patch for the 4.0rc6 version adrressing this issues:

-> A option in samba-tool domain classicupgrade: --add-upn=(yes|no) default yes
By default a userPrincipalName for each user with form username@realm is added to the account.

-> A subcclass in samba-tool user upn set|delete|show
Manipulate userPrincipalName for each account

-> A option in samba-tool user create --upn=UPN
You can add any userPrincipalName to a Account when this is created.

-> A option in samba-tool user create --no-upn=(yes|no) -> exclusive with --upn default no
You can choose don't create the userPrincipalName attribute or choose the standard username@REALM

-> Changes in smbtorture test in samba.tests.samba_tool.user:
- Check if userPrincipalName is created in form username@REALM
- Check samba-tool user upn set|delete with URL and without URL

Tests with classicupgrade was correct, all the users (no machines) have a userPrincipalName.
Ovirt find any Domain Admin user without problems.
Comment 1 Andrew Bartlett 2012-12-09 22:13:48 UTC
I would like this in 4.0 so we don't have a behaviour change after the release.
Comment 2 Andrew Bartlett 2012-12-09 22:24:54 UTC
(09:15:34) abartlet: jelmer: can you check https://bugzilla.samba.org/show_bug.cgi?id=9486 for pythonic goodness?
(09:17:02) jelmer: abartlet: it should be using type=bool there rather than choice; that simplifies things quite a bit
(09:17:34) abartlet: ok, so how should we fix that up?  Just do another patch on top?
(09:17:50) jelmer: it seems to go over 80 characters per line
(09:17:55) abartlet: I would go back to the submittor, but we are quite tight for time
(09:18:10) jelmer: abartlet: that would be reasonable I think, especially if somebody else is making the changes
(09:18:40) jelmer: abartlet: it seems 'samba-tool upn show' doesn't print anything if the user wasn't found - that seems broken to me?
(09:18:53) abartlet: good point

(09:19:25) jelmer: abartlet: either it's using a mix of tabs and spaces, or the indentation is off in line 245
(09:22:14) jelmer: I don't see anything else
Comment 3 Alejandro Escanero Blanco 2012-12-10 10:08:35 UTC
Created attachment 8321 [details]
Patch for samba4rc6 with options to manage UPN (rev1)
Comment 4 Alejandro Escanero Blanco 2012-12-10 10:09:05 UTC
Added a patch revision. Changes are:

-> A option in samba-tool domain classicupgrade: --no-upn
By default a userPrincipalName for each user with form username@realm is added
to the account. This option disabled it.

If the user is not found in upn show (lan(res)==0) then raise a error.

Check with pylint give a lot of "Line too long" in any python scripts.
Comment 5 Alejandro Escanero Blanco 2012-12-10 11:26:17 UTC
Created attachment 8325 [details]
From master -> git format-patch -1

From master
git format-patch -1
Comment 6 Alejandro Escanero Blanco 2012-12-10 13:18:37 UTC
Created attachment 8326 [details]
From master -> git format-patch -1 (without trailing tabulars or spaces)

From master -> git format-patch -1 (without trailing tabulars or spaces)
Comment 7 Alejandro Escanero Blanco 2012-12-10 13:20:34 UTC
Created attachment 8327 [details]
Patch for samba4rc6 with options to manage UPN (rev1)(without trailing tabulars or spaces)
Comment 8 Stefan Metzmacher 2013-01-27 14:10:45 UTC
Andrew, what will we do with this patches?
Comment 9 Andrew Bartlett 2013-01-28 08:32:12 UTC
It looks like I lost them.  I was wanting them in 4.0, but they got lost.

What should we do about the behaviour change?  I'm happy with the idea, as it makes samba-tool behave more like ADUC, but it is a change compared with 4.0.0
Comment 10 Alejandro Escanero Blanco 2013-01-28 21:53:31 UTC
Is really add userPrincipalName a big change?
UPN is neccesary for a lot of apps.

I find some details in the patch that must be polished if Andrew wants to add this feature in a future samba4 release.

For example is needed to check the userPrincipalName format (email format is not checked in samba-tool), limit the upn domain part to uPNSuffixes in cn=Partitions container in the default cn=Configuration container, and last add a domain upn tool to manipulate uPNSuffixes.
Comment 11 Alejandro Escanero Blanco 2013-03-13 14:29:19 UTC
Updated patch for master


2013/1/28 <samba-bugs@samba.org>

> https://bugzilla.samba.org/show_bug.cgi?id=9486
>
> --- Comment #9 from Andrew Bartlett <abartlet@samba.org> 2013-01-28
> 08:32:12 UTC ---
> It looks like I lost them.  I was wanting them in 4.0, but they got lost.
>
> What should we do about the behaviour change?  I'm happy with the idea, as
> it
> makes samba-tool behave more like ADUC, but it is a change compared with
> 4.0.0
>
> --
> Configure bugmail: https://bugzilla.samba.org/userprefs.cgi?tab=email
> ------- You are receiving this mail because: -------
> You reported the bug.
>
Comment 12 Alejandro Escanero Blanco 2013-03-14 12:27:37 UTC
Created attachment 8645 [details]
Updated patch for userPrincipalName (git patch -1)

-> A option in samba-tool domain classicupgrade: --no-upn
By default a userPrincipalName for each user with form username@realm is added
to the account. This option don't add the userPrincipalName.

-> A subcclass in samba-tool user upn set|delete|show
Manipulate userPrincipalName for each account
Check upnsuffix before set userPrincipalName attribute

-> A option in samba-tool user create --upn=UPN
You can add a different userPrincipalName that username@realm to a Account when this is created.

-> A option in samba-tool user create --no-upn exclusive with --upn
default no
You can choose don't create any userPrincipalName attribute

-> Changes in smbtorture test in samba.tests.samba_tool.user:
- Check if userPrincipalName is created in form username@REALM
- Check samba-tool user upn set|delete with URL and without URL
Comment 13 Andrew Bartlett 2013-03-14 21:37:25 UTC
(In reply to comment #12)
> Created attachment 8645 [details]
> Updated patch for userPrincipalName (git patch -1)
> 
> -> A option in samba-tool domain classicupgrade: --no-upn
> By default a userPrincipalName for each user with form username@realm is added
> to the account. This option don't add the userPrincipalName.
> 
> -> A subcclass in samba-tool user upn set|delete|show
> Manipulate userPrincipalName for each account
> Check upnsuffix before set userPrincipalName attribute
> 
> -> A option in samba-tool user create --upn=UPN
> You can add a different userPrincipalName that username@realm to a Account when
> this is created.
> 
> -> A option in samba-tool user create --no-upn exclusive with --upn
> default no
> You can choose don't create any userPrincipalName attribute
> 
> -> Changes in smbtorture test in samba.tests.samba_tool.user:
> - Check if userPrincipalName is created in form username@REALM
> - Check samba-tool user upn set|delete with URL and without URL

This is looking good, except for the upgrade.py changes.  Please do not pass in an xid_type to the add_userPrincipalName() routine.  Instead, only call it for normal users.

The xid_type is specifically about idmap, and should not be overrused for other things, particularly when it is being checked and set to the same value.
Comment 14 Alejandro Escanero Blanco 2013-03-15 10:39:38 UTC
Created attachment 8648 [details]
Updated patch for userPrincipalName (git patch -1)

add_userPrincipalName has been cleared and added check upnsuffix in user create.

-> A option in samba-tool domain classicupgrade: --no-upn
By default a userPrincipalName for each user with form username@realm is added
to the account. This option don't add the userPrincipalName.

-> A subcclass in samba-tool user upn set|delete|show
Manipulate userPrincipalName for each account
Check upnsuffix before set userPrincipalName attribute

-> A option in samba-tool user create --upn=UPN
You can add a different userPrincipalName that username@realm to a Account when
this is created.
Check upnsuffix before set userPrincipalName attribute

-> A option in samba-tool user create --no-upn exclusive with --upn
default no
You can choose don't create any userPrincipalName attribute

-> Changes in smbtorture test in samba.tests.samba_tool.user:
- Check if userPrincipalName is created in form username@REALM
- Check samba-tool user upn set|delete with URL and without URL
Comment 15 Andrew Bartlett 2013-03-16 08:24:23 UTC
Almost there!

A proper commit message (like the detail in the bug) and please follow the python style on whitespace between functions

There should be two lines between each function.

For example, this hunk is wrong:
@@ -158,7 +180,6 @@ def add_ad_posix_idmap_entry(samdb, sid, xid, xid_type, logger):
             'Could not modify AD idmap entry for sid=%s, id=%s, type=%s (%s)',
             str(sid), str(xid), xid_type, str(e))
 
-
 def add_idmap_entry(idmapdb, sid, xid, xid_type, logger):
     """Create idmap entry
 

and other hunks don't add the right whitespace

Sorry to hold this up over whitespace, but I need you to write it up with a correct commit message anyway.  Thanks for fixing up the other concerns.

Andrew Bartlett
Comment 16 Alejandro Escanero Blanco 2013-03-18 08:09:39 UTC
I can't see the problem with this hunk, because only remove a whitespace with no sense.

I'll check for tabulars in place of whitespaces (I must use vi in place of joe...)
Comment 17 Alejandro Escanero Blanco 2013-03-18 13:15:46 UTC
Created attachment 8657 [details]
Updated patch for userPrincipalName (git patch -1)

cleaned
Comment 18 Andrew Bartlett 2013-03-18 21:04:27 UTC
The problematic removal of whitespace still exists exactly where I complained about it.  Did you really upload the corrected patch?

There is also still no detailed commit message.

I'm looking forward to applying this, so please fix these issues.  Also ensure it passes make test.
Comment 19 Alejandro Escanero Blanco 2013-03-19 15:28:37 UTC
Created attachment 8662 [details]
userPrincipalName changes for BUG 9486. It makes samba-tool  behave more like ADUC

This is the correct patch
Comment 20 Andrew Bartlett 2013-03-21 11:59:48 UTC
I tried to push this to autobuild, but it fails:

[1501/1562 in 1h13m53s] samba.tests.samba_tool.user(dc:local)
UNEXPECTED(failure): samba.tests.samba_tool.user.UserCmdTestCase.test_newuser
REASON: _StringException: Content-Type: text/x-traceback;charset=utf8,language=python
traceback
125
Traceback (most recent call last):
  File "/memdisk/abartlet/a/b982071/samba/bin/python/samba/tests/samba_tool/user.py", line 106, in test_newuser
    self.assertEquals("%s" % found.get("userPrincipalName"), upn)
MismatchError: 'sambatool1@samba.example.com' != 'sambatool1@SAMBA.EXAMPLE.COM'
0

FAILED (1 failures, 0 errors and 0 unexpected successes in 0 testsuites)

A summary with detailed information can be found in:
  ./bin/ab/summary
ERROR: test failed with exit code 1
Comment 21 Alejandro Escanero Blanco 2013-03-21 19:02:37 UTC
Created attachment 8670 [details]
userPrincipalName changes for BUG 9486. It makes samba-tool  behave more like ADUC

Tests corrected, all the text is lowercase.
Comment 22 Andrew Bartlett 2013-03-22 23:04:05 UTC
Have you *personally* run make test?

Does it pass for you?

Thanks,
Comment 23 Alejandro Escanero Blanco 2013-03-24 19:53:41 UTC
Created attachment 8677 [details]
userPrincipalName changes for BUG 9486. It makes samba-tool behave more like ADUC

$./buildtools/bin/waf test --tests="samba.tests.samba_tool.user.*"
...
SAMBA LOG of: LOCALDC
samba version 4.1.0pre1-DEVELOPERBUILD started.
Copyright Andrew Tridgell and the Samba Team 1992-2013
Called with maxruntime 7500 - current ts 1364154092
samba: using 'standard' process model
[1/1 in 0s] samba.tests.samba_tool.user(dc:local)
samba: EOF on stdin - terminating
samba child process 4905 exited with value 0

ALL OK (6 tests in 1 testsuites)
Comment 24 Alejandro Escanero Blanco 2013-03-24 19:56:20 UTC
I was doing the tests in the samba 4.0.3, but the patch isn't complete there. I'm terribly sorry.
Comment 25 Andrew Bartlett 2013-04-10 21:51:19 UTC
It seems I was unclear:

Patches like this must be prepared for master, then cherry-picked back into 4.0.x.

As such, I need a patch that will without any errors pass a full autobuild in master.  Run ./script/autobuild.py --testbase=/tmp

I need a positive statement from you that a specific named patch has for you passed a full autobuild on master.  Then I can review and autobuild it. 

This confusion has cost us both a lot of time, I'm sorry I wasn't clearer earlier.

If the patch does not then apply and autobuild in v4-0-test directly, we need to find out what other patches it relies on, and cherry-pick those as well.  We prefer this over modifying the patches for backport, if at all possible.

Andrew Bartlett
Comment 26 Alejandro Escanero Blanco 2013-07-01 13:06:02 UTC
autobuild passed

./script/autobuild.py --testbase=/tmp/test --verbose --tail --keeplogs --nocleanup 2>&1 |tee ../build.log

'build' finished successfully (12m6.047s)
Comment 27 Alejandro Escanero Blanco 2013-07-01 13:07:33 UTC
Created attachment 9008 [details]
userPrincipalName changes for BUG 9486. It makes samba-tool behave more like ADUC

Changes are:

-> A option in samba-tool domain classicupgrade: --no-upn
By default a userPrincipalName for each user with form username@realm is added
to the account. This option don't add the userPrincipalName.

-> Subcommand samba-tool user upn set|delete|show
Manipulate userPrincipalName for each account
Check upnsuffix before set userPrincipalName attribute

-> A option in samba-tool user create --upn=UPN
You can add a different userPrincipalName that username@realm to a Account when
this is created.
Check upnsuffix before set userPrincipalName attribute

-> A option in samba-tool user create --no-upn exclusive with --upn
default no
You can choose don't create any userPrincipalName attribute

-> Changes in smbtorture test in samba.tests.samba_tool.user:
- Check if userPrincipalName is created in form username@REALM
- Check samba-tool user upn set|delete with URL and without URL
Comment 28 Alejandro Escanero Blanco 2013-07-01 13:08:47 UTC
Created attachment 9009 [details]
autobuild log

Result of:
./script/autobuild.py --testbase=/tmp/test --verbose --tail --keeplogs --nocleanup 2>&1 |tee ../build.log
Comment 29 Jelmer Vernooij 2013-07-02 23:57:04 UTC
Some quick comments on the patch:

There is quite a bit of code in here that I can't imagine would work. E.g.:

 -  the line "if upn is not None and no-upn:" clearly should be "if upn is not None and no_upn:"
 - the boolean values are spelled with the first letter in uppercase in Python. This patch uses "false" and "true" in a couple of places, which will be undefined
 - In cmd_upn_delete.run, the code that is supposed to print that the delete was successful is only actually run when an error occurs (presumably an indentation issue)

Style nits:

 - please wrap lines at 80 characters

 - there should not be spaces after parentheses, e.g. use "(a != 1 and b != 2)" rather than "( a != 1 and b != 2 )"

 - please don't add unnecessary parentheses - e.g. use "if len(foo) == 0:" rather than "if (len(foo) == 0:"

 - please use spaces after commas; "bla(foo, bar)" rather than "bla(foo,bar)"

 - don't use "%s" where a simple string would do. E.g. :

self.assertEquals("%s" % found.get("userPrincipalName"), "%s" % upn)

Can just as well be:

self.assertEquals(found.get("userPrincipalName"), upn)

or if you must enforce a string, call str(found.get("userPrincipalName"))

Please don't catch the general "Exception" type when handling errors, but rather something more specific. This prevents interpreting genuine issues as signs that e.g. a user doesn't exist.
Comment 30 Alejandro Escanero Blanco 2013-07-03 11:00:47 UTC
Created attachment 9021 [details]
userPrincipalName changes for BUG 9486. It makes samba-tool behave more like ADUC

Thanks to Jelmer for the comments.
There is quite a bit of code in here that I can't imagine would work. E.g.:

> -  the line "if upn is not None and no-upn:" clearly should be "if upn is not
>None and no_upn:"

Corrected

> - the boolean values are spelled with the first letter in uppercase in Python.
>This patch uses "false" and "true" in a couple of places, which will be
>undefined

Corrected

> - In cmd_upn_delete.run, the code that is supposed to print that the delete
>was successful is only actually run when an error occurs (presumably an
>indentation issue)

Corrected


>Style nits:

> - please wrap lines at 80 characters

Corrected

> - there should not be spaces after parentheses, e.g. use "(a != 1 and b != 2)"
>rather than "( a != 1 and b != 2 )"

Corrected

> - please don't add unnecessary parentheses - e.g. use "if len(foo) == 0:"
>rather than "if (len(foo) == 0:"

Corrected

> - please use spaces after commas; "bla(foo, bar)" rather than "bla(foo,bar)"

Corrected

> - don't use "%s" where a simple string would do. E.g. :
>self.assertEquals("%s" % found.get("userPrincipalName"), "%s" % upn)
>Can just as well be:
>self.assertEquals(found.get("userPrincipalName"), upn)
or if you must enforce a string, call str(found.get("userPrincipalName"))

Corrected


>Please don't catch the general "Exception" type when handling errors, but
>rather something more specific. This prevents interpreting genuine issues as
>signs that e.g. a user doesn't exist.

I catch the generic Exception because I don't find any defined Exception (only in sites.py there are some Exceptions defined)
Comment 31 Jelmer Vernooij 2013-07-16 12:54:32 UTC
upnsuffix_check() looks like a function disguised as a class? Is there a particular reason it's not a def ?
Comment 32 Alejandro Escanero Blanco 2013-08-07 17:49:23 UTC
Created attachment 9113 [details]
serPrincipalName changes for BUG 9486. It makes samba-tool behave more like ADUC

Need your check Jelmer, this patch add Exceptions for UPN checks.
The class upnsuffix_check is separated from cmd_upn_set because I need to use this class outside upn.py.
Comment 33 Jelmer Vernooij 2013-08-07 17:57:23 UTC
Thanks, looks a lot better! 

Some more comments:

- Please don't create a new system_session() every time you need to pass session_info to a function. Instead, create it once and pass the same copy.
- cmd_upn_show has a Spanish docstring 
- in cmd_upn_delete.run, don't catch just any Exception but the Exception you are expecting only.
Comment 34 Björn Jacke 2013-08-28 22:26:08 UTC
this patch is still growing. make this a blocker of 4.2, not 4.1. If this is is master and finished it might maybe get into 4.1 as a fix later.
Comment 35 Karolin Seeger 2013-12-10 15:43:01 UTC
Any news on this one?
Comment 36 Karolin Seeger 2014-11-27 10:52:02 UTC
Is this a showstopper for 4.2.0?
Comment 37 Stefan Metzmacher 2014-11-29 10:15:20 UTC
This is not a blocker for 4.2.0