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

[Aline Manera]

On 10/22/2014 04:02 AM, Royce Lv wrote:
> 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)
>

OK.

> 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.

If you don't want to change it, please rename "create" by "get" or 
something as Kimchi does not create any 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
>>
>