3 bug fixes #3

Merged
merged 3 commits into from Mar 29, 2013

Conversation

Projects
None yet
2 participants

yscumc commented Mar 27, 2013

  • Added check for request.path to ensure idleFor parameter is processed only for session_security_ping url
  • Assume a negative idleFor value means 0 to prevent a log out but if the value is negative
  • Ignore non-integer idleFor values instead of throwing an exception
ys
3 bug fixes
Added check for request.path to ensure idleFor parameter is processed only for session_security_ping url
Assume a negative idleFor value means 0 to prevent a log out but if the value is negative
Ignore non-integer idleFor values instead of throwing an exception
Owner

jpic commented Mar 28, 2013

I think line 52 is too long and breaks PEP8, could you fix it ?

You just need to pip install pep8 and run this command from the repo root: pep8 --exclude=tests,migrations --ignore=E124,E128 session_security

Unit tests would be welcome too (in tests/middleware.py), else I'll do them.

Really interresting commit BTW, thanks a heap !

yscumc commented Mar 28, 2013

Sorry I'm new to Git & GitHub, not sure how I would modify a pull request. Would I have to do a new pull request?

Would it work if you merge it and fix it?

Also, I just realized it may make more sense if the entire logic for checking idleFor is moved from the middleware to the view instead.

Owner

jpic commented Mar 28, 2013

On Thu, Mar 28, 2013 at 5:46 PM, yscumc notifications@github.com wrote:

Sorry I'm new to Git & GitHub, not sure how I would modify a pull request. Would I have to do a new pull request?

Actually, you should have commited this in a branch.

Anyway, you can just push new commits to the branch and the pull
request should update automatically. The pull request is branch-based.

Would it work if you merge it and fix it?

Yes

Also, I just realized it may make more sense if the entire logic for checking idleFor is moved from the middleware to the view instead.

But if this was moved to the ping view which is only requested by
javascript, would the security also work on non-javascript browsers ?

http://yourlabs.org
Customer is king - Le client est roi - El cliente es rey.

yscumc commented Mar 28, 2013

Ah I came from the SVN world so it's more rare for branches. Seems like it's always about branching in Git.

Anyway, I've committed the styling changes.

Regarding moving the idleFor logic, I don't think it would make a difference, since the middleware would still update based on regular requests. What I mean is only the following section should be moved:

       if (request.path == reverse('session_security_ping') and
                'idleFor' in request.GET):
            # Gracefully ignore non-integer values
            try:
                client_idle_for = int(request.GET['idleFor'])
            except ValueError:
                return

            # Do not allow negative values since delta would be calculated incorrectly 
            if client_idle_for < 0:
                client_idle_for = 0

            if client_idle_for < server_idle_for:
                # Client has more recent activity than we have in the session
                last_activity = now - timedelta(seconds=client_idle_for)

                # Update the session
                request.session['_session_security'] = last_activity

If idleFor is not in the request, this section would not get executed anyway, which means if there's no JavaScript, then there's no support for updating activity based on non-page-reload user actions

Owner

jpic commented Mar 28, 2013

PEP8 is not quite ready:

$ pep8 --exclude=tests,migrations --ignore=E124,E128 session_security
session_security/middleware.py:59:1: W293 blank line contains whitespace
session_security/middleware.py:60:80: E501 line too long (86 > 79 characters)
session_security/middleware.py:60:87: W291 trailing whitespace
session_security/middleware.py:63:1: W293 blank line contains whitespace
The command "pep8 --exclude=tests,migrations --ignore=E124,E128 session_security" failed and exited with 1 during before_script.

yscumc commented Mar 29, 2013

Ok I finally ran PEP8 and fixed the problems listed. Didn't know it was so easy, thanks for providing the command.

jpic added a commit that referenced this pull request Mar 29, 2013

@jpic jpic merged commit 846685c into yourlabs:master Mar 29, 2013

1 check passed

default The Travis build passed
Details
Owner

jpic commented Mar 29, 2013

Thanks ! Could you close the related issues ?

yscumc commented Mar 29, 2013

Okay, #2 is closed. #1 is still open because the issue is not yet addressed...

yscumc commented Apr 1, 2013

Seems like the patch fails a lot of tests if I run it from within my project. Looks like the solution is to move the idleFor logic from the middleware into the view.

Traceback (most recent call last):
  File "C:\Python27\lib\site-packages\django\contrib\auth\tests\views.py", line 301, in test_password_change_done_fails
    response = self.client.get('/password_change/done/')
  File "C:\Python27\lib\site-packages\django\test\client.py", line 453, in get
    response = super(Client, self).get(path, data=data, **extra)
  File "C:\Python27\lib\site-packages\django\test\client.py", line 279, in get
    return self.request(**r)
  File "C:\Python27\lib\site-packages\django\test\client.py", line 406, in request
    response = self.handler(environ)
  File "C:\Python27\lib\site-packages\django\test\client.py", line 102, in __call__
    self.load_middleware()
  File "C:\Python27\lib\site-packages\django\core\handlers\base.py", line 51, in load_middleware
    mod = import_module(mw_module)
  File "C:\Python27\lib\site-packages\django\utils\importlib.py", line 35, in import_module
    __import__(name)
  File "C:\Python27\lib\site-packages\session_security\middleware.py", line 18, in <module>
    from settings import *
  File "C:\Python27\lib\site-packages\session_security\settings.py", line 37, in <module>
    urlresolvers.reverse('session_security_ping'),
  File "C:\Python27\lib\site-packages\django\core\urlresolvers.py", line 496, in reverse
    return iri_to_uri(resolver._reverse_with_prefix(view, prefix, *args, **kwargs))
  File "C:\Python27\lib\site-packages\django\core\urlresolvers.py", line 416, in _reverse_with_prefix
    "arguments '%s' not found." % (lookup_view_s, args, kwargs))
NoReverseMatch: Reverse for 'session_security_ping' with arguments '()' and keyword arguments '{}' not found.
Owner

jpic commented Apr 2, 2013

True, for the moment the tests are ment to be run from test_project.

I'm trying to fix that in a local branch, but it is not a priority
(priority is to release your contributions).

Right now I'm moving home but I will have internet tomorrow (in theory) so
I'll be able to restore the maintenance rythm.

Thanks for your contribution, your curiosity, and your patience, well
appreciated ;)

On Mon, Apr 1, 2013 at 9:45 PM, yscumc notifications@github.com wrote:

Seems like the patch fails a lot of tests if I run it from within my
project. Looks like the solution is to move the idleFor logic from the
middleware into the view.

Traceback (most recent call last):
File "C:\Python27\lib\site-packages\django\contrib\auth\tests\views.py", line 301, in test_password_change_done_fails
response = self.client.get('/password_change/done/')
File "C:\Python27\lib\site-packages\django\test\client.py", line 453, in get
response = super(Client, self).get(path, data=data, **extra)
File "C:\Python27\lib\site-packages\django\test\client.py", line 279, in get
return self.request(**r)
File "C:\Python27\lib\site-packages\django\test\client.py", line 406, in request
response = self.handler(environ)
File "C:\Python27\lib\site-packages\django\test\client.py", line 102, in call
self.load_middleware()
File "C:\Python27\lib\site-packages\django\core\handlers\base.py", line 51, in load_middleware
mod = import_module(mw_module)
File "C:\Python27\lib\site-packages\django\utils\importlib.py", line 35, in import_module
import(name)
File "C:\Python27\lib\site-packages\session_security\middleware.py", line 18, in
from settings import *
File "C:\Python27\lib\site-packages\session_security\settings.py", line 37, in
urlresolvers.reverse('session_security_ping'),
File "C:\Python27\lib\site-packages\django\core\urlresolvers.py", line 496, in reverse
return iri_to_uri(resolver._reverse_with_prefix(view, prefix, _args, *_kwargs))
File "C:\Python27\lib\site-packages\django\core\urlresolvers.py", line 416, in _reverse_with_prefix
"arguments '%s' not found." % (lookup_view_s, args, kwargs))
NoReverseMatch: Reverse for 'session_security_ping' with arguments '()' and keyword arguments '{}' not found.


Reply to this email directly or view it on GitHubhttps://github.com/yourlabs/django-session-security/pull/3#issuecomment-15733154
.

http://yourlabs.org http://blog.yourlabs.org
Customer is king - Le client est roi - El cliente es rey.

yscumc commented Apr 2, 2013

Not at all, thank YOU for saving me tons of time by making this and for being so informative.

@yscumc yscumc referenced this pull request Jan 9, 2014

Merged

Fix clock sync problems #13

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment