Discussion:
[Freeipa-devel] [freeipa PR#546][opened] Store session cookie in a ccache option
simo5
2017-03-07 13:45:09 UTC
Permalink
URL: https://github.com/freeipa/freeipa/pull/546
Author: simo5
Title: #546: Store session cookie in a ccache option
Action: opened

PR body:
"""
Instead of using the kernel keyring, store the session cookie within the
ccache. This way kdestroy will really wipe away all crededntials.

Ticket: https://pagure.io/freeipa/issue/6661
"""

To pull the PR as Git branch:
git remote add ghfreeipa https://github.com/freeipa/freeipa
git fetch ghfreeipa pull/546/head:pr546
git checkout pr546
MartinBasti
2017-03-07 15:32:42 UTC
Permalink
URL: https://github.com/freeipa/freeipa/pull/546
Title: #546: Store session cookie in a ccache option

MartinBasti commented:
"""
Pylint failed and I have a few inline comments
```
************* Module ipapython.ccache_storage
ipapython/ccache_storage.py:234: [C0305(trailing-newlines), ] Trailing newlines)
ipapython/ccache_storage.py:32: [W1612(unicode-builtin), c_text_p.from_param] unicode built-in referenced)
ipapython/ccache_storage.py:45: [E1101(no-member), c_text_p.text] Class 'value' has no 'decode' member)
ipapython/ccache_storage.py:128: [C1001(old-style-class), session_store] Old-style class defined.)
ipapython/ccache_storage.py:132: [E0710(raising-non-exception), session_store.__init__] Raising a new style class which doesn't inherit from BaseException)
ipapython/ccache_storage.py:6: [W0611(unused-import), ] Unused import os)
```
"""

See the full comment at https://github.com/freeipa/freeipa/pull/546#issuecomment-284755511
rcritten
2017-03-07 15:52:52 UTC
Permalink
URL: https://github.com/freeipa/freeipa/pull/546
Title: #546: Store session cookie in a ccache option

rcritten commented:
"""
Should this patch not also remove the keyring code?

Unit tests should be provided.
"""

See the full comment at https://github.com/freeipa/freeipa/pull/546#issuecomment-284761915
simo5
2017-03-07 16:08:55 UTC
Permalink
URL: https://github.com/freeipa/freeipa/pull/546
Title: #546: Store session cookie in a ccache option

simo5 commented:
"""
@rcritten the keyring stuff is still used for detection of keyring in other places, so I did not touch it as those uses are still vaild

"""

See the full comment at https://github.com/freeipa/freeipa/pull/546#issuecomment-284767193
simo5
2017-03-07 16:09:42 UTC
Permalink
URL: https://github.com/freeipa/freeipa/pull/546
Title: #546: Store session cookie in a ccache option

simo5 commented:
"""
Not sure how to provide unit tests, these functions work only if you have a valid ccache in the name of the principal you are trying to store a session cookie for.
"""

See the full comment at https://github.com/freeipa/freeipa/pull/546#issuecomment-284767456
simo5
2017-03-07 16:34:07 UTC
Permalink
URL: https://github.com/freeipa/freeipa/pull/546
Author: simo5
Title: #546: Store session cookie in a ccache option
Action: synchronized

To pull the PR as Git branch:
git remote add ghfreeipa https://github.com/freeipa/freeipa
git fetch ghfreeipa pull/546/head:pr546
git checkout pr546
simo5
2017-03-07 16:34:45 UTC
Permalink
URL: https://github.com/freeipa/freeipa/pull/546
Title: #546: Store session cookie in a ccache option

simo5 commented:
"""
Ok removed a bunch of code and made sure pylint passes.
"""

See the full comment at https://github.com/freeipa/freeipa/pull/546#issuecomment-284775623
simo5
2017-03-07 16:35:08 UTC
Permalink
URL: https://github.com/freeipa/freeipa/pull/546
Title: #546: Store session cookie in a ccache option

simo5 commented:
"""
I also renamed the module and the class, makes more sense to me this way around.
"""

See the full comment at https://github.com/freeipa/freeipa/pull/546#issuecomment-284775755
simo5
2017-03-07 16:39:47 UTC
Permalink
URL: https://github.com/freeipa/freeipa/pull/546
Author: simo5
Title: #546: Store session cookie in a ccache option
Action: synchronized

To pull the PR as Git branch:
git remote add ghfreeipa https://github.com/freeipa/freeipa
git fetch ghfreeipa pull/546/head:pr546
git checkout pr546
simo5
2017-03-09 12:33:20 UTC
Permalink
URL: https://github.com/freeipa/freeipa/pull/546
Author: simo5
Title: #546: Store session cookie in a ccache option
Action: synchronized

To pull the PR as Git branch:
git remote add ghfreeipa https://github.com/freeipa/freeipa
git fetch ghfreeipa pull/546/head:pr546
git checkout pr546
simo5
2017-03-09 12:34:43 UTC
Permalink
URL: https://github.com/freeipa/freeipa/pull/546
Title: #546: Store session cookie in a ccache option

simo5 commented:
"""
Ok I decide to do away with the whole class stuff, given we never really keep a round the class object for more than one operation at a time in actual use.
As @rcritten requested I also added a test, and I am glad it was asked as I found a failure case we need to handle (see the exception handling in remove_data()
"""

See the full comment at https://github.com/freeipa/freeipa/pull/546#issuecomment-285339682
MartinBasti
2017-03-09 12:54:48 UTC
Permalink
URL: https://github.com/freeipa/freeipa/pull/546
Title: #546: Store session cookie in a ccache option

MartinBasti commented:
"""
```
************* Module ipapython.session_storage
ipapython/session_storage.py:187: [W1624(indexing-exception), remove_data] Indexing exceptions will not work on Python 3)
************* Module ipalib.rpc
ipalib/rpc.py:114: [E1120(no-value-for-parameter), read_persistent_client_session_data] No value for argument 'value' in function call)
************* Module ipatests.test_ipapython.test_session_storage
ipatests/test_ipapython/test_session_storage.py:39: [W0612(unused-variable), test_session_storage.test_03] Unused variable 'e')
ipatests/test_ipapython/test_session_storage.py:9: [W0611(unused-import), ] Unused raises imported from nose.tools)
ipatests/test_ipapython/test_session_storage.py:12: [W0611(unused-import), ] Unused import pytest)
```
"""

See the full comment at https://github.com/freeipa/freeipa/pull/546#issuecomment-285343721
simo5
2017-03-09 13:52:50 UTC
Permalink
URL: https://github.com/freeipa/freeipa/pull/546
Title: #546: Store session cookie in a ccache option

simo5 commented:
"""
Oops sorry, forgot to run make pylint on my last iteration, should be all fixed now
"""

See the full comment at https://github.com/freeipa/freeipa/pull/546#issuecomment-285356420
simo5
2017-03-09 13:52:48 UTC
Permalink
URL: https://github.com/freeipa/freeipa/pull/546
Author: simo5
Title: #546: Store session cookie in a ccache option
Action: synchronized

To pull the PR as Git branch:
git remote add ghfreeipa https://github.com/freeipa/freeipa
git fetch ghfreeipa pull/546/head:pr546
git checkout pr546
MartinBasti
2017-03-09 16:47:30 UTC
Permalink
URL: https://github.com/freeipa/freeipa/pull/546
Title: #546: Store session cookie in a ccache option

Label: +ack
MartinBasti
2017-03-10 11:40:55 UTC
Permalink
URL: https://github.com/freeipa/freeipa/pull/546
Author: simo5
Title: #546: Store session cookie in a ccache option
Action: closed

To pull the PR as Git branch:
git remote add ghfreeipa https://github.com/freeipa/freeipa
git fetch ghfreeipa pull/546/head:pr546
git checkout pr546
MartinBasti
2017-03-10 11:40:52 UTC
Permalink
URL: https://github.com/freeipa/freeipa/pull/546
Title: #546: Store session cookie in a ccache option

Label: +pushed
MartinBasti
2017-03-10 11:40:53 UTC
Permalink
URL: https://github.com/freeipa/freeipa/pull/546
Title: #546: Store session cookie in a ccache option

MartinBasti commented:
"""
master:

* 7cab95955539b5f9596dcda5886ea3d9755fb193 Store session cookie in a ccache option
"""

See the full comment at https://github.com/freeipa/freeipa/pull/546#issuecomment-285648539
Loading...