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

Allow to configure auto logout after browser inactivity #20298

Merged
merged 6 commits into from
Apr 24, 2020

Conversation

juliushaertl
Copy link
Member

With this a a browser will automatically log out the user after no activity during session_lifetime even if the session_keepalive is enabled.

Copy link
Member

@skjnldsv skjnldsv left a comment

Choose a reason for hiding this comment

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

Clever approach! Code looks great!

@rullzer rullzer mentioned this pull request Apr 4, 2020
80 tasks
@rullzer
Copy link
Member

rullzer commented Apr 6, 2020

/compile amend /

@skjnldsv skjnldsv added 4. to release Ready to be released and/or waiting for tests to finish 2. developing Work in progress and removed 3. to review Waiting for reviews labels Apr 7, 2020
@skjnldsv
Copy link
Member

skjnldsv commented Apr 7, 2020

Jsunit says no

"message": "Error: Could not find initial state config of core\nat core/js/dist/main.js:279:1669",
--
997 | "str": "Error: Could not find initial state config of core\nat core/js/dist/main.js:279:1669"
998 | }
999 | PhantomJS 2.1.1 (Linux 0.0.0): Executed 0 of 0 ERROR (0.225 secs / 0 secs)
1000 | PhantomJS 2.1.1 (Linux 0.0.0) ERROR
1001 | {
1002 | "message": "ReferenceError: Can't find variable: OC\nat core/js/tests/specHelper.js:90:3",
1003 | "str": "ReferenceError: Can't find variable: OC\nat core/js/tests/specHelper.js:90:3"
1004 | }
1005 | PhantomJS 2.1.1 (Linux 0.0.0): Executed 0 of 0 ERROR (0.227 secs / 0 secs)
1006 | PhantomJS 2.1.1 (Linux 0.0.0) ERROR
1007 | {
1008 | "message": "ReferenceError: Can't find variable: $\nat core/js/public/publicpage.js:21:3247",
1009 | "str": "ReferenceError: Can't find variable: $\nat core/js/public/publicpage.js:21:3247"
1010 | }
1011 | PhantomJS 2.1.1 (Linux 0.0.0): Executed 0 of 0 ERROR (0.228 secs / 0 secs)
1012 | PhantomJS 2.1.1 (Linux 0.0.0) ERROR
1013 | {
1014 | "message": "ReferenceError: Can't find variable: OC\nat core/js/setupchecks.js:1:31696",
1015 | "str": "ReferenceError: Can't find variable: OC\nat core/js/setupchecks.js:1:31696"
1016 | }
1017 | PhantomJS 2.1.1 (Linux 0.0.0): Executed 0 of 0 ERROR (0.228 secs / 0 secs)
1018 | PhantomJS 2.1.1 (Linux 0.0.0) ERROR
1019 | {
1020 | "message": "ReferenceError: Can't find variable: OCA\nat core/search/js/search.js:21:15042",
1021 | "str": "ReferenceError: Can't find variable: OCA\nat core/search/js/search.js:21:15042"
1022 | }
1023 | PhantomJS 2.1.1 (Linux 0.0.0): Executed 0 of 0 ERROR (0.229 secs / 0 secs)
1024 | PhantomJS 2.1.1 (Linux 0.0.0) ERROR
1025 | {
1026 | "message": "ReferenceError: Can't find variable: OC\nat core/js/mimetype.js:19:8990",
1027 | "str": "ReferenceError: Can't find variable: OC\nat core/js/mimetype.js:19:8990"
1028 | }
1029 | PhantomJS 2.1.1 (Linux 0.0.0): Executed 0 of 0 ERROR (0.229 secs / 0 secs)
1030 | PhantomJS 2.1.1 (Linux 0.0.0) ERROR
1031 | {
1032 | "message": "ReferenceError: Can't find variable: OC\nat core/js/mimetypelist.js:1:646",
1033 | "str": "ReferenceError: Can't find variable: OC\nat core/js/mimetypelist.js:1:646"
1034 | }
1035 | PhantomJS 2.1.1 (Linux 0.0.0): Executed 0 of 0 ERROR (0.23 secs / 0 secs)
1036 | PhantomJS 2.1.1 (Linux 0.0.0) ERROR
1037 | {
1038 | "message": "ReferenceError: Can't find variable: OC\nat core/js/files/fileinfo.js:1:5480",
1039 | "str": "ReferenceError: Can't find variable: OC\nat core/js/files/fileinfo.js:1:5480"
1040 | }
1041 | PhantomJS 2.1.1 (Linux 0.0.0): Executed 0 of 0 ERROR (0.23 secs / 0 secs)
1042 | PhantomJS 2.1.1 (Linux 0.0.0) ERROR
1043 | {
1044 | "message": "ReferenceError: Can't find variable: OC\nat core/js/files/client.js:1:72403",
1045 | "str": "ReferenceError: Can't find variable: OC\nat core/js/files/client.js:1:72403"
1046 | }
1047 | PhantomJS 2.1.1 (Linux 0.0.0): Executed 0 of 0 ERROR (0.231 secs / 0 secs)
1048 | PhantomJS 2.1.1 (Linux 0.0.0) ERROR
1049 | {
1050 | "message": "ReferenceError: Can't find variable: OC\nat core/js/systemtags/systemtags.js:1:4240",
1051 | "str": "ReferenceError: Can't find variable: OC\nat core/js/systemtags/systemtags.js:1:4240"
1052 | }
1053 | PhantomJS 2.1.1 (Linux 0.0.0): Executed 0 of 0 ERROR (0.231 secs / 0 secs)
1054 | PhantomJS 2.1.1 (Linux 0.0.0) ERROR
1055 | {
1056 | "message": "ReferenceError: Can't find variable: Handlebars\nat core/js/systemtags/templates.js:1:29032",
1057 | "str": "ReferenceError: Can't find variable: Handlebars\nat core/js/systemtags/templates.js:1:29032"
1058 | }
1059 | PhantomJS 2.1.1 (Linux 0.0.0): Executed 0 of 0 ERROR (0.231 secs / 0 secs)
1060 | PhantomJS 2.1.1 (Linux 0.0.0) ERROR
1061 | {
1062 | "message": "ReferenceError: Can't find variable: OC\nat core/js/systemtags/systemtagmodel.js:1:3585",
1063 | "str": "ReferenceError: Can't find variable: OC\nat core/js/systemtags/systemtagmodel.js:1:3585"
1064 | }
1065 | PhantomJS 2.1.1 (Linux 0.0.0): Executed 0 of 0 ERROR (0.233 secs / 0 secs)
1066 | PhantomJS 2.1.1 (Linux 0.0.0) ERROR
1067 | {
1068 | "message": "ReferenceError: Can't find variable: OC\nat core/js/systemtags/systemtagscollection.js:1:6710",
1069 | "str": "ReferenceError: Can't find variable: OC\nat core/js/systemtags/systemtagscollection.js:1:6710"
1070 | }
1071 | PhantomJS 2.1.1 (Linux 0.0.0): Executed 0 of 0 ERROR (0.233 secs / 0 secs)
1072 | PhantomJS 2.1.1 (Linux 0.0.0) ERROR
1073 | {
1074 | "message": "ReferenceError: Can't find variable: OC\nat core/js/systemtags/systemtagsmappingcollection.js:1:4799",
1075 | "str": "ReferenceError: Can't find variable: OC\nat core/js/systemtags/systemtagsmappingcollection.js:1:4799"
1076 | }
1077 | PhantomJS 2.1.1 (Linux 0.0.0): Executed 0 of 0 ERROR (0.234 secs / 0 secs)
1078 | PhantomJS 2.1.1 (Linux 0.0.0) ERROR
1079 | {
1080 | "message": "ReferenceError: Can't find variable: OC\nat core/js/systemtags/systemtagsinputfield.js:1:35435",
1081 | "str": "ReferenceError: Can't find variable: OC\nat core/js/systemtags/systemtagsinputfield.js:1:35435"
1082 | }
1083 | PhantomJS 2.1.1 (Linux 0.0.0): Executed 0 of 0 ERROR (0.234 secs / 0 secs)

@juliushaertl
Copy link
Member Author

@ChristophWurst @skjnldsv Any idea how i could properly mock the loadState call in the legacy js tests?

Otherwise I'd just add a fallback to OC.config in https://github.com/nextcloud/server/pull/20298/files#diff-ec0ce8cb7acc57624086f5f4b6a47998R30 as a quick fix 🙈

@skjnldsv
Copy link
Member

skjnldsv commented Apr 9, 2020

@ChristophWurst @skjnldsv Any idea how i could properly mock the loadState call in the legacy js tests?

oh wow 🙈
Do the quick fix I guess, Is it later fixable by cleaning?

@rullzer rullzer mentioned this pull request Apr 9, 2020
59 tasks
@ChristophWurst
Copy link
Member

@ChristophWurst @skjnldsv Any idea how i could properly mock the loadState call in the legacy js tests?

I'm afraid you can't 😢

@skjnldsv skjnldsv removed the 4. to release Ready to be released and/or waiting for tests to finish label Apr 11, 2020
@skjnldsv
Copy link
Member

I guess this will be for 20 ?

This was referenced Apr 15, 2020
*
* Defaults to ``false``
*/
'auto_logout' => false,
Copy link
Member

Choose a reason for hiding this comment

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

We should probably make sure that if this is set to true we don't generate a remember me cookie

Copy link
Member Author

Choose a reason for hiding this comment

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

6052f82 should to the trick, please have a look

@juliushaertl juliushaertl force-pushed the enh/noid/auto-logout branch 2 times, most recently from efc016c to 6052f82 Compare April 21, 2020 08:56
Copy link
Member

@ChristophWurst ChristophWurst left a comment

Choose a reason for hiding this comment

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

Looks good otherwise!

core/src/session-heartbeat.js Show resolved Hide resolved
@rullzer rullzer mentioned this pull request Apr 23, 2020
11 tasks
Signed-off-by: Julius Härtl <jus@bitgrid.net>
Signed-off-by: Julius Härtl <jus@bitgrid.net>
Signed-off-by: Julius Härtl <jus@bitgrid.net>
Signed-off-by: Julius Härtl <jus@bitgrid.net>
@rullzer
Copy link
Member

rullzer commented Apr 23, 2020

Guess this is good to go
mind to rebase @juliushaertl then we can do the merge :)

Signed-off-by: Julius Härtl <jus@bitgrid.net>
@juliushaertl
Copy link
Member Author

So this took me quite some time to figure out why the legacy jsunit tests failed. I pushed a workaround for now (see a143e36 for details), but ideally we should move those tests to the new chai/sinon tests. I'll give that a try in a follow up.

Signed-off-by: Julius Härtl <jus@bitgrid.net>
@skjnldsv
Copy link
Member

I pushed a workaround for now

It passed

@blizzz blizzz merged commit d458fcd into master Apr 24, 2020
@blizzz blizzz deleted the enh/noid/auto-logout branch April 24, 2020 11:20
@skjnldsv
Copy link
Member

So, this recently broke viewer's tests ^^

public function __construct(IL10N $l,
Defaults $defaults,
IAppManager $appManager,
ISession $session,
$currentUser,
IConfig $config,
IGroupManager $groupManager,
IniGetWrapper $iniWrapper,
IURLGenerator $urlGenerator,
CapabilitiesManager $capabilitiesManager,
IInitialStateService $initialStateService) {

Requires 11 arguments, while 10 are passed here:
$this->helper = new JSConfigHelper(
$l10nFactory->get('lib'),
$defaults,
$appManager,
$session,
$userSession->getUser(),
$config,
$groupManager,
$iniWrapper,
$urlGenerator,
$capabilitiesManager
);

@skjnldsv skjnldsv added 3. to review Waiting for reviews and removed 2. developing Work in progress labels Apr 27, 2020
@skjnldsv
Copy link
Member

Fixing

@skjnldsv
Copy link
Member

Fix in #20678

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

Successfully merging this pull request may close these issues.

None yet

5 participants