[Freeipa-devel] [PATCH 0074] Make token window sizes configurable

[Nathaniel McCallum]

On Fri, 2014-11-07 at 15:02 +0100, thierry bordaz wrote:
> On 11/04/2014 05:17 PM, Nathaniel McCallum wrote:
> 
> > On Wed, 2014-10-29 at 09:34 -0400, Nathaniel McCallum wrote:
> > > On Wed, 2014-10-29 at 12:21 +0100, Petr Viktorin wrote:
> > > > On 10/29/2014 10:37 AM, Martin Kosek wrote:
> > > > > On 10/28/2014 09:59 PM, Nathaniel McCallum wrote:
> > > > > > On Thu, 2014-10-23 at 18:07 -0400, Nathaniel McCallum wrote:
> > > > > > > This patch gives the administrator variables to control the size of
> > > > > > > the authentication and synchronization windows for OTP tokens.
> > > > > > > 
> > > > > > > https://fedorahosted.org/freeipa/ticket/4511
> > > > > > > 
> > > > > > > NOTE: There is one known issue with this patch which I don't know how to
> > > > > > > solve. This patch changes the schema in install/share/60ipaconfig.ldif.
> > > > > > > On an upgrade, all of the new attributeTypes appear correctly. However,
> > > > > > > the modifications to the pre-existing objectClass do not show up on the
> > > > > > > server. What am I doing wrong?
> > > > > > > 
> > > > > > > After modifying ipaGuiConfig manually, everything in this patch works
> > > > > > > just fine.
> > > > > > This new version takes into account the new (proper) OIDs and attribute
> > > > > > names.
> > > > > Thanks Nathaniel!
> > > > > 
> > > > > > The above known issue still remains.
> > > > > Petr3, any idea what could have gone wrong? ObjectClass MAY list extension
> > > > > should work just fine, AFAIK.
> > > > You added a blank line to the LDIF file. This is an entry separator, so 
> > > > the objectClasses after the blank line don't belong to cn=schema, so 
> > > > they aren't considered in the update.
> > > > Without the blank line it works fine.
> > > Thanks for the catch!
> > > 
> > > Here is a version without the blank line.
> > I forgot to remove the old steps defines. This patch performs this
> > cleanup.
> > 
> > 
> > 
> > 
> > _______________________________________________
> > Freeipa-devel mailing list
> > Freeipa-devel at redhat.com
> > https://www.redhat.com/mailman/listinfo/freeipa-devel
> Hello Nathaniel,
> 
>         Few comments on the review:
>               * in authcfg
>                       * in string_to_types, I would prefer you set the
>                         last element of 'map' to { NULL, 0 }.

Why? What I have is perfectly legal ISO C and is exactly the equivalent
of your code. In a structure initializer, undefined structure elements
are zero'd.

>                       * in entry_to_window, you may declare the
>                         'defaults' array as 'static const'

Fixed.

>                       * Would use define for
>                         "ipaUserAuthType","ipaHOTPAuthWindow",
>                         "ipaTOTPAuthWindow",
>                         "ipaHOTPSyncWindow","ipaTOTPSyncWindow" that
>                         are present multiple times

Fixed.

>                       * suffix_to_config: cfg is set (and returned)
>                         calling entry_to_config(entry). Now the
>                         entry_to_config returns a structure on the
>                         stack so it is not valid to access outside of
>                         the entry_to_config

Nope. We are not passing by reference but by copy. This is perfectly
valid C.

>                       *  authcfg_fini free the configs. config->cfg
>                         should have been allocated and must be freed
>                         (be care that configs->cfg may contains
>                         DEFAULTS)

Nope. The config->cfg structure is allocated and freed when config is.
This is assignment by copy not by reference.

>                       * authcfg_get_auth_types:322 should it return
>                         'gbl' or AUTHCFG_AUTH_TYPE_PASSWORD

If both the global and per-user auth type is unset, the default is
AUTHCFG_AUTH_TYPE_PASSWORD. We special case this here so that we don't
have to special case it everywhere else in the code. The code is correct
as stands.

>                       * authcfg_get_auth_window/authcfg_get_sync_window returns a window structure that is on the stack. It is not valid outside of those functions

Nope. Structure return by copy is perfectly legal ISO C.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: freeipa-npmccallum-0074.4-Make-token-window-sizes-configurable.patch
Type: text/x-patch
Size: 38126 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/freeipa-devel/attachments/20141107/039d1451/attachment.bin>