Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Feature request: no reload #94

Open
wants to merge 4 commits into
base: master
Choose a base branch
from
Open

Feature request: no reload #94

wants to merge 4 commits into from

Conversation

mattbo
Copy link

@mattbo mattbo commented Apr 6, 2017

Several of the pages on the site that I run take a long time to load or several steps to configure. As a result, having them jump to a login page after expiration is frustrating. This feature adds a noReload property to the sessionSecurity object that expires the session without reloading the page.

I also added a comment in the all.html file to indicate how it works, and a test in tests/test_script.py.

Cheers,
Matt

mattbo and others added 3 commits October 24, 2014 12:56
…) to

urlresolvers.reverse_lazy so that the PASSIVE_URLS array will include the
correct path
@jpic
Copy link
Member

jpic commented Apr 7, 2017

Ok I understand your problem, I've not used this app for a long time tbh but I recon supporting this kind of pages is more than reasonable.

It's a lot of selenium, would a unit test in JS be better for this patch ? After all, it doesn't interact with the server at all.

Also, could you add a warning about the security tradeoff in the comment ? Or find a way to encrypt private data, ie. by adding a data-dss-encrypt to encrypt form values or other sensible data, and decrypt it when the users logs back in using the ping mechanism ?

But I don't want to prevent this from being integrated as early as possible so you can get feedback, but don't bother adding a selenium tests for this when the others don't even pass anymore (guess who's gonna chip in another 12 hours dealing with travis when the tests probably already are passing on any decent non-free machine)

@@ -15,6 +15,9 @@ if (window.yourlabs == undefined) window.yourlabs = {};
// onbeforeunload handler that doesn't block expire().
// - events: a list of event types to watch for activity updates.
// - returnToUrl: a url to redirect users to expired sessions to. If this is not defined we just reload the page
// - noReload: If this is defined then we expire the session without reloading
// the page. Useful if the page should stay visible but no further actions
// should be taken.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't understand the last sentence, what do you mean stay visible ?

Isn't there a way to at least encrypt the html until the user logs in again for example rather than taking no further action ?

@mattbo
Copy link
Author

mattbo commented Apr 7, 2017

I don't see any javascript unit tests. Am I missing something?

I can definitely add something about the security tradeoff.

Not sure about encrypting anything -- is the idea to scramble form inputs? To be honest, my use case involves pages that are results of big DB queries; lots of data, no form inputs. In the case when there are form inputs you probably wouldn't want this setting -- submitting the form would send you back to the login page and unless you were very careful you'd lose all your form contents!

What do you think?

@claytondaley
Copy link
Contributor

I don't think there's a singular answer here. It comes down to a broader question of "what are you trying to protect".

  • In healthcare (my immediate concern), we're protecting the information itself. We don't distinguish between data generated by the system and data being entered by an individual. It's all health information needing protected.
  • In other situations, it may be the server session that is of greater concern. For example, your bank may not care that your balance is still displayed on the screen. Rather, they're focused on preventing a user from transferring money out of that account.

There are probably intermediate cases too. For example, a password field should probably be reset in the second case. Similarly, it might make sense to flag some form fields (e.g. SSN) as "protected".

@mattbo
Copy link
Author

mattbo commented Apr 11, 2017

Makes sense, I see what you're saying. So how do we proceed?

We could add in a hook; a function that defaults to None, but can be used to encrypt data either before or after logout in a custom way.

We could clear any input fields present on the page.

My (admittedly simplistic) site has a separate page for login, it's not AJAX. I think that means any encrypted data would need to be recorded on the server and then sent back after login. Which seems quite a bit more complicated than the feature I was hoping to add.

Am I missing something?

@claytondaley
Copy link
Contributor

JS-wise, my inclination (only that -- I'm not in charge) is to move the page redirect into a dedicated "timeout" function. If you don't want to redirect, you just assign an empty function to that name, roughly:

var sessionSecurity = new yourlabs.SessionSecurity(...);
sessionSecurity.timeout = function() {}
// or even
sessionSecurity.timeout = sessionSecurity.noAction

This last line would provide a pattern inviting the contribution of other strategies like a form sanitizer.

@claytondaley
Copy link
Contributor

... oops that name is currently used, but you get the idea.

@jpic
Copy link
Member

jpic commented Apr 11, 2017 via email

@mattbo
Copy link
Author

mattbo commented Apr 11, 2017

I can definitely get behind the dedicated timeout function. It defaults to reloading the page (which is how the code works currently) and can be easily overridden to do nothing (or something else).

I like the trigger idea as well. There would have to be a default listener associated with the event (reload the page) and the user would have to remove/dissociate the listener and/or add their own.

@jpic do you have an opinion here?

@jpic
Copy link
Member

jpic commented Apr 11, 2017

A default callback in the session_security/script.js script looks a bit like a pain to then disable "don't fight the controls".

But, it can be coded in the template, would that work ?

Ie. after instanciating a SessionSecurity, connect a listenner.

Perhaps it can be in a separate template, ie. templates/session_security/callback.js, that we could include in all.html after instanciating the SessionSecurity instance ?

@claytondaley
Copy link
Contributor

In principle, the two strategies can be combined:

  • a "timeout" function that triggers an event
  • a default event that redirects the page

It feels like overkill, but I feel like it lowers the barriers to usage:

  • a "basic" user can drop-in replace the timeout function (i.e. the event system)
  • an "advanced" user can mess with subscriptions

I use the scarce quotes because the basic strategy appeals to me -- I certainly could use an event system if the situation warranted.

@jpic
Copy link
Member

jpic commented Apr 11, 2017 via email

@mattbo
Copy link
Author

mattbo commented Apr 11, 2017

Looking at the code, there's already a callback. Effectively we could skip my patch entirely and I could simply override yourlabs.SessionSecurity.prototype.expire()

So -- close the pull request?

@jpic
Copy link
Member

jpic commented Apr 12, 2017

Otherwise perhaps change it to a documentation patch ?

@jpic
Copy link
Member

jpic commented Apr 25, 2017

The PR is reasonable for now, a new, disabled-by-default setting named after the effect on security such as protectData (thanks @claytondaley for the good thinking !).

@mattbo does that seem reasonable for you ?

If no other use case comes up then we won't need a callback at all or anything more complicated.

Thanks a heap for your feedback !

@claytondaley
Copy link
Contributor

I just ran into a similar case and replacing .expire() certainly worked. However, I do feel it would be better if we had a settings entry (plus view-level overrides), especially for usability.

The current PR requires a JS-level override so it's not significantly easier than patching expire(). If someone wants to extend this PR with a settings entry that is passed to noReload, I'd be +1 to merging it. Bonus points if they implement (or can provide a simple workaround for) view-level overrides of that setting.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants