[Freeipa-devel] [PATCH 82] Compliant client side session cookie behavior
John Dennis
jdennis at redhat.com
Fri Dec 7 17:37:08 UTC 2012
On 11/13/2012 07:39 AM, Petr Viktorin wrote:
> Continuing from yesterday. I tried building the RPMs, installing a
> server, running the tests, and checking the Web UI. Each of these steps
> failed.
>
>
> On 11/11/2012 11:18 PM, John Dennis wrote:
>> Note: This has been tested with both the command line api and the
>> browser on both Fedora and RHEL-6. It has also been tested to make sure
>> any cookies stored before an upgrade will work correctly.
>>
>> --
>> John Dennis <jdennis at redhat.com>
>>
>> Looking to carve out IT costs?
>> www.redhat.com/carveoutcosts/
>>
>> freeipa-jdennis-0082-Compliant-client-side-session-cookie-behavior.patch
>>
>>
>> >From 089d69a1e06636bbd2836fcb9072b5a2ffef7ae2 Mon Sep 17 00:00:00 2001
>> From: John Dennis<jdennis at redhat.com>
>> Date: Sun, 11 Nov 2012 17:05:32 -0500
>> Subject: [PATCH 82] Compliant client side session cookie behavior
>> Content-Type: text/plain; charset="utf-8"
>> Content-Transfer-Encoding: 8bit
>>
> [...]
>>
>> Ticket https://fedorahosted.org/freeipa/ticket/3022
>> ---
>> ipalib/rpc.py | 224 +++++++++++++----
>> ipalib/session.py | 39 +--
>> ipapython/cookie.py | 486 ++++++++++++++++++++++++++++++++++++
>> ipaserver/rpcserver.py | 6 +-
>> tests/test_ipapython/test_cookie.py | 332 ++++++++++++++++++++++++
>> 5 files changed, 1017 insertions(+), 70 deletions(-)
>> create mode 100644 ipapython/cookie.py
>> create mode 100644 tests/test_ipapython/test_cookie.py
>>
>> diff --git a/ipalib/rpc.py b/ipalib/rpc.py
>> index c555105..b2021c1 100644
>> --- a/ipalib/rpc.py
>> +++ b/ipalib/rpc.py
>> @@ -61,7 +64,8 @@ from ipalib.krb_utils import KRB5KDC_ERR_S_PRINCIPAL_UNKNOWN, KRB5KRB_AP_ERR_TKT
>> KRB5_FCC_PERM, KRB5_FCC_NOFILE, KRB5_CC_FORMAT, KRB5_REALM_CANT_RESOLVE
>> from ipapython.dn import DN
>>
>> -COOKIE_NAME = 'ipa_session_cookie:%s'
>> +COOKIE_NAME = 'ipa_session'
>
> COOKIE_NAME is used in ipa-client-install, which promptly fails when it
> does `kernel_keyring.del_key(COOKIE_NAME % host_principal)`.
> (There should probably be a helper function to encapsulate this call.)
Fixed. The whole keyring storage mechanism should never have been
exposed in the first place. Instead it should have been encapsulated in
some other functions that hide the implementation and instead export a
logical abstraction. rpc.py now exports these functions:
update_persistent_client_session_data(principal, data)
read_persistent_client_session_data(principal)
delete_persistent_client_session_data(principal)
>
>> +KEYRING_COOKIE_NAME = '%s_cookie:%%s' % COOKIE_NAME
>>
>> def xml_wrap(value):
>> """
>
>> diff --git a/ipalib/session.py b/ipalib/session.py
>> index 36beece..900259a 100644
>> --- a/ipalib/session.py
>> +++ b/ipalib/session.py
>> @@ -955,13 +955,18 @@ class MemcacheSessionManager(SessionManager):
>> Session id as string or None if not found.
>> '''
>> session_id = None
>> - if cookie_header is not None:
>> - cookie = Cookie.SimpleCookie()
>> - cookie.load(cookie_header)
>> - session_cookie = cookie.get(self.session_cookie_name)
>> - if session_cookie is not None:
>> - session_id = session_cookie.value
>> - self.debug('found session cookie_id = %s', session_id)
>> + try:
>> + session_cookie = Cookie.get_named_cookie_from_string(cookie_header, self.session_cookie_name)
>> + except Exception, e:
>> + session_cookie = None
>> + else:
>> + session_id = session_cookie.value
>
> When the user first accesses the Web UI, session_cookie will be None,
> resulting in an Internal Server Error.
Hmm... I didn't see this in testing. I think you mean the cookie_header
will be None, not the session_cookie being None. That case should have
been caught by the try/except block surrounding
get_named_cookie_from_string(). But in any event I added a check for the
cookie_header being None at the top of the function. Or am I
misunderstanding the problem you saw?
>
>> diff --git a/ipapython/cookie.py b/ipapython/cookie.py
>> new file mode 100644
>> index 0000000..0033aed
>> --- /dev/null
>> +++ b/ipapython/cookie.py
> [...]
>> +
>> + @property
>> + def timestamp(self):
>
> Pylint complains here and in similar places below:
> ipapython/cookie.py:313: [E0202, Cookie.timestamp] An attribute affected
> in ipapython.cookie line 310 hide this method
This is documented bug in pylint. The errors have been disabled along
with a comment list the two bug reports and a FIXME saying to remove the
error exclusion when pylint has fixed the bug.
>
>> diff --git a/tests/test_ipapython/test_cookie.py b/tests/test_ipapython/test_cookie.py
>> new file mode 100644
>> index 0000000..4b3c317
>> --- /dev/null
>> +++ b/tests/test_ipapython/test_cookie.py
>> @@ -0,0 +1,332 @@
>> +# Authors:
>> +# John Dennis<jdennis at redhat.com>
>> +#
>> +# Copyright (C) 2012 Red Hat
>> +# see file 'COPYING' for use and warranty information
>> +#
>> +# This program is free software; you can redistribute it and/or modify
>> +# it under the terms of the GNU General Public License as published by
>> +# the Free Software Foundation, either version 3 of the License, or
>> +# (at your option) any later version.
>> +#
>> +# This program is distributed in the hope that it will be useful,
>> +# but WITHOUT ANY WARRANTY; without even the implied warranty of
>> +# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
>> +# GNU General Public License for more details.
>> +#
>> +# You should have received a copy of the GNU General Public License
>> +# along with this program. If not, see<http://www.gnu.org/licenses/>.
>> +
>> +import unittest
>> +import time
>> +import datetime
>> +import calendar
>> +from cookie import Cookie
>
> Here I get ImportError: No module named cookie.
Fixed, silly typo from stand-alone testing that omitted the module. I
was remiss in not catching this in the patch, sorry.
>
>
--
John Dennis <jdennis at redhat.com>
Looking to carve out IT costs?
www.redhat.com/carveoutcosts/
More information about the Freeipa-devel
mailing list