[PATCHES] PAM: authenticate agains cache

[Pavel Reichl]

Hello,

please see first version of these patches. I'm currently working on unit 
test for the second patch which will be part of the second revision of 
the patch set.

Thanks!

[Pavel Reichl]

On 05/20/2015 05:16 PM, Pavel Reichl wrote:
> Hello,
>
> please see first version of these patches. I'm currently working on 
> unit test for the second patch which will be part of the second 
> revision of the patch set.
>
> Thanks!
>
> sssd-devel mailing list
> [e-mail]
> https://lists.fedorahosted.org/mailman/listinfo/sssd-devel

Jakub asked me off list to move resetting of 
SYSDB_LAST_ONLINE_AUTH_WITH_CURRENT_TOKEN attribute from ldap code to 
pam code. Please see updated patch set.

Thanks!

[Lukas Slebodnik]

On (05/06/15 19:01), Pavel Reichl wrote:
> On 05/20/2015 05:16 PM, Pavel Reichl wrote:
> > Hello,
> >
> > please see first version of these patches. I'm currently working on unit
> > test for the second patch which will be part of the second revision of the
> > patch set.
> >
> > Thanks!
> >
> > sssd-devel mailing list
> > [e-mail]
> > https://lists.fedorahosted.org/mailman/listinfo/sssd-devel
>
> Jakub asked me off list to move resetting of
> SYSDB_LAST_ONLINE_AUTH_WITH_CURRENT_TOKEN attribute from ldap code to pam
> code. Please see updated patch set.
>
> Thanks!
> From d106a73df3508db94ed8936fab2864e18d838b49 Mon Sep 17 00:00:00 2001
> From: Pavel Reichl 
> Date: Mon, 18 May 2015 09:59:38 -0400
> Subject: [PATCH 1/2] sysdb: new attribute lastOnlineAuthWithCurrentToken
>
> Introduce new user attribute lastOnlineAuthWithCurrentToken. This attribute
> behaves similarly to lastOnlineAuth but is set to NULL after password is
> changed.
>
> This attribute is needed for use-case when cached authentication is used, to
> request online authentication after password is locally changed.
>
> Resolves:
> https://fedorahosted.org/sssd/ticket/1807
>
> src/db/sysdb.h                 | 14 +++++++
> src/db/sysdb_ops.c             | 95 ++++++++++++++++++++++++++++++++++++++++++
> src/responder/pam/pamsrv_cmd.c | 18 ++++++++
> src/tests/sysdb-tests.c        | 76 +++++++++++++++++++++++++++++++++
> 4 files changed, 203 insertions(+)
>
> diff --git a/src/db/sysdb.h b/src/db/sysdb.h
> index c3d2c1406321e01b325e04e0e3f263893eb91b2e..3934e6e11afb63b1f51ed81f18452d1bd79190ed 100644
> --- a/src/db/sysdb.h
> +++ b/src/db/sysdb.h
> @@ -87,6 +87,7 @@
> #define SYSDB_LAST_ONLINE_AUTH "lastOnlineAuth"
> #define SYSDB_LAST_FAILED_LOGIN "lastFailedLogin"
> #define SYSDB_FAILED_LOGIN_ATTEMPTS "failedLoginAttempts"
> +#define SYSDB_LAST_ONLINE_AUTH_WITH_CURR_TOKEN "lastOnlineAuthWithCurrentToken"
>
> #define SYSDB_LAST_UPDATE "lastUpdate"
> #define SYSDB_CACHE_EXPIRE "dataExpireTimestamp"
> @@ -897,6 +898,19 @@ int sysdb_cache_password_ex(struct sss_domain_info *domain,
>                             enum sss_authtok_type authtok_type,
>                             size_t second_factor_size);
>
> +errno_t
> +sysdb_set_last_online_auth_with_curr_token(struct sss_domain_info *domain,
>                                       const char *username,
>                                       uint64_t value);
>
> +errno_t
> +sysdb_null_last_online_auth_with_curr_token(struct sss_domain_info *domain,
>                                        const char *username);
>
> +errno_t
> +sysdb_get_last_online_auth_with_curr_token(struct sss_domain_info *domain,
>                                       const char *name,
>                                       uint64_t *_value);

Should we also create following sydb functions?

sysdb_set_last_online_auth
sysdb_null_last_online_auth
sysdb_get_last_online_auth
sysdb_set_last_failed_login
sysdb_null_last_failed_login
sysdb_get_last_failed_login
sysdb_set_data_expire_timestamp
sysdb_null_data_expire_timestamp
sysdb_get_data_expire_timestamp
sysdb_set_initgroups_expire_timestamp
sysdb_null_initgroups_expire_timestamp
sysdb_get_initgroups_expire_timestamp
...

LS

[Pavel Reichl]

On 06/05/2015 07:45 PM, Lukas Slebodnik wrote:
> On (05/06/15 19:01), Pavel Reichl wrote:
> > On 05/20/2015 05:16 PM, Pavel Reichl wrote:
> > > Hello,
> > >
> > > please see first version of these patches. I'm currently working on unit
> > > test for the second patch which will be part of the second revision of the
> > > patch set.
> > >
> > > Thanks!
> > >
> > > sssd-devel mailing list
> > > [e-mail]
> > > https://lists.fedorahosted.org/mailman/listinfo/sssd-devel
> >
> > Jakub asked me off list to move resetting of
> > SYSDB_LAST_ONLINE_AUTH_WITH_CURRENT_TOKEN attribute from ldap code to pam
> > code. Please see updated patch set.
> >
> > Thanks!
> > From d106a73df3508db94ed8936fab2864e18d838b49 Mon Sep 17 00:00:00 2001
> > From: Pavel Reichl 
> > Date: Mon, 18 May 2015 09:59:38 -0400
> > Subject: [PATCH 1/2] sysdb: new attribute lastOnlineAuthWithCurrentToken
> >
> > Introduce new user attribute lastOnlineAuthWithCurrentToken. This attribute
> > behaves similarly to lastOnlineAuth but is set to NULL after password is
> > changed.
> >
> > This attribute is needed for use-case when cached authentication is used, to
> > request online authentication after password is locally changed.
> >
> > Resolves:
> > https://fedorahosted.org/sssd/ticket/1807
> >
> > src/db/sysdb.h                 | 14 +++++++
> > src/db/sysdb_ops.c             | 95 ++++++++++++++++++++++++++++++++++++++++++
> > src/responder/pam/pamsrv_cmd.c | 18 ++++++++
> > src/tests/sysdb-tests.c        | 76 +++++++++++++++++++++++++++++++++
> > 4 files changed, 203 insertions(+)
> >
> > diff --git a/src/db/sysdb.h b/src/db/sysdb.h
> > index c3d2c1406321e01b325e04e0e3f263893eb91b2e..3934e6e11afb63b1f51ed81f18452d1bd79190ed 100644
> > --- a/src/db/sysdb.h
> > +++ b/src/db/sysdb.h
> > @@ -87,6 +87,7 @@
> > #define SYSDB_LAST_ONLINE_AUTH "lastOnlineAuth"
> > #define SYSDB_LAST_FAILED_LOGIN "lastFailedLogin"
> > #define SYSDB_FAILED_LOGIN_ATTEMPTS "failedLoginAttempts"
> > +#define SYSDB_LAST_ONLINE_AUTH_WITH_CURR_TOKEN "lastOnlineAuthWithCurrentToken"
> >
> > #define SYSDB_LAST_UPDATE "lastUpdate"
> > #define SYSDB_CACHE_EXPIRE "dataExpireTimestamp"
> > @@ -897,6 +898,19 @@ int sysdb_cache_password_ex(struct sss_domain_info *domain,
> >                              enum sss_authtok_type authtok_type,
> >                              size_t second_factor_size);
> >
> > +errno_t
> > +sysdb_set_last_online_auth_with_curr_token(struct sss_domain_info *domain,
> >                                       const char *username,
> >                                       uint64_t value);
> >
> > +errno_t
> > +sysdb_null_last_online_auth_with_curr_token(struct sss_domain_info *domain,
> >                                        const char *username);
> >
> > +errno_t
> > +sysdb_get_last_online_auth_with_curr_token(struct sss_domain_info *domain,
> >                                       const char *name,
> >                                       uint64_t *_value);
>
> Should we also create following sydb functions?
>
> sysdb_set_last_online_auth
> sysdb_null_last_online_auth
> sysdb_get_last_online_auth
> sysdb_set_last_failed_login
> sysdb_null_last_failed_login
> sysdb_get_last_failed_login
> sysdb_set_data_expire_timestamp
> sysdb_null_data_expire_timestamp
> sysdb_get_data_expire_timestamp
> sysdb_set_initgroups_expire_timestamp
> sysdb_null_initgroups_expire_timestamp
> sysdb_get_initgroups_expire_timestamp
> ...
>
> LS

Please do whatever you find to be the best for the code.

If you have comments to the patch please be more specific and state what 
and why you don't like so I don't have to guess and waste neither my 
time nor yours. Thanks!
> sssd-devel mailing list
> [e-mail]
> https://lists.fedorahosted.org/mailman/listinfo/sssd-devel

[Jakub Hrozek]

On Fri, Jun 05, 2015 at 07:01:30PM +0200, Pavel Reichl wrote:
> On 05/20/2015 05:16 PM, Pavel Reichl wrote:
> > Hello,
> >
> > please see first version of these patches. I'm currently working on unit
> > test for the second patch which will be part of the second revision of the
> > patch set.
> >
> > Thanks!
> >
> > sssd-devel mailing list
> > [e-mail]
> > https://lists.fedorahosted.org/mailman/listinfo/sssd-devel
>
> Jakub asked me off list to move resetting of
> SYSDB_LAST_ONLINE_AUTH_WITH_CURRENT_TOKEN attribute from ldap code to pam
> code. Please see updated patch set.
>
> Thanks!

Can you rebase the patches, please? I'll take a look at them..

[Pavel Reichl]

On 06/23/2015 10:34 AM, Jakub Hrozek wrote:
> On Fri, Jun 05, 2015 at 07:01:30PM +0200, Pavel Reichl wrote:
> > On 05/20/2015 05:16 PM, Pavel Reichl wrote:
> > > Hello,
> > >
> > > please see first version of these patches. I'm currently working on unit
> > > test for the second patch which will be part of the second revision of the
> > > patch set.
> > >
> > > Thanks!
> > >
> > > sssd-devel mailing list
> > > [e-mail]
> > > https://lists.fedorahosted.org/mailman/listinfo/sssd-devel
> >
> > Jakub asked me off list to move resetting of
> > SYSDB_LAST_ONLINE_AUTH_WITH_CURRENT_TOKEN attribute from ldap code to pam
> > code. Please see updated patch set.
> >
> > Thanks!
>
> Can you rebase the patches, please? I'll take a look at them..
> _______________________________________________
> sssd-devel mailing list
> [e-mail]
> https://lists.fedorahosted.org/mailman/listinfo/sssd-devel

Sure. Rebased patches attached.

[Jakub Hrozek]

On Tue, Jun 23, 2015 at 10:55:41AM +0200, Pavel Reichl wrote:
> On 06/23/2015 10:34 AM, Jakub Hrozek wrote:
> > On Fri, Jun 05, 2015 at 07:01:30PM +0200, Pavel Reichl wrote:
> > > On 05/20/2015 05:16 PM, Pavel Reichl wrote:
> > > > Hello,
> > > >
> > > > please see first version of these patches. I'm currently working on unit
> > > > test for the second patch which will be part of the second revision of the
> > > > patch set.
> > > >
> > > > Thanks!
> > > >
> > > > sssd-devel mailing list
> > > > [e-mail]
> > > > https://lists.fedorahosted.org/mailman/listinfo/sssd-devel
> > >
> > > Jakub asked me off list to move resetting of
> > > SYSDB_LAST_ONLINE_AUTH_WITH_CURRENT_TOKEN attribute from ldap code to pam
> > > code. Please see updated patch set.
> > >
> > > Thanks!
> >
> > Can you rebase the patches, please? I'll take a look at them..
> > _______________________________________________
> > sssd-devel mailing list
> > [e-mail]
> > https://lists.fedorahosted.org/mailman/listinfo/sssd-devel
>
> Sure. Rebased patches attached.

Hi,

We should extend the PAM responder unit test together with these
patches so that the new functionality is covered.

see some comments inline.
> From 754b855b2c2877cd2121cc0644a7450a52a5b0d4 Mon Sep 17 00:00:00 2001
> From: Pavel Reichl 
> Date: Mon, 18 May 2015 09:59:38 -0400
> Subject: [PATCH 1/2] sysdb: new attribute lastOnlineAuthWithCurrentToken
>
> Introduce new user attribute lastOnlineAuthWithCurrentToken. This attribute
> behaves similarly to lastOnlineAuth but is set to NULL after password is
> changed.
>
> This attribute is needed for use-case when cached authentication is used, to
> request online authentication after password is locally changed.
>
> Resolves:
> https://fedorahosted.org/sssd/ticket/1807
>
> src/db/sysdb.h                 | 14 +++++++
>  src/db/sysdb_ops.c             | 95 ++++++++++++++++++++++++++++++++++++++++++
>  src/responder/pam/pamsrv_cmd.c | 18 ++++++++
>  src/tests/sysdb-tests.c        | 76 +++++++++++++++++++++++++++++++++
>  4 files changed, 203 insertions(+)
>
> diff --git a/src/db/sysdb.h b/src/db/sysdb.h
> index 4dc382f6fb3f1b6bd084312463edf1b6189ebba9..3e8ddb7b169abd84d8cbcd2f58c347622860ce98 100644
> --- a/src/db/sysdb.h
> +++ b/src/db/sysdb.h
> @@ -87,6 +87,7 @@
>  #define SYSDB_LAST_ONLINE_AUTH "lastOnlineAuth"
>  #define SYSDB_LAST_FAILED_LOGIN "lastFailedLogin"
>  #define SYSDB_FAILED_LOGIN_ATTEMPTS "failedLoginAttempts"
> +#define SYSDB_LAST_ONLINE_AUTH_WITH_CURR_TOKEN "lastOnlineAuthWithCurrentToken"
>
> #define SYSDB_LAST_UPDATE "lastUpdate"
>  #define SYSDB_CACHE_EXPIRE "dataExpireTimestamp"
> @@ -904,6 +905,19 @@ int sysdb_cache_password_ex(struct sss_domain_info *domain,
>                              enum sss_authtok_type authtok_type,
>                              size_t second_factor_size);
>
> +errno_t
> +sysdb_set_last_online_auth_with_curr_token(struct sss_domain_info *domain,
>                                       const char *username,
>                                       uint64_t value);

Since set_last_login() sets the attribute directly, can we make this
function static?

btw I think that the diff from src/responder/pam/pamsrv_cmd.c should be
a separate patch so that we have one patch that adds the sysdb API and
one that uses the new attribute in PAM responder.
> +errno_t
> +sysdb_null_last_online_auth_with_curr_token(struct sss_domain_info *domain,
>                                        const char *username);
>
> +errno_t
> +sysdb_get_last_online_auth_with_curr_token(struct sss_domain_info *domain,
>                                       const char *name,
>                                       uint64_t *_value);
>
> errno_t check_failed_login_attempts(struct confdb_ctx *cdb,
>                                      struct ldb_message *ldb_msg,
>                                      uint32_t *failed_login_attempts,

[...]
> From ed449b272beafecd217cea86f514766ad35fd5b9 Mon Sep 17 00:00:00 2001
> From: Pavel Reichl 
> Date: Thu, 16 Apr 2015 03:41:58 -0400
> Subject: [PATCH 2/2] PAM: authenticate agains cache
>
> Enable authenticating users from cache even when SSSD is in online mode.
>
> Introduce new option `cached_auth_timeout`.
>
> Resolves:
> https://fedorahosted.org/sssd/ticket/1807
>
> src/confdb/confdb.c                  |  49 +++++++++++++
>  src/confdb/confdb.h                  |   2 +
>  src/config/SSSDConfig/__init__.py.in |   1 +
>  src/config/SSSDConfigTest.py         |   6 +-
>  src/config/etc/sssd.api.conf         |   1 +
>  src/db/sysdb_ops.c                   |   1 -
>  src/man/sssd.conf.5.xml              |  14 ++++
>  src/responder/pam/pamsrv.h           |   3 +
>  src/responder/pam/pamsrv_cmd.c       | 134 +++++++++++++++++++++++++++++++++--
>  9 files changed, 201 insertions(+), 10 deletions(-)

[...]
> diff --git a/src/config/SSSDConfig/__init__.py.in b/src/config/SSSDConfig/__init__.py.in
> index f2d9bf0190745d4a5296c56212e5ff97681db59c..79a831dbec31e96f663f5472c40e6aedc7259f11 100644
> --- a/src/config/SSSDConfig/__init__.py.in
> +++ b/src/config/SSSDConfig/__init__.py.in
> @@ -149,6 +149,7 @@ option_strings = {
>      'subdomain_enumerate' : _('Control enumeration of trusted domains'),
>      'subdomain_refresh_interval' : _('How often should subdomains list be refreshed'),
>      'subdomain_inherit' : _('List of options that should be inherited into a subdomain'),
> 'cached_auth_timeout' : _('How long can be cached credentials used for cached authentication'),

I think this should better read 'How long can cached credentials be
used..' but feel free to ping some native speaker.
>  # [provider/ipa]
>  'ipa_domain' : _('IPA domain'),

[..]
> --- a/src/man/sssd.conf.5.xml
> +++ b/src/man/sssd.conf.5.xml
> @@ -988,6 +988,20 @@ pam_account_expired_message = Account expired, please call help desk.
>                          </para>
>                      </listitem>
>                  </varlistentry>
>            <varlistentry>
>                <term>cached_auth_timeout (int)</term>
>                <listitem>
>                    <para>
>                        Specifies time in seconds since last successful
>                        online authentication for which user will be
>                        authenticated using cached credentials while
>                        SSSD is in the online mode.
>                    </para>
>                    <para>
>                        Default: 0

The manpage should specify what does the default mean.
>                    </para>
>                </listitem>
>            </varlistentry>
>
>        </variablelist>
>    </refsect2>
>
> diff --git a/src/responder/pam/pamsrv.h b/src/responder/pam/pamsrv.h
> index 066f35a428a9af81d665309b4ab5a80cf69561ba..0278006467a41b28a1d47a8777074f265c5c478e 100644
> --- a/src/responder/pam/pamsrv.h
> +++ b/src/responder/pam/pamsrv.h
> @@ -60,6 +60,9 @@ struct pam_auth_req {
>      bool is_uid_trusted;
>      bool check_provider;
>      void *data;
>
> bool use_cached_auth;
>
> /* whether cached authentication was tried and failed */
>
> bool cached_auth_failed;
>
> struct pam_auth_dp_req *dpreq_spy;
>
> };
> diff --git a/src/responder/pam/pamsrv_cmd.c b/src/responder/pam/pamsrv_cmd.c
> index ae571fc969bfaf8c9ec90023ca1f4f38d31211e2..833fd096f64a7db1ad580f72cbd5022f77267b33 100644
> --- a/src/responder/pam/pamsrv_cmd.c
> +++ b/src/responder/pam/pamsrv_cmd.c
> @@ -565,7 +565,7 @@ static errno_t get_password_for_cache_auth(struct sss_auth_token *authtok,
>
> static int pam_forwarder(struct cli_ctx *cctx, int pam_cmd);
>  static void pam_handle_cached_login(struct pam_auth_req *preq, int ret,
>                                time_t expire_date, time_t delayed_until);
>                                time_t expire_date, time_t delayed_until, bool cached_auth);
>
> static void pam_reply(struct pam_auth_req *preq)
>  {
> @@ -603,14 +603,21 @@ static void pam_reply(struct pam_auth_req *preq)
>      DEBUG(SSSDBG_FUNC_DATA,
>            "pam_reply called with result [%d]: %s.\n",
>            pd->pam_status, pam_strerror(NULL, pd->pam_status));
> if (pd->pam_status == PAM_AUTHINFO_UNAVAIL || preq->use_cached_auth) {
> if (pd->pam_status == PAM_AUTHINFO_UNAVAIL) {
>     switch(pd->cmd) {
>     case SSS_PAM_AUTHENTICATE:
>         if ((preq->domain != NULL) &&
>             (preq->domain->cache_credentials == true) &&
>             (pd->offline_auth == false)) {
>             const char *password = NULL;
>            bool use_cached_auth;
>            /* backup value of preq->use_cached_auth*/
>            use_cached_auth = preq->use_cached_auth;
>            /* set to false to avoid entering this branch when pam_reply()
>             * is recursively called from pam_handle_cached_login() */
>            preq->use_cached_auth = false;
>
>            /* do auth with offline credentials */
>            pd->offline_auth = true;
>
> @@ -634,7 +641,8 @@ static void pam_reply(struct pam_auth_req *preq)
>                                         pctx->rctx->cdb, false,
>                                         &exp_date, &delay_until);
>            pam_handle_cached_login(preq, ret, exp_date, delay_until);
>            pam_handle_cached_login(preq, ret, exp_date, delay_until,
>                                    use_cached_auth);
>            return;
>        }
>        break;
>
> @@ -802,8 +810,11 @@ done:
>      sss_cmd_done(cctx, preq);
>  }
>
> +static void pam_dom_forwarder(struct pam_auth_req *preq);
>
> static void pam_handle_cached_login(struct pam_auth_req *preq, int ret,
>                                time_t expire_date, time_t delayed_until)
>                                time_t expire_date, time_t delayed_until,
>                                bool use_cached_auth)
>
> {
>      uint32_t resp_type;
>      size_t resp_len;
> @@ -852,6 +863,18 @@ static void pam_handle_cached_login(struct pam_auth_req *preq, int ret,
>                  }
>              }
>              break;
>    case PAM_AUTH_ERR:
>        /* Was this attempt to authenticate from cache? */
>        if (use_cached_auth) {
>            /* Don't try cached authentication again, try online check. */
>            DEBUG(SSSDBG_FUNC_DATA,
>                  "Cached authentication failed for: %s\n",
>                  preq->pd->user);
>            preq->cached_auth_failed = true;
>            pam_dom_forwarder(preq);
>            return;
>        }
>        break;
>    default:
>        DEBUG(SSSDBG_TRACE_LIBS,
>              "cached login returned: %d\n", preq->pd->pam_status);
>
> @@ -866,7 +889,6 @@ static void pam_check_user_dp_callback(uint16_t err_maj, uint32_t err_min,
>                                         const char *err_msg, void *ptr);
>  static int pam_check_user_search(struct pam_auth_req *preq);
>  static int pam_check_user_done(struct pam_auth_req *preq, int ret);
> -static void pam_dom_forwarder(struct pam_auth_req *preq);
>
> /* TODO: we should probably return some sort of cookie that is set in the
> PAM_ENVIRONMENT, so that we can save performing some calls and cache
>
> @@ -1004,7 +1026,6 @@ static int pam_forwarder(struct cli_ctx *cctx, int pam_cmd)
>                cctx->client_euid);
>      }
> pd->cmd = pam_cmd;
>    pd->priv = cctx->priv;
>
> @@ -1420,9 +1441,91 @@ static void pam_check_user_dp_callback(uint16_t err_maj, uint32_t err_min,
>      }
>  }
>
> +static errno_t pam_is_last_online_login_fresh(struct sss_domain_info *domain,
>                                          const char* user,
>                                          struct confdb_ctx *cdb,
>                                          int cached_auth_timeout,
>                                          bool *_result)

I think the code would be simpler if this function simple returned bool.
The failure is reported in the function after all. If you prefer to
logically layer retrieving the value and using it, then
maybe pam_can_user_cache_auth could be simpler and return bool straight
away?
> +{
> errno_t ret;
> bool result;
> uint64_t last_login;
> ret = sysdb_get_last_online_auth_with_curr_token(domain, user,
>                                                 &last_login);
> if (ret != EOK) {
>    DEBUG(SSSDBG_MINOR_FAILURE,
>          "sysdb_get_last_online_auth_with_curr_token failed: %s:[%d]\n",
>          sss_strerror(ret), ret);
>    goto done;
> }
> result = time(NULL) < (last_login + cached_auth_timeout);
> ret = EOK;
>
> +done:
> if (ret == EOK) {
>    *_result = result;
> }
> return ret;
>
> +}
>
> +static bool pam_is_cmd_cachable(int cmd)
> +{
> bool is_cachable;
> switch(cmd) {
> case SSS_PAM_AUTHENTICATE:
>    is_cachable = true;
>    break;
> default:
>    is_cachable = false;
> }
> return is_cachable;
>
> +}

[Jakub Hrozek]

On Wed, Jun 24, 2015 at 10:55:05AM +0200, Jakub Hrozek wrote:
> > +errno_t
> > +sysdb_set_last_online_auth_with_curr_token(struct sss_domain_info *domain,
> >                                       const char *username,
> >                                       uint64_t value);
>
> Since set_last_login() sets the attribute directly, can we make this
> function static?

btw if you'd like to keep the function tested, then moving the
declaration to sysdb_private.h would work as well I guess.

[Pavel Reichl]

On 06/24/2015 11:06 AM, Jakub Hrozek wrote:
> On Wed, Jun 24, 2015 at 10:55:05AM +0200, Jakub Hrozek wrote:
> > > +errno_t
> > > +sysdb_set_last_online_auth_with_curr_token(struct sss_domain_info *domain,
> > >                                       const char *username,
> > >                                       uint64_t value);
> >
> > Since set_last_login() sets the attribute directly, can we make this
> > function static?
>
> btw if you'd like to keep the function tested, then moving the
> declaration to sysdb_private.h would work as well I guess.

IIRC this function is mainly not static because I needed it for testing 
of 'sysdb_get_last_online_auth_with_curr_token()' but I agree that 
moving it to sysdb_private.h is a good idea.

Thanks!
> sssd-devel mailing list
> [e-mail]
> https://lists.fedorahosted.org/mailman/listinfo/sssd-devel

[Pavel Reichl]

On 06/24/2015 10:55 AM, Jakub Hrozek wrote:
> On Tue, Jun 23, 2015 at 10:55:41AM +0200, Pavel Reichl wrote:
> > On 06/23/2015 10:34 AM, Jakub Hrozek wrote:
> > > On Fri, Jun 05, 2015 at 07:01:30PM +0200, Pavel Reichl wrote:
> > > > On 05/20/2015 05:16 PM, Pavel Reichl wrote:
> > > > > Hello,
> > > > >
> > > > > please see first version of these patches. I'm currently working on unit
> > > > > test for the second patch which will be part of the second revision of the
> > > > > patch set.
> > > > >
> > > > > Thanks!
> > > > >
> > > > > sssd-devel mailing list
> > > > > [e-mail]
> > > > > https://lists.fedorahosted.org/mailman/listinfo/sssd-devel
> > > >
> > > > Jakub asked me off list to move resetting of
> > > > SYSDB_LAST_ONLINE_AUTH_WITH_CURRENT_TOKEN attribute from ldap code to pam
> > > > code. Please see updated patch set.
> > > >
> > > > Thanks!
> > >
> > > Can you rebase the patches, please? I'll take a look at them..
> > > _______________________________________________
> > > sssd-devel mailing list
> > > [e-mail]
> > > https://lists.fedorahosted.org/mailman/listinfo/sssd-devel
> >
> > Sure. Rebased patches attached.
>
> Hi,
>
> We should extend the PAM responder unit test together with these
> patches so that the new functionality is covered.

Hello,  I think I addressed all your concerns except this one. Feel free 
to continue reviewing updated patches or delay reviewing untill I add 
the unit test.

Thanks!

[snip]

[Jakub Hrozek]

On Wed, Jun 24, 2015 at 05:34:06PM +0200, Pavel Reichl wrote:
> On 06/24/2015 10:55 AM, Jakub Hrozek wrote:
> > On Tue, Jun 23, 2015 at 10:55:41AM +0200, Pavel Reichl wrote:
> > > On 06/23/2015 10:34 AM, Jakub Hrozek wrote:
> > > > On Fri, Jun 05, 2015 at 07:01:30PM +0200, Pavel Reichl wrote:
> > > > > On 05/20/2015 05:16 PM, Pavel Reichl wrote:
> > > > > > Hello,
> > > > > >
> > > > > > please see first version of these patches. I'm currently working on unit
> > > > > > test for the second patch which will be part of the second revision of the
> > > > > > patch set.
> > > > > >
> > > > > > Thanks!
> > > > > >
> > > > > > sssd-devel mailing list
> > > > > > [e-mail]
> > > > > > https://lists.fedorahosted.org/mailman/listinfo/sssd-devel
> > > > >
> > > > > Jakub asked me off list to move resetting of
> > > > > SYSDB_LAST_ONLINE_AUTH_WITH_CURRENT_TOKEN attribute from ldap code to pam
> > > > > code. Please see updated patch set.
> > > > >
> > > > > Thanks!
> > > >
> > > > Can you rebase the patches, please? I'll take a look at them..
> > > > _______________________________________________
> > > > sssd-devel mailing list
> > > > [e-mail]
> > > > https://lists.fedorahosted.org/mailman/listinfo/sssd-devel
> > >
> > > Sure. Rebased patches attached.
> >
> > Hi,
> >
> > We should extend the PAM responder unit test together with these
> > patches so that the new functionality is covered.
>
> Hello,  I think I addressed all your concerns except this one. Feel free to
> continue reviewing updated patches or delay reviewing untill I add the unit
> test.
>
> Thanks!

The patches now work fine for me. I have two more comments:

1) We should add a note to the documentation and maybe even a
    MINOR_FAILURE message if the cached_auth_timeout is longer than
    pam_id_timeout. I think the expectation by the reporter is that the
    back end would not be called at all, not even for initgroups, so we
    should inform the admin that even though auth won't run, initgroups
    might unless they fine-tune pam_id_timeout as well.

2) cached_auth_timeout is described in the PAM section of the man
    page but only works in the domain section. Did we design the feature
    like this or did we want cached_auth_timeout in the [pam] section to
    affect all domains?

[Pavel Reichl]

On 06/25/2015 01:56 PM, Jakub Hrozek wrote:
> On Wed, Jun 24, 2015 at 05:34:06PM +0200, Pavel Reichl wrote:
> > On 06/24/2015 10:55 AM, Jakub Hrozek wrote:
> > > On Tue, Jun 23, 2015 at 10:55:41AM +0200, Pavel Reichl wrote:
> > > > On 06/23/2015 10:34 AM, Jakub Hrozek wrote:
> > > > > On Fri, Jun 05, 2015 at 07:01:30PM +0200, Pavel Reichl wrote:
> > > > > > On 05/20/2015 05:16 PM, Pavel Reichl wrote:
> > > > > > > Hello,
> > > > > > >
> > > > > > > please see first version of these patches. I'm currently working on unit
> > > > > > > test for the second patch which will be part of the second revision of the
> > > > > > > patch set.
> > > > > > >
> > > > > > > Thanks!
> > > > > > >
> > > > > > >
> > > > > > > _______________________________________________
> > > > > > > sssd-devel mailing list
> > > > > > > [e-mail]
> > > > > > > https://lists.fedorahosted.org/mailman/listinfo/sssd-devel
> > > > > > Jakub asked me off list to move resetting of
> > > > > > SYSDB_LAST_ONLINE_AUTH_WITH_CURRENT_TOKEN attribute from ldap code to pam
> > > > > > code. Please see updated patch set.
> > > > > >
> > > > > > Thanks!
> > > > >
> > > > > Can you rebase the patches, please? I'll take a look at them..
> > > > > _______________________________________________
> > > > > sssd-devel mailing list
> > > > > [e-mail]
> > > > > https://lists.fedorahosted.org/mailman/listinfo/sssd-devel
> > > >
> > > > Sure. Rebased patches attached.
> > >
> > > Hi,
> > >
> > > We should extend the PAM responder unit test together with these
> > > patches so that the new functionality is covered.
> >
> > Hello,  I think I addressed all your concerns except this one. Feel free to
> > continue reviewing updated patches or delay reviewing untill I add the unit
> > test.
> >
> > Thanks!
>
> The patches now work fine for me. I have two more comments:
>  1) We should add a note to the documentation and maybe even a
>  MINOR_FAILURE message if the cached_auth_timeout is longer than
>  pam_id_timeout. I think the expectation by the reporter is that the
>  back end would not be called at all, not even for initgroups, so we
>  should inform the admin that even though auth won't run, initgroups
>  might unless they fine-tune pam_id_timeout as well.

OK, will do. I didn't know about this option.
>  2) cached_auth_timeout is described in the PAM section of the man
>  page but only works in the domain section. Did we design the feature
>  like this or did we want cached_auth_timeout in the [pam] section to
>  affect all domains?

Definitely wrong section in man page. I believe that cached_auth_timeout 
must be set per domain (as is implemented now).

I'll post updated patchset.
> sssd-devel mailing list
> [e-mail]
> https://lists.fedorahosted.org/mailman/listinfo/sssd-devel

[Pavel Reichl]

On 06/25/2015 01:56 PM, Jakub Hrozek wrote:
> On Wed, Jun 24, 2015 at 05:34:06PM +0200, Pavel Reichl wrote:
> > On 06/24/2015 10:55 AM, Jakub Hrozek wrote:
> > > On Tue, Jun 23, 2015 at 10:55:41AM +0200, Pavel Reichl wrote:
> > > > On 06/23/2015 10:34 AM, Jakub Hrozek wrote:
> > > > > On Fri, Jun 05, 2015 at 07:01:30PM +0200, Pavel Reichl wrote:
> > > > > > On 05/20/2015 05:16 PM, Pavel Reichl wrote:
> > > > > > > Hello,
> > > > > > >
> > > > > > > please see first version of these patches. I'm currently working on unit
> > > > > > > test for the second patch which will be part of the second revision of the
> > > > > > > patch set.
> > > > > > >
> > > > > > > Thanks!
> > > > > > >
> > > > > > >
> > > > > > > _______________________________________________
> > > > > > > sssd-devel mailing list
> > > > > > > [e-mail]
> > > > > > > https://lists.fedorahosted.org/mailman/listinfo/sssd-devel
> > > > > > Jakub asked me off list to move resetting of
> > > > > > SYSDB_LAST_ONLINE_AUTH_WITH_CURRENT_TOKEN attribute from ldap code to pam
> > > > > > code. Please see updated patch set.
> > > > > >
> > > > > > Thanks!
> > > > >
> > > > > Can you rebase the patches, please? I'll take a look at them..
> > > > > _______________________________________________
> > > > > sssd-devel mailing list
> > > > > [e-mail]
> > > > > https://lists.fedorahosted.org/mailman/listinfo/sssd-devel
> > > >
> > > > Sure. Rebased patches attached.
> > >
> > > Hi,
> > >
> > > We should extend the PAM responder unit test together with these
> > > patches so that the new functionality is covered.
> >
> > Hello,  I think I addressed all your concerns except this one. Feel free to
> > continue reviewing updated patches or delay reviewing untill I add the unit
> > test.
> >
> > Thanks!
>
> The patches now work fine for me. I have two more comments:
>  1) We should add a note to the documentation and maybe even a
>  MINOR_FAILURE message if the cached_auth_timeout is longer than
>  pam_id_timeout. I think the expectation by the reporter is that the
>  back end would not be called at all, not even for initgroups, so we
>  should inform the admin that even though auth won't run, initgroups
>  might unless they fine-tune pam_id_timeout as well.
>
>  2) cached_auth_timeout is described in the PAM section of the man
>  page but only works in the domain section. Did we design the feature
>  like this or did we want cached_auth_timeout in the [pam] section to
>  affect all domains?
>
> sssd-devel mailing list
> [e-mail]
> https://lists.fedorahosted.org/mailman/listinfo/sssd-devel

Please see updated patchset. I created a ticket to not forget to add 
unit tests (https://fedorahosted.org/sssd/ticket/2697).

[Lukas Slebodnik]

On (18/06/15 10:02), Pavel Reichl wrote:
> On 06/05/2015 07:45 PM, Lukas Slebodnik wrote:
> > > +errno_t
> > > +sysdb_set_last_online_auth_with_curr_token(struct sss_domain_info *domain,
> > >                                       const char *username,
> > >                                       uint64_t value);
> > >
> > > +errno_t
> > > +sysdb_null_last_online_auth_with_curr_token(struct sss_domain_info *domain,
> > >                                        const char *username);
> > >
> > > +errno_t
> > > +sysdb_get_last_online_auth_with_curr_token(struct sss_domain_info *domain,
> > >                                       const char *name,
> > >                                       uint64_t *_value);
> >
> > Should we also create following sydb functions?
> >
> > sysdb_set_last_online_auth
> > sysdb_null_last_online_auth
> > sysdb_get_last_online_auth
> > sysdb_set_last_failed_login
> > sysdb_null_last_failed_login
> > sysdb_get_last_failed_login
> > sysdb_set_data_expire_timestamp
> > sysdb_null_data_expire_timestamp
> > sysdb_get_data_expire_timestamp
> > sysdb_set_initgroups_expire_timestamp
> > sysdb_null_initgroups_expire_timestamp
> > sysdb_get_initgroups_expire_timestamp
> > ...
> >
> > LS
>
> Please do whatever you find to be the best for the code.
>
> If you have comments to the patch please be more specific and state what and
> why you don't like so I don't have to guess and waste neither my time nor
> yours. Thanks!

So maybe I just had higher expectation but I do not have a problem to
elaborate. Your added new functions evoke object oriented paradigm(OOP).
It's called encapsulation in OOP. You hide internal data types and write
getters/setters (properties in some languages) to hide internal data
representation. The main benefit is to have free hands in future and
change internal representations without changing public API.

But encapsulation is just one part of OOP it should be combined with
composition, inheritance and polymorphism to have real OOP result.

However sssd is written in C and in procedural (imperative) paradigm.
Even though I prefer other paradigm as well. I will not try to
create an abortive hybrid.

It's fine to use "decoupling" and have small functions. I give you
+1 for this. There is also dark side of this change. Functions in
imperative paradigm should be reusable especially public ones.
So there is small difference between OOP and procedural paradigm
(encapsulation vs. reusability).

I consider sysdb as internal library even though it is a part of
libssss_util.so. We have many reusable functions there which are well tested.
If your aim was to add test to the patches then it's fine.
But you should have focused on different part. You should have
write tests for fast offline authentication. There is already cmocka test
for pam_srv. Moreover you have very simplified task. The most of your changes
was on pam responder side which can be easily tested (without mocking
ldap searches).

As I already wrote I like small functions. but public ones should be reusable.
In your case you could use these hepler functions as static directly
in pam responder + write tests for new functionality.
So you would "kill two birds with one stone". The decoupling would be used
and these static function would be indirectly tested as a part of fast
authentication tests.

On (25/06/15 17:05), Pavel Reichl wrote:
> Please see updated patchset. I created a ticket to not forget to add unit
> tests (https://fedorahosted.org/sssd/ticket/2697).
> From fca6a4275a4cfb298641949cd5b8b799d5eddd22 Mon Sep 17 00:00:00 2001
> From: Pavel Reichl 
> Date: Wed, 24 Jun 2015 09:07:55 -0400
> Subject: [PATCH 1/3] sysdb: new attribute lastOnlineAuthWithCurrentToken
>
> Introduce new user attribute lastOnlineAuthWithCurrentToken.
>
> Resolves:
> https://fedorahosted.org/sssd/ticket/1807
>
> src/db/sysdb.h          | 10 ++++++
> src/db/sysdb_ops.c      | 95 +++++++++++++++++++++++++++++++++++++++++++++++++
> src/db/sysdb_private.h  |  5 +++
> src/tests/sysdb-tests.c | 76 +++++++++++++++++++++++++++++++++++++++
> 4 files changed, 186 insertions(+)
>
> diff --git a/src/db/sysdb.h b/src/db/sysdb.h
> index 4dc382f6fb3f1b6bd084312463edf1b6189ebba9..2319a0cae1bdff65453f69c4a5667fb178556fce 100644
> --- a/src/db/sysdb.h
> +++ b/src/db/sysdb.h
> @@ -87,6 +87,7 @@
> #define SYSDB_LAST_ONLINE_AUTH "lastOnlineAuth"
> #define SYSDB_LAST_FAILED_LOGIN "lastFailedLogin"
> #define SYSDB_FAILED_LOGIN_ATTEMPTS "failedLoginAttempts"
> +#define SYSDB_LAST_ONLINE_AUTH_WITH_CURR_TOKEN "lastOnlineAuthWithCurrentToken"
>
> #define SYSDB_LAST_UPDATE "lastUpdate"
> #define SYSDB_CACHE_EXPIRE "dataExpireTimestamp"
> @@ -904,6 +905,15 @@ int sysdb_cache_password_ex(struct sss_domain_info *domain,
>                             enum sss_authtok_type authtok_type,
>                             size_t second_factor_size);
>
> +errno_t
> +sysdb_null_last_online_auth_with_curr_token(struct sss_domain_info *domain,
>                                        const char *username);
>
> +errno_t
> +sysdb_get_last_online_auth_with_curr_token(struct sss_domain_info *domain,
>                                       const char *name,
>                                       uint64_t *_value);
>
> errno_t check_failed_login_attempts(struct confdb_ctx *cdb,
>                                     struct ldb_message *ldb_msg,
>                                     uint32_t *failed_login_attempts,

As you can see you defined setters and getters and in the same time
you defined public constant SYSDB_LAST_ONLINE_AUTH_WITH_CURR_TOKEN.
It's sort of equivalent to creating getter and setter for public variable
in object.

I'm not aggainst getters and setters but there should be a RFC + design page
and broad discussion for removing all "SYSDB_*" constants and replace them with
functions. But this version of patch is unacceptable.

LS

[Jakub Hrozek]

On Fri, Jun 26, 2015 at 11:00:06AM +0200, Lukas Slebodnik wrote:
> On (25/06/15 17:05), Pavel Reichl wrote:
> > Please see updated patchset. I created a ticket to not forget to add unit
> > tests (https://fedorahosted.org/sssd/ticket/2697).
> > From fca6a4275a4cfb298641949cd5b8b799d5eddd22 Mon Sep 17 00:00:00 2001
> > From: Pavel Reichl 
> > Date: Wed, 24 Jun 2015 09:07:55 -0400
> > Subject: [PATCH 1/3] sysdb: new attribute lastOnlineAuthWithCurrentToken
> >
> > Introduce new user attribute lastOnlineAuthWithCurrentToken.
> >
> > Resolves:
> > https://fedorahosted.org/sssd/ticket/1807
> >
> > src/db/sysdb.h          | 10 ++++++
> > src/db/sysdb_ops.c      | 95 +++++++++++++++++++++++++++++++++++++++++++++++++
> > src/db/sysdb_private.h  |  5 +++
> > src/tests/sysdb-tests.c | 76 +++++++++++++++++++++++++++++++++++++++
> > 4 files changed, 186 insertions(+)
> >
> > diff --git a/src/db/sysdb.h b/src/db/sysdb.h
> > index 4dc382f6fb3f1b6bd084312463edf1b6189ebba9..2319a0cae1bdff65453f69c4a5667fb178556fce 100644
> > --- a/src/db/sysdb.h
> > +++ b/src/db/sysdb.h
> > @@ -87,6 +87,7 @@
> > #define SYSDB_LAST_ONLINE_AUTH "lastOnlineAuth"
> > #define SYSDB_LAST_FAILED_LOGIN "lastFailedLogin"
> > #define SYSDB_FAILED_LOGIN_ATTEMPTS "failedLoginAttempts"
> > +#define SYSDB_LAST_ONLINE_AUTH_WITH_CURR_TOKEN "lastOnlineAuthWithCurrentToken"
> >
> > #define SYSDB_LAST_UPDATE "lastUpdate"
> > #define SYSDB_CACHE_EXPIRE "dataExpireTimestamp"
> > @@ -904,6 +905,15 @@ int sysdb_cache_password_ex(struct sss_domain_info *domain,
> >                             enum sss_authtok_type authtok_type,
> >                             size_t second_factor_size);
> >
> > +errno_t
> > +sysdb_null_last_online_auth_with_curr_token(struct sss_domain_info *domain,
> >                                        const char *username);
> >
> > +errno_t
> > +sysdb_get_last_online_auth_with_curr_token(struct sss_domain_info *domain,
> >                                       const char *name,
> >                                       uint64_t *_value);
> >
> > errno_t check_failed_login_attempts(struct confdb_ctx *cdb,
> >                                     struct ldb_message *ldb_msg,
> >                                     uint32_t *failed_login_attempts,
>
> As you can see you defined setters and getters and in the same time
> you defined public constant SYSDB_LAST_ONLINE_AUTH_WITH_CURR_TOKEN.
> It's sort of equivalent to creating getter and setter for public variable
> in object.

Well, then let's move these functions to pam_helpers.c along with the
tests for now.
> I'm not aggainst getters and setters but there should be a RFC + design page
> and broad discussion for removing all "SYSDB_*" constants and replace them with
> functions.

Yes, long term it would be better to have a more layered sysdb API,
currently it's just a bunch of functions. Something like:
    sysdb_attrs - create attr, set, del, ...
    sysdb_users - create user, delete user, ..
    sysdb_user_attrs - getsetters for attributes

...and responders/providers would only include the sysdb_user_attrs
header.

But I don't think we have the time for this now. In the end, we have
to compromise and do the best to keep the code maintainable with the
limited time.

I think the interfaces between subystems (DP interface, resolver
interface, ...) are much more important to keep lean and clean. There
you can really influence maintainability of the code than with an
attribute setter.
> But this version of patch is unacceptable.

I got a great book from Andreas recently:
    http://shop.oreilly.com/product/0636920018025.do
I already finished it, so I'll lend it to you next.

[Lukas Slebodnik]

On (26/06/15 17:31), Pavel Reichl wrote:
>

So it seems you still do not understand what's wrong from desing point of view.
If you do not understand something please ask it before. We would save your
time with preparing patches and time of other developers reviewing unnecessary
round of patches.

I would like to repeat what was not right from design point of view in previous
version. You used totally different approach for functions. They weren't wrong
they just did not fit among other sysdb functions.

Let's imagine pure object oriented library and someone decides to create new
class with just static functions. He wrote this class because he wanted to
emulate procedural paradigm. It wouldn't be a problem if other classes
were written in such way. But mixing them together make a stinky code.

It's the same as let chimpanzee sit in an auditorium. it is not dangerous and
can clap as well but it doesn't fit among other people. It would be fine
if auditorious contained just chimpanzees.

And now why is current version even worse then previous one from design point
of view. You renamed public functions and moved them to tottaly unrelated part
of code. It is an equivalent of get dressed chimpanzee to different clothes and
let it sit in city council. It still does not fit there.
> From 38dc66b619d6838610940f23ef84e0851a003319 Mon Sep 17 00:00:00 2001
> From: Pavel Reichl 
> Date: Wed, 24 Jun 2015 09:07:55 -0400
> Subject: [PATCH 1/3] sysdb: new attribute lastOnlineAuthWithCurrentToken
>
> Introduce new user attribute lastOnlineAuthWithCurrentToken.
>
> Resolves:
> https://fedorahosted.org/sssd/ticket/1807
>
> src/db/sysdb.h                  |  1 +
> src/responder/pam/pam_helpers.c | 98 ++++++++++++++++++++++++++++++++++++++++-
> src/responder/pam/pam_helpers.h | 15 +++++++
> src/tests/cmocka/test_pam_srv.c | 37 ++++++++++++++++
> 4 files changed, 150 insertions(+), 1 deletion(-)
>
> diff --git a/src/db/sysdb.h b/src/db/sysdb.h
> index 4dc382f6fb3f1b6bd084312463edf1b6189ebba9..f309c9e0c542f8b9c02e48f74a9cefdb1e0a1752 100644
> --- a/src/db/sysdb.h
> +++ b/src/db/sysdb.h
> @@ -87,6 +87,7 @@
> #define SYSDB_LAST_ONLINE_AUTH "lastOnlineAuth"
> #define SYSDB_LAST_FAILED_LOGIN "lastFailedLogin"
> #define SYSDB_FAILED_LOGIN_ATTEMPTS "failedLoginAttempts"
> +#define SYSDB_LAST_ONLINE_AUTH_WITH_CURR_TOKEN "lastOnlineAuthWithCurrentToken"
>
> #define SYSDB_LAST_UPDATE "lastUpdate"
> #define SYSDB_CACHE_EXPIRE "dataExpireTimestamp"
> diff --git a/src/responder/pam/pam_helpers.h b/src/responder/pam/pam_helpers.h
> index 1e1d914dfe3cd358972cc1ba6deb683a49b68235..b23713c2b51185ca11369b17f1ca30e295d1cb21 100644
> --- a/src/responder/pam/pam_helpers.h
> +++ b/src/responder/pam/pam_helpers.h
> @@ -23,6 +23,8 @@
> #ifndef PAM_HELPERS_H_
> #define PAM_HELPERS_H_
>
> +#include "util/util.h"
>
> errno_t pam_initgr_cache_set(struct tevent_context *ev,
>                              hash_table_t *id_table,
>                              char *name,
> @@ -35,4 +37,17 @@ errno_t pam_initgr_cache_set(struct tevent_context *ev,
> errno_t pam_initgr_check_timeout(hash_table_t *id_table,
>                               char *name);
>
> +errno_t
> +pam_null_last_online_auth_with_curr_token(struct sss_domain_info *domain,
>                                      const char *username);
>
> +errno_t
> +pam_get_last_online_auth_with_curr_token(struct sss_domain_info *domain,
>                                     const char *name,
>                                     uint64_t *_value);
>
> +errno_t
> +pam_set_last_online_auth_with_curr_token(struct sss_domain_info *domain,
>                                     const char *username,
>                                     uint64_t value);
>
> #endif /* PAM_HELPERS_H_ */

Here are three proposal which you can use.
a) create reusable public functions in sysdb and not a single purpose
   functions. Reusable public functions would fit to current sysdb API.
b) create internal helper functions in pam_responder. They should be internal
   (static) and they purpose should be to improve readalibity of code; avoid
   long functions. If you want them in separate module then create a new file
   for fast pam authentication with public pam functions.
   Helper functions (wrappers) must be internal in this module. So compiler can
   inline them and readability is not decreased.
   If you warry about code coverage of static functions then you needn't.
   They already use tested public functions from sysdb and it's recommended to
   write tests for new functionality. So they will be indirectly tested.
c) Relace all SYSDB_* constants with setters and getters. So internal sysdb
   library will be consistent.
Here is a list. (I hope I didn't forget anything)
    SYSDB_SUBDOMAIN_CLASS, SYSDB_USER_CLASS, SYSDB_GROUP_CLASS,
    SYSDB_NETGROUP_CLASS, SYSDB_HOST_CLASS, SYSDB_HOSTGROUP_CLASS,
    SYSDB_SELINUX_USERMAP_CLASS, SYSDB_SELINUX_CLASS, SYSDB_ID_RANGE_CLASS,
    SYSDB_DOMAIN_ID_RANGE_CLASS, SYSDB_TRUSTED_AD_DOMAIN_RANGE_CLASS,
    SYSDB_NAME, SYSDB_NAME_ALIAS, SYSDB_OBJECTCLASS, SYSDB_NEXTID,
    SYSDB_UIDNUM, SYSDB_GIDNUM, SYSDB_CREATE_TIME, SYSDB_PWD, SYSDB_FULLNAME,
    SYSDB_HOMEDIR, SYSDB_SHELL, SYSDB_MEMBEROF, SYSDB_DISABLED, SYSDB_MEMBER,
    SYSDB_MEMBERUID, SYSDB_GHOST, SYSDB_POSIX, SYSDB_USER_CATEGORY,
    SYSDB_HOST_CATEGORY, SYSDB_GROUP_TYPE, SYSDB_GECOS, SYSDB_LAST_LOGIN,
    SYSDB_LAST_ONLINE_AUTH, SYSDB_LAST_FAILED_LOGIN,
    SYSDB_FAILED_LOGIN_ATTEMPTS, SYSDB_LAST_ONLINE_AUTH_WITH_CURR_TOKEN,
    SYSDB_LAST_UPDATE, SYSDB_CACHE_EXPIRE, SYSDB_INITGR_EXPIRE,
    SYSDB_IFP_CACHED, SYSDB_AUTHORIZED_SERVICE, SYSDB_AUTHORIZED_HOST,
    SYSDB_NETGROUP_TRIPLE, SYSDB_ORIG_NETGROUP_MEMBER,
    SYSDB_ORIG_NETGROUP_EXTERNAL_HOST, SYSDB_NETGROUP_DOMAIN,
    SYSDB_NETGROUP_MEMBER, SYSDB_DESCRIPTION  , SYSDB_FQDN,
    SYSDB_SERVERHOSTNAME, SYSDB_CACHEDPWD, SYSDB_CACHEDPWD_TYPE,
    SYSDB_CACHEDPWD_FA2_LEN, SYSDB_UUID, SYSDB_SID, SYSDB_PRIMARY_GROUP,
    SYSDB_PRIMARY_GROUP_GIDNUM, SYSDB_SID_STR, SYSDB_UPN, SYSDB_CANONICAL_UPN,
    SYSDB_CCACHE_FILE, SYSDB_ORIG_DN, SYSDB_ORIG_MODSTAMP, SYSDB_ORIG_MEMBEROF,
    SYSDB_ORIG_MEMBER, SYSDB_ORIG_MEMBER_USER, SYSDB_ORIG_MEMBER_HOST,
    SYSDB_USN, SYSDB_HIGH_USN, SYSDB_SSH_PUBKEY, SYSDB_AUTH_TYPE,
    SYSDB_USER_CERT, SYSDB_SUBDOMAIN_REALM, SYSDB_SUBDOMAIN_FLAT,
    SYSDB_SUBDOMAIN_ID, SYSDB_SUBDOMAIN_MPG, SYSDB_SUBDOMAIN_ENUM,
    SYSDB_SUBDOMAIN_FOREST, SYSDB_SUBDOMAIN_TRUST_DIRECTION, SYSDB_BASE_ID,
    SYSDB_ID_RANGE_SIZE, SYSDB_BASE_RID, SYSDB_SECONDARY_BASE_RID,
    SYSDB_DOMAIN_ID, SYSDB_ID_RANGE_TYPE, SYSDB_DEFAULT_OVERRIDE_NAME,
    SYSDB_AD_ACCOUNT_EXPIRES, SYSDB_AD_USER_ACCOUNT_CONTROL, SYSDB_VIEW_CLASS,
    SYSDB_VIEW_NAME, SYSDB_DEFAULT_VIEW_NAME, SYSDB_OVERRIDE_CLASS,
    SYSDB_OVERRIDE_ANCHOR_UUID, SYSDB_OVERRIDE_USER_CLASS,
    SYSDB_OVERRIDE_GROUP_CLASS, SYSDB_OVERRIDE_DN, SYSDB_OVERRIDE_OBJECT_DN,
    SYSDB_HAS_ENUMERATED, SYSDB_IDMAP_SUBTREE, SYSDB_IDMAP_MAPPING_OC,
    SYSDB_IDMAP_SID_ATTR, SYSDB_IDMAP_SLICE_ATTR, SYSDB_GPO_OC,
    SYSDB_GPO_GUID_ATTR, SYSDB_GPO_VERSION_ATTR, SYSDB_GPO_TIMEOUT_ATTR,
    SYSDB_GPO_RESULT_OC

LS

[Pavel Reichl]

On 06/29/2015 11:56 AM, Lukas Slebodnik wrote:
> On (26/06/15 17:31), Pavel Reichl wrote:
> So it seems you still do not understand what's wrong from desing point of view.
> If you do not understand something please ask it before. We would save your
> time with preparing patches and time of other developers reviewing unnecessary
> round of patches.

Sorry, that you feel that way.

I don't agree with essence of your concerns. I don't think I'm forcing 
new paradigm into SSSD. You  can discuss this topic with pbrezina, I'm 
sure he has plenty to say.  I introduced 2 new functions into sysdb 
which operated on higher level than other functions in sysdb. I can 
agree that this was not completely consistent with the rest of sysdb API 
but I don't think I was lowering the quality of code.  However Jakub 
agreed with you and proposed solution.  So I had to respect the majority.

Have I somehow strayed from Jakub's proposal? If it is so, how?

If you don't like Jakub proposal please respond to his post directly.
> I would like to repeat what was not right from design point of view in previous
> version. You used totally different approach for functions. They weren't wrong
> they just did not fit among other sysdb functions.
>
> Let's imagine pure object oriented library and someone decides to create new
> class with just static functions. He wrote this class because he wanted to
> emulate procedural paradigm. It wouldn't be a problem if other classes
> were written in such way. But mixing them together make a stinky code.
>
> It's the same as let chimpanzee sit in an auditorium. it is not dangerous and
> can clap as well but it doesn't fit among other people. It would be fine
> if auditorious contained just chimpanzees.
>
> And now why is current version even worse then previous one from design point
> of view. You renamed public functions and moved them to tottaly unrelated part
> of code. It is an equivalent of get dressed chimpanzee to different clothes and
> let it sit in city council. It still does not fit there.

Lukas, I'm getting old and honestly I don't find this kind of message 
funny but plainly hurtful and it makes me feel really sick. If you want 
to get rid of me just talk to my manager instead of trying to induce me 
a heart attack. Thank you.
> > From 38dc66b619d6838610940f23ef84e0851a003319 Mon Sep 17 00:00:00 2001
> > From: Pavel Reichl 
> > Date: Wed, 24 Jun 2015 09:07:55 -0400
> > Subject: [PATCH 1/3] sysdb: new attribute lastOnlineAuthWithCurrentToken
> >
> > Introduce new user attribute lastOnlineAuthWithCurrentToken.
> >
> > Resolves:
> > https://fedorahosted.org/sssd/ticket/1807
> >
> > src/db/sysdb.h                  |  1 +
> > src/responder/pam/pam_helpers.c | 98 ++++++++++++++++++++++++++++++++++++++++-
> > src/responder/pam/pam_helpers.h | 15 +++++++
> > src/tests/cmocka/test_pam_srv.c | 37 ++++++++++++++++
> > 4 files changed, 150 insertions(+), 1 deletion(-)
> >
> > diff --git a/src/db/sysdb.h b/src/db/sysdb.h
> > index 4dc382f6fb3f1b6bd084312463edf1b6189ebba9..f309c9e0c542f8b9c02e48f74a9cefdb1e0a1752 100644
> > --- a/src/db/sysdb.h
> > +++ b/src/db/sysdb.h
> > @@ -87,6 +87,7 @@
> > #define SYSDB_LAST_ONLINE_AUTH "lastOnlineAuth"
> > #define SYSDB_LAST_FAILED_LOGIN "lastFailedLogin"
> > #define SYSDB_FAILED_LOGIN_ATTEMPTS "failedLoginAttempts"
> > +#define SYSDB_LAST_ONLINE_AUTH_WITH_CURR_TOKEN "lastOnlineAuthWithCurrentToken"
> >
> > #define SYSDB_LAST_UPDATE "lastUpdate"
> > #define SYSDB_CACHE_EXPIRE "dataExpireTimestamp"
> > diff --git a/src/responder/pam/pam_helpers.h b/src/responder/pam/pam_helpers.h
> > index 1e1d914dfe3cd358972cc1ba6deb683a49b68235..b23713c2b51185ca11369b17f1ca30e295d1cb21 100644
> > --- a/src/responder/pam/pam_helpers.h
> > +++ b/src/responder/pam/pam_helpers.h
> > @@ -23,6 +23,8 @@
> > #ifndef PAM_HELPERS_H_
> > #define PAM_HELPERS_H_
> >
> > +#include "util/util.h"
> >
> > errno_t pam_initgr_cache_set(struct tevent_context *ev,
> >                               hash_table_t *id_table,
> >                               char *name,
> > @@ -35,4 +37,17 @@ errno_t pam_initgr_cache_set(struct tevent_context *ev,
> > errno_t pam_initgr_check_timeout(hash_table_t *id_table,
> >                                char *name);
> >
> > +errno_t
> > +pam_null_last_online_auth_with_curr_token(struct sss_domain_info *domain,
> >                                      const char *username);
> >
> > +errno_t
> > +pam_get_last_online_auth_with_curr_token(struct sss_domain_info *domain,
> >                                     const char *name,
> >                                     uint64_t *_value);
> >
> > +errno_t
> > +pam_set_last_online_auth_with_curr_token(struct sss_domain_info *domain,
> >                                     const char *username,
> >                                     uint64_t value);
> >
> > #endif /* PAM_HELPERS_H_ */
>
> Here are three proposal which you can use.
> a) create reusable public functions in sysdb and not a single purpose
>     functions. Reusable public functions would fit to current sysdb API.
> b) create internal helper functions in pam_responder. They should be internal
>     (static) and they purpose should be to improve readalibity of code; avoid
>     long functions. If you want them in separate module then create a new file
>     for fast pam authentication with public pam functions.
>     Helper functions (wrappers) must be internal in this module. So compiler can
>     inline them and readability is not decreased.
>     If you warry about code coverage of static functions then you needn't.
>     They already use tested public functions from sysdb and it's recommended to
>     write tests for new functionality. So they will be indirectly tested.
> c) Relace all SYSDB_* constants with setters and getters. So internal sysdb
>     library will be consistent.
> Here is a list. (I hope I didn't forget anything)
>      SYSDB_SUBDOMAIN_CLASS, SYSDB_USER_CLASS, SYSDB_GROUP_CLASS,
>      SYSDB_NETGROUP_CLASS, SYSDB_HOST_CLASS, SYSDB_HOSTGROUP_CLASS,
>      SYSDB_SELINUX_USERMAP_CLASS, SYSDB_SELINUX_CLASS, SYSDB_ID_RANGE_CLASS,
>      SYSDB_DOMAIN_ID_RANGE_CLASS, SYSDB_TRUSTED_AD_DOMAIN_RANGE_CLASS,
>      SYSDB_NAME, SYSDB_NAME_ALIAS, SYSDB_OBJECTCLASS, SYSDB_NEXTID,
>      SYSDB_UIDNUM, SYSDB_GIDNUM, SYSDB_CREATE_TIME, SYSDB_PWD, SYSDB_FULLNAME,
>      SYSDB_HOMEDIR, SYSDB_SHELL, SYSDB_MEMBEROF, SYSDB_DISABLED, SYSDB_MEMBER,
>      SYSDB_MEMBERUID, SYSDB_GHOST, SYSDB_POSIX, SYSDB_USER_CATEGORY,
>      SYSDB_HOST_CATEGORY, SYSDB_GROUP_TYPE, SYSDB_GECOS, SYSDB_LAST_LOGIN,
>      SYSDB_LAST_ONLINE_AUTH, SYSDB_LAST_FAILED_LOGIN,
>      SYSDB_FAILED_LOGIN_ATTEMPTS, SYSDB_LAST_ONLINE_AUTH_WITH_CURR_TOKEN,
>      SYSDB_LAST_UPDATE, SYSDB_CACHE_EXPIRE, SYSDB_INITGR_EXPIRE,
>      SYSDB_IFP_CACHED, SYSDB_AUTHORIZED_SERVICE, SYSDB_AUTHORIZED_HOST,
>      SYSDB_NETGROUP_TRIPLE, SYSDB_ORIG_NETGROUP_MEMBER,
>      SYSDB_ORIG_NETGROUP_EXTERNAL_HOST, SYSDB_NETGROUP_DOMAIN,
>      SYSDB_NETGROUP_MEMBER, SYSDB_DESCRIPTION  , SYSDB_FQDN,
>      SYSDB_SERVERHOSTNAME, SYSDB_CACHEDPWD, SYSDB_CACHEDPWD_TYPE,
>      SYSDB_CACHEDPWD_FA2_LEN, SYSDB_UUID, SYSDB_SID, SYSDB_PRIMARY_GROUP,
>      SYSDB_PRIMARY_GROUP_GIDNUM, SYSDB_SID_STR, SYSDB_UPN, SYSDB_CANONICAL_UPN,
>      SYSDB_CCACHE_FILE, SYSDB_ORIG_DN, SYSDB_ORIG_MODSTAMP, SYSDB_ORIG_MEMBEROF,
>      SYSDB_ORIG_MEMBER, SYSDB_ORIG_MEMBER_USER, SYSDB_ORIG_MEMBER_HOST,
>      SYSDB_USN, SYSDB_HIGH_USN, SYSDB_SSH_PUBKEY, SYSDB_AUTH_TYPE,
>      SYSDB_USER_CERT, SYSDB_SUBDOMAIN_REALM, SYSDB_SUBDOMAIN_FLAT,
>      SYSDB_SUBDOMAIN_ID, SYSDB_SUBDOMAIN_MPG, SYSDB_SUBDOMAIN_ENUM,
>      SYSDB_SUBDOMAIN_FOREST, SYSDB_SUBDOMAIN_TRUST_DIRECTION, SYSDB_BASE_ID,
>      SYSDB_ID_RANGE_SIZE, SYSDB_BASE_RID, SYSDB_SECONDARY_BASE_RID,
>      SYSDB_DOMAIN_ID, SYSDB_ID_RANGE_TYPE, SYSDB_DEFAULT_OVERRIDE_NAME,
>      SYSDB_AD_ACCOUNT_EXPIRES, SYSDB_AD_USER_ACCOUNT_CONTROL, SYSDB_VIEW_CLASS,
>      SYSDB_VIEW_NAME, SYSDB_DEFAULT_VIEW_NAME, SYSDB_OVERRIDE_CLASS,
>      SYSDB_OVERRIDE_ANCHOR_UUID, SYSDB_OVERRIDE_USER_CLASS,
>      SYSDB_OVERRIDE_GROUP_CLASS, SYSDB_OVERRIDE_DN, SYSDB_OVERRIDE_OBJECT_DN,
>      SYSDB_HAS_ENUMERATED, SYSDB_IDMAP_SUBTREE, SYSDB_IDMAP_MAPPING_OC,
>      SYSDB_IDMAP_SID_ATTR, SYSDB_IDMAP_SLICE_ATTR, SYSDB_GPO_OC,
>      SYSDB_GPO_GUID_ATTR, SYSDB_GPO_VERSION_ATTR, SYSDB_GPO_TIMEOUT_ATTR,
>      SYSDB_GPO_RESULT_OC
>
> LS
> _______________________________________________
> sssd-devel mailing list
> [e-mail]
> https://lists.fedorahosted.org/mailman/listinfo/sssd-devel

[Lukas Slebodnik]

On (29/06/15 14:17), Pavel Reichl wrote:
> On 06/29/2015 11:56 AM, Lukas Slebodnik wrote:
> > On (26/06/15 17:31), Pavel Reichl wrote:
> > So it seems you still do not understand what's wrong from desing point of view.
> > If you do not understand something please ask it before. We would save your
> > time with preparing patches and time of other developers reviewing unnecessary
> > round of patches.
>
> Sorry, that you feel that way.
>
> I don't agree with essence of your concerns. I don't think I'm forcing new
> paradigm into SSSD. You  can discuss this topic with pbrezina, I'm sure he
> has plenty to say.

I'm sorry I do not see any correlation between your patches and pbrezina.
He does not include inconsistencies into sssd. He mostly reduces them.
> introduced 2 new functions into sysdb which operated on
> higher level than other functions in sysdb. I can agree that this was not
> completely consistent with the rest of sysdb API but I don't think I was
> lowering the quality of code.

I didn't say that code has low quality. So please either quote
or do not put words in my mouth.
I wrote many times that it is inconsistent approach. We have many hacks/
inconsistencies in sssd so we needn't introduce a new one.

I didn't rejected getters and setters therefore I proposed to replace all
current functions + public constants with getters and setters.
> However Jakub agreed with you and proposed
> solution.  So I had to respect the majority.
>
> Have I somehow strayed from Jakub's proposal? If it is so, how?
> If you don't like Jakub proposal please respond to his post directly.

I'm sorry but you were faster and sent patches earlier than
I was able to respond to Jakub's mail. I explained what was wrong with
the latest version and described three clean ways from design point of view
in my previous mail. I didn't want to force you to implement the concrete one.
You had a choice and you could implement way which you like the most.
> > I would like to repeat what was not right from design point of view in previous
> > version. You used totally different approach for functions. They weren't wrong
> > they just did not fit among other sysdb functions.
> >
> > Let's imagine pure object oriented library and someone decides to create new
> > class with just static functions. He wrote this class because he wanted to
> > emulate procedural paradigm. It wouldn't be a problem if other classes
> > were written in such way. But mixing them together make a stinky code.
> >
> > It's the same as let chimpanzee sit in an auditorium. it is not dangerous and
> > can clap as well but it doesn't fit among other people. It would be fine
> > if auditorious contained just chimpanzees.
> >
> > And now why is current version even worse then previous one from design point
> > of view. You renamed public functions and moved them to tottaly unrelated part
> > of code. It is an equivalent of get dressed chimpanzee to different clothes and
> > let it sit in city council. It still does not fit there.
>
> Lukas, I'm getting old and honestly I don't find this kind of message funny
> but plainly hurtful and it makes me feel really sick. If you want to get rid
> of me just talk to my manager instead of trying to induce me a heart attack.
> Thank you.

The communication is two way channel. You wanted me to elaborate. I did.
The long mail was written but it seems it was not clear. So I tried to
write a more figurative mail and I hoped it would be easier to understand.
I encouraged you to ask questions. Unfortunately I miss a response from your
side what was not clear. I just saw a mails with patches.
So I'm really disappointed from lack of communication from your side.

Even this mail contains just complaints and still I do not know what wasn't
clearly explained. Which of the proposals is bad from your POV or what you
don't like or which one you like the most. I'm really opened to discussion
to have the nice and clear design but it's not possible with lack of discussion
from your side. (sending patches is not a real communication)
> > > From 38dc66b619d6838610940f23ef84e0851a003319 Mon Sep 17 00:00:00 2001
> > > From: Pavel Reichl 
> > > Date: Wed, 24 Jun 2015 09:07:55 -0400
> > > Subject: [PATCH 1/3] sysdb: new attribute lastOnlineAuthWithCurrentToken
> > >
> > > Introduce new user attribute lastOnlineAuthWithCurrentToken.
> > >
> > > Resolves:
> > > https://fedorahosted.org/sssd/ticket/1807
> > >
> > > src/db/sysdb.h                  |  1 +
> > > src/responder/pam/pam_helpers.c | 98 ++++++++++++++++++++++++++++++++++++++++-
> > > src/responder/pam/pam_helpers.h | 15 +++++++
> > > src/tests/cmocka/test_pam_srv.c | 37 ++++++++++++++++
> > > 4 files changed, 150 insertions(+), 1 deletion(-)
> > >
> > > diff --git a/src/db/sysdb.h b/src/db/sysdb.h
> > > index 4dc382f6fb3f1b6bd084312463edf1b6189ebba9..f309c9e0c542f8b9c02e48f74a9cefdb1e0a1752 100644
> > > --- a/src/db/sysdb.h
> > > +++ b/src/db/sysdb.h
> > > @@ -87,6 +87,7 @@
> > > #define SYSDB_LAST_ONLINE_AUTH "lastOnlineAuth"
> > > #define SYSDB_LAST_FAILED_LOGIN "lastFailedLogin"
> > > #define SYSDB_FAILED_LOGIN_ATTEMPTS "failedLoginAttempts"
> > > +#define SYSDB_LAST_ONLINE_AUTH_WITH_CURR_TOKEN "lastOnlineAuthWithCurrentToken"
> > >
> > > #define SYSDB_LAST_UPDATE "lastUpdate"
> > > #define SYSDB_CACHE_EXPIRE "dataExpireTimestamp"
> > > diff --git a/src/responder/pam/pam_helpers.h b/src/responder/pam/pam_helpers.h
> > > index 1e1d914dfe3cd358972cc1ba6deb683a49b68235..b23713c2b51185ca11369b17f1ca30e295d1cb21 100644
> > > --- a/src/responder/pam/pam_helpers.h
> > > +++ b/src/responder/pam/pam_helpers.h
> > > @@ -23,6 +23,8 @@
> > > #ifndef PAM_HELPERS_H_
> > > #define PAM_HELPERS_H_
> > >
> > > +#include "util/util.h"
> > >
> > > errno_t pam_initgr_cache_set(struct tevent_context *ev,
> > >                              hash_table_t *id_table,
> > >                              char *name,
> > > @@ -35,4 +37,17 @@ errno_t pam_initgr_cache_set(struct tevent_context *ev,
> > > errno_t pam_initgr_check_timeout(hash_table_t *id_table,
> > >                               char *name);
> > >
> > > +errno_t
> > > +pam_null_last_online_auth_with_curr_token(struct sss_domain_info *domain,
> > >                                      const char *username);
> > >
> > > +errno_t
> > > +pam_get_last_online_auth_with_curr_token(struct sss_domain_info *domain,
> > >                                     const char *name,
> > >                                     uint64_t *_value);
> > >
> > > +errno_t
> > > +pam_set_last_online_auth_with_curr_token(struct sss_domain_info *domain,
> > >                                     const char *username,
> > >                                     uint64_t value);
> > >
> > > #endif /* PAM_HELPERS_H_ */
> >
> > Here are three proposal which you can use.
> > a) create reusable public functions in sysdb and not a single purpose
> >    functions. Reusable public functions would fit to current sysdb API.
> > b) create internal helper functions in pam_responder. They should be internal
> >    (static) and they purpose should be to improve readalibity of code; avoid
> >    long functions. If you want them in separate module then create a new file
> >    for fast pam authentication with public pam functions.
> >    Helper functions (wrappers) must be internal in this module. So compiler can
> >    inline them and readability is not decreased.
> >    If you warry about code coverage of static functions then you needn't.
> >    They already use tested public functions from sysdb and it's recommended to
> >    write tests for new functionality. So they will be indirectly tested.
> > c) Relace all SYSDB_* constants with setters and getters. So internal sysdb
> >    library will be consistent.
> > Here is a list. (I hope I didn't forget anything)
> >     SYSDB_SUBDOMAIN_CLASS, SYSDB_USER_CLASS, SYSDB_GROUP_CLASS,
> >     SYSDB_NETGROUP_CLASS, SYSDB_HOST_CLASS, SYSDB_HOSTGROUP_CLASS,
> >     SYSDB_SELINUX_USERMAP_CLASS, SYSDB_SELINUX_CLASS, SYSDB_ID_RANGE_CLASS,
> >     SYSDB_DOMAIN_ID_RANGE_CLASS, SYSDB_TRUSTED_AD_DOMAIN_RANGE_CLASS,
> >     SYSDB_NAME, SYSDB_NAME_ALIAS, SYSDB_OBJECTCLASS, SYSDB_NEXTID,
> >     SYSDB_UIDNUM, SYSDB_GIDNUM, SYSDB_CREATE_TIME, SYSDB_PWD, SYSDB_FULLNAME,
> >     SYSDB_HOMEDIR, SYSDB_SHELL, SYSDB_MEMBEROF, SYSDB_DISABLED, SYSDB_MEMBER,
> >     SYSDB_MEMBERUID, SYSDB_GHOST, SYSDB_POSIX, SYSDB_USER_CATEGORY,
> >     SYSDB_HOST_CATEGORY, SYSDB_GROUP_TYPE, SYSDB_GECOS, SYSDB_LAST_LOGIN,
> >     SYSDB_LAST_ONLINE_AUTH, SYSDB_LAST_FAILED_LOGIN,
> >     SYSDB_FAILED_LOGIN_ATTEMPTS, SYSDB_LAST_ONLINE_AUTH_WITH_CURR_TOKEN,
> >     SYSDB_LAST_UPDATE, SYSDB_CACHE_EXPIRE, SYSDB_INITGR_EXPIRE,
> >     SYSDB_IFP_CACHED, SYSDB_AUTHORIZED_SERVICE, SYSDB_AUTHORIZED_HOST,
> >     SYSDB_NETGROUP_TRIPLE, SYSDB_ORIG_NETGROUP_MEMBER,
> >     SYSDB_ORIG_NETGROUP_EXTERNAL_HOST, SYSDB_NETGROUP_DOMAIN,
> >     SYSDB_NETGROUP_MEMBER, SYSDB_DESCRIPTION  , SYSDB_FQDN,
> >     SYSDB_SERVERHOSTNAME, SYSDB_CACHEDPWD, SYSDB_CACHEDPWD_TYPE,
> >     SYSDB_CACHEDPWD_FA2_LEN, SYSDB_UUID, SYSDB_SID, SYSDB_PRIMARY_GROUP,
> >     SYSDB_PRIMARY_GROUP_GIDNUM, SYSDB_SID_STR, SYSDB_UPN, SYSDB_CANONICAL_UPN,
> >     SYSDB_CCACHE_FILE, SYSDB_ORIG_DN, SYSDB_ORIG_MODSTAMP, SYSDB_ORIG_MEMBEROF,
> >     SYSDB_ORIG_MEMBER, SYSDB_ORIG_MEMBER_USER, SYSDB_ORIG_MEMBER_HOST,
> >     SYSDB_USN, SYSDB_HIGH_USN, SYSDB_SSH_PUBKEY, SYSDB_AUTH_TYPE,
> >     SYSDB_USER_CERT, SYSDB_SUBDOMAIN_REALM, SYSDB_SUBDOMAIN_FLAT,
> >     SYSDB_SUBDOMAIN_ID, SYSDB_SUBDOMAIN_MPG, SYSDB_SUBDOMAIN_ENUM,
> >     SYSDB_SUBDOMAIN_FOREST, SYSDB_SUBDOMAIN_TRUST_DIRECTION, SYSDB_BASE_ID,
> >     SYSDB_ID_RANGE_SIZE, SYSDB_BASE_RID, SYSDB_SECONDARY_BASE_RID,
> >     SYSDB_DOMAIN_ID, SYSDB_ID_RANGE_TYPE, SYSDB_DEFAULT_OVERRIDE_NAME,
> >     SYSDB_AD_ACCOUNT_EXPIRES, SYSDB_AD_USER_ACCOUNT_CONTROL, SYSDB_VIEW_CLASS,
> >     SYSDB_VIEW_NAME, SYSDB_DEFAULT_VIEW_NAME, SYSDB_OVERRIDE_CLASS,
> >     SYSDB_OVERRIDE_ANCHOR_UUID, SYSDB_OVERRIDE_USER_CLASS,
> >     SYSDB_OVERRIDE_GROUP_CLASS, SYSDB_OVERRIDE_DN, SYSDB_OVERRIDE_OBJECT_DN,
> >     SYSDB_HAS_ENUMERATED, SYSDB_IDMAP_SUBTREE, SYSDB_IDMAP_MAPPING_OC,
> >     SYSDB_IDMAP_SID_ATTR, SYSDB_IDMAP_SLICE_ATTR, SYSDB_GPO_OC,
> >     SYSDB_GPO_GUID_ATTR, SYSDB_GPO_VERSION_ATTR, SYSDB_GPO_TIMEOUT_ATTR,
> >     SYSDB_GPO_RESULT_OC

Please be constructive and choose one of these clean proposals from design
point of view. Or propose the better one.

LS

[Pavel Reichl]

On 06/29/2015 02:56 PM, Lukas Slebodnik wrote:
> On (29/06/15 14:17), Pavel Reichl wrote:
> > On 06/29/2015 11:56 AM, Lukas Slebodnik wrote:
> > > On (26/06/15 17:31), Pavel Reichl wrote:
> > > So it seems you still do not understand what's wrong from desing point of view.
> > > If you do not understand something please ask it before. We would save your
> > > time with preparing patches and time of other developers reviewing unnecessary
> > > round of patches.
> >
> > Sorry, that you feel that way.
> >
> > I don't agree with essence of your concerns. I don't think I'm forcing new
> > paradigm into SSSD. You  can discuss this topic with pbrezina, I'm sure he
> > has plenty to say.
>
> I'm sorry I do not see any correlation between your patches and pbrezina.
> He does not include inconsistencies into sssd. He mostly reduces them.

What I had in mind was that pbrezina has recently shared with me his 
ideas about development of SSSD and he mainly emphasized his extensive 
usage of OOP principles that he uses while developing SSSD. I suppose 
that he might want to discuss some of your opinions like:
> However sssd is written in C and in procedural (imperative) paradigm.
> Even though I prefer other paradigm as well. I will not try to
> create an abortive hybrid.

Alas he is on PTO now so we have to wait for his input.
> > introduced 2 new functions into sysdb which operated on
> > higher level than other functions in sysdb. I can agree that this was not
> > completely consistent with the rest of sysdb API but I don't think I was
> > lowering the quality of code.
>
> I didn't say that code has low quality. So please either quote
> or do not put words in my mouth.
> I wrote many times that it is inconsistent approach. We have many hacks/
> inconsistencies in sssd so we needn't introduce a new one.

Sorry but in my opinion there are only 2 reasons to NACK a patch:
1) it's not working correctly
2) there are problems with code quality (code inconsistency belongs here 
IMO)
> I didn't rejected getters and setters therefore I proposed to replace all
> current functions + public constants with getters and setters.
> > However Jakub agreed with you and proposed
> > solution.  So I had to respect the majority.
> >
> > Have I somehow strayed from Jakub's proposal? If it is so, how?
> > If you don't like Jakub proposal please respond to his post directly.
>
> I'm sorry but you were faster and sent patches earlier than
> I was able to respond to Jakub's mail. I explained what was wrong with
> the latest version and described three clean ways from design point of view
> in my previous mail. I didn't want to force you to implement the concrete one.
> You had a choice and you could implement way which you like the most.
> > > I would like to repeat what was not right from design point of view in previous
> > > version. You used totally different approach for functions. They weren't wrong
> > > they just did not fit among other sysdb functions.
> > >
> > > Let's imagine pure object oriented library and someone decides to create new
> > > class with just static functions. He wrote this class because he wanted to
> > > emulate procedural paradigm. It wouldn't be a problem if other classes
> > > were written in such way. But mixing them together make a stinky code.
> > >
> > > It's the same as let chimpanzee sit in an auditorium. it is not dangerous and
> > > can clap as well but it doesn't fit among other people. It would be fine
> > > if auditorious contained just chimpanzees.
> > >
> > > And now why is current version even worse then previous one from design point
> > > of view. You renamed public functions and moved them to tottaly unrelated part
> > > of code. It is an equivalent of get dressed chimpanzee to different clothes and
> > > let it sit in city council. It still does not fit there.
> >
> > Lukas, I'm getting old and honestly I don't find this kind of message funny
> > but plainly hurtful and it makes me feel really sick. If you want to get rid
> > of me just talk to my manager instead of trying to induce me a heart attack.
> > Thank you.
>
> The communication is two way channel. You wanted me to elaborate. I did.
> The long mail was written but it seems it was not clear. So I tried to
> write a more figurative mail and I hoped it would be easier to understand.
> I encouraged you to ask questions. Unfortunately I miss a response from your
> side what was not clear. I just saw a mails with patches.
> So I'm really disappointed from lack of communication from your side.
>
> Even this mail contains just complaints and still I do not know what wasn't
> clearly explained. Which of the proposals is bad from your POV or what you
> don't like or which one you like the most. I'm really opened to discussion
> to have the nice and clear design but it's not possible with lack of discussion
> from your side. (sending patches is not a real communication)
> > > > From 38dc66b619d6838610940f23ef84e0851a003319 Mon Sep 17 00:00:00 2001
> > > > From: Pavel Reichl 
> > > > Date: Wed, 24 Jun 2015 09:07:55 -0400
> > > > Subject: [PATCH 1/3] sysdb: new attribute lastOnlineAuthWithCurrentToken
> > > >
> > > > Introduce new user attribute lastOnlineAuthWithCurrentToken.
> > > >
> > > > Resolves:
> > > > https://fedorahosted.org/sssd/ticket/1807
> > > >
> > > > src/db/sysdb.h                  |  1 +
> > > > src/responder/pam/pam_helpers.c | 98 ++++++++++++++++++++++++++++++++++++++++-
> > > > src/responder/pam/pam_helpers.h | 15 +++++++
> > > > src/tests/cmocka/test_pam_srv.c | 37 ++++++++++++++++
> > > > 4 files changed, 150 insertions(+), 1 deletion(-)
> > > >
> > > > diff --git a/src/db/sysdb.h b/src/db/sysdb.h
> > > > index 4dc382f6fb3f1b6bd084312463edf1b6189ebba9..f309c9e0c542f8b9c02e48f74a9cefdb1e0a1752 100644
> > > > --- a/src/db/sysdb.h
> > > > +++ b/src/db/sysdb.h
> > > > @@ -87,6 +87,7 @@
> > > > #define SYSDB_LAST_ONLINE_AUTH "lastOnlineAuth"
> > > > #define SYSDB_LAST_FAILED_LOGIN "lastFailedLogin"
> > > > #define SYSDB_FAILED_LOGIN_ATTEMPTS "failedLoginAttempts"
> > > > +#define SYSDB_LAST_ONLINE_AUTH_WITH_CURR_TOKEN "lastOnlineAuthWithCurrentToken"
> > > >
> > > > #define SYSDB_LAST_UPDATE "lastUpdate"
> > > > #define SYSDB_CACHE_EXPIRE "dataExpireTimestamp"
> > > > diff --git a/src/responder/pam/pam_helpers.h b/src/responder/pam/pam_helpers.h
> > > > index 1e1d914dfe3cd358972cc1ba6deb683a49b68235..b23713c2b51185ca11369b17f1ca30e295d1cb21 100644
> > > > --- a/src/responder/pam/pam_helpers.h
> > > > +++ b/src/responder/pam/pam_helpers.h
> > > > @@ -23,6 +23,8 @@
> > > > #ifndef PAM_HELPERS_H_
> > > > #define PAM_HELPERS_H_
> > > >
> > > > +#include "util/util.h"
> > > >
> > > > errno_t pam_initgr_cache_set(struct tevent_context *ev,
> > > >                               hash_table_t *id_table,
> > > >                               char *name,
> > > > @@ -35,4 +37,17 @@ errno_t pam_initgr_cache_set(struct tevent_context *ev,
> > > > errno_t pam_initgr_check_timeout(hash_table_t *id_table,
> > > >                                char *name);
> > > >
> > > > +errno_t
> > > > +pam_null_last_online_auth_with_curr_token(struct sss_domain_info *domain,
> > > >                                      const char *username);
> > > >
> > > > +errno_t
> > > > +pam_get_last_online_auth_with_curr_token(struct sss_domain_info *domain,
> > > >                                     const char *name,
> > > >                                     uint64_t *_value);
> > > >
> > > > +errno_t
> > > > +pam_set_last_online_auth_with_curr_token(struct sss_domain_info *domain,
> > > >                                     const char *username,
> > > >                                     uint64_t value);
> > > >
> > > > #endif /* PAM_HELPERS_H_ */
> > >
> > > Here are three proposal which you can use.
> > > a) create reusable public functions in sysdb and not a single purpose
> > >     functions. Reusable public functions would fit to current sysdb API.
> > > b) create internal helper functions in pam_responder. They should be internal
> > >     (static) and they purpose should be to improve readalibity of code; avoid
> > >     long functions. If you want them in separate module then create a new file
> > >     for fast pam authentication with public pam functions.
> > >     Helper functions (wrappers) must be internal in this module. So compiler can
> > >     inline them and readability is not decreased.
> > >     If you warry about code coverage of static functions then you needn't.
> > >     They already use tested public functions from sysdb and it's recommended to
> > >     write tests for new functionality. So they will be indirectly tested.
> > > c) Relace all SYSDB_* constants with setters and getters. So internal sysdb
> > >     library will be consistent.
> > > Here is a list. (I hope I didn't forget anything)
> > >      SYSDB_SUBDOMAIN_CLASS, SYSDB_USER_CLASS, SYSDB_GROUP_CLASS,
> > >      SYSDB_NETGROUP_CLASS, SYSDB_HOST_CLASS, SYSDB_HOSTGROUP_CLASS,
> > >      SYSDB_SELINUX_USERMAP_CLASS, SYSDB_SELINUX_CLASS, SYSDB_ID_RANGE_CLASS,
> > >      SYSDB_DOMAIN_ID_RANGE_CLASS, SYSDB_TRUSTED_AD_DOMAIN_RANGE_CLASS,
> > >      SYSDB_NAME, SYSDB_NAME_ALIAS, SYSDB_OBJECTCLASS, SYSDB_NEXTID,
> > >      SYSDB_UIDNUM, SYSDB_GIDNUM, SYSDB_CREATE_TIME, SYSDB_PWD, SYSDB_FULLNAME,
> > >      SYSDB_HOMEDIR, SYSDB_SHELL, SYSDB_MEMBEROF, SYSDB_DISABLED, SYSDB_MEMBER,
> > >      SYSDB_MEMBERUID, SYSDB_GHOST, SYSDB_POSIX, SYSDB_USER_CATEGORY,
> > >      SYSDB_HOST_CATEGORY, SYSDB_GROUP_TYPE, SYSDB_GECOS, SYSDB_LAST_LOGIN,
> > >      SYSDB_LAST_ONLINE_AUTH, SYSDB_LAST_FAILED_LOGIN,
> > >      SYSDB_FAILED_LOGIN_ATTEMPTS, SYSDB_LAST_ONLINE_AUTH_WITH_CURR_TOKEN,
> > >      SYSDB_LAST_UPDATE, SYSDB_CACHE_EXPIRE, SYSDB_INITGR_EXPIRE,
> > >      SYSDB_IFP_CACHED, SYSDB_AUTHORIZED_SERVICE, SYSDB_AUTHORIZED_HOST,
> > >      SYSDB_NETGROUP_TRIPLE, SYSDB_ORIG_NETGROUP_MEMBER,
> > >      SYSDB_ORIG_NETGROUP_EXTERNAL_HOST, SYSDB_NETGROUP_DOMAIN,
> > >      SYSDB_NETGROUP_MEMBER, SYSDB_DESCRIPTION  , SYSDB_FQDN,
> > >      SYSDB_SERVERHOSTNAME, SYSDB_CACHEDPWD, SYSDB_CACHEDPWD_TYPE,
> > >      SYSDB_CACHEDPWD_FA2_LEN, SYSDB_UUID, SYSDB_SID, SYSDB_PRIMARY_GROUP,
> > >      SYSDB_PRIMARY_GROUP_GIDNUM, SYSDB_SID_STR, SYSDB_UPN, SYSDB_CANONICAL_UPN,
> > >      SYSDB_CCACHE_FILE, SYSDB_ORIG_DN, SYSDB_ORIG_MODSTAMP, SYSDB_ORIG_MEMBEROF,
> > >      SYSDB_ORIG_MEMBER, SYSDB_ORIG_MEMBER_USER, SYSDB_ORIG_MEMBER_HOST,
> > >      SYSDB_USN, SYSDB_HIGH_USN, SYSDB_SSH_PUBKEY, SYSDB_AUTH_TYPE,
> > >      SYSDB_USER_CERT, SYSDB_SUBDOMAIN_REALM, SYSDB_SUBDOMAIN_FLAT,
> > >      SYSDB_SUBDOMAIN_ID, SYSDB_SUBDOMAIN_MPG, SYSDB_SUBDOMAIN_ENUM,
> > >      SYSDB_SUBDOMAIN_FOREST, SYSDB_SUBDOMAIN_TRUST_DIRECTION, SYSDB_BASE_ID,
> > >      SYSDB_ID_RANGE_SIZE, SYSDB_BASE_RID, SYSDB_SECONDARY_BASE_RID,
> > >      SYSDB_DOMAIN_ID, SYSDB_ID_RANGE_TYPE, SYSDB_DEFAULT_OVERRIDE_NAME,
> > >      SYSDB_AD_ACCOUNT_EXPIRES, SYSDB_AD_USER_ACCOUNT_CONTROL, SYSDB_VIEW_CLASS,
> > >      SYSDB_VIEW_NAME, SYSDB_DEFAULT_VIEW_NAME, SYSDB_OVERRIDE_CLASS,
> > >      SYSDB_OVERRIDE_ANCHOR_UUID, SYSDB_OVERRIDE_USER_CLASS,
> > >      SYSDB_OVERRIDE_GROUP_CLASS, SYSDB_OVERRIDE_DN, SYSDB_OVERRIDE_OBJECT_DN,
> > >      SYSDB_HAS_ENUMERATED, SYSDB_IDMAP_SUBTREE, SYSDB_IDMAP_MAPPING_OC,
> > >      SYSDB_IDMAP_SID_ATTR, SYSDB_IDMAP_SLICE_ATTR, SYSDB_GPO_OC,
> > >      SYSDB_GPO_GUID_ATTR, SYSDB_GPO_VERSION_ATTR, SYSDB_GPO_TIMEOUT_ATTR,
> > >      SYSDB_GPO_RESULT_OC
> Please be constructive and choose one of these clean proposals from design
> point of view. Or propose the better one.
>
> LS
> _______________________________________________
> sssd-devel mailing list
> [e-mail]
> https://lists.fedorahosted.org/mailman/listinfo/sssd-devel

[Lukas Slebodnik]

On (30/06/15 10:33), Pavel Reichl wrote:
> On 06/29/2015 02:56 PM, Lukas Slebodnik wrote:
> > On (29/06/15 14:17), Pavel Reichl wrote:
> > > On 06/29/2015 11:56 AM, Lukas Slebodnik wrote:
> > > > On (26/06/15 17:31), Pavel Reichl wrote:
> > > > So it seems you still do not understand what's wrong from desing point of view.
> > > > If you do not understand something please ask it before. We would save your
> > > > time with preparing patches and time of other developers reviewing unnecessary
> > > > round of patches.
> > >
> > > Sorry, that you feel that way.
> > >
> > > I don't agree with essence of your concerns. I don't think I'm forcing new
> > > paradigm into SSSD. You  can discuss this topic with pbrezina, I'm sure he
> > > has plenty to say.
> >
> > I'm sorry I do not see any correlation between your patches and pbrezina.
> > He does not include inconsistencies into sssd. He mostly reduces them.
>
> What I had in mind was that pbrezina has recently shared with me his ideas
> about development of SSSD and he mainly emphasized his extensive usage of OOP
> principles that he uses while developing SSSD. I suppose that he might want
> to discuss some of your opinions like:

It's fine if it is implemented in consistent way.
> > However sssd is written in C and in procedural (imperative) paradigm.
> > Even though I prefer other paradigm as well. I will not try to
> > create an abortive hybrid.
>
> Alas he is on PTO now so we have to wait for his input.
> > > introduced 2 new functions into sysdb which operated on
> > > higher level than other functions in sysdb. I can agree that this was not
> > > completely consistent with the rest of sysdb API but I don't think I was
> > > lowering the quality of code.
> >
> > I didn't say that code has low quality. So please either quote
> > or do not put words in my mouth.
> > I wrote many times that it is inconsistent approach. We have many hacks/
> > inconsistencies in sssd so we needn't introduce a new one.
>
> Sorry but in my opinion there are only 2 reasons to NACK a patch:
> it's not working correctly
> there are problems with code quality (code inconsistency belongs here IMO)

Software engineer are used to thinking in binary way. Real word offer more
choices. I know it is your opinion and I cannot influence it.
But there are more reasons why patch should not be accepted.

a) bad design (code may look good and work but design is strange)
b) missing unit tests for critical parts of code
...

LS

[Jakub Hrozek]

On Mon, Jun 29, 2015 at 11:56:01AM +0200, Lukas Slebodnik wrote:
> On (26/06/15 17:31), Pavel Reichl wrote:
> >
>
> So it seems you still do not understand what's wrong from desing point of view.
> If you do not understand something please ask it before. We would save your
> time with preparing patches and time of other developers reviewing unnecessary
> round of patches.
>
> I would like to repeat what was not right from design point of view in previous
> version. You used totally different approach for functions. They weren't wrong
> they just did not fit among other sysdb functions.
>
> Let's imagine pure object oriented library and someone decides to create new
> class with just static functions. He wrote this class because he wanted to
> emulate procedural paradigm. It wouldn't be a problem if other classes
> were written in such way. But mixing them together make a stinky code.
>
> It's the same as let chimpanzee sit in an auditorium. it is not dangerous and
> can clap as well but it doesn't fit among other people. It would be fine
> if auditorious contained just chimpanzees.
>
> And now why is current version even worse then previous one from design point
> of view. You renamed public functions and moved them to tottaly unrelated part
> of code. It is an equivalent of get dressed chimpanzee to different clothes and
> let it sit in city council. It still does not fit there.
> > From 38dc66b619d6838610940f23ef84e0851a003319 Mon Sep 17 00:00:00 2001
> > From: Pavel Reichl 
> > Date: Wed, 24 Jun 2015 09:07:55 -0400
> > Subject: [PATCH 1/3] sysdb: new attribute lastOnlineAuthWithCurrentToken
> >
> > Introduce new user attribute lastOnlineAuthWithCurrentToken.
> >
> > Resolves:
> > https://fedorahosted.org/sssd/ticket/1807
> >
> > src/db/sysdb.h                  |  1 +
> > src/responder/pam/pam_helpers.c | 98 ++++++++++++++++++++++++++++++++++++++++-
> > src/responder/pam/pam_helpers.h | 15 +++++++
> > src/tests/cmocka/test_pam_srv.c | 37 ++++++++++++++++
> > 4 files changed, 150 insertions(+), 1 deletion(-)
> >
> > diff --git a/src/db/sysdb.h b/src/db/sysdb.h
> > index 4dc382f6fb3f1b6bd084312463edf1b6189ebba9..f309c9e0c542f8b9c02e48f74a9cefdb1e0a1752 100644
> > --- a/src/db/sysdb.h
> > +++ b/src/db/sysdb.h
> > @@ -87,6 +87,7 @@
> > #define SYSDB_LAST_ONLINE_AUTH "lastOnlineAuth"
> > #define SYSDB_LAST_FAILED_LOGIN "lastFailedLogin"
> > #define SYSDB_FAILED_LOGIN_ATTEMPTS "failedLoginAttempts"
> > +#define SYSDB_LAST_ONLINE_AUTH_WITH_CURR_TOKEN "lastOnlineAuthWithCurrentToken"
> >
> > #define SYSDB_LAST_UPDATE "lastUpdate"
> > #define SYSDB_CACHE_EXPIRE "dataExpireTimestamp"
> > diff --git a/src/responder/pam/pam_helpers.h b/src/responder/pam/pam_helpers.h
> > index 1e1d914dfe3cd358972cc1ba6deb683a49b68235..b23713c2b51185ca11369b17f1ca30e295d1cb21 100644
> > --- a/src/responder/pam/pam_helpers.h
> > +++ b/src/responder/pam/pam_helpers.h
> > @@ -23,6 +23,8 @@
> > #ifndef PAM_HELPERS_H_
> > #define PAM_HELPERS_H_
> >
> > +#include "util/util.h"
> >
> > errno_t pam_initgr_cache_set(struct tevent_context *ev,
> >                              hash_table_t *id_table,
> >                              char *name,
> > @@ -35,4 +37,17 @@ errno_t pam_initgr_cache_set(struct tevent_context *ev,
> > errno_t pam_initgr_check_timeout(hash_table_t *id_table,
> >                               char *name);
> >
> > +errno_t
> > +pam_null_last_online_auth_with_curr_token(struct sss_domain_info *domain,
> >                                      const char *username);
> >
> > +errno_t
> > +pam_get_last_online_auth_with_curr_token(struct sss_domain_info *domain,
> >                                     const char *name,
> >                                     uint64_t *_value);
> >
> > +errno_t
> > +pam_set_last_online_auth_with_curr_token(struct sss_domain_info *domain,
> >                                     const char *username,
> >                                     uint64_t value);
> >
> > #endif /* PAM_HELPERS_H_ */
>
> Here are three proposal which you can use.
> a) create reusable public functions in sysdb and not a single purpose
>    functions. Reusable public functions would fit to current sysdb API.
> b) create internal helper functions in pam_responder. They should be internal
>    (static) and they purpose should be to improve readalibity of code; avoid
>    long functions. If you want them in separate module then create a new file
>    for fast pam authentication with public pam functions.

I don't think it's such a big deal at this point because the getsetters
are in the PAM tree and don't pullute the sysdb API. It's just a helper
now.

Yes, I guess even more structured approach would be to have a PAM responder
module that handles the cached authentication, but then the question
is whether this refactoring wouldn't be too invasive.

On the other hand, the tests for the utilities are not too critical, so
if you feel strongly about these three helpers, then making them static
would be less invasive than creating the module, unless there are tests.

I'm totally fine with refactoring code that is well covered with tests,
but I don't think we're there yet with PAM responder.
> Helper functions (wrappers) must be internal in this module. So compiler can
>    inline them and readability is not decreased.
>    If you warry about code coverage of static functions then you needn't.
>    They already use tested public functions from sysdb and it's recommended to
>    write tests for new functionality. So they will be indirectly tested.
> c) Relace all SYSDB_* constants with setters and getters. So internal sysdb
>    library will be consistent.

[Lukas Slebodnik]

On (29/06/15 17:23), Jakub Hrozek wrote:
> On Mon, Jun 29, 2015 at 11:56:01AM +0200, Lukas Slebodnik wrote:
> > On (26/06/15 17:31), Pavel Reichl wrote:
> > >
> >
> > So it seems you still do not understand what's wrong from desing point of view.
> > If you do not understand something please ask it before. We would save your
> > time with preparing patches and time of other developers reviewing unnecessary
> > round of patches.
> >
> > I would like to repeat what was not right from design point of view in previous
> > version. You used totally different approach for functions. They weren't wrong
> > they just did not fit among other sysdb functions.
> >
> > Let's imagine pure object oriented library and someone decides to create new
> > class with just static functions. He wrote this class because he wanted to
> > emulate procedural paradigm. It wouldn't be a problem if other classes
> > were written in such way. But mixing them together make a stinky code.
> >
> > It's the same as let chimpanzee sit in an auditorium. it is not dangerous and
> > can clap as well but it doesn't fit among other people. It would be fine
> > if auditorious contained just chimpanzees.
> >
> > And now why is current version even worse then previous one from design point
> > of view. You renamed public functions and moved them to tottaly unrelated part
> > of code. It is an equivalent of get dressed chimpanzee to different clothes and
> > let it sit in city council. It still does not fit there.
> > > From 38dc66b619d6838610940f23ef84e0851a003319 Mon Sep 17 00:00:00 2001
> > > From: Pavel Reichl 
> > > Date: Wed, 24 Jun 2015 09:07:55 -0400
> > > Subject: [PATCH 1/3] sysdb: new attribute lastOnlineAuthWithCurrentToken
> > >
> > > Introduce new user attribute lastOnlineAuthWithCurrentToken.
> > >
> > > Resolves:
> > > https://fedorahosted.org/sssd/ticket/1807
> > >
> > > src/db/sysdb.h                  |  1 +
> > > src/responder/pam/pam_helpers.c | 98 ++++++++++++++++++++++++++++++++++++++++-
> > > src/responder/pam/pam_helpers.h | 15 +++++++
> > > src/tests/cmocka/test_pam_srv.c | 37 ++++++++++++++++
> > > 4 files changed, 150 insertions(+), 1 deletion(-)
> > >
> > > diff --git a/src/db/sysdb.h b/src/db/sysdb.h
> > > index 4dc382f6fb3f1b6bd084312463edf1b6189ebba9..f309c9e0c542f8b9c02e48f74a9cefdb1e0a1752 100644
> > > --- a/src/db/sysdb.h
> > > +++ b/src/db/sysdb.h
> > > @@ -87,6 +87,7 @@
> > > #define SYSDB_LAST_ONLINE_AUTH "lastOnlineAuth"
> > > #define SYSDB_LAST_FAILED_LOGIN "lastFailedLogin"
> > > #define SYSDB_FAILED_LOGIN_ATTEMPTS "failedLoginAttempts"
> > > +#define SYSDB_LAST_ONLINE_AUTH_WITH_CURR_TOKEN "lastOnlineAuthWithCurrentToken"
> > >
> > > #define SYSDB_LAST_UPDATE "lastUpdate"
> > > #define SYSDB_CACHE_EXPIRE "dataExpireTimestamp"
> > > diff --git a/src/responder/pam/pam_helpers.h b/src/responder/pam/pam_helpers.h
> > > index 1e1d914dfe3cd358972cc1ba6deb683a49b68235..b23713c2b51185ca11369b17f1ca30e295d1cb21 100644
> > > --- a/src/responder/pam/pam_helpers.h
> > > +++ b/src/responder/pam/pam_helpers.h
> > > @@ -23,6 +23,8 @@
> > > #ifndef PAM_HELPERS_H_
> > > #define PAM_HELPERS_H_
> > >
> > > +#include "util/util.h"
> > >
> > > errno_t pam_initgr_cache_set(struct tevent_context *ev,
> > >                              hash_table_t *id_table,
> > >                              char *name,
> > > @@ -35,4 +37,17 @@ errno_t pam_initgr_cache_set(struct tevent_context *ev,
> > > errno_t pam_initgr_check_timeout(hash_table_t *id_table,
> > >                               char *name);
> > >
> > > +errno_t
> > > +pam_null_last_online_auth_with_curr_token(struct sss_domain_info *domain,
> > >                                      const char *username);
> > >
> > > +errno_t
> > > +pam_get_last_online_auth_with_curr_token(struct sss_domain_info *domain,
> > >                                     const char *name,
> > >                                     uint64_t *_value);
> > >
> > > +errno_t
> > > +pam_set_last_online_auth_with_curr_token(struct sss_domain_info *domain,
> > >                                     const char *username,
> > >                                     uint64_t value);
> > >
> > > #endif /* PAM_HELPERS_H_ */
> >
> > Here are three proposal which you can use.
> > a) create reusable public functions in sysdb and not a single purpose
> >    functions. Reusable public functions would fit to current sysdb API.
> > b) create internal helper functions in pam_responder. They should be internal
> >    (static) and they purpose should be to improve readalibity of code; avoid
> >    long functions. If you want them in separate module then create a new file
> >    for fast pam authentication with public pam functions.
>
> I don't think it's such a big deal at this point because the getsetters
> are in the PAM tree and don't pullute the sysdb API. It's just a helper
> now.

Yes, but they are still sysdb public functions but in wrong place
and moreover there is still public constant
SYSDB_LAST_ONLINE_AUTH_WITH_CURR_TOKEN which can be used with other sysdb
functions. So one developer can use old way of manipulating with
this attribute and other can use getter/setter. That's the problem.
It isn't encapsulation. It's inconsistent and thus unacceptable.
> Yes, I guess even more structured approach would be to have a PAM responder
> module that handles the cached authentication, but then the question
> is whether this refactoring wouldn't be too invasive.

Adding new functionality needn't be invasive. In this case it isn't.
Here is a statistic of last two patches. (excluding public sysdb functions)

src/confdb/confdb.c                  |  62 +++++++++++++++++++++++++++++++
 src/confdb/confdb.h                  |   2 ++
 src/config/SSSDConfig/__init__.py.in |   1 +
 src/config/SSSDConfigTest.py         |   6 ++--
 src/config/etc/sssd.api.conf         |   1 +
 src/db/sysdb_ops.c                   |   1 -
 src/man/sssd.conf.5.xml              |  24 +++++++++++++
 src/responder/pam/pamsrv.h           |   3 ++
 src/responder/pam/pamsrv_cmd.c       | 115 +++++++++++++++++++++++++++++++
 9 files changed, 205 insertions(+), 10 deletions(-)
> On the other hand, the tests for the utilities are not too critical, so
> if you feel strongly about these three helpers, then making them static
> would be less invasive than creating the module, unless there are tests.

3 public functions would make sense if they was part of sysdb public API
(point c). But all these functions are single purpose and will never be
used on other places (excluding unit test). They are used JUST on one place.
So they are definitely simple wrappers (helper functions) and the only purpose
is to improve readability. No reason to make them public.
BTW functions pam_set_last_online_auth_with_curr_token
pam_null_last_online_auth_with_curr_token can be merged. We needn't
have such granularity for helper functions.
> I'm totally fine with refactoring code that is well covered with tests,
> but I don't think we're there yet with PAM responder.

Pavel didn't refactor the code. He just added 115 lines to pamsrv_cmd.c.
He added a new functionality and moreover he needn't change anything if
he want to test fast authentication. There is already pam_srv unit test.
It just need to be extended.
> > Helper functions (wrappers) must be internal in this module. So compiler can
> >    inline them and readability is not decreased.
> >    If you warry about code coverage of static functions then you needn't.
> >    They already use tested public functions from sysdb and it's recommended to
> >    write tests for new functionality. So they will be indirectly tested.
> > c) Relace all SYSDB_* constants with setters and getters. So internal sysdb
> >    library will be consistent.

I do not force anyone to implement "B version" there are two other
possibilities which will not introduce inconsistencies.

BTW Jakub you mentioned a week ago that it would be good to have unit tests
for fast authentication. I hope there was enough time to write them
and new version of patches will contain them.

LS

[Jakub Hrozek]

On Tue, Jun 30, 2015 at 09:41:19AM +0200, Lukas Slebodnik wrote:
> On (29/06/15 17:23), Jakub Hrozek wrote:
> > On Mon, Jun 29, 2015 at 11:56:01AM +0200, Lukas Slebodnik wrote:
> > > On (26/06/15 17:31), Pavel Reichl wrote:
> > > >
> > >
> > > So it seems you still do not understand what's wrong from desing point of view.
> > > If you do not understand something please ask it before. We would save your
> > > time with preparing patches and time of other developers reviewing unnecessary
> > > round of patches.
> > >
> > > I would like to repeat what was not right from design point of view in previous
> > > version. You used totally different approach for functions. They weren't wrong
> > > they just did not fit among other sysdb functions.
> > >
> > > Let's imagine pure object oriented library and someone decides to create new
> > > class with just static functions. He wrote this class because he wanted to
> > > emulate procedural paradigm. It wouldn't be a problem if other classes
> > > were written in such way. But mixing them together make a stinky code.
> > >
> > > It's the same as let chimpanzee sit in an auditorium. it is not dangerous and
> > > can clap as well but it doesn't fit among other people. It would be fine
> > > if auditorious contained just chimpanzees.
> > >
> > > And now why is current version even worse then previous one from design point
> > > of view. You renamed public functions and moved them to tottaly unrelated part
> > > of code. It is an equivalent of get dressed chimpanzee to different clothes and
> > > let it sit in city council. It still does not fit there.
> > > > From 38dc66b619d6838610940f23ef84e0851a003319 Mon Sep 17 00:00:00 2001
> > > > From: Pavel Reichl 
> > > > Date: Wed, 24 Jun 2015 09:07:55 -0400
> > > > Subject: [PATCH 1/3] sysdb: new attribute lastOnlineAuthWithCurrentToken
> > > >
> > > > Introduce new user attribute lastOnlineAuthWithCurrentToken.
> > > >
> > > > Resolves:
> > > > https://fedorahosted.org/sssd/ticket/1807
> > > >
> > > > src/db/sysdb.h                  |  1 +
> > > > src/responder/pam/pam_helpers.c | 98 ++++++++++++++++++++++++++++++++++++++++-
> > > > src/responder/pam/pam_helpers.h | 15 +++++++
> > > > src/tests/cmocka/test_pam_srv.c | 37 ++++++++++++++++
> > > > 4 files changed, 150 insertions(+), 1 deletion(-)
> > > >
> > > > diff --git a/src/db/sysdb.h b/src/db/sysdb.h
> > > > index 4dc382f6fb3f1b6bd084312463edf1b6189ebba9..f309c9e0c542f8b9c02e48f74a9cefdb1e0a1752 100644
> > > > --- a/src/db/sysdb.h
> > > > +++ b/src/db/sysdb.h
> > > > @@ -87,6 +87,7 @@
> > > > #define SYSDB_LAST_ONLINE_AUTH "lastOnlineAuth"
> > > > #define SYSDB_LAST_FAILED_LOGIN "lastFailedLogin"
> > > > #define SYSDB_FAILED_LOGIN_ATTEMPTS "failedLoginAttempts"
> > > > +#define SYSDB_LAST_ONLINE_AUTH_WITH_CURR_TOKEN "lastOnlineAuthWithCurrentToken"
> > > >
> > > > #define SYSDB_LAST_UPDATE "lastUpdate"
> > > > #define SYSDB_CACHE_EXPIRE "dataExpireTimestamp"
> > > > diff --git a/src/responder/pam/pam_helpers.h b/src/responder/pam/pam_helpers.h
> > > > index 1e1d914dfe3cd358972cc1ba6deb683a49b68235..b23713c2b51185ca11369b17f1ca30e295d1cb21 100644
> > > > --- a/src/responder/pam/pam_helpers.h
> > > > +++ b/src/responder/pam/pam_helpers.h
> > > > @@ -23,6 +23,8 @@
> > > > #ifndef PAM_HELPERS_H_
> > > > #define PAM_HELPERS_H_
> > > >
> > > > +#include "util/util.h"
> > > >
> > > > errno_t pam_initgr_cache_set(struct tevent_context *ev,
> > > >                              hash_table_t *id_table,
> > > >                              char *name,
> > > > @@ -35,4 +37,17 @@ errno_t pam_initgr_cache_set(struct tevent_context *ev,
> > > > errno_t pam_initgr_check_timeout(hash_table_t *id_table,
> > > >                               char *name);
> > > >
> > > > +errno_t
> > > > +pam_null_last_online_auth_with_curr_token(struct sss_domain_info *domain,
> > > >                                      const char *username);
> > > >
> > > > +errno_t
> > > > +pam_get_last_online_auth_with_curr_token(struct sss_domain_info *domain,
> > > >                                     const char *name,
> > > >                                     uint64_t *_value);
> > > >
> > > > +errno_t
> > > > +pam_set_last_online_auth_with_curr_token(struct sss_domain_info *domain,
> > > >                                     const char *username,
> > > >                                     uint64_t value);
> > > >
> > > > #endif /* PAM_HELPERS_H_ */
> > >
> > > Here are three proposal which you can use.
> > > a) create reusable public functions in sysdb and not a single purpose
> > >    functions. Reusable public functions would fit to current sysdb API.
> > > b) create internal helper functions in pam_responder. They should be internal
> > >    (static) and they purpose should be to improve readalibity of code; avoid
> > >    long functions. If you want them in separate module then create a new file
> > >    for fast pam authentication with public pam functions.
> >
> > I don't think it's such a big deal at this point because the getsetters
> > are in the PAM tree and don't pullute the sysdb API. It's just a helper
> > now.
>
> Yes, but they are still sysdb public functions but in wrong place
> and moreover there is still public constant
> SYSDB_LAST_ONLINE_AUTH_WITH_CURR_TOKEN which can be used with other sysdb
> functions. So one developer can use old way of manipulating with
> this attribute and other can use getter/setter. That's the problem.
> It isn't encapsulation. It's inconsistent and thus unacceptable.

I more or less agree with the reasoning (except I don't think it's such
a big deal and I'd let it go myself). What Sumit brought up on IRC the
other day and what's true is that unless you define the attribute as a
constant, then there's no way to query as part of an array.

(Yes, we probably won't need it for this attribute in particular, but
for others it's true)
> > Yes, I guess even more structured approach would be to have a PAM responder
> > module that handles the cached authentication, but then the question
> > is whether this refactoring wouldn't be too invasive.
>
> Adding new functionality needn't be invasive. In this case it isn't.
> Here is a statistic of last two patches. (excluding public sysdb functions)

No, I meant something else. I meant that the best way might be to create a
module where all the cached auth logic would be contained instead of
pamsrv_cmd. But that would be refactoring and that would be invasive.
> src/confdb/confdb.c                  |  62 +++++++++++++++++++++++++++++++
>  src/confdb/confdb.h                  |   2 ++
>  src/config/SSSDConfig/__init__.py.in |   1 +
>  src/config/SSSDConfigTest.py         |   6 ++--
>  src/config/etc/sssd.api.conf         |   1 +
>  src/db/sysdb_ops.c                   |   1 -
>  src/man/sssd.conf.5.xml              |  24 +++++++++++++
>  src/responder/pam/pamsrv.h           |   3 ++
>  src/responder/pam/pamsrv_cmd.c       | 115 +++++++++++++++++++++++++++++++
>  9 files changed, 205 insertions(+), 10 deletions(-)
> > On the other hand, the tests for the utilities are not too critical, so
> > if you feel strongly about these three helpers, then making them static
> > would be less invasive than creating the module, unless there are tests.
>
> 3 public functions would make sense if they was part of sysdb public API
> (point c). But all these functions are single purpose and will never be
> used on other places (excluding unit test). They are used JUST on one place.
> So they are definitely simple wrappers (helper functions) and the only purpose
> is to improve readability. No reason to make them public.

Well, they are "public" only in the scope of the PAM responder now..
> BTW functions pam_set_last_online_auth_with_curr_token
> pam_null_last_online_auth_with_curr_token can be merged. We needn't
> have such granularity for helper functions.

Again, I wouldn't mind too much here, but if you feel so strongly..
> > I'm totally fine with refactoring code that is well covered with tests,
> > but I don't think we're there yet with PAM responder.
>
> Pavel didn't refactor the code. He just added 115 lines to pamsrv_cmd.c.
> He added a new functionality and moreover he needn't change anything if
> he want to test fast authentication. There is already pam_srv unit test.
> It just need to be extended.

Again, I meant the new pamsrv_cached_auth.c module (or similar).

[Lukas Slebodnik]

On (30/06/15 12:10), Jakub Hrozek wrote:
> On Tue, Jun 30, 2015 at 09:41:19AM +0200, Lukas Slebodnik wrote:
> > On (29/06/15 17:23), Jakub Hrozek wrote:
> > > On Mon, Jun 29, 2015 at 11:56:01AM +0200, Lukas Slebodnik wrote:
> > > > On (26/06/15 17:31), Pavel Reichl wrote:
> > > > >
> > > >
> > > > So it seems you still do not understand what's wrong from desing point of view.
> > > > If you do not understand something please ask it before. We would save your
> > > > time with preparing patches and time of other developers reviewing unnecessary
> > > > round of patches.
> > > >
> > > > I would like to repeat what was not right from design point of view in previous
> > > > version. You used totally different approach for functions. They weren't wrong
> > > > they just did not fit among other sysdb functions.
> > > >
> > > > Let's imagine pure object oriented library and someone decides to create new
> > > > class with just static functions. He wrote this class because he wanted to
> > > > emulate procedural paradigm. It wouldn't be a problem if other classes
> > > > were written in such way. But mixing them together make a stinky code.
> > > >
> > > > It's the same as let chimpanzee sit in an auditorium. it is not dangerous and
> > > > can clap as well but it doesn't fit among other people. It would be fine
> > > > if auditorious contained just chimpanzees.
> > > >
> > > > And now why is current version even worse then previous one from design point
> > > > of view. You renamed public functions and moved them to tottaly unrelated part
> > > > of code. It is an equivalent of get dressed chimpanzee to different clothes and
> > > > let it sit in city council. It still does not fit there.
> > > > > From 38dc66b619d6838610940f23ef84e0851a003319 Mon Sep 17 00:00:00 2001
> > > > > From: Pavel Reichl 
> > > > > Date: Wed, 24 Jun 2015 09:07:55 -0400
> > > > > Subject: [PATCH 1/3] sysdb: new attribute lastOnlineAuthWithCurrentToken
> > > > >
> > > > > Introduce new user attribute lastOnlineAuthWithCurrentToken.
> > > > >
> > > > > Resolves:
> > > > > https://fedorahosted.org/sssd/ticket/1807
> > > > >
> > > > > src/db/sysdb.h                  |  1 +
> > > > > src/responder/pam/pam_helpers.c | 98 ++++++++++++++++++++++++++++++++++++++++-
> > > > > src/responder/pam/pam_helpers.h | 15 +++++++
> > > > > src/tests/cmocka/test_pam_srv.c | 37 ++++++++++++++++
> > > > > 4 files changed, 150 insertions(+), 1 deletion(-)
> > > > >
> > > > > diff --git a/src/db/sysdb.h b/src/db/sysdb.h
> > > > > index 4dc382f6fb3f1b6bd084312463edf1b6189ebba9..f309c9e0c542f8b9c02e48f74a9cefdb1e0a1752 100644
> > > > > --- a/src/db/sysdb.h
> > > > > +++ b/src/db/sysdb.h
> > > > > @@ -87,6 +87,7 @@
> > > > > #define SYSDB_LAST_ONLINE_AUTH "lastOnlineAuth"
> > > > > #define SYSDB_LAST_FAILED_LOGIN "lastFailedLogin"
> > > > > #define SYSDB_FAILED_LOGIN_ATTEMPTS "failedLoginAttempts"
> > > > > +#define SYSDB_LAST_ONLINE_AUTH_WITH_CURR_TOKEN "lastOnlineAuthWithCurrentToken"
> > > > >
> > > > > #define SYSDB_LAST_UPDATE "lastUpdate"
> > > > > #define SYSDB_CACHE_EXPIRE "dataExpireTimestamp"
> > > > > diff --git a/src/responder/pam/pam_helpers.h b/src/responder/pam/pam_helpers.h
> > > > > index 1e1d914dfe3cd358972cc1ba6deb683a49b68235..b23713c2b51185ca11369b17f1ca30e295d1cb21 100644
> > > > > --- a/src/responder/pam/pam_helpers.h
> > > > > +++ b/src/responder/pam/pam_helpers.h
> > > > > @@ -23,6 +23,8 @@
> > > > > #ifndef PAM_HELPERS_H_
> > > > > #define PAM_HELPERS_H_
> > > > >
> > > > > +#include "util/util.h"
> > > > >
> > > > > errno_t pam_initgr_cache_set(struct tevent_context *ev,
> > > > >                              hash_table_t *id_table,
> > > > >                              char *name,
> > > > > @@ -35,4 +37,17 @@ errno_t pam_initgr_cache_set(struct tevent_context *ev,
> > > > > errno_t pam_initgr_check_timeout(hash_table_t *id_table,
> > > > >                               char *name);
> > > > >
> > > > > +errno_t
> > > > > +pam_null_last_online_auth_with_curr_token(struct sss_domain_info *domain,
> > > > >                                      const char *username);
> > > > >
> > > > > +errno_t
> > > > > +pam_get_last_online_auth_with_curr_token(struct sss_domain_info *domain,
> > > > >                                     const char *name,
> > > > >                                     uint64_t *_value);
> > > > >
> > > > > +errno_t
> > > > > +pam_set_last_online_auth_with_curr_token(struct sss_domain_info *domain,
> > > > >                                     const char *username,
> > > > >                                     uint64_t value);
> > > > >
> > > > > #endif /* PAM_HELPERS_H_ */
> > > >
> > > > Here are three proposal which you can use.
> > > > a) create reusable public functions in sysdb and not a single purpose
> > > >    functions. Reusable public functions would fit to current sysdb API.
> > > > b) create internal helper functions in pam_responder. They should be internal
> > > >    (static) and they purpose should be to improve readalibity of code; avoid
> > > >    long functions. If you want them in separate module then create a new file
> > > >    for fast pam authentication with public pam functions.
> > >
> > > I don't think it's such a big deal at this point because the getsetters
> > > are in the PAM tree and don't pullute the sysdb API. It's just a helper
> > > now.
> >
> > Yes, but they are still sysdb public functions but in wrong place
> > and moreover there is still public constant
> > SYSDB_LAST_ONLINE_AUTH_WITH_CURR_TOKEN which can be used with other sysdb
> > functions. So one developer can use old way of manipulating with
> > this attribute and other can use getter/setter. That's the problem.
> > It isn't encapsulation. It's inconsistent and thus unacceptable.
>
> I more or less agree with the reasoning (except I don't think it's such
> a big deal and I'd let it go myself). What Sumit brought up on IRC the
> other day and what's true is that unless you define the attribute as a
> constant, then there's no way to query as part of an array.

So it means that getters and setters are suitable for sysdb API.

This is another proof that functions *last_online_auth_with*
are just wrappers (helper functions) and should not be part of
public API. They cannot be used for proper encapsulation without
exponential count of new function.
   //sysdb_set_username_gid_uid_..._()
> (Yes, we probably won't need it for this attribute in particular, but
> for others it's true)
> > > Yes, I guess even more structured approach would be to have a PAM responder
> > > module that handles the cached authentication, but then the question
> > > is whether this refactoring wouldn't be too invasive.
> >
> > Adding new functionality needn't be invasive. In this case it isn't.
> > Here is a statistic of last two patches. (excluding public sysdb functions)
>
> No, I meant something else. I meant that the best way might be to create a
> module where all the cached auth logic would be contained instead of
> pamsrv_cmd. But that would be refactoring and that would be invasive.

I have to miss something.

There were added 115 lines to pamsrv_cmd.c. They can be added to
new module as public functions (as You propoesd) and can be called
from pamsrv_cmd.c.

I do not understand what would cause invasive change.
It would be a problem if there were many changes in old code
(many removed and adde lines)
> > src/confdb/confdb.c                  |  62 +++++++++++++++++++++++++++++++
> >  src/confdb/confdb.h                  |   2 ++
> >  src/config/SSSDConfig/__init__.py.in |   1 +
> >  src/config/SSSDConfigTest.py         |   6 ++--
> >  src/config/etc/sssd.api.conf         |   1 +
> >  src/db/sysdb_ops.c                   |   1 -
> >  src/man/sssd.conf.5.xml              |  24 +++++++++++++
> >  src/responder/pam/pamsrv.h           |   3 ++
> >  src/responder/pam/pamsrv_cmd.c       | 115 +++++++++++++++++++++++++++++++
> >  9 files changed, 205 insertions(+), 10 deletions(-)
> > > On the other hand, the tests for the utilities are not too critical, so
> > > if you feel strongly about these three helpers, then making them static
> > > would be less invasive than creating the module, unless there are tests.
> >
> > 3 public functions would make sense if they was part of sysdb public API
> > (point c). But all these functions are single purpose and will never be
> > used on other places (excluding unit test). They are used JUST on one place.
> > So they are definitely simple wrappers (helper functions) and the only purpose
> > is to improve readability. No reason to make them public.
>
> Well, they are "public" only in the scope of the PAM responder now..

We do not have such convency and nobody check it in review
which public function is used from which module.
> > BTW functions pam_set_last_online_auth_with_curr_token
> > pam_null_last_online_auth_with_curr_token can be merged. We needn't
> > have such granularity for helper functions.
>
> Again, I wouldn't mind too much here, but if you feel so strongly..

Michal has patches for simplification of sysdb API with using
fqnames. But if you think we needn't care about public API
and we can complicate it with other patches feel free to push
any patches you want.

LS

[Pavel Reichl]

On 06/29/2015 11:56 AM, Lukas Slebodnik wrote:
> On (26/06/15 17:31), Pavel Reichl wrote:
> So it seems you still do not understand what's wrong from desing point of view.
> If you do not understand something please ask it before. We would save your
> time with preparing patches and time of other developers reviewing unnecessary
> round of patches.
>
> I would like to repeat what was not right from design point of view in previous
> version. You used totally different approach for functions. They weren't wrong
> they just did not fit among other sysdb functions.
>
> Let's imagine pure object oriented library and someone decides to create new
> class with just static functions. He wrote this class because he wanted to
> emulate procedural paradigm. It wouldn't be a problem if other classes
> were written in such way. But mixing them together make a stinky code.
>
> It's the same as let chimpanzee sit in an auditorium. it is not dangerous and
> can clap as well but it doesn't fit among other people. It would be fine
> if auditorious contained just chimpanzees.
>
> And now why is current version even worse then previous one from design point
> of view. You renamed public functions and moved them to tottaly unrelated part
> of code. It is an equivalent of get dressed chimpanzee to different clothes and
> let it sit in city council. It still does not fit there.
> > From 38dc66b619d6838610940f23ef84e0851a003319 Mon Sep 17 00:00:00 2001
> > From: Pavel Reichl 
> > Date: Wed, 24 Jun 2015 09:07:55 -0400
> > Subject: [PATCH 1/3] sysdb: new attribute lastOnlineAuthWithCurrentToken
> >
> > Introduce new user attribute lastOnlineAuthWithCurrentToken.
> >
> > Resolves:
> > https://fedorahosted.org/sssd/ticket/1807
> >
> > src/db/sysdb.h                  |  1 +
> > src/responder/pam/pam_helpers.c | 98 ++++++++++++++++++++++++++++++++++++++++-
> > src/responder/pam/pam_helpers.h | 15 +++++++
> > src/tests/cmocka/test_pam_srv.c | 37 ++++++++++++++++
> > 4 files changed, 150 insertions(+), 1 deletion(-)
> >
> > diff --git a/src/db/sysdb.h b/src/db/sysdb.h
> > index 4dc382f6fb3f1b6bd084312463edf1b6189ebba9..f309c9e0c542f8b9c02e48f74a9cefdb1e0a1752 100644
> > --- a/src/db/sysdb.h
> > +++ b/src/db/sysdb.h
> > @@ -87,6 +87,7 @@
> > #define SYSDB_LAST_ONLINE_AUTH "lastOnlineAuth"
> > #define SYSDB_LAST_FAILED_LOGIN "lastFailedLogin"
> > #define SYSDB_FAILED_LOGIN_ATTEMPTS "failedLoginAttempts"
> > +#define SYSDB_LAST_ONLINE_AUTH_WITH_CURR_TOKEN "lastOnlineAuthWithCurrentToken"
> >
> > #define SYSDB_LAST_UPDATE "lastUpdate"
> > #define SYSDB_CACHE_EXPIRE "dataExpireTimestamp"
> > diff --git a/src/responder/pam/pam_helpers.h b/src/responder/pam/pam_helpers.h
> > index 1e1d914dfe3cd358972cc1ba6deb683a49b68235..b23713c2b51185ca11369b17f1ca30e295d1cb21 100644
> > --- a/src/responder/pam/pam_helpers.h
> > +++ b/src/responder/pam/pam_helpers.h
> > @@ -23,6 +23,8 @@
> > #ifndef PAM_HELPERS_H_
> > #define PAM_HELPERS_H_
> >
> > +#include "util/util.h"
> >
> > errno_t pam_initgr_cache_set(struct tevent_context *ev,
> >                               hash_table_t *id_table,
> >                               char *name,
> > @@ -35,4 +37,17 @@ errno_t pam_initgr_cache_set(struct tevent_context *ev,
> > errno_t pam_initgr_check_timeout(hash_table_t *id_table,
> >                                char *name);
> >
> > +errno_t
> > +pam_null_last_online_auth_with_curr_token(struct sss_domain_info *domain,
> >                                      const char *username);
> >
> > +errno_t
> > +pam_get_last_online_auth_with_curr_token(struct sss_domain_info *domain,
> >                                     const char *name,
> >                                     uint64_t *_value);
> >
> > +errno_t
> > +pam_set_last_online_auth_with_curr_token(struct sss_domain_info *domain,
> >                                     const char *username,
> >                                     uint64_t value);
> >
> > #endif /* PAM_HELPERS_H_ */
>
> Here are three proposal which you can use.
> a) create reusable public functions in sysdb and not a single purpose
>     functions. Reusable public functions would fit to current sysdb API.

I have agreed that setter/getter might introduce little inconsistency.  
But on the other hand in your option c) proposes to use them anywhere, 
so I'm wondering if this little inconsistency could not be simply 
considered as an evolution step - later we could continue in this way or 
abandon it.
> b) create internal helper functions in pam_responder. They should be internal
>     (static) and they purpose should be to improve readalibity of code; avoid
>     long functions. If you want them in separate module then create a new file
>     for fast pam authentication with public pam functions.
>     Helper functions (wrappers) must be internal in this module. So compiler can
>     inline them and readability is not decreased.
>     If you warry about code coverage of static functions then you needn't.
>     They already use tested public functions from sysdb and it's recommended to
>     write tests for new functionality. So they will be indirectly tested.

I suppose this is the way updated patches are being developed now, but I 
have strong feelings against being forbidden to unit test these 
functions directly.
> c) Relace all SYSDB_* constants with setters and getters. So internal sysdb
>     library will be consistent.
> Here is a list. (I hope I didn't forget anything)
>      SYSDB_SUBDOMAIN_CLASS, SYSDB_USER_CLASS, SYSDB_GROUP_CLASS,
>      SYSDB_NETGROUP_CLASS, SYSDB_HOST_CLASS, SYSDB_HOSTGROUP_CLASS,
>      SYSDB_SELINUX_USERMAP_CLASS, SYSDB_SELINUX_CLASS, SYSDB_ID_RANGE_CLASS,
>      SYSDB_DOMAIN_ID_RANGE_CLASS, SYSDB_TRUSTED_AD_DOMAIN_RANGE_CLASS,
>      SYSDB_NAME, SYSDB_NAME_ALIAS, SYSDB_OBJECTCLASS, SYSDB_NEXTID,
>      SYSDB_UIDNUM, SYSDB_GIDNUM, SYSDB_CREATE_TIME, SYSDB_PWD, SYSDB_FULLNAME,
>      SYSDB_HOMEDIR, SYSDB_SHELL, SYSDB_MEMBEROF, SYSDB_DISABLED, SYSDB_MEMBER,
>      SYSDB_MEMBERUID, SYSDB_GHOST, SYSDB_POSIX, SYSDB_USER_CATEGORY,
>      SYSDB_HOST_CATEGORY, SYSDB_GROUP_TYPE, SYSDB_GECOS, SYSDB_LAST_LOGIN,
>      SYSDB_LAST_ONLINE_AUTH, SYSDB_LAST_FAILED_LOGIN,
>      SYSDB_FAILED_LOGIN_ATTEMPTS, SYSDB_LAST_ONLINE_AUTH_WITH_CURR_TOKEN,
>      SYSDB_LAST_UPDATE, SYSDB_CACHE_EXPIRE, SYSDB_INITGR_EXPIRE,
>      SYSDB_IFP_CACHED, SYSDB_AUTHORIZED_SERVICE, SYSDB_AUTHORIZED_HOST,
>      SYSDB_NETGROUP_TRIPLE, SYSDB_ORIG_NETGROUP_MEMBER,
>      SYSDB_ORIG_NETGROUP_EXTERNAL_HOST, SYSDB_NETGROUP_DOMAIN,
>      SYSDB_NETGROUP_MEMBER, SYSDB_DESCRIPTION  , SYSDB_FQDN,
>      SYSDB_SERVERHOSTNAME, SYSDB_CACHEDPWD, SYSDB_CACHEDPWD_TYPE,
>      SYSDB_CACHEDPWD_FA2_LEN, SYSDB_UUID, SYSDB_SID, SYSDB_PRIMARY_GROUP,
>      SYSDB_PRIMARY_GROUP_GIDNUM, SYSDB_SID_STR, SYSDB_UPN, SYSDB_CANONICAL_UPN,
>      SYSDB_CCACHE_FILE, SYSDB_ORIG_DN, SYSDB_ORIG_MODSTAMP, SYSDB_ORIG_MEMBEROF,
>      SYSDB_ORIG_MEMBER, SYSDB_ORIG_MEMBER_USER, SYSDB_ORIG_MEMBER_HOST,
>      SYSDB_USN, SYSDB_HIGH_USN, SYSDB_SSH_PUBKEY, SYSDB_AUTH_TYPE,
>      SYSDB_USER_CERT, SYSDB_SUBDOMAIN_REALM, SYSDB_SUBDOMAIN_FLAT,
>      SYSDB_SUBDOMAIN_ID, SYSDB_SUBDOMAIN_MPG, SYSDB_SUBDOMAIN_ENUM,
>      SYSDB_SUBDOMAIN_FOREST, SYSDB_SUBDOMAIN_TRUST_DIRECTION, SYSDB_BASE_ID,
>      SYSDB_ID_RANGE_SIZE, SYSDB_BASE_RID, SYSDB_SECONDARY_BASE_RID,
>      SYSDB_DOMAIN_ID, SYSDB_ID_RANGE_TYPE, SYSDB_DEFAULT_OVERRIDE_NAME,
>      SYSDB_AD_ACCOUNT_EXPIRES, SYSDB_AD_USER_ACCOUNT_CONTROL, SYSDB_VIEW_CLASS,
>      SYSDB_VIEW_NAME, SYSDB_DEFAULT_VIEW_NAME, SYSDB_OVERRIDE_CLASS,
>      SYSDB_OVERRIDE_ANCHOR_UUID, SYSDB_OVERRIDE_USER_CLASS,
>      SYSDB_OVERRIDE_GROUP_CLASS, SYSDB_OVERRIDE_DN, SYSDB_OVERRIDE_OBJECT_DN,
>      SYSDB_HAS_ENUMERATED, SYSDB_IDMAP_SUBTREE, SYSDB_IDMAP_MAPPING_OC,
>      SYSDB_IDMAP_SID_ATTR, SYSDB_IDMAP_SLICE_ATTR, SYSDB_GPO_OC,
>      SYSDB_GPO_GUID_ATTR, SYSDB_GPO_VERSION_ATTR, SYSDB_GPO_TIMEOUT_ATTR,
>      SYSDB_GPO_RESULT_OC

Sorry, but I don't consider this as a valid option. We have limited time 
to develop new features and this would take great amount of time and 
would require wide consensus.  IMO clearly out of scope.
> LS
> _______________________________________________
> sssd-devel mailing list
> [e-mail]
> https://lists.fedorahosted.org/mailman/listinfo/sssd-devel

[Pavel Reichl]

Hello,

new round of patches is attached. I discussed the required changes with 
Jakub to be sure we are on the same page as this thread is quite voluminous.

Basically I just made the setters/getters static and removed tests of 
this functions. I hope that by this change we finally reached the 
desired level of design quality.

Thanks!

PS: Feel free to ignore patch 3. It's just a by-product of this effort.

[Jakub Hrozek]

On Thu, Jul 02, 2015 at 03:06:57PM +0200, Pavel Reichl wrote:
> Hello,
>
> new round of patches is attached. I discussed the required changes with
> Jakub to be sure we are on the same page as this thread is quite voluminous.
>
> Basically I just made the setters/getters static and removed tests of this
> functions. I hope that by this change we finally reached the desired level
> of design quality.
>
> Thanks!
>
> PS: Feel free to ignore patch 3. It's just a by-product of this effort.

If you squash the extra line removal that Sumit noticed into Patch 3,
I'll ACK.

btw I'm still to test OTPs, but the single-factor authentication works.

[Pavel Reichl]

On 07/02/2015 03:06 PM, Pavel Reichl wrote:
> Hello,
>
> new round of patches is attached. I discussed the required changes 
> with Jakub to be sure we are on the same page as this thread is quite 
> voluminous.
>
> Basically I just made the setters/getters static and removed tests of 
> this functions. I hope that by this change we finally reached the 
> desired level of design quality.
>
> Thanks!
>
> PS: Feel free to ignore patch 3. It's just a by-product of this effort.
>
> sssd-devel mailing list
> [e-mail]
> https://lists.fedorahosted.org/mailman/listinfo/sssd-devel

Attached patch set fixes minor code style issues.

[Jakub Hrozek]

On Fri, Jul 03, 2015 at 05:44:27PM +0200, Pavel Reichl wrote:
> On 07/02/2015 03:06 PM, Pavel Reichl wrote:
> > Hello,
> >
> > new round of patches is attached. I discussed the required changes with
> > Jakub to be sure we are on the same page as this thread is quite
> > voluminous.
> >
> > Basically I just made the setters/getters static and removed tests of this
> > functions. I hope that by this change we finally reached the desired level
> > of design quality.
> >
> > Thanks!
> >
> > PS: Feel free to ignore patch 3. It's just a by-product of this effort.
> >
> > sssd-devel mailing list
> > [e-mail]
> > https://lists.fedorahosted.org/mailman/listinfo/sssd-devel
>
> Attached patch set fixes minor code style issues.

These patches work for me, but I also tested OTP and it doesn't behave
as we agreed on in this thread:
    https://www.redhat.com/archives/freeipa-users/2015-June/msg00575.html

Currently only the first factor was considered during the cached auth
period, so anything could be input as the second factor. I propose this
patch:
    https://fedorapeople.org/cgit/jhrozek/public_git/sssd.git/commit/?h=review&a...

We should also amend the manpage if the code is OK.

[Sumit Bose]

On Mon, Jul 06, 2015 at 01:44:36PM +0200, Jakub Hrozek wrote:
> On Fri, Jul 03, 2015 at 05:44:27PM +0200, Pavel Reichl wrote:
> > On 07/02/2015 03:06 PM, Pavel Reichl wrote:
> > > Hello,
> > >
> > > new round of patches is attached. I discussed the required changes with
> > > Jakub to be sure we are on the same page as this thread is quite
> > > voluminous.
> > >
> > > Basically I just made the setters/getters static and removed tests of this
> > > functions. I hope that by this change we finally reached the desired level
> > > of design quality.
> > >
> > > Thanks!
> > >
> > > PS: Feel free to ignore patch 3. It's just a by-product of this effort.
> > >
> > > sssd-devel mailing list
> > > [e-mail]
> > > https://lists.fedorahosted.org/mailman/listinfo/sssd-devel
> >
> > Attached patch set fixes minor code style issues.
>
> These patches work for me, but I also tested OTP and it doesn't behave
> as we agreed on in this thread:
>     https://www.redhat.com/archives/freeipa-users/2015-June/msg00575.html
>
> Currently only the first factor was considered during the cached auth
> period, so anything could be input as the second factor. I propose this
> patch:
>     https://fedorapeople.org/cgit/jhrozek/public_git/sssd.git/commit/?h=review&a...

ACK. One might argue that pam_is_authtok_cachable() can be written in a
more compact way, but imo it is the compilier's job to optimize to
code.

bye,
Sumit
> We should also amend the manpage if the code is OK.
> _______________________________________________
> sssd-devel mailing list
> [e-mail]
> https://lists.fedorahosted.org/mailman/listinfo/sssd-devel

[Jakub Hrozek]

On Mon, Jul 06, 2015 at 04:02:59PM +0200, Sumit Bose wrote:
> On Mon, Jul 06, 2015 at 01:44:36PM +0200, Jakub Hrozek wrote:
> > On Fri, Jul 03, 2015 at 05:44:27PM +0200, Pavel Reichl wrote:
> > > On 07/02/2015 03:06 PM, Pavel Reichl wrote:
> > > > Hello,
> > > >
> > > > new round of patches is attached. I discussed the required changes with
> > > > Jakub to be sure we are on the same page as this thread is quite
> > > > voluminous.
> > > >
> > > > Basically I just made the setters/getters static and removed tests of this
> > > > functions. I hope that by this change we finally reached the desired level
> > > > of design quality.
> > > >
> > > > Thanks!
> > > >
> > > > PS: Feel free to ignore patch 3. It's just a by-product of this effort.
> > > >
> > > > sssd-devel mailing list
> > > > [e-mail]
> > > > https://lists.fedorahosted.org/mailman/listinfo/sssd-devel
> > >
> > > Attached patch set fixes minor code style issues.
> >
> > These patches work for me, but I also tested OTP and it doesn't behave
> > as we agreed on in this thread:
> >     https://www.redhat.com/archives/freeipa-users/2015-June/msg00575.html
> >
> > Currently only the first factor was considered during the cached auth
> > period, so anything could be input as the second factor. I propose this
> > patch:
> >     https://fedorapeople.org/cgit/jhrozek/public_git/sssd.git/commit/?h=review&a...
>
> ACK.

CI: http://sssd-ci.duckdns.org/logs/job/18/63/summary.html
> One might argue that pam_is_authtok_cachable() can be written in a
> more compact way, but imo it is the compilier's job to optimize to
> code.

Yes, but I tried to keep the same style as pam_is_cmd_cachable that is
just above that.

And from the experience of debugging optimized code I know the result is
nothing like the source :)

Thank you for the review. Pushed to master:
    * 7e798b94cfffe7bf8f7b477d540b95d52ca1f6e4
    * 6aff93510b36799c1773d368cc218cd533c43161
    * 0aa18cc0bf3447ca734476926724f1632e160807
    * 32cc237aa0f3c70a4e0bc0491ec0cba0016aaf5a