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.
I would like this in 4.0 so we don't have a behaviour change after the release.
(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
Created attachment 8321 [details] Patch for samba4rc6 with options to manage UPN (rev1)
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.
Created attachment 8325 [details] From master -> git format-patch -1 From master git format-patch -1
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)
Created attachment 8327 [details] Patch for samba4rc6 with options to manage UPN (rev1)(without trailing tabulars or spaces)
Andrew, what will we do with this patches?
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
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.
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. >
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
(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.
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
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
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...)
Created attachment 8657 [details] Updated patch for userPrincipalName (git patch -1) cleaned
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.
Created attachment 8662 [details] userPrincipalName changes for BUG 9486. It makes samba-tool behave more like ADUC This is the correct patch
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
Created attachment 8670 [details] userPrincipalName changes for BUG 9486. It makes samba-tool behave more like ADUC Tests corrected, all the text is lowercase.
Have you *personally* run make test? Does it pass for you? Thanks,
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)
I was doing the tests in the samba 4.0.3, but the patch isn't complete there. I'm terribly sorry.
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
autobuild passed ./script/autobuild.py --testbase=/tmp/test --verbose --tail --keeplogs --nocleanup 2>&1 |tee ../build.log 'build' finished successfully (12m6.047s)
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
Created attachment 9009 [details] autobuild log Result of: ./script/autobuild.py --testbase=/tmp/test --verbose --tail --keeplogs --nocleanup 2>&1 |tee ../build.log
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.
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)
upnsuffix_check() looks like a function disguised as a class? Is there a particular reason it's not a def ?
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.
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.
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.
Any news on this one?
Is this a showstopper for 4.2.0?
This is not a blocker for 4.2.0