[PATCH] Modify behavior of pam_pwd_expiration_warning

[Jan Zelený]

- rename the option to pwd_expiration_warning
- move the option from PAM responder to domains
- if pwd_expiration_warning == 0, don't apply the filter at all
- default value for Kerberos: 7 days
- default value for LDAP: don't apply the filter

Technical note: default value when creating the domain is -1. This is
important so we can distinguish between "no value set" and 0. Without
this possibility it would be impossible to set different values for LDAP
and Kerberos provider.

https://fedorahosted.org/sssd/ticket/1140

Thanks
Jan

[Stephen Gallagher]

On Tue, 2012-05-01 at 19:16 +0200, Jan Zeleny wrote:
> rename the option to pwd_expiration_warning
> move the option from PAM responder to domains
> if pwd_expiration_warning == 0, don't apply the filter at all
> default value for Kerberos: 7 days
> default value for LDAP: don't apply the filter
>
> Technical note: default value when creating the domain is -1. This is
> important so we can distinguish between "no value set" and 0. Without
> this possibility it would be impossible to set different values for LDAP
> and Kerberos provider.
>
> https://fedorahosted.org/sssd/ticket/1140

Nack

We cannot remove options without a deprecation period. Please do not
eliminate pam_pwd_expiration_warning. The better approach would be to
treat it as a global setting that the domain-level
pwd_expiration_warning options could override.

So setting pam_pwd_expiration_warning = 0 would be the same as saying
all domains would never set the filter. And having neither
pam_pwd_expiration_warning nor pwd_expiration_warning set would be "use
the defaults for every domain".

Please change the documentation for the pwd_expiration_warning to make
it clear that this is a limiter. The description for setting it to zero
sounds like it's saying "never display the expiration warning".

[Jan Zelený]

> On Tue, 2012-05-01 at 19:16 +0200, Jan Zeleny wrote:
> > rename the option to pwd_expiration_warning
> > move the option from PAM responder to domains
> > if pwd_expiration_warning == 0, don't apply the filter at all
> > default value for Kerberos: 7 days
> > default value for LDAP: don't apply the filter
> >
> > Technical note: default value when creating the domain is -1. This is
> > important so we can distinguish between "no value set" and 0. Without
> > this possibility it would be impossible to set different values for LDAP
> > and Kerberos provider.
> >
> > https://fedorahosted.org/sssd/ticket/1140
>
> Nack
>
> We cannot remove options without a deprecation period. Please do not
> eliminate pam_pwd_expiration_warning. The better approach would be to
> treat it as a global setting that the domain-level
> pwd_expiration_warning options could override.
>
> So setting pam_pwd_expiration_warning = 0 would be the same as saying
> all domains would never set the filter. And having neither
> pam_pwd_expiration_warning nor pwd_expiration_warning set would be "use
> the defaults for every domain".
>
> Please change the documentation for the pwd_expiration_warning to make
> it clear that this is a limiter. The description for setting it to zero
> sounds like it's saying "never display the expiration warning".

I'm sending corrected patch, all your comments are addressed.

Jan

[Stephen Gallagher]

On Thu, 2012-05-03 at 13:08 +0200, Jan Zelený wrote:
> > On Tue, 2012-05-01 at 19:16 +0200, Jan Zeleny wrote:
> > > rename the option to pwd_expiration_warning
> > > move the option from PAM responder to domains
> > > if pwd_expiration_warning == 0, don't apply the filter at all
> > > default value for Kerberos: 7 days
> > > default value for LDAP: don't apply the filter
> > >
> > > Technical note: default value when creating the domain is -1. This is
> > > important so we can distinguish between "no value set" and 0. Without
> > > this possibility it would be impossible to set different values for LDAP
> > > and Kerberos provider.
> > >
> > > https://fedorahosted.org/sssd/ticket/1140
> >
> > Nack
> >
> > We cannot remove options without a deprecation period. Please do not
> > eliminate pam_pwd_expiration_warning. The better approach would be to
> > treat it as a global setting that the domain-level
> > pwd_expiration_warning options could override.
> >
> > So setting pam_pwd_expiration_warning = 0 would be the same as saying
> > all domains would never set the filter. And having neither
> > pam_pwd_expiration_warning nor pwd_expiration_warning set would be "use
> > the defaults for every domain".
> >
> > Please change the documentation for the pwd_expiration_warning to make
> > it clear that this is a limiter. The description for setting it to zero
> > sounds like it's saying "never display the expiration warning".
>
> I'm sending corrected patch, all your comments are addressed.

Nack: you're still deleting the pam_pwd_expiration_warning option from
sssd-ldap.conf and you need to update the commit message. Otherwise,
looks good.

[Jan Zelený]

> On Thu, 2012-05-03 at 13:08 +0200, Jan Zelený wrote:
> > > On Tue, 2012-05-01 at 19:16 +0200, Jan Zeleny wrote:
> > > > rename the option to pwd_expiration_warning
> > > > move the option from PAM responder to domains
> > > > if pwd_expiration_warning == 0, don't apply the filter at all
> > > > default value for Kerberos: 7 days
> > > > default value for LDAP: don't apply the filter
> > > >
> > > > Technical note: default value when creating the domain is -1. This is
> > > > important so we can distinguish between "no value set" and 0. Without
> > > > this possibility it would be impossible to set different values for
> > > > LDAP and Kerberos provider.
> > > >
> > > > https://fedorahosted.org/sssd/ticket/1140
> > >
> > > Nack
> > >
> > > We cannot remove options without a deprecation period. Please do not
> > > eliminate pam_pwd_expiration_warning. The better approach would be to
> > > treat it as a global setting that the domain-level
> > > pwd_expiration_warning options could override.
> > >
> > > So setting pam_pwd_expiration_warning = 0 would be the same as saying
> > > all domains would never set the filter. And having neither
> > > pam_pwd_expiration_warning nor pwd_expiration_warning set would be "use
> > > the defaults for every domain".
> > >
> > > Please change the documentation for the pwd_expiration_warning to make
> > > it clear that this is a limiter. The description for setting it to zero
> > > sounds like it's saying "never display the expiration warning".
> >
> > I'm sending corrected patch, all your comments are addressed.
>
> Nack: you're still deleting the pam_pwd_expiration_warning option from
> sssd-ldap.conf and you need to update the commit message. Otherwise,
> looks good.

Thanks for noticing, I missed that completely. All done.

Jan

[Stephen Gallagher]

On Fri, 2012-05-04 at 09:02 +0200, Jan Zelený wrote:
> > Nack: you're still deleting the pam_pwd_expiration_warning option from
> > sssd-ldap.conf and you need to update the commit message. Otherwise,
> > looks good.
>
> Thanks for noticing, I missed that completely. All done.

Ack

[Stephen Gallagher]

On Fri, 2012-05-04 at 13:34 -0400, Stephen Gallagher wrote:
> On Fri, 2012-05-04 at 09:02 +0200, Jan Zelený wrote:
> > > Nack: you're still deleting the pam_pwd_expiration_warning option from
> > > sssd-ldap.conf and you need to update the commit message. Otherwise,
> > > looks good.
> >
> > Thanks for noticing, I missed that completely. All done.
>
> Ack

Pushed to master.

[Stephen Gallagher]

On Fri, 2012-05-04 at 13:37 -0400, Stephen Gallagher wrote:
> On Fri, 2012-05-04 at 13:34 -0400, Stephen Gallagher wrote:
> > On Fri, 2012-05-04 at 09:02 +0200, Jan Zelený wrote:
> > > > Nack: you're still deleting the pam_pwd_expiration_warning option from
> > > > sssd-ldap.conf and you need to update the commit message. Otherwise,
> > > > looks good.
> > >
> > > Thanks for noticing, I missed that completely. All done.
> >
> > Ack
>
> Pushed to master.

I should have re-tested. There was a bug in the SSSDConfigTest.py. I
pushed the attached patch under the one-liner and unbreak-the-build
rules.