Re: [Freeipa-devel] [PATCHES] 0012-13 Don't allow deleting required config options

[Martin Kosek]

On Fri, 2012-03-02 at 10:07 +0100, Petr Viktorin wrote:
 On 02/29/2012 04:09 PM, Petr Viktorin wrote:
  On 02/29/2012 03:53 PM, Rob Crittenden wrote:
  Petr Viktorin wrote:
  On 02/29/2012 11:14 AM, Jan Cholasta wrote:
  On 29.2.2012 11:09, Petr Viktorin wrote:
  On 02/28/2012 03:19 PM, Jan Cholasta wrote:
  On 28.2.2012 11:54, Petr Viktorin wrote:
  On 02/27/2012 10:44 PM, Rob Crittenden wrote:
  Petr Viktorin wrote:
  On 02/20/2012 08:51 PM, Rob Crittenden wrote:
  Petr Viktorin wrote:
  https://fedorahosted.org/freeipa/ticket/2159 says various config
  options
  are not marked Required, so entering an empty value for it will
  pass
  validation (and IPA will blow up later when it expects a
  string,not
  None). Forexample the following:
  $ ipa config-mod --groupsearch=
  fails with AttributeError: 'NoneType' object has no attribute
  'split'
 
  There is a more general problem behind this, though: even if the
  attributes *are* marked as Required, an empty string will pass
  validation. This is because `None` is used in
  `Param.validate` to
  mean
  both No value supplied and Empty value supplied. The method
  currently assumes the former, and skips validation entirely for
  `None`
  values to optional parameters.
 
  For example, the following will delete membergroup, even
  though
  it's a
  required attribute :
 
  $ ipa delegation-add --attrs=street --group=editors \
  --membergroup=admins td1
  $ ipa delegation-mod --membergroup= td1
 
  Note that some LDAPObjects handle this with a _check_empty_attrs
  function, so they aren't affected. That function is specific to
  LADP
  objects, though. So I needed to tackle this on a lower level.
 
  This patch solves the problem by
  * adding a 'nonempty' flag when a required parameter of a CRUD
  Update
  object is auto-converted to a non-required parameter
  * making the`validate` method aware of whether the parameter was
  supplied; and if it was, honor the nonempty flag.
 
 
  The second patch fixes
  https://fedorahosted.org/freeipa/ticket/2159 by
  marking required config options as required.
 
  This looks good but I think there are other things to protect in
  config
  as well such as the default e-mail domain. It is probably safe to
  say
  that everything in there is required.
 
  rob
 
  Let me just double-check this with you.
 
  According to code in the user plugin (around line 330), if the
  default
  e-mail domain is not set, users don't get an address
  auto-assigned. Do
  we really want to require user e-mails?
 
  ipaconfigstring (the password plugin flags) are a set (multivalue,
  not
  required).
 
  The rest of the values I left as not required are for optional
  features
  or limits: search results  time limit, max. username length,
  password
  expiry notification. Currently if these are missing, the
  feature/limit
  is disabled (well, except for the time limit).
  But, there are also special values (0 or -1) that have the same
  effect
  as a missing value. Sometimes they're documented.
  So we want to enforce that users use these special values
  instead of
  removing the config entry?
 
  I think we want to enforce that these are defined. It will be
  confusing
  for users if these are not there at all. I don't think we need to
  show
  the special options, just declare that the attribute is required.
 
  rob
 
 
  Attaching updated patch 13.
 
  Only the default e-mail domain
  (https://fedorahosted.org/freeipa/ticket/2409) and ipaconfigstring
  are
  still optional.
 
 
  You have removed all the config-related defensive code in the
  patch, is
  this a good idea? What will happen if someone e.g. deletes a required
  config attribute directly from LDAP?
 
 
  Then IPA crashes. The defensive code wasn't there for all cases
  anyway,
  as ticket #2159 shows.
 
  If we want to protect against this it would probably be better to make
  the config class itself give the default when a required value is
  missing.
 
 
  This, and raise an error in cases where no default is available (the
  check should probably be done in ldap.get_ipa_config).
 
  Honza
 
 
  Would a better approach be to modify the LDAP schema to require these
  values?
 
 
  I think that may be a longer-term fix. I propose we keep the defensive
  code in for now and correct it in the future.
 
  rob
 
  Here is an updated patch 13 that does that.
 
 
 And here is patch 12 rebased against current master.
 

ACK for 0012-02 and 0013-03. I think this is a good compromise for now.

Pushed to master, ipa-2-2.

Martin

___
Freeipa-devel mailing list
Freeipa-devel@redhat.com
https://www.redhat.com/mailman/listinfo/freeipa-devel

Re: [Freeipa-devel] [PATCHES] 0012-13 Don't allow deleting required config options

[Petr Viktorin]

On 02/29/2012 04:09 PM, Petr Viktorin wrote:

On 02/29/2012 03:53 PM, Rob Crittenden wrote:

Petr Viktorin wrote:

On 02/29/2012 11:14 AM, Jan Cholasta wrote:

On 29.2.2012 11:09, Petr Viktorin wrote:

On 02/28/2012 03:19 PM, Jan Cholasta wrote:

On 28.2.2012 11:54, Petr Viktorin wrote:

On 02/27/2012 10:44 PM, Rob Crittenden wrote:

Petr Viktorin wrote:

On 02/20/2012 08:51 PM, Rob Crittenden wrote:

Petr Viktorin wrote:

https://fedorahosted.org/freeipa/ticket/2159 says various config
options
are not marked Required, so entering an empty value for it will
pass
validation (and IPA will blow up later when it expects a
string,not
None). Forexample the following:
$ ipa config-mod --groupsearch=
fails with AttributeError: 'NoneType' object has no attribute
'split'

There is a more general problem behind this, though: even if the
attributes *are* marked as Required, an empty string will pass
validation. This is because `None` is used in
`Param.validate` to
mean
both No value supplied and Empty value supplied. The method
currently assumes the former, and skips validation entirely for
`None`
values to optional parameters.

For example, the following will delete membergroup, even
though
it's a
required attribute :

$ ipa delegation-add --attrs=street --group=editors \
--membergroup=admins td1
$ ipa delegation-mod --membergroup= td1

Note that some LDAPObjects handle this with a _check_empty_attrs
function, so they aren't affected. That function is specific to
LADP
objects, though. So I needed to tackle this on a lower level.

This patch solves the problem by
* adding a 'nonempty' flag when a required parameter of a CRUD
Update
object is auto-converted to a non-required parameter
* making the`validate` method aware of whether the parameter was
supplied; and if it was, honor the nonempty flag.


The second patch fixes
https://fedorahosted.org/freeipa/ticket/2159 by
marking required config options as required.


This looks good but I think there are other things to protect in
config
as well such as the default e-mail domain. It is probably safe to
say
that everything in there is required.

rob


Let me just double-check this with you.

According to code in the user plugin (around line 330), if the
default
e-mail domain is not set, users don't get an address
auto-assigned. Do
we really want to require user e-mails?

ipaconfigstring (the password plugin flags) are a set (multivalue,
not
required).

The rest of the values I left as not required are for optional
features
or limits: search results  time limit, max. username length,
password
expiry notification. Currently if these are missing, the
feature/limit
is disabled (well, except for the time limit).
But, there are also special values (0 or -1) that have the same
effect
as a missing value. Sometimes they're documented.
So we want to enforce that users use these special values
instead of
removing the config entry?


I think we want to enforce that these are defined. It will be
confusing
for users if these are not there at all. I don't think we need to
show
the special options, just declare that the attribute is required.

rob



Attaching updated patch 13.

Only the default e-mail domain
(https://fedorahosted.org/freeipa/ticket/2409) and ipaconfigstring
are
still optional.



You have removed all the config-related defensive code in the
patch, is
this a good idea? What will happen if someone e.g. deletes a required
config attribute directly from LDAP?



Then IPA crashes. The defensive code wasn't there for all cases
anyway,
as ticket #2159 shows.

If we want to protect against this it would probably be better to make
the config class itself give the default when a required value is
missing.



This, and raise an error in cases where no default is available (the
check should probably be done in ldap.get_ipa_config).

Honza



Would a better approach be to modify the LDAP schema to require these
values?



I think that may be a longer-term fix. I propose we keep the defensive
code in for now and correct it in the future.

rob


Here is an updated patch 13 that does that.



And here is patch 12 rebased against current master.

--
Petr³
From 00e06fef644ee538a49b4443f100611e2f99c9a0 Mon Sep 17 00:00:00 2001
From: Petr Viktorin pvikt...@redhat.com
Date: Thu, 16 Feb 2012 07:11:56 -0500
Subject: [PATCH 12/17] Enforce that required attributes can't be set to None
 in CRUD Update

The `required` parameter attribute didn't distinguish between cases
where the parameter is not given and all, and where the parameter is
given but empty. The case of updating a required attribute couldn't
be validated properly, because when it is given but empty, validators
don't run.
This patch introduces a new flag, 'nonempty', that specifies the
parameter can be missing (if not required), but it can't be None.
This flag gets added automatically to required parameters in CRUD
Update.
---
 ipalib/crud.py   |   13 +++--
 ipalib/frontend.py   |2 +-
 ipalib/parameters.py |9 

Re: [Freeipa-devel] [PATCHES] 0012-13 Don't allow deleting required config options

[Petr Viktorin]

On 02/28/2012 03:19 PM, Jan Cholasta wrote:

On 28.2.2012 11:54, Petr Viktorin wrote:

On 02/27/2012 10:44 PM, Rob Crittenden wrote:

Petr Viktorin wrote:

On 02/20/2012 08:51 PM, Rob Crittenden wrote:

Petr Viktorin wrote:

https://fedorahosted.org/freeipa/ticket/2159 says various config
options
are not marked Required, so entering an empty value for it will pass
validation (and IPA will blow up later when it expects a string,not
None). Forexample the following:
$ ipa config-mod --groupsearch=
fails with AttributeError: 'NoneType' object has no attribute 'split'

There is a more general problem behind this, though: even if the
attributes *are* marked as Required, an empty string will pass
validation. This is because `None` is used in `Param.validate` to
mean
both No value supplied and Empty value supplied. The method
currently assumes the former, and skips validation entirely for
`None`
values to optional parameters.

For example, the following will delete membergroup, even though
it's a
required attribute :

$ ipa delegation-add --attrs=street --group=editors \
--membergroup=admins td1
$ ipa delegation-mod --membergroup= td1

Note that some LDAPObjects handle this with a _check_empty_attrs
function, so they aren't affected. That function is specific to LADP
objects, though. So I needed to tackle this on a lower level.

This patch solves the problem by
* adding a 'nonempty' flag when a required parameter of a CRUD Update
object is auto-converted to a non-required parameter
* making the`validate` method aware of whether the parameter was
supplied; and if it was, honor the nonempty flag.


The second patch fixes
https://fedorahosted.org/freeipa/ticket/2159 by
marking required config options as required.


This looks good but I think there are other things to protect in
config
as well such as the default e-mail domain. It is probably safe to say
that everything in there is required.

rob


Let me just double-check this with you.

According to code in the user plugin (around line 330), if the default
e-mail domain is not set, users don't get an address auto-assigned. Do
we really want to require user e-mails?

ipaconfigstring (the password plugin flags) are a set (multivalue, not
required).

The rest of the values I left as not required are for optional features
or limits: search results  time limit, max. username length, password
expiry notification. Currently if these are missing, the feature/limit
is disabled (well, except for the time limit).
But, there are also special values (0 or -1) that have the same effect
as a missing value. Sometimes they're documented.
So we want to enforce that users use these special values instead of
removing the config entry?


I think we want to enforce that these are defined. It will be confusing
for users if these are not there at all. I don't think we need to show
the special options, just declare that the attribute is required.

rob



Attaching updated patch 13.

Only the default e-mail domain
(https://fedorahosted.org/freeipa/ticket/2409) and ipaconfigstring are
still optional.



You have removed all the config-related defensive code in the patch, is
this a good idea? What will happen if someone e.g. deletes a required
config attribute directly from LDAP?



Then IPA crashes. The defensive code wasn't there for all cases anyway, 
as ticket #2159 shows.


If we want to protect against this it would probably be better to make 
the config class itself give the default when a required value is missing.



--
Petr³

___
Freeipa-devel mailing list
Freeipa-devel@redhat.com
https://www.redhat.com/mailman/listinfo/freeipa-devel

Re: [Freeipa-devel] [PATCHES] 0012-13 Don't allow deleting required config options

[Jan Cholasta]

On 29.2.2012 11:09, Petr Viktorin wrote:

On 02/28/2012 03:19 PM, Jan Cholasta wrote:

On 28.2.2012 11:54, Petr Viktorin wrote:

On 02/27/2012 10:44 PM, Rob Crittenden wrote:

Petr Viktorin wrote:

On 02/20/2012 08:51 PM, Rob Crittenden wrote:

Petr Viktorin wrote:

https://fedorahosted.org/freeipa/ticket/2159 says various config
options
are not marked Required, so entering an empty value for it will pass
validation (and IPA will blow up later when it expects a string,not
None). Forexample the following:
$ ipa config-mod --groupsearch=
fails with AttributeError: 'NoneType' object has no attribute
'split'

There is a more general problem behind this, though: even if the
attributes *are* marked as Required, an empty string will pass
validation. This is because `None` is used in `Param.validate` to
mean
both No value supplied and Empty value supplied. The method
currently assumes the former, and skips validation entirely for
`None`
values to optional parameters.

For example, the following will delete membergroup, even though
it's a
required attribute :

$ ipa delegation-add --attrs=street --group=editors \
--membergroup=admins td1
$ ipa delegation-mod --membergroup= td1

Note that some LDAPObjects handle this with a _check_empty_attrs
function, so they aren't affected. That function is specific to LADP
objects, though. So I needed to tackle this on a lower level.

This patch solves the problem by
* adding a 'nonempty' flag when a required parameter of a CRUD
Update
object is auto-converted to a non-required parameter
* making the`validate` method aware of whether the parameter was
supplied; and if it was, honor the nonempty flag.


The second patch fixes
https://fedorahosted.org/freeipa/ticket/2159 by
marking required config options as required.


This looks good but I think there are other things to protect in
config
as well such as the default e-mail domain. It is probably safe to say
that everything in there is required.

rob


Let me just double-check this with you.

According to code in the user plugin (around line 330), if the default
e-mail domain is not set, users don't get an address auto-assigned. Do
we really want to require user e-mails?

ipaconfigstring (the password plugin flags) are a set (multivalue, not
required).

The rest of the values I left as not required are for optional
features
or limits: search results  time limit, max. username length, password
expiry notification. Currently if these are missing, the feature/limit
is disabled (well, except for the time limit).
But, there are also special values (0 or -1) that have the same effect
as a missing value. Sometimes they're documented.
So we want to enforce that users use these special values instead of
removing the config entry?


I think we want to enforce that these are defined. It will be confusing
for users if these are not there at all. I don't think we need to show
the special options, just declare that the attribute is required.

rob



Attaching updated patch 13.

Only the default e-mail domain
(https://fedorahosted.org/freeipa/ticket/2409) and ipaconfigstring are
still optional.



You have removed all the config-related defensive code in the patch, is
this a good idea? What will happen if someone e.g. deletes a required
config attribute directly from LDAP?



Then IPA crashes. The defensive code wasn't there for all cases anyway,
as ticket #2159 shows.

If we want to protect against this it would probably be better to make
the config class itself give the default when a required value is missing.



This, and raise an error in cases where no default is available (the 
check should probably be done in ldap.get_ipa_config).


Honza

--
Jan Cholasta

___
Freeipa-devel mailing list
Freeipa-devel@redhat.com
https://www.redhat.com/mailman/listinfo/freeipa-devel

Re: [Freeipa-devel] [PATCHES] 0012-13 Don't allow deleting required config options

[Petr Viktorin]

On 02/29/2012 11:14 AM, Jan Cholasta wrote:

On 29.2.2012 11:09, Petr Viktorin wrote:

On 02/28/2012 03:19 PM, Jan Cholasta wrote:

On 28.2.2012 11:54, Petr Viktorin wrote:

On 02/27/2012 10:44 PM, Rob Crittenden wrote:

Petr Viktorin wrote:

On 02/20/2012 08:51 PM, Rob Crittenden wrote:

Petr Viktorin wrote:

https://fedorahosted.org/freeipa/ticket/2159 says various config
options
are not marked Required, so entering an empty value for it will
pass
validation (and IPA will blow up later when it expects a string,not
None). Forexample the following:
$ ipa config-mod --groupsearch=
fails with AttributeError: 'NoneType' object has no attribute
'split'

There is a more general problem behind this, though: even if the
attributes *are* marked as Required, an empty string will pass
validation. This is because `None` is used in `Param.validate` to
mean
both No value supplied and Empty value supplied. The method
currently assumes the former, and skips validation entirely for
`None`
values to optional parameters.

For example, the following will delete membergroup, even though
it's a
required attribute :

$ ipa delegation-add --attrs=street --group=editors \
--membergroup=admins td1
$ ipa delegation-mod --membergroup= td1

Note that some LDAPObjects handle this with a _check_empty_attrs
function, so they aren't affected. That function is specific to
LADP
objects, though. So I needed to tackle this on a lower level.

This patch solves the problem by
* adding a 'nonempty' flag when a required parameter of a CRUD
Update
object is auto-converted to a non-required parameter
* making the`validate` method aware of whether the parameter was
supplied; and if it was, honor the nonempty flag.


The second patch fixes
https://fedorahosted.org/freeipa/ticket/2159 by
marking required config options as required.


This looks good but I think there are other things to protect in
config
as well such as the default e-mail domain. It is probably safe to
say
that everything in there is required.

rob


Let me just double-check this with you.

According to code in the user plugin (around line 330), if the
default
e-mail domain is not set, users don't get an address
auto-assigned. Do
we really want to require user e-mails?

ipaconfigstring (the password plugin flags) are a set (multivalue,
not
required).

The rest of the values I left as not required are for optional
features
or limits: search results  time limit, max. username length,
password
expiry notification. Currently if these are missing, the
feature/limit
is disabled (well, except for the time limit).
But, there are also special values (0 or -1) that have the same
effect
as a missing value. Sometimes they're documented.
So we want to enforce that users use these special values instead of
removing the config entry?


I think we want to enforce that these are defined. It will be
confusing
for users if these are not there at all. I don't think we need to show
the special options, just declare that the attribute is required.

rob



Attaching updated patch 13.

Only the default e-mail domain
(https://fedorahosted.org/freeipa/ticket/2409) and ipaconfigstring are
still optional.



You have removed all the config-related defensive code in the patch, is
this a good idea? What will happen if someone e.g. deletes a required
config attribute directly from LDAP?



Then IPA crashes. The defensive code wasn't there for all cases anyway,
as ticket #2159 shows.

If we want to protect against this it would probably be better to make
the config class itself give the default when a required value is
missing.



This, and raise an error in cases where no default is available (the
check should probably be done in ldap.get_ipa_config).

Honza



Would a better approach be to modify the LDAP schema to require these 
values?


--
Petr³

___
Freeipa-devel mailing list
Freeipa-devel@redhat.com
https://www.redhat.com/mailman/listinfo/freeipa-devel

Re: [Freeipa-devel] [PATCHES] 0012-13 Don't allow deleting required config options

[Rob Crittenden]

Petr Viktorin wrote:

On 02/29/2012 11:14 AM, Jan Cholasta wrote:

On 29.2.2012 11:09, Petr Viktorin wrote:

On 02/28/2012 03:19 PM, Jan Cholasta wrote:

On 28.2.2012 11:54, Petr Viktorin wrote:

On 02/27/2012 10:44 PM, Rob Crittenden wrote:

Petr Viktorin wrote:

On 02/20/2012 08:51 PM, Rob Crittenden wrote:

Petr Viktorin wrote:

https://fedorahosted.org/freeipa/ticket/2159 says various config
options
are not marked Required, so entering an empty value for it will
pass
validation (and IPA will blow up later when it expects a
string,not
None). Forexample the following:
$ ipa config-mod --groupsearch=
fails with AttributeError: 'NoneType' object has no attribute
'split'

There is a more general problem behind this, though: even if the
attributes *are* marked as Required, an empty string will pass
validation. This is because `None` is used in `Param.validate` to
mean
both No value supplied and Empty value supplied. The method
currently assumes the former, and skips validation entirely for
`None`
values to optional parameters.

For example, the following will delete membergroup, even though
it's a
required attribute :

$ ipa delegation-add --attrs=street --group=editors \
--membergroup=admins td1
$ ipa delegation-mod --membergroup= td1

Note that some LDAPObjects handle this with a _check_empty_attrs
function, so they aren't affected. That function is specific to
LADP
objects, though. So I needed to tackle this on a lower level.

This patch solves the problem by
* adding a 'nonempty' flag when a required parameter of a CRUD
Update
object is auto-converted to a non-required parameter
* making the`validate` method aware of whether the parameter was
supplied; and if it was, honor the nonempty flag.


The second patch fixes
https://fedorahosted.org/freeipa/ticket/2159 by
marking required config options as required.


This looks good but I think there are other things to protect in
config
as well such as the default e-mail domain. It is probably safe to
say
that everything in there is required.

rob


Let me just double-check this with you.

According to code in the user plugin (around line 330), if the
default
e-mail domain is not set, users don't get an address
auto-assigned. Do
we really want to require user e-mails?

ipaconfigstring (the password plugin flags) are a set (multivalue,
not
required).

The rest of the values I left as not required are for optional
features
or limits: search results  time limit, max. username length,
password
expiry notification. Currently if these are missing, the
feature/limit
is disabled (well, except for the time limit).
But, there are also special values (0 or -1) that have the same
effect
as a missing value. Sometimes they're documented.
So we want to enforce that users use these special values instead of
removing the config entry?


I think we want to enforce that these are defined. It will be
confusing
for users if these are not there at all. I don't think we need to
show
the special options, just declare that the attribute is required.

rob



Attaching updated patch 13.

Only the default e-mail domain
(https://fedorahosted.org/freeipa/ticket/2409) and ipaconfigstring are
still optional.



You have removed all the config-related defensive code in the patch, is
this a good idea? What will happen if someone e.g. deletes a required
config attribute directly from LDAP?



Then IPA crashes. The defensive code wasn't there for all cases anyway,
as ticket #2159 shows.

If we want to protect against this it would probably be better to make
the config class itself give the default when a required value is
missing.



This, and raise an error in cases where no default is available (the
check should probably be done in ldap.get_ipa_config).

Honza



Would a better approach be to modify the LDAP schema to require these
values?



I think that may be a longer-term fix. I propose we keep the defensive 
code in for now and correct it in the future.


rob

___
Freeipa-devel mailing list
Freeipa-devel@redhat.com
https://www.redhat.com/mailman/listinfo/freeipa-devel

Re: [Freeipa-devel] [PATCHES] 0012-13 Don't allow deleting required config options

[Petr Viktorin]

On 02/29/2012 03:53 PM, Rob Crittenden wrote:

Petr Viktorin wrote:

On 02/29/2012 11:14 AM, Jan Cholasta wrote:

On 29.2.2012 11:09, Petr Viktorin wrote:

On 02/28/2012 03:19 PM, Jan Cholasta wrote:

On 28.2.2012 11:54, Petr Viktorin wrote:

On 02/27/2012 10:44 PM, Rob Crittenden wrote:

Petr Viktorin wrote:

On 02/20/2012 08:51 PM, Rob Crittenden wrote:

Petr Viktorin wrote:

https://fedorahosted.org/freeipa/ticket/2159 says various config
options
are not marked Required, so entering an empty value for it will
pass
validation (and IPA will blow up later when it expects a
string,not
None). Forexample the following:
$ ipa config-mod --groupsearch=
fails with AttributeError: 'NoneType' object has no attribute
'split'

There is a more general problem behind this, though: even if the
attributes *are* marked as Required, an empty string will pass
validation. This is because `None` is used in `Param.validate` to
mean
both No value supplied and Empty value supplied. The method
currently assumes the former, and skips validation entirely for
`None`
values to optional parameters.

For example, the following will delete membergroup, even though
it's a
required attribute :

$ ipa delegation-add --attrs=street --group=editors \
--membergroup=admins td1
$ ipa delegation-mod --membergroup= td1

Note that some LDAPObjects handle this with a _check_empty_attrs
function, so they aren't affected. That function is specific to
LADP
objects, though. So I needed to tackle this on a lower level.

This patch solves the problem by
* adding a 'nonempty' flag when a required parameter of a CRUD
Update
object is auto-converted to a non-required parameter
* making the`validate` method aware of whether the parameter was
supplied; and if it was, honor the nonempty flag.


The second patch fixes
https://fedorahosted.org/freeipa/ticket/2159 by
marking required config options as required.


This looks good but I think there are other things to protect in
config
as well such as the default e-mail domain. It is probably safe to
say
that everything in there is required.

rob


Let me just double-check this with you.

According to code in the user plugin (around line 330), if the
default
e-mail domain is not set, users don't get an address
auto-assigned. Do
we really want to require user e-mails?

ipaconfigstring (the password plugin flags) are a set (multivalue,
not
required).

The rest of the values I left as not required are for optional
features
or limits: search results  time limit, max. username length,
password
expiry notification. Currently if these are missing, the
feature/limit
is disabled (well, except for the time limit).
But, there are also special values (0 or -1) that have the same
effect
as a missing value. Sometimes they're documented.
So we want to enforce that users use these special values
instead of
removing the config entry?


I think we want to enforce that these are defined. It will be
confusing
for users if these are not there at all. I don't think we need to
show
the special options, just declare that the attribute is required.

rob



Attaching updated patch 13.

Only the default e-mail domain
(https://fedorahosted.org/freeipa/ticket/2409) and ipaconfigstring
are
still optional.



You have removed all the config-related defensive code in the
patch, is
this a good idea? What will happen if someone e.g. deletes a required
config attribute directly from LDAP?



Then IPA crashes. The defensive code wasn't there for all cases anyway,
as ticket #2159 shows.

If we want to protect against this it would probably be better to make
the config class itself give the default when a required value is
missing.



This, and raise an error in cases where no default is available (the
check should probably be done in ldap.get_ipa_config).

Honza



Would a better approach be to modify the LDAP schema to require these
values?



I think that may be a longer-term fix. I propose we keep the defensive
code in for now and correct it in the future.

rob


Here is an updated patch 13 that does that.

--
Petr³
From fc0261471398999816581765ce28004a068cdfa2 Mon Sep 17 00:00:00 2001
From: Petr Viktorin pvikt...@redhat.com
Date: Mon, 20 Feb 2012 04:03:27 -0500
Subject: [PATCH] Mark most config options as required

IPA assumes most config options are present, but allowed the user
to delete them. This patch marks them as required.

https://fedorahosted.org/freeipa/ticket/2159
---
 ipalib/plugins/config.py |   30 +++---
 1 files changed, 15 insertions(+), 15 deletions(-)

diff --git a/ipalib/plugins/config.py b/ipalib/plugins/config.py
index c4615e3d1843a848e65090d64fd50fa833d81220..df960f4c0117e453ffb79ae7469476b5ff234f0d 100644
--- a/ipalib/plugins/config.py
+++ b/ipalib/plugins/config.py
@@ -97,22 +97,22 @@ class config(LDAPObject):
 label_singular = _('Configuration')
 
 takes_params = (
-Int('ipamaxusernamelength?',
+Int('ipamaxusernamelength',
 cli_name='maxusername',
 

Re: [Freeipa-devel] [PATCHES] 0012-13 Don't allow deleting required config options

[Petr Viktorin]

On 02/27/2012 10:44 PM, Rob Crittenden wrote:

Petr Viktorin wrote:

On 02/20/2012 08:51 PM, Rob Crittenden wrote:

Petr Viktorin wrote:

https://fedorahosted.org/freeipa/ticket/2159 says various config
options
are not marked Required, so entering an empty value for it will pass
validation (and IPA will blow up later when it expects a string,not
None). Forexample the following:
$ ipa config-mod --groupsearch=
fails with AttributeError: 'NoneType' object has no attribute 'split'

There is a more general problem behind this, though: even if the
attributes *are* marked as Required, an empty string will pass
validation. This is because `None` is used in `Param.validate` to mean
both No value supplied and Empty value supplied. The method
currently assumes the former, and skips validation entirely for `None`
values to optional parameters.

For example, the following will delete membergroup, even though
it's a
required attribute :

$ ipa delegation-add --attrs=street --group=editors \
--membergroup=admins td1
$ ipa delegation-mod --membergroup= td1

Note that some LDAPObjects handle this with a _check_empty_attrs
function, so they aren't affected. That function is specific to LADP
objects, though. So I needed to tackle this on a lower level.

This patch solves the problem by
* adding a 'nonempty' flag when a required parameter of a CRUD Update
object is auto-converted to a non-required parameter
* making the`validate` method aware of whether the parameter was
supplied; and if it was, honor the nonempty flag.


The second patch fixes https://fedorahosted.org/freeipa/ticket/2159 by
marking required config options as required.


This looks good but I think there are other things to protect in config
as well such as the default e-mail domain. It is probably safe to say
that everything in there is required.

rob


Let me just double-check this with you.

According to code in the user plugin (around line 330), if the default
e-mail domain is not set, users don't get an address auto-assigned. Do
we really want to require user e-mails?

ipaconfigstring (the password plugin flags) are a set (multivalue, not
required).

The rest of the values I left as not required are for optional features
or limits: search results  time limit, max. username length, password
expiry notification. Currently if these are missing, the feature/limit
is disabled (well, except for the time limit).
But, there are also special values (0 or -1) that have the same effect
as a missing value. Sometimes they're documented.
So we want to enforce that users use these special values instead of
removing the config entry?


I think we want to enforce that these are defined. It will be confusing
for users if these are not there at all. I don't think we need to show
the special options, just declare that the attribute is required.

rob



Attaching updated patch 13.

Only the default e-mail domain 
(https://fedorahosted.org/freeipa/ticket/2409) and ipaconfigstring are 
still optional.


--
Petr³
From a899c08e1b10c09f5c757d858db057e1d64f312f Mon Sep 17 00:00:00 2001
From: Petr Viktorin pvikt...@redhat.com
Date: Mon, 20 Feb 2012 04:03:27 -0500
Subject: [PATCH] Mark most config options as required

IPA assumes most config options are present, but allowed the user
to delete them. This patch marks them as required.

https://fedorahosted.org/freeipa/ticket/2159

Also, access to the options is changed to use indexing instead of get()
in all cases, as they no nonger need a default.
---
 ipalib/plugins/config.py  |   34 +++---
 ipalib/plugins/group.py   |2 +-
 ipalib/plugins/migration.py   |4 ++--
 ipalib/plugins/user.py|   12 ++--
 ipaserver/plugins/ldap2.py|8 ++--
 ipaserver/plugins/selfsign.py |2 +-
 6 files changed, 27 insertions(+), 35 deletions(-)

diff --git a/ipalib/plugins/config.py b/ipalib/plugins/config.py
index c4615e3d1843a848e65090d64fd50fa833d81220..6da51af544dcb5b36eb257b2dacbdfddbdb540cb 100644
--- a/ipalib/plugins/config.py
+++ b/ipalib/plugins/config.py
@@ -97,22 +97,22 @@ class config(LDAPObject):
 label_singular = _('Configuration')
 
 takes_params = (
-Int('ipamaxusernamelength?',
+Int('ipamaxusernamelength',
 cli_name='maxusername',
 label=_('Maximum username length'),
 minvalue=1,
 ),
-IA5Str('ipahomesrootdir?',
+IA5Str('ipahomesrootdir',
 cli_name='homedirectory',
 label=_('Home directory base'),
 doc=_('Default location of home directories'),
 ),
-Str('ipadefaultloginshell?',
+Str('ipadefaultloginshell',
 cli_name='defaultshell',
 label=_('Default shell'),
 doc=_('Default shell for new users'),
 ),
-Str('ipadefaultprimarygroup?',
+Str('ipadefaultprimarygroup',
 cli_name='defaultgroup',
 label=_('Default users group'),
 

Re: [Freeipa-devel] [PATCHES] 0012-13 Don't allow deleting required config options

[Jan Cholasta]

On 28.2.2012 11:54, Petr Viktorin wrote:

On 02/27/2012 10:44 PM, Rob Crittenden wrote:

Petr Viktorin wrote:

On 02/20/2012 08:51 PM, Rob Crittenden wrote:

Petr Viktorin wrote:

https://fedorahosted.org/freeipa/ticket/2159 says various config
options
are not marked Required, so entering an empty value for it will pass
validation (and IPA will blow up later when it expects a string,not
None). Forexample the following:
$ ipa config-mod --groupsearch=
fails with AttributeError: 'NoneType' object has no attribute 'split'

There is a more general problem behind this, though: even if the
attributes *are* marked as Required, an empty string will pass
validation. This is because `None` is used in `Param.validate` to mean
both No value supplied and Empty value supplied. The method
currently assumes the former, and skips validation entirely for `None`
values to optional parameters.

For example, the following will delete membergroup, even though
it's a
required attribute :

$ ipa delegation-add --attrs=street --group=editors \
--membergroup=admins td1
$ ipa delegation-mod --membergroup= td1

Note that some LDAPObjects handle this with a _check_empty_attrs
function, so they aren't affected. That function is specific to LADP
objects, though. So I needed to tackle this on a lower level.

This patch solves the problem by
* adding a 'nonempty' flag when a required parameter of a CRUD Update
object is auto-converted to a non-required parameter
* making the`validate` method aware of whether the parameter was
supplied; and if it was, honor the nonempty flag.


The second patch fixes https://fedorahosted.org/freeipa/ticket/2159 by
marking required config options as required.


This looks good but I think there are other things to protect in config
as well such as the default e-mail domain. It is probably safe to say
that everything in there is required.

rob


Let me just double-check this with you.

According to code in the user plugin (around line 330), if the default
e-mail domain is not set, users don't get an address auto-assigned. Do
we really want to require user e-mails?

ipaconfigstring (the password plugin flags) are a set (multivalue, not
required).

The rest of the values I left as not required are for optional features
or limits: search results  time limit, max. username length, password
expiry notification. Currently if these are missing, the feature/limit
is disabled (well, except for the time limit).
But, there are also special values (0 or -1) that have the same effect
as a missing value. Sometimes they're documented.
So we want to enforce that users use these special values instead of
removing the config entry?


I think we want to enforce that these are defined. It will be confusing
for users if these are not there at all. I don't think we need to show
the special options, just declare that the attribute is required.

rob



Attaching updated patch 13.

Only the default e-mail domain
(https://fedorahosted.org/freeipa/ticket/2409) and ipaconfigstring are
still optional.



You have removed all the config-related defensive code in the patch, is 
this a good idea? What will happen if someone e.g. deletes a required 
config attribute directly from LDAP?


Honza

--
Jan Cholasta

___
Freeipa-devel mailing list
Freeipa-devel@redhat.com
https://www.redhat.com/mailman/listinfo/freeipa-devel

Re: [Freeipa-devel] [PATCHES] 0012-13 Don't allow deleting required config options

[Rob Crittenden]

Petr Viktorin wrote:

On 02/20/2012 08:51 PM, Rob Crittenden wrote:

Petr Viktorin wrote:

https://fedorahosted.org/freeipa/ticket/2159 says various config options
are not marked Required, so entering an empty value for it will pass
validation (and IPA will blow up later when it expects a string,not
None). Forexample the following:
$ ipa config-mod --groupsearch=
fails with AttributeError: 'NoneType' object has no attribute 'split'

There is a more general problem behind this, though: even if the
attributes *are* marked as Required, an empty string will pass
validation. This is because `None` is used in `Param.validate` to mean
both No value supplied and Empty value supplied. The method
currently assumes the former, and skips validation entirely for `None`
values to optional parameters.

For example, the following will delete membergroup, even though it's a
required attribute :

$ ipa delegation-add --attrs=street --group=editors \
--membergroup=admins td1
$ ipa delegation-mod --membergroup= td1

Note that some LDAPObjects handle this with a _check_empty_attrs
function, so they aren't affected. That function is specific to LADP
objects, though. So I needed to tackle this on a lower level.

This patch solves the problem by
* adding a 'nonempty' flag when a required parameter of a CRUD Update
object is auto-converted to a non-required parameter
* making the`validate` method aware of whether the parameter was
supplied; and if it was, honor the nonempty flag.


The second patch fixes https://fedorahosted.org/freeipa/ticket/2159 by
marking required config options as required.


This looks good but I think there are other things to protect in config
as well such as the default e-mail domain. It is probably safe to say
that everything in there is required.

rob


Let me just double-check this with you.

According to code in the user plugin (around line 330), if the default
e-mail domain is not set, users don't get an address auto-assigned. Do
we really want to require user e-mails?

ipaconfigstring (the password plugin flags) are a set (multivalue, not
required).

The rest of the values I left as not required are for optional features
or limits: search results  time limit, max. username length, password
expiry notification. Currently if these are missing, the feature/limit
is disabled (well, except for the time limit).
But, there are also special values (0 or -1) that have the same effect
as a missing value. Sometimes they're documented.
So we want to enforce that users use these special values instead of
removing the config entry?


I think we want to enforce that these are defined. It will be confusing 
for users if these are not there at all. I don't think we need to show 
the special options, just declare that the attribute is required.


rob

___
Freeipa-devel mailing list
Freeipa-devel@redhat.com
https://www.redhat.com/mailman/listinfo/freeipa-devel

Re: [Freeipa-devel] [PATCHES] 0012-13 Don't allow deleting required config options

[Petr Viktorin]

On 02/20/2012 08:51 PM, Rob Crittenden wrote:

Petr Viktorin wrote:

https://fedorahosted.org/freeipa/ticket/2159 says various config options
are not marked Required, so entering an empty value for it will pass
validation (and IPA will blow up later when it expects a string,not
None). Forexample the following:
$ ipa config-mod --groupsearch=
fails with AttributeError: 'NoneType' object has no attribute 'split'

There is a more general problem behind this, though: even if the
attributes *are* marked as Required, an empty string will pass
validation. This is because `None` is used in `Param.validate` to mean
both No value supplied and Empty value supplied. The method
currently assumes the former, and skips validation entirely for `None`
values to optional parameters.

For example, the following will delete membergroup, even though it's a
required attribute :

$ ipa delegation-add --attrs=street --group=editors \
--membergroup=admins td1
$ ipa delegation-mod --membergroup= td1

Note that some LDAPObjects handle this with a _check_empty_attrs
function, so they aren't affected. That function is specific to LADP
objects, though. So I needed to tackle this on a lower level.

This patch solves the problem by
* adding a 'nonempty' flag when a required parameter of a CRUD Update
object is auto-converted to a non-required parameter
* making the`validate` method aware of whether the parameter was
supplied; and if it was, honor the nonempty flag.


The second patch fixes https://fedorahosted.org/freeipa/ticket/2159 by
marking required config options as required.


This looks good but I think there are other things to protect in config
as well such as the default e-mail domain. It is probably safe to say
that everything in there is required.

rob


Let me just double-check this with you.

According to code in the user plugin (around line 330), if the default 
e-mail domain is not set, users don't get an address auto-assigned. Do 
we really want to require user e-mails?


ipaconfigstring (the password plugin flags) are a set (multivalue, not 
required).


The rest of the values I left as not required are for optional features 
or limits: search results  time limit, max. username length, password 
expiry notification. Currently if these are missing, the feature/limit 
is disabled (well, except for the time limit).
But, there are also special values (0 or -1) that have the same effect 
as a missing value. Sometimes they're documented.
So we want to enforce that users use these special values instead of 
removing the config entry?



--
Petr³

___
Freeipa-devel mailing list
Freeipa-devel@redhat.com
https://www.redhat.com/mailman/listinfo/freeipa-devel

Re: [Freeipa-devel] [PATCHES] 0012-13 Don't allow deleting required config options

[Rob Crittenden]

Petr Viktorin wrote:

https://fedorahosted.org/freeipa/ticket/2159 says various config options
are not marked Required, so entering an empty value for it will pass
validation (and IPA will blow up later when it expects a string,not
None). Forexample the following:
$ ipa config-mod --groupsearch=
fails with AttributeError: 'NoneType' object has no attribute 'split'

There is a more general problem behind this, though: even if the
attributes *are* marked as Required, an empty string will pass
validation. This is because `None` is used in `Param.validate` to mean
both No value supplied and Empty value supplied. The method
currently assumes the former, and skips validation entirely for `None`
values to optional parameters.

For example, the following will delete membergroup, even though it's a
required attribute :

$ ipa delegation-add --attrs=street --group=editors \
--membergroup=admins td1
$ ipa delegation-mod --membergroup= td1

Note that some LDAPObjects handle this with a _check_empty_attrs
function, so they aren't affected. That function is specific to LADP
objects, though. So I needed to tackle this on a lower level.

This patch solves the problem by
* adding a 'nonempty' flag when a required parameter of a CRUD Update
object is auto-converted to a non-required parameter
* making the`validate` method aware of whether the parameter was
supplied; and if it was, honor the nonempty flag.


The second patch fixes https://fedorahosted.org/freeipa/ticket/2159 by
marking required config options as required.


This looks good but I think there are other things to protect in config 
as well such as the default e-mail domain. It is probably safe to say 
that everything in there is required.


rob

___
Freeipa-devel mailing list
Freeipa-devel@redhat.com
https://www.redhat.com/mailman/listinfo/freeipa-devel