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 <preichl(a)redhat.com>
> 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
sssd-devel(a)lists.fedorahosted.org
https://lists.fedorahosted.org/mailman/listinfo/sssd-devel