Re: [Freeipa-devel] Re: [PATCH] Add --all to LDAPCreate and make LDAP commands always display default attributes.
Pavel Zuna wrote:
Rob Crittenden wrote:
Pavel Zuna wrote:
And here's the actual patch. :)
Pavel Zuna wrote:
This should fix the issue:
Rob Crittenden wrote:
Michael Gregg wrote:
Rob, did the support for posix groups change?
If I create a group specifying "--posix" the cli does create the
group.
Then, using ipa group-find, I do not see any way to determine if
that group is a posixgroup or not.
group-find -all used to reveal a PosixGroup field.
How do I determine if a group is a posix group or not?
Michael-
Ok, I suppose I could have looked at this before firing off an
e-mail :-)
I do see the group number when showing a group:
$ ./ipa group-show g9
---
group-show:
---
Group: g9
name: g9
description: test posix group
group id: 1117
But when adding it this doesn't appear. Oddly enough we show the
ipquniqueid when adding a group but not when showing it!
Pavel, do you have time to investigate this inconsistency?
rob
Pavel
I'm not sure how this addresses the issue that when adding a group
different values are returned than when you show one.
When an entry is created, we show the default attributes and all
attributes that were created explicitly. Before this patch, it was just
all attributes, that were created explicitly, so for example gid didn't
show up on groups, because it was created by the DNA plugin.
When showing an entry, we return the default attributes.
Should I change LDAPCreate to only return default attributes?
No. I understand the problem now. I think in earlier versions we were
actually doing a lookup of the entry after creation and returning that.
This would resolve the problem.
This also causes a whole ton of tests to fail. I think in baseldap.oy
instead of:
if options['all']:
You want:
if options.get('all', False):
Some of the tests were failing before this patch. I submitted a fix for
most of them.
if options['all'] is fine, because --all is a Flag parameters and is
required.
The service plugin overrides takes_options() in some cases, hence no
'all. Probably something to fix but we still should handle this case
(all not in options).
rob
smime.p7s
Description: S/MIME Cryptographic Signature
___
Freeipa-devel mailing list
Freeipa-devel@redhat.com
https://www.redhat.com/mailman/listinfo/freeipa-devel
Re: [Freeipa-devel] [PATCH] Fix a bunch of unit tests.
Pavel Zuna wrote: Rob Crittenden wrote: Pavel Zuna wrote: Only pwpolicy test is still broken - I'm looking into it. Pavel This brings up the return values question again. I thought we had decided that any attribute that had only one value would be returned as a scalar. In this case userCertificate is being returned as a list which is causing things to fail. Now arguably userCertificate is a multi-valued attribute but we will only store one certificate at a time there so I think we're ok. Yeah, I remember, but I'm not sure if we agreed on the logic. There are 2 ways of doing this: 1) Make ldap backend check the schema. If it's multi-value - leave it as a list. If it's single-value - convert it to a scalar. 2) Make ldap backend check if the attribute contains 1 or more values. If there's only one, convert it to a scalar. With 1) plugin authors can depend on the schema when manipulating attributes, but they have to know the schema and handle multi/single attributes differently. Yes but they already have to deal with it/be aware of it because updates may fail if you try to add another value to a single-valued attribute. With 2) plugin authors have to always check, if the attribute is a list or a scalar. Not necessarly. If the author has some awareness of the schema they can get by ok. I think that having attributes always returned as list makes things easier on plugin authors - no checks required, everything is handled the same way. What's the advantage of returning attribute values as 2 different types? Because some values are single-value by nature and we're treating them like multi-values. Also, why the change to the principal name in the service tests? At first I didn't know where the problem in this test was. So, I tried a few different things and this is a leftover. Doesn't hurt anything, but I can always change it back. Yes, please do. You're effectively adding a subdomain onto the hostname. rob smime.p7s Description: S/MIME Cryptographic Signature ___ Freeipa-devel mailing list Freeipa-devel@redhat.com https://www.redhat.com/mailman/listinfo/freeipa-devel
[Freeipa-devel] [PATCH] 285 CRL publishing
This enables CRL publishing by dogtag to a place where Apache can get the files. I have to do a couple of tricks here because dogtag is an optional component. This is why in the installer I first see if the dogtag SELinux policy is installed and if not add it. Similarly the installer will remove it upon uninstall. The policy itself just lets dogtag write to some Apache-labeled directories. dogtag uses symlinks to mark the latest CRL hence the permissions for links. rob freeipa-285-crl.patch Description: application/mbox smime.p7s Description: S/MIME Cryptographic Signature ___ Freeipa-devel mailing list Freeipa-devel@redhat.com https://www.redhat.com/mailman/listinfo/freeipa-devel
Re: [Freeipa-devel] [PATCH] 309 make exception from ipautil.run() optional
Jason Gerard DeRose wrote:
On Wed, 2009-11-11 at 11:41 -0500, Rob Crittenden wrote:
Rob Crittenden wrote:
There are probably occasions where a caller will want more control over
what happens when running a command fails. I've added an optional
argument to run where it will not raise an exception on errors.
I've also added returncode to the tuple of things returned.
rob
I forgot to include this additional change in the patch. When acked I'll
add this bit too and commit it.
--- a/ipaserver/install/httpinstance.py
+++ b/ipaserver/install/httpinstance.py
@@ -100,7 +100,7 @@ class HTTPInstance(service.Service):
if selinux:
try:
# returns e.g. "httpd_can_network_connect --> off"
-(stdout, stderr) = ipautil.run(["/usr/sbin/getsebool",
+(stdout, stderr, returncode) =
ipautil.run(["/usr/sbin/getsebool",
"httpd_can_network_connect"])
self.backup_state("httpd_can_network_connect",
stdout.split()[2])
except:
ack. It all looks fine to me, although I can't get this patch to apply.
Ok, it is failing because it relies on a patch I didn't submit yet. I'll
send that one out shortly and hold onto this one for now.
rob
smime.p7s
Description: S/MIME Cryptographic Signature
___
Freeipa-devel mailing list
Freeipa-devel@redhat.com
https://www.redhat.com/mailman/listinfo/freeipa-devel
Re: [Freeipa-devel] [PATCH] 309 make exception from ipautil.run() optional
On Wed, 2009-11-11 at 11:41 -0500, Rob Crittenden wrote:
> Rob Crittenden wrote:
> > There are probably occasions where a caller will want more control over
> > what happens when running a command fails. I've added an optional
> > argument to run where it will not raise an exception on errors.
> >
> > I've also added returncode to the tuple of things returned.
> >
> > rob
>
> I forgot to include this additional change in the patch. When acked I'll
> add this bit too and commit it.
>
> --- a/ipaserver/install/httpinstance.py
> +++ b/ipaserver/install/httpinstance.py
> @@ -100,7 +100,7 @@ class HTTPInstance(service.Service):
> if selinux:
> try:
> # returns e.g. "httpd_can_network_connect --> off"
> -(stdout, stderr) = ipautil.run(["/usr/sbin/getsebool",
> +(stdout, stderr, returncode) =
> ipautil.run(["/usr/sbin/getsebool",
>
> "httpd_can_network_connect"])
> self.backup_state("httpd_can_network_connect",
> stdout.split()[2])
> except:
ack. It all looks fine to me, although I can't get this patch to apply.
___
Freeipa-devel mailing list
Freeipa-devel@redhat.com
https://www.redhat.com/mailman/listinfo/freeipa-devel
Re: [Freeipa-devel] [PATCH] 308 manage arbitrary attributes
On Tue, 2009-11-10 at 12:28 -0500, Rob Crittenden wrote: > Jason Gerard DeRose wrote: > > Oops, was this missing the attachment? ;) > > Bah, here it is. > > rob ack. pushed to master. > > > > On Wed, 2009-11-04 at 16:04 -0500, Rob Crittenden wrote: > >> This adds 2 new parameters, --setattr and --addattr and lets you manage > >> whatever attribute you want (within the given set of objectclasses). > >> > >> Both take a name/value pair. > >> > >> --setattr sets the attribute to the given value > >> --addattr adds the value to an attribute. Can be used to manage > >> multi-valued attributes > >> > >> For example: > >> > >> ipa user-mod --addattr=postalcode=90210 jsmith > >> > >> If the attribute to be modified is an another param then the value is > >> silently dropped. > >> > >> You can include multiples of these on a single command-line: > >> > >> ipa user-mod --addattr=postalcode=20601 --addattr=postalcode=30330 jsmith > >> > >> Setting an attribute to "" deletes it: > >> > >> ipa user-mod --setattr=postalcode= jsmith > >> > >> rob > >> ___ > >> Freeipa-devel mailing list > >> Freeipa-devel@redhat.com > >> https://www.redhat.com/mailman/listinfo/freeipa-devel > > > ___ Freeipa-devel mailing list Freeipa-devel@redhat.com https://www.redhat.com/mailman/listinfo/freeipa-devel
Re: [Freeipa-devel] [PATCH] 307 enforce scalar
On Wed, 2009-11-04 at 09:46 -0500, Rob Crittenden wrote: > _convert_scalar() should not handle tuples/lists (by definition). A > parameter may be mutivalued but even then _convert_scalar() gets the > values one at a time. > > rob ack. pushed to master. ___ Freeipa-devel mailing list Freeipa-devel@redhat.com https://www.redhat.com/mailman/listinfo/freeipa-devel
Re: [Freeipa-devel] Return values, CRUD, webUI
Jason Gerard DeRose wrote:
The vast majority of our Command plugins subclass from one of the CRUD
base classes, so in terms of return value consistency and API style, we
need to focus most on them (and then adapt their style to the few
non-CRUD commands).
While hooking up the webUI there have been many, many small problems in
the core library and plugins that have caused unexpected setbacks for
me. Some features that I needed got changed without me noticing, some
of my half-baked designs needed more baking, some features were missing,
and some new code I was just unfamiliar with. Point is, I've spent a
lot of time battling little gotchas and thinking about how best to clean
these things up. Here are the guidelines I propose we follow:
A return value dict
===
As much as possible, I want to keep our return values very simple and
regular. This 1) makes our API easy to learn and use, and 2) makes it
easy to use the return values to drive UI logic on both the CLI and
webUI.
One current source of irregularity is the need to pass the "this isn't
all the entries" flag from LDAP when we do searches. For example,
`user_find` returns an (entry_list, more_remains) tuple. The problem is
that most of the code paths don't care about the `more_remains` flag...
they just need to know whether a list of entries was returned (result is
a list) or whether a single entry was returned (result is a dict).
At the same time, we obviously need a way to pass extra data like the
`more_remains` flag and it would be nice to be able to extend a return
value with additional special data without breaking code or causing an
explosion of special cases. So I propose that our return values always
be a dict containing at least a 'result', leaving us the option to
extend the return value without breaking code that just looks at
ret['result'].
So in the case of a search, instead of:
([{'uid': 'foo'}, {'uid': 'bar'}, ...], True)
We should return:
{
'result': [{'uid': 'foo'}, {'uid': 'bar'}, ...],
'more_remains': True,
...
'extend': 'as-needed',
}
The following all assume we are returning {'result': blah} even though
they don't show it...
Well, I think that we'll always need some sort of special value to
indicate when a limit has been exceeded. In v1 we decided to return what
we got instead of failing the entire search. This proposal works for me.
Entries
===
95% of our return values are LDAP entries. Currently we're returning
pretty much the raw value from python-ldap (although we are decoding
UTF-8 into `unicode` objects for use in the Python pipeline and encoding
back to UTF-8 on the way out, which is good). But the data structure
returned from python-ldap is pretty awkward to work with.
First, at the top it's typically a (dn, entry) tuple. Assuming the 'dn'
key doesn't conflict with any sane LDAP attribute names, I think we
should return a single dict with the dn stored under the 'dn' key.
So instead of:
('uid=jdoe,cn=users,cn=accounts,dc=example,dc=com', {'sn': ['Doe']})
We should return:
{'dn': 'uid=jdoe,cn=users,cn=accounts,dc=example,dc=com', 'sn': ['Doe']}
I don't know, it is kinda handy to have the dn separate. But if it makes
things more uniform I can deal with it. The advantage of having it
separate is it draws a clear line between what you can change and what
you can't. We'd have to strip out dn from updates and that might be
confusing for some users. I suppose what we could do is have the LDAP
module return a specific error if dn is included in an update.
Second, currently we return all attribute values inside a list whether
or not they're multi-value. This leads to lots of special cases
throughout the code that would be better dealt with in a single place,
in LDAP Backend adapter, IHMO.
So instead of:
{'uid': ['jdoe'], 'group': ['foo', 'bar']}
We should return:
{'uid': 'jdoe', 'group': ['foo', 'bar']}
Yes, I agree. What do you propose for a multi-valued attribute with a
single value, returned as a list or a scalar? In v1 we returned any
single value as a scalar and any multi-valued attr as a list.
Lists of Entries
When a command returns multiple entries, the entries should be in the
same form as they are from commands that return only one entry. For
example, currently user-find returns each entry as a (uid, entry) tuple.
I think this should again be replaced with a single dict without the uid
being duplicated.
Same but just within a list, right? So multiple entries are a list of
dicts that represent the result.
Create
==
If successful, we should return the resulting entry in standard form.
If any error occurs, we should raise an appropriate exception.
I think that in early versions of the framework we would return the
results of Retrieve if the creation was successful. I think we should do
that again in order to provide uniformity.
Retrieve
If
