Supporting custom 'EXPIRE_AFTER' value (use case: user-defined expiration value)

[VuongN]

No description provided.

[Sapphire64]

I would suggest to redesign WARN_AFTER field or at least add alternative. Currently, you have to specify two values, but for me logically it looks better to have WARN_AFTER as delta of the expiration field in seconds or in percents. It will make user-specific autologout time easier to implement.

[VuongN]

@Sapphire64: having a single setting value serving two purposes not only makes it confusing but would also break backward compatibility. I'm pretty sure that WARN_AFTER currently works for majority of the cases and the custom/user-specific would be a subset and wouldn't warrant a redesign.

@jpic: I'll add the unittests shortly.

[yscumc]

I think he was thinking about a scenario such as this:

WARN_AFTER = 900
EXPIRE_AFTER = 1200
current_session_expire_after = 600

In this case, WARN_AFTER would be more than the session's custom EXPIRE_AFTER.

If a multiplier such as 0.75 can be used as a calculation for WARN_AFTER, then in the custom case above, the effective value of WARN_AFTER would be 450. This would make a lot more sense and still serve a single purpose (to warn the user a percentage of the time before the EXPIRE_AFTER).

[VuongN]

@yscumc, @Sapphire64: I misread WARN_AFTER as EXPIRE_AFTER. Even so, that discussion is out of scope of this pull request. As stated, this is only to support the use case of custom/user-defined expiration.

[Sapphire64]

The problem with this PR is that JS side notifications will never work right. If custom value is less than warning from settings, user will not be redirected to login page, but all AJAX calls will fail. If custom value is larger user will be warned as usual and even redirected to login page without real reason.

[VuongN]

@Sapphire64: what is the current behavior when you set settings.EXPIRE_AFTER to be smaller than settings.WARN_AFTER?

[Sapphire64]

I'm not sure, but it looks like it will not show notification warning. Only front end whistle. But I realised another possible problem. UI will redirect user to login page after system's default logout value. If user's logout value is bigger this redirect will be understood on middleware level as user activity during active session. It means larger values will never work and logout will never occur.

[yscumc]

session_security_tags.py will need to be updated as well for the front end. It still doesn't solve the problem if the user's EXPIRE_AFTER is smaller than the global WARN_AFTER though. Haven't tested this, but I think Sapphire64's prediction sounds right

[VuongN]

If it's a pre-existing issue, then I would prefer to push this PR through first and then have a follow-up PR to address that issue specifically. Does that sound fair, @yscumc & @Sapphire64?

[yscumc]

Well I wouldn't call it a pre-existing issue because normally, the EXPIRE_AFTER and WARN_AFTER settings are statically configured by the administrator and if he/she isn't ignorant, this wouldn't be a problem.

The problem is when the EXPIRE_AFTER is dynamically overridden at run time on a per-session basis. I'm assuming that this feature would be used to allow a user to set his/her own session time-out. This puts the burden on the programmer to ensure that the session EXPIRE_AFTER value is never less than the global WARN_AFTER value. At the minimum, this should be be documented.

Regardless of the above issue, I think that session_security_tags.py still needs to be updated in order to take into account the updated session-level EXPIRE_AFTER on the client side.

[VuongN]

@yscumc: You're right, session_security_tags.py will need to be updated. For now, it's programmer's responsibility to ensure that user can't go below the WARN_AFTER value. As I don't have a lot of time to spare and detest scope creeping, I'll make the refactoring of EXPIRE_AFTER & WARN_AFTER as a follow up PR. For now, I'll just update session_security_tags.py and functional test.

[Sapphire64]

isinstance, can it be float? Should we log somewhere that value is not valid?

[VuongN]

I'll make this change to use isinstance for int checking. I think fraction of second is an edge case at this point.

[Sapphire64]

I've made two branches:
https://github.com/Sapphire64/django-session-security/commits/master - clean up of current PR
https://github.com/Sapphire64/django-session-security/commits/development - refactored middleware to be easy to customize. I found some middleware logic to be duplicating and overcomplicated.

I'm going to test my branches now.

[VuongN]

@Sapphire64: I made the change to the int checking to use isinstance instead of type. I purposefully did not make the other changes you made because:

1. I did not want to make changes beyond the my intended scope
2. circular import issues
3. I don't agree with your thoughts on subclassing in this case
4. I mentioned in my PEP8 commit that I did not want to remove unused imports because @jpic might be using them for something else that I don't see. Can't be willy nilly removing imports.

Last but not least, none of your changes in both branches actually pass tests (you're running into circular import problems). Try running tests on your local changes and of course you can always make a PR with your changes.

[VuongN]

@jpic: added test to middleware as you suggested. I think we should try to speed it up by lowering the setting values but perhaps it's for a follow up PR.

To summarize my changes, I simply added a way for people to set an EXPIRE_AFTER per user session.

Developers can simply hook to user_logged_in signal and set a value in request.session[settings.SESSION_SECURITY_CUSTOM_SESSION_KEY] and our middleware layer will respect this.

In supporting this use-case, I've also added settings.SESSION_SECURITY_WARN_BEFORE, which is time (in seconds) counting back from session expiration when user is warned that session will expire because of inactivity. This allow us to easily set a fixed value to when warning dialog should appears. settings.WARN_AFTER value can be calculated from this value.

I've attempted to only make changes where necessary. There are a couple more improvements I would suggest, but I'll keep those for separate future PRs.

Just an aside, while running flake8, I've found that there are a couple of unused imports, I'm not sure if they could be safely removed. I've brought the entire package to be in compliant with PEP8 specs, but I'll leave the unused imports for you :)

[Sapphire64]

@VuongN thanks for checking, I published changes faster for you to check first. Do you think you should keep "business" logic inside of settings?

[jpic]

Excellent work guys !!

I submitted a fix for the circular import in @Sapphire64's master branch, but it didn't do the trick.

All Python improvements are welcome in our codebase, I learnt great things by reviewing your code live type() is evil when not used to actually create a type. Keep up the great work ! The work you guys did on the refactor and documentation is awesome as well !

Using the user_logged_in signal is really elegant BTW, although settings.py might not be the best place for business logic.

It's funny how the WARN_BEFORE came back to life after 2 years: 130da20 It is gone because it made the codebase more complex and was not required, although it was intuitive.

That said, here are my conclusions in terms of requirements:

• we'll have to ensure coherent values are provided by the developer to support dynamic WARN_AFTER/EXPIRE_AFTER,
• django-session-security needs a place it's logic to be easy to override,

Maybe it would all be easier if we moved all the logic in the middleware, then we could provide other middlewares and even mixins like SessionSecurityWarnBeforeMixin, SessionSecuritySessionSettingsMixin and so on.

Then you could choose the logic you want by choosing the midldeware you add to settings.MIDDLEWARE_CLASSES. This would probably also permit a cool refactor of the codebase and tests. We could have as many session_security.contrib.* submodules with different middlewares or complete apps (ie. forms for WARN_AFTER/EXPIRE_AFTER ...).

Sorry for the late reply, looking forward to read your feedback !

[Sapphire64]

@jpic Great to hear from you, James :) thanks for checking my branch. Last time I was thinking that to make it good we need to follow some of the django's practices.

We may create interface with probably few base methods: is_session_expired, update_activity. By implementing your own classes or by including standard .contrib. classes (like request.session code from @VuongN ) you will be able to parse request through different layers of session_security checks, like django does with middlewares.

To include them we can add to options something like this:

SESSION_SECURITY_HANDLERS = ['session_security.contrib.default', 'session_security.contrib.session_based', 'your.own.very.unique.one'] 

By reading this options our code in def is_session_expired(self, request, current_time, last_activity_time): will loop through handlers and if any of them will return True we will exit and log user out.

I've just imagined that by doing like this developers can attach all kinds of logic here: like if user didn't paid for subscription he will be logged out. But for this we need to think in advance about such things like custom warning templates and custom logout templates. We can attach them to that interface as a fields with value like expire_template_url='', warning_template_url=''. All ping requests should be provided to handler's implemented methods to make their own updates.

Many of the things, requires lot of work, but I think that's the way to make this project real shine :) What do you think?

[jpic]

Hi all,

I'm really sorry but I merged the PR by mistake (actually I merged it locally a while ago, and started working on a maintainance release locally yesterday ...) and I had to revert the merge commit. Bummer ... I got the maintenance release out but the PR is definitely closed due to my mistake (merged locally then reverted merge commit). To be clear, it's not because your contributions were refused.

As a matter of fact, all code improvements are welcome ! But it's better if they are submitted in one PR per topic. I will eventuelly get to cherry-pick code improvement commits but I can't right now: django-cities-light (which has a badass mysql bug to figure out) and django-autocomplete-light (which has 43 open issues and 12 prs ....) also need a maintenance release.

Basically if you want to make it really easy to merge some of your changes immediatly and other later, it's better to make one PR per topic (why not try "feature branches" ?).

Back to your use cases: django-session-security really needs an official hooking method, I'm thinking about #9 and #29 which also represent different use cases we have no reason not to support, but were not thought of during design of django-session-security. It's what I meant in my previous post:

• django-session-security has enough features just to "secure the session of a django project" although it's still far from perfect (see Session incorrectly times out early if there is a long running request #1), it has already been re-thought over and over again,
• django-session-security should be more flexible and support various use cases, short lived CSRF protection on the login page: What to do? #29 also sounds really challenging but I think both use cases could work through the same "hook" interface (hopefully signals will do the trick, gotta spend more time to figure out the common factors).

Very important note for new features: it would be great to open an issue first and discuss implementation details before diving into actual code implementations.

Again, I apologize for closing this PR by inadvertance and for my lack of time in general to find out a satisfying solution for all those cool use cases.

Sincerely

James

[jpic]

@Sapphire64 Overall I agree with your last post, however:

• we'd have to really make sure that we can't rely on custom django signals before implementing our own handler system
• I don't think it's a lot of work in terms of code, but summarizing requirements and making the design document will take some time (heck I remember it took a while to make the one for django-session-security 2.0 even though there is not much code !)
• there is a lot of work for requirements, design and usage documentation indeed ;)

I learnt from this dev process and I think we would need at least need a requirements and a design document for django-session-security 3.0.

Any help will be appreciated ! Please try to keep topics as much isolated from each other as possible ;)

[jpic]

I've summarized the guidelines we should follow to avoid such a debacle (partly caused by me, sorry !): http://docs.yourlabs.org/

It's hosted on rtfd/github so feel free to request changes.

Also, if we're going to start all over for this PR, we should follow these guidelines to make it easier.

Thanks B)

[Sapphire64]

@jpic hello there. So how can I help you now? Create new bug and start discussion? So you say that the kind of attachment I proposed is covered basically by django-signals?

[jpic]

Hello @Sapphire64 , a discussion in a separate issue would be great, we've got to define the exact requirements for the new kind of attachement, which we could call something like "Plugin API" or something technical like that ;)