Re: [Kimchi-devel] [PATCHv1 2/4] Split PAM and LDAP authentication

On 2014年10月22日 02:39, Aline Manera wrote: > > On 10/20/2014 11:52 AM, lvroyce0210(a)gmail.com wrote: >> From: Royce Lv <lvroyce(a)linux.vnet.ibm.com&gt; >> >> Split PAM authentication implementation and abstract a common class. >> >> Signed-off-by: Royce Lv <lvroyce(a)linux.vnet.ibm.com&gt; >> --- >> src/kimchi/auth.py | 111 >> ++++++++++++++++++++++++++++++++++------------------- >> 1 file changed, 71 insertions(+), 40 deletions(-) >> >> diff --git a/src/kimchi/auth.py b/src/kimchi/auth.py >> index c8801a5..10c7c1f 100644 >> --- a/src/kimchi/auth.py >> +++ b/src/kimchi/auth.py >> @@ -31,6 +31,7 @@ import urllib2 >> >> >> from kimchi import template >> +from kimchi.config import config >> from kimchi.exception import InvalidOperation, OperationFailed >> from kimchi.utils import get_all_tabs, run_command >> >> @@ -59,6 +60,22 @@ def debug(msg): >> >> >> class User(object): > > As it is related to authentication, I suggest renaming it to > "Authentication" It is not just authentication, it is related to user authentication, authorization, user role/group get. (see things already there in PAMUser)

See below: In my refactor, Users only allows to be created when it has right auth_type as well as pass the authentication of this type. Otherwise we don't have the need to create such type of user.

> >> + @classmethod >> + def create(cls, auth_args): >> + auth_type = auth_args.pop('auth_type') >> + for klass in cls.__subclasses__(): >> + if auth_type == klass.auth_type: >> + try: >> + if not klass.authenticate(**auth_args): >> + debug("cannot verify user with the given password") >> + return None >> + except OperationFailed: >> + raise >> + return klass(auth_args['username']) >> + >> + > > We could do it in the __init__() function and return the right > instance according to the authentication method. > Example: > > class Authentication(object): > def __init__(self, auth_type): > > + for klass in cls.__subclasses__(): > + if auth_type == klass.auth_type: > + return klass() > > >> +class PAMUser(User): > > class PAMAuthentication(Authentication); > >> + auth_type = "pam" >> >> def __init__(self, username): > > You can get the username/password only when doing the authentication > >> self.user = {} >> @@ -124,40 +141,54 @@ class User(object): >> def get_user(self): >> return self.user >> >> + @staticmethod > > It will not be static with you have a instance according to type as I > suggested above. > >> + def authenticate(username, password, service="passwd"): >> + '''Returns True if authenticate is OK via PAM.''' >> + def _pam_conv(auth, query_list, userData=None): >> + resp = [] >> + for i in range(len(query_list)): >> + query, qtype = query_list[i] >> + if qtype == PAM.PAM_PROMPT_ECHO_ON: >> + resp.append((username, 0)) >> + elif qtype == PAM.PAM_PROMPT_ECHO_OFF: >> + resp.append((password, 0)) >> + elif qtype == PAM.PAM_PROMPT_ERROR_MSG: >> + cherrypy.log.error_log.error( >> + "PAM authenticate prompt error: %s" % query) >> + resp.append(('', 0)) >> + elif qtype == PAM.PAM_PROMPT_TEXT_INFO: >> + resp.append(('', 0)) >> + else: >> + return None >> + return resp >> + >> + auth = PAM.pam() >> + auth.start(service) >> + auth.set_item(PAM.PAM_USER, username) >> + auth.set_item(PAM.PAM_CONV, _pam_conv) >> + try: >> + auth.authenticate() >> + except PAM.error, (resp, code): >> + msg_args = {'username': username, 'code': code} >> + raise OperationFailed("KCHAUTH0001E", msg_args) >> >> -def authenticate(username, password, service="passwd"): >> - '''Returns True if authenticate is OK via PAM.''' >> - def _pam_conv(auth, query_list, userData=None): >> - resp = [] >> - for i in range(len(query_list)): >> - query, qtype = query_list[i] >> - if qtype == PAM.PAM_PROMPT_ECHO_ON: >> - resp.append((username, 0)) >> - elif qtype == PAM.PAM_PROMPT_ECHO_OFF: >> - resp.append((password, 0)) >> - elif qtype == PAM.PAM_PROMPT_ERROR_MSG: >> - cherrypy.log.error_log.error("PAM authenticate prompt error " >> - "message: %s" % query) >> - resp.append(('', 0)) >> - elif qtype == PAM.PAM_PROMPT_TEXT_INFO: >> - resp.append(('', 0)) >> - else: >> - return None >> - return resp >> - >> - auth = PAM.pam() >> - auth.start(service) >> - auth.set_item(PAM.PAM_USER, username) >> - auth.set_item(PAM.PAM_CONV, _pam_conv) >> - >> - try: >> - auth.authenticate() >> - except PAM.error: >> - raise >> - >> - return True >> + return True >> >> >> +class LDAPUser(User): > > class LDAPAuthentication(Authentication): > >> + auth_type = "ldap" >> + def __init__(self, username): > > Same I commented above. You can receive the username/password only > when doing the authentication itself. > >> + self.user = {} >> + self.user[USER_NAME] = username >> + self.user[USER_GROUPS] = None >> + # FIXME: user roles will be changed according roles assignment after >> + # objstore is integrated >> + self.user[USER_ROLES] = dict.fromkeys(tabs, 'user') >> + >> + @staticmethod >> + def authenticate(username, password): >> + return False >> + >> def from_browser(): >> # Enable Basic Authentication for REST tools. >> # Ajax request sent from jQuery in browser will have "X-Requested-With" >> @@ -216,15 +247,15 @@ def check_auth_httpba(): >> >> >> def login(username, password, **kwargs): >> - try: >> - if not authenticate(username, password): >> - debug("User cannot be verified with the supplied password") >> - return None >> - except PAM.error, (resp, code): >> - msg_args = {'username': username, 'code': code} >> - raise OperationFailed("KCHAUTH0001E", msg_args) >> - >> - user = User(username) > >> + auth_args = {'auth_type': config.get("authentication", "method"), >> + 'username': username, >> + 'password': password} >> + >> + user = User.create(auth_args) >> + if not user: >> + debug("User cannot be verified with the supplied password") >> + return None >> + > > auth = Authentication(config.get("authentication", "method")) > user = auth.authentication(username, password) Nope, this part already covered in factory method > >> debug("User verified, establishing session") >> cherrypy.session.acquire_lock() >> cherrypy.session.regenerate() > > _______________________________________________ > Kimchi-devel mailing list > Kimchi-devel(a)ovirt.org > http://lists.ovirt.org/mailman/listinfo/kimchi-devel >