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

[Royce Lv]

On 2014年10月22日 02:39, Aline Manera wrote:
>
> On 10/20/2014 11:52 AM, lvroyce0210 at gmail.com wrote:
>> From: Royce Lv <lvroyce at linux.vnet.ibm.com>
>>
>> Split PAM authentication implementation and abstract a common class.
>>
>> Signed-off-by: Royce Lv <lvroyce at linux.vnet.ibm.com>
>> ---
>> 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 at ovirt.org
> http://lists.ovirt.org/mailman/listinfo/kimchi-devel
>