Re: [Freeipa-devel] [PATCH] Change the behaviour of addattr/setattr parameters
Here's my version, that just calls the parameter prior to updating
the attr. I tested it with:
[r...@ipa ~]# ipa user-mod --setattr uidnumber=555 kfrog
-
Modified user "kfrog"
-
User login: kfrog
First name: Kermit
Last name: Frog
Home directory: /home/kfrog
Login shell: /bin/sh
UID: 555
Groups: ipausers
[r...@ipa ~]# ipa user-mod --setattr uidnumber=frog kfrog
ipa: ERROR: invalid 'uidnumber': must be an integer
___
Freeipa-devel mailing list
Freeipa-devel@redhat.com
https://www.redhat.com/mailman/listinfo/
Pushed to Master. ACKed in IRC by rcrit, and based on a pzuna patch
reviewd by both myself and rcrit
freeipa-devel
Since this was a diff to a Patch ACK
___
Freeipa-devel mailing list
Freeipa-devel@redhat.com
https://www.redhat.com/mailman/listinfo/freeipa-devel
Just ran the details patch on top of this, and the user details page
does not work with it.
If you submit the page even with a minor edit to the full name you get
an error: 'login' is required.
I've tested it out with the JSON and CURL: Here is the JSON it is sending:
{"method":"user_mod","params":[["zoe"],{"all":true,"setattr":["cn=Zoe
MacLeod","gidnumber=1044896486","title=","displayname=","initials=","uid=","mail=","street=","location=","postalcode=","ou=","carlicense="],"addattr":[],"givenname":"Zoe","sn":"MacPhearson","uidnumber":"1044896486","homedirectory":"/home/zoe","sizelimit":100}],"id":4}
The message 'login' is required is coming from the call I added to
value = self.params[attr](value)
As it is the message inside RequirementError, which gets called from
parameters.py, specifically:
def validate(self, value):
"""
Check validity of ``value``.
:param value: A proposed value for this parameter.
"""
if value is None:
if self.required:
raise RequirementError(name=self.cli_name)
My guess is the correct change is to skip this call if value is null,
which seems to be what is happening. But I suspect we are sending in
bogus values to setattr. Notice this part of the JSON
"setattr":["cn=Zoe
MacLeod","gidnumber=1044896486","title=","displayname=","initials=","uid=","mail=","street=","location=","postalcode=","ou=","carlicense="]
My guess is that the details page shouldn't send any unset values.
"uid=" in particular is probably a mistake. REmoving that from the JSON
gets us to: an internal error has occurred.
Here's the stack trace:
ipa: ERROR: non-public: TypeError: 'NoneType' object is not iterable
Traceback (most recent call last):
File "/home/ayoung/devel/freeipa/ipaserver/rpcserver.py", line 206,
in wsgi_execute
result = self.Command[name](*args, **options)
File "/home/ayoung/devel/freeipa/ipalib/frontend.py", line 401, in
__call__
ret = self.run(*args, **options)
File "/home/ayoung/devel/freeipa/ipalib/frontend.py", line 674, in run
return self.execute(*args, **options)
File "/home/ayoung/devel/freeipa/ipalib/plugins/baseldap.py", line
431, in execute
addset = set(get_attributes(options.get('addattr', [])))
File "/home/ayoung/devel/freeipa/ipalib/plugins/baseldap.py", line
52, in get_attributes
for attr in attrs:
Again, this is from the __call__ code, which means it is from the code I
added, although now I'm not sure which parameter, if any is tripped the
code.
I ran the full body of unit tests on the code as commited, and they all
pass, as did the set of tests that Rob cobbled up (reminder: Lets get
those added) so I don't think the call to self.params[attr](value) is
wrong, but that it catches input errors that would have bitten us.
___
Freeipa-devel mailing list
Freeipa-devel@redhat.com
https://www.redhat.com/mailman/listinfo/freeipa-devel
Re: [Freeipa-devel] [PATCH] Change the behaviour of addattr/setattr parameters
On 08/13/2010 04:24 PM, Adam Young wrote: On 08/12/2010 09:35 AM, Pavel Zůna wrote: On 2010-08-12 14:38, Rob Crittenden wrote: Pavel Zůna wrote: On 2010-08-12 04:46, Rob Crittenden wrote: Pavel Zůna wrote: setattr and addattr can now be used both to set all values of ANY attribute. the last setattr always resets the attribute to the specified value and all addattr append to it. Examples: user-mod testuser --setattr=title=msc title: msc user-mod testuser --setattr=title=msb title: msb user-mod testuser --addattr=title=msc title: msb, msc user-mod testuser --setattr=title= title: user-mod testuser --setattr=title=msc --addattr=msb title: msc, msb user-mod testuser --setattr=title=ing --addattr=bc title: ing, bc user-mod testuser --setattr=title=doc title: doc It's not very user friendly, but it's going to be used very very rarely in special conditions in the CLI and we can use it to save lots of JSON-RPC roundtrips in the webUI. Pavel It was my intention when I added addattr and setattr that one couldn't set already-defined params this way. They were silently ignored. So you couldn't do: user-mod testuser --setattr=givenname=Jeff This would be possible with this patch. Was that intentional? BTW I have the start of a test suite for this functionality. rob Yes, it is intentional. I forgot to mention it in the description. I'm using setattr/addattr for everything in the webUI - it makes the code a lot simpler. Doesn't that invalidate all the validators we have in the plugins? This is why I disallowed it. rob It does, but I see these options as something only experienced users, who need to set something we don't support directly, will use. Sometimes they might want to disable the validators, if they know what they're doing. We could also make the setattr/addattr handler in frontend.py detect if a there's a validator available and use it. Validators in the webUI is still something we need to figure out. Adam was proposing having validators in the form of regex strings, which is not a bad idea as it's easy to implement on any platform/language. On the other hand, I don't know if it's good enough for all parameters we have. Hmm. There's a lot to think about here actually. I'll make it my homework for the weekend. :) Pavel ___ Freeipa-devel mailing list Freeipa-devel@redhat.com https://www.redhat.com/mailman/listinfo/freeipa-devel Here's my version, that just calls the parameter prior to updating the attr. I tested it with: [r...@ipa ~]# ipa user-mod --setattr uidnumber=555 kfrog - Modified user "kfrog" - User login: kfrog First name: Kermit Last name: Frog Home directory: /home/kfrog Login shell: /bin/sh UID: 555 Groups: ipausers [r...@ipa ~]# ipa user-mod --setattr uidnumber=frog kfrog ipa: ERROR: invalid 'uidnumber': must be an integer ___ Freeipa-devel mailing list Freeipa-devel@redhat.com https://www.redhat.com/mailman/listinfo/ Pushed to Master. ACKed in IRC by rcrit, and based on a pzuna patch reviewd by both myself and rcrit freeipa-devel Since this was a diff to a Patch ACK ___ Freeipa-devel mailing list Freeipa-devel@redhat.com https://www.redhat.com/mailman/listinfo/freeipa-devel
Re: [Freeipa-devel] [PATCH] Change the behaviour of addattr/setattr parameters
On 08/12/2010 09:35 AM, Pavel Zůna wrote: On 2010-08-12 14:38, Rob Crittenden wrote: Pavel Zůna wrote: On 2010-08-12 04:46, Rob Crittenden wrote: Pavel Zůna wrote: setattr and addattr can now be used both to set all values of ANY attribute. the last setattr always resets the attribute to the specified value and all addattr append to it. Examples: user-mod testuser --setattr=title=msc title: msc user-mod testuser --setattr=title=msb title: msb user-mod testuser --addattr=title=msc title: msb, msc user-mod testuser --setattr=title= title: user-mod testuser --setattr=title=msc --addattr=msb title: msc, msb user-mod testuser --setattr=title=ing --addattr=bc title: ing, bc user-mod testuser --setattr=title=doc title: doc It's not very user friendly, but it's going to be used very very rarely in special conditions in the CLI and we can use it to save lots of JSON-RPC roundtrips in the webUI. Pavel It was my intention when I added addattr and setattr that one couldn't set already-defined params this way. They were silently ignored. So you couldn't do: user-mod testuser --setattr=givenname=Jeff This would be possible with this patch. Was that intentional? BTW I have the start of a test suite for this functionality. rob Yes, it is intentional. I forgot to mention it in the description. I'm using setattr/addattr for everything in the webUI - it makes the code a lot simpler. Doesn't that invalidate all the validators we have in the plugins? This is why I disallowed it. rob It does, but I see these options as something only experienced users, who need to set something we don't support directly, will use. Sometimes they might want to disable the validators, if they know what they're doing. We could also make the setattr/addattr handler in frontend.py detect if a there's a validator available and use it. Validators in the webUI is still something we need to figure out. Adam was proposing having validators in the form of regex strings, which is not a bad idea as it's easy to implement on any platform/language. On the other hand, I don't know if it's good enough for all parameters we have. Hmm. There's a lot to think about here actually. I'll make it my homework for the weekend. :) Pavel ___ Freeipa-devel mailing list Freeipa-devel@redhat.com https://www.redhat.com/mailman/listinfo/freeipa-devel Here's my version, that just calls the parameter prior to updating the attr. I tested it with: [r...@ipa ~]# ipa user-mod --setattr uidnumber=555 kfrog - Modified user "kfrog" - User login: kfrog First name: Kermit Last name: Frog Home directory: /home/kfrog Login shell: /bin/sh UID: 555 Groups: ipausers [r...@ipa ~]# ipa user-mod --setattr uidnumber=frog kfrog ipa: ERROR: invalid 'uidnumber': must be an integer >From 030b5dab93971495d8656f7886c29136e118a9e6 Mon Sep 17 00:00:00 2001 From: Adam Young Date: Fri, 13 Aug 2010 16:20:41 -0400 Subject: [PATCH] Change the behaviour of addattr/setattr parameters. setattr and addattr can now be used both to set all values of ANY attribute. the last setattr always resets the attribute to the specified value and all addattr append to it. Examples: user-mod testuser --setattr=title=msc title: msc user-mod testuser --setattr=title=msb title: msb user-mod testuser --addattr=title=msc title: msb, msc user-mod testuser --setattr=title= title: user-mod testuser --setattr=title=msc --addattr=msb title: msc, msb user-mod testuser --setattr=title=ing --addattr=bc title: ing, bc user-mod testuser --setattr=title=doc title: doc It's not very user friendly, but it's going to be used very very rarely in special conditions in the CLI and we can use it to save lots of JSON-RPC roundtrips in the webUI. This version includes calling the validation of Params during the setting of the attrs. --- ipalib/frontend.py | 17 ipalib/plugins/baseldap.py | 58 ++-- 2 files changed, 40 insertions(+), 35 deletions(-) diff --git a/ipalib/frontend.py b/ipalib/frontend.py index d320f02..1c4fea8 100644 --- a/ipalib/frontend.py +++ b/ipalib/frontend.py @@ -519,11 +519,12 @@ class Command(HasParam): if len(value) == 0: # None means "delete this attribute" value = None -if attr not in self.params: -if append and attr in newdict: -newdict[attr].append(value) -else: -newdict[attr] = [value] +if attr in self.params: +value = self.params[attr](value) +if append and attr in newdict: +newdict[attr].append(value) +else: +newdict[attr] = [value] return newdict def __attributes_2_entry(self, kw): @@ -540,7 +541,11 @@ class Command(HasParam): adddict = self.__convert_2
Re: [Freeipa-devel] [PATCH] Change the behaviour of addattr/setattr parameters
On 2010-08-12 14:38, Rob Crittenden wrote: Pavel Zůna wrote: On 2010-08-12 04:46, Rob Crittenden wrote: Pavel Zůna wrote: setattr and addattr can now be used both to set all values of ANY attribute. the last setattr always resets the attribute to the specified value and all addattr append to it. Examples: user-mod testuser --setattr=title=msc title: msc user-mod testuser --setattr=title=msb title: msb user-mod testuser --addattr=title=msc title: msb, msc user-mod testuser --setattr=title= title: user-mod testuser --setattr=title=msc --addattr=msb title: msc, msb user-mod testuser --setattr=title=ing --addattr=bc title: ing, bc user-mod testuser --setattr=title=doc title: doc It's not very user friendly, but it's going to be used very very rarely in special conditions in the CLI and we can use it to save lots of JSON-RPC roundtrips in the webUI. Pavel It was my intention when I added addattr and setattr that one couldn't set already-defined params this way. They were silently ignored. So you couldn't do: user-mod testuser --setattr=givenname=Jeff This would be possible with this patch. Was that intentional? BTW I have the start of a test suite for this functionality. rob Yes, it is intentional. I forgot to mention it in the description. I'm using setattr/addattr for everything in the webUI - it makes the code a lot simpler. Doesn't that invalidate all the validators we have in the plugins? This is why I disallowed it. rob It does, but I see these options as something only experienced users, who need to set something we don't support directly, will use. Sometimes they might want to disable the validators, if they know what they're doing. We could also make the setattr/addattr handler in frontend.py detect if a there's a validator available and use it. Validators in the webUI is still something we need to figure out. Adam was proposing having validators in the form of regex strings, which is not a bad idea as it's easy to implement on any platform/language. On the other hand, I don't know if it's good enough for all parameters we have. Hmm. There's a lot to think about here actually. I'll make it my homework for the weekend. :) Pavel ___ Freeipa-devel mailing list Freeipa-devel@redhat.com https://www.redhat.com/mailman/listinfo/freeipa-devel
Re: [Freeipa-devel] [PATCH] Change the behaviour of addattr/setattr parameters
Pavel Zůna wrote: On 2010-08-12 04:46, Rob Crittenden wrote: Pavel Zůna wrote: setattr and addattr can now be used both to set all values of ANY attribute. the last setattr always resets the attribute to the specified value and all addattr append to it. Examples: user-mod testuser --setattr=title=msc title: msc user-mod testuser --setattr=title=msb title: msb user-mod testuser --addattr=title=msc title: msb, msc user-mod testuser --setattr=title= title: user-mod testuser --setattr=title=msc --addattr=msb title: msc, msb user-mod testuser --setattr=title=ing --addattr=bc title: ing, bc user-mod testuser --setattr=title=doc title: doc It's not very user friendly, but it's going to be used very very rarely in special conditions in the CLI and we can use it to save lots of JSON-RPC roundtrips in the webUI. Pavel It was my intention when I added addattr and setattr that one couldn't set already-defined params this way. They were silently ignored. So you couldn't do: user-mod testuser --setattr=givenname=Jeff This would be possible with this patch. Was that intentional? BTW I have the start of a test suite for this functionality. rob Yes, it is intentional. I forgot to mention it in the description. I'm using setattr/addattr for everything in the webUI - it makes the code a lot simpler. Doesn't that invalidate all the validators we have in the plugins? This is why I disallowed it. rob ___ Freeipa-devel mailing list Freeipa-devel@redhat.com https://www.redhat.com/mailman/listinfo/freeipa-devel
Re: [Freeipa-devel] [PATCH] Change the behaviour of addattr/setattr parameters
On 2010-08-12 04:46, Rob Crittenden wrote: Pavel Zůna wrote: setattr and addattr can now be used both to set all values of ANY attribute. the last setattr always resets the attribute to the specified value and all addattr append to it. Examples: user-mod testuser --setattr=title=msc title: msc user-mod testuser --setattr=title=msb title: msb user-mod testuser --addattr=title=msc title: msb, msc user-mod testuser --setattr=title= title: user-mod testuser --setattr=title=msc --addattr=msb title: msc, msb user-mod testuser --setattr=title=ing --addattr=bc title: ing, bc user-mod testuser --setattr=title=doc title: doc It's not very user friendly, but it's going to be used very very rarely in special conditions in the CLI and we can use it to save lots of JSON-RPC roundtrips in the webUI. Pavel It was my intention when I added addattr and setattr that one couldn't set already-defined params this way. They were silently ignored. So you couldn't do: user-mod testuser --setattr=givenname=Jeff This would be possible with this patch. Was that intentional? BTW I have the start of a test suite for this functionality. rob Yes, it is intentional. I forgot to mention it in the description. I'm using setattr/addattr for everything in the webUI - it makes the code a lot simpler. Pavel ___ Freeipa-devel mailing list Freeipa-devel@redhat.com https://www.redhat.com/mailman/listinfo/freeipa-devel
Re: [Freeipa-devel] [PATCH] Change the behaviour of addattr/setattr parameters
Pavel Zůna wrote: setattr and addattr can now be used both to set all values of ANY attribute. the last setattr always resets the attribute to the specified value and all addattr append to it. Examples: user-mod testuser --setattr=title=msc title: msc user-mod testuser --setattr=title=msb title: msb user-mod testuser --addattr=title=msc title: msb, msc user-mod testuser --setattr=title= title: user-mod testuser --setattr=title=msc --addattr=msb title: msc, msb user-mod testuser --setattr=title=ing --addattr=bc title: ing, bc user-mod testuser --setattr=title=doc title: doc It's not very user friendly, but it's going to be used very very rarely in special conditions in the CLI and we can use it to save lots of JSON-RPC roundtrips in the webUI. Pavel It was my intention when I added addattr and setattr that one couldn't set already-defined params this way. They were silently ignored. So you couldn't do: user-mod testuser --setattr=givenname=Jeff This would be possible with this patch. Was that intentional? BTW I have the start of a test suite for this functionality. rob ___ Freeipa-devel mailing list Freeipa-devel@redhat.com https://www.redhat.com/mailman/listinfo/freeipa-devel
[Freeipa-devel] [PATCH] Change the behaviour of addattr/setattr parameters
setattr and addattr can now be used both to set all values of
ANY attribute. the last setattr always resets the attribute to
the specified value and all addattr append to it.
Examples:
user-mod testuser --setattr=title=msc
title: msc
user-mod testuser --setattr=title=msb
title: msb
user-mod testuser --addattr=title=msc
title: msb, msc
user-mod testuser --setattr=title=
title:
user-mod testuser --setattr=title=msc --addattr=msb
title: msc, msb
user-mod testuser --setattr=title=ing --addattr=bc
title: ing, bc
user-mod testuser --setattr=title=doc
title: doc
It's not very user friendly, but it's going to be used very very
rarely in special conditions in the CLI and we can use it to save
lots of JSON-RPC roundtrips in the webUI.
Pavel
From 5467a93dc7e4e24e82ba3559b333ac5e55814127 Mon Sep 17 00:00:00 2001
From: Pavel Zuna
Date: Mon, 9 Aug 2010 19:43:00 -0400
Subject: [PATCH 2/4] Change the behaviour of addattr/setattr parameters.
setattr and addattr can now be used both to set all values of
ANY attribute. the last setattr always resets the attribute to
the specified value and all addattr append to it.
Examples:
user-mod testuser --setattr=title=msc
title: msc
user-mod testuser --setattr=title=msb
title: msb
user-mod testuser --addattr=title=msc
title: msb, msc
user-mod testuser --setattr=title=
title:
user-mod testuser --setattr=title=msc --addattr=msb
title: msc, msb
user-mod testuser --setattr=title=ing --addattr=bc
title: ing, bc
user-mod testuser --setattr=title=doc
title: doc
It's not very user friendly, but it's going to be used very very
rarely in special conditions in the CLI and we can use it to save
lots of JSON-RPC roundtrips in the webUI.
---
ipalib/frontend.py | 15 +++
ipalib/plugins/baseldap.py | 58 ++--
2 files changed, 38 insertions(+), 35 deletions(-)
diff --git a/ipalib/frontend.py b/ipalib/frontend.py
index d320f02..950fa7b 100644
--- a/ipalib/frontend.py
+++ b/ipalib/frontend.py
@@ -519,11 +519,10 @@ class Command(HasParam):
if len(value) == 0:
# None means "delete this attribute"
value = None
-if attr not in self.params:
-if append and attr in newdict:
-newdict[attr].append(value)
-else:
-newdict[attr] = [value]
+if append and attr in newdict:
+newdict[attr].append(value)
+else:
+newdict[attr] = [value]
return newdict
def __attributes_2_entry(self, kw):
@@ -540,7 +539,11 @@ class Command(HasParam):
adddict = self.__convert_2_dict(kw['setattr'], append=False)
if kw.get('addattr'):
-adddict.update(self.__convert_2_dict(kw['addattr']))
+for (k, v) in self.__convert_2_dict(kw['addattr']).iteritems():
+if k in adddict:
+adddict[k] += v
+else:
+adddict[k] = v
for name in adddict:
value = adddict[name]
diff --git a/ipalib/plugins/baseldap.py b/ipalib/plugins/baseldap.py
index 52f32e3..c995a61 100644
--- a/ipalib/plugins/baseldap.py
+++ b/ipalib/plugins/baseldap.py
@@ -415,6 +415,35 @@ class LDAPUpdate(LDAPQuery, crud.Update):
entry_attrs = self.args_options_2_entry(**options)
+"""
+Some special handling is needed because we need to update the
+values here rather than letting ldap.update_entry() do the work. We
+have to do the work of adding new values to an existing attribute
+because if we pass just what is addded only the new values get
+set.
+"""
+if 'addattr' in options:
+setset = set(get_attributes(options.get('setattr', [])))
+addset = set(get_attributes(options.get('addattr', [])))
+difflist = list(addset.difference(setset))
+if difflist:
+try:
+(dn, old_entry) = ldap.get_entry(
+dn, difflist, normalize=self.obj.normalize_dn
+)
+except errors.ExecutionError, e:
+try:
+(dn, old_entry) = self._call_exc_callbacks(
+keys, options, e, ldap.get_entry, dn, attrs_list,
+normalize=self.obj.normalize_dn
+)
+except errors.NotFound:
+self.obj.handle_not_found(*keys)
+for a in old_entry:
+if not isinstance(entry_attrs[a], (list, tuple)):
+entry_attrs[a] = [entry_attrs[a]]
+entry_attrs[a] += old_entry[a]
+
if options.get('all', False):
attrs_list = ['*']
else:
@@ -432,35 +461,6 @@ class LDAPUpdate(LDAPQuery, crud.Update):
self, ldap, dn, entry_attrs, attr
