Fix EZP-26179: REST session refresh must throw a 404 if session has expired #1769

bdunogier · 2016-09-07T15:35:14Z

Relates to EZP-26179 / ezsystems/platform-ui-bundle#668
Implements EZP-26720 (use session auth in REST functional tests)
Fixes EZP-25038 (error when logging in with REST if a session was started before)

According to the specifications, /api/ezp/v2/user/sessions/<id>/refresh must throw a 404 if the session doesn't exist / has expired.

Because of the CSRF token check in the CsrfListener, a 401 is thrown because the csrf token verification fails, as the session has expired, and we don't have a stored token anymore.

This changes the CsrfListener to skip this route, and explicitly checks in the refresh controller action if the token has expired using the Csrf TokenStorage.

andrerom · 2016-09-08T06:02:53Z

Some questions:

what is going on in catch (SessionUnavailableException $e) => it seems RestNotFoundException is not thrown anymore => and is logout safe not matter what? (session id change, session deleted, ..)
is there any possibility of get login to reuse the new abstract methods?
we also talked about doing similar things to DELETE, still on the agenda or not relevant?

:)

dpobel · 2016-09-08T09:18:23Z

it seems to work :)

we also talked about doing similar things to DELETE, still on the agenda or not relevant?

As far as I can tell, that's not strictly needed to fix the original issue but IMHO, it would make sense to have the same behavior when deleting the session through REST.

dpobel · 2016-09-08T09:19:20Z

eZ/Bundle/EzPublishRestBundle/EventListener/CsrfListener.php

@@ -136,7 +136,8 @@ protected function isMethodSafe($method)
     */
    protected function isLoginRequest($route)


now it does more than just checking if we try to login the user

Deprecated in favour of isSessionRoute($route)

dpobel · 2016-09-08T09:22:07Z

in addition to the previous comments, it would also be nice to complete https://github.com/ezsystems/ezpublish-kernel/blob/master/doc/specifications/rest/REST-API-V2.rst#refresh-session-get-sessions-user-information (and potentially the DELETE part as well) to precise that the session cookie is also deleted in error cases.

bdunogier · 2016-09-08T14:45:47Z

we also talked about doing similar things to DELETE, still on the agenda or not relevant?

Changed to use the same behaviour (ignored in CsrfListener, check if we have a token in the controller).

what is going on in catch (SessionUnavailableException $e) => it seems RestNotFoundException is not thrown anymore => and is logout safe not matter what? (session id change, session deleted, ..)

Yes. I've decided to take a shortcut and return a response here. RestNotFoundException implies that visitors are able to do that. The DeletedUserSession one does it by passing the Response object returned by logout(), and have the visitor copy the headers. I found it a bit heavy, and not that useful, since we don't even have a response body, and chose to return a response directly, since we have one.

bdunogier · 2016-09-08T14:51:02Z

But now I'm thinking that the csrf check in refresh and delete session are a bit wrong: the actual value of the csrf token won't be checked, since we only use the TokenStorage to check if there is a token.

I guess that we should first check if there is a token, and if there isn't, throw a 404. Then if we have a token, check that the token that was provided as input is correct.

bdunogier · 2016-09-08T15:23:08Z

Added csrf-token value check in both refresh and delete session. Also reorganized the code a little bit.

andrerom · 2016-09-08T15:39:22Z

eZ/Bundle/EzPublishRestBundle/EventListener/CsrfListener.php

     */
    protected function isLoginRequest($route)
    {
-        return $route === 'ezpublish_rest_createSession';
+        return $route === 'ezpublish_rest_createSession' ||
+               $route === 'ezpublish_rest_refreshSession';


given it is deprecated and not used this change should be removed I guess.

Good point :-)

bdunogier · 2016-09-08T17:01:40Z

Updated createSession() to use the newly added methods, improves readability a lot.

Will consider fixing the case where a session exists without a csrf token fixed (reported in JIRA), preventing from logging into PlatformUI if a frontend session was previously created.

andrerom · 2016-09-12T09:34:32Z

Will consider fixing the case where a session exists without a csrf token fixed (reported in JIRA), preventing from logging into PlatformUI if a frontend session was previously created.

Did you think about how we can do that?
Also note that issue was reported on 1.3.

Otherwise PR looks good :)

bdunogier · 2016-09-12T12:34:12Z

@andrerom : see d0f758b :)

Do we need to fix it in 1.3 ?

bdunogier · 2016-09-12T13:25:22Z

We should be good for review, everything's in.

andrerom · 2016-09-13T06:37:33Z

eZ/Bundle/EzPublishRestBundle/Tests/Functional/RoleTest.php

@@ -490,7 +490,7 @@ public function testUnassignRoleFromUser($roleAssignmentHref)
     *
     * @return string role assignment href
     */
-    public function testAssignRoleToUserGroup($roleHref)
+    public function _testAssignRoleToUserGroup($roleHref)


temp change?

I need to rephrase that a bit. This particular test breaks roles in the database, and prevents further tests from working.

Marked the test as skipped with the reason.

andrerom · 2016-09-13T06:39:31Z

Besides comment, +1

Do we need to fix it in 1.3 ?

Not a must, stay with current target.

bdunogier · 2016-09-13T15:37:51Z

@pcardiga : this is ready for QA. There are several fixes / changes in there. They must be tested on 1.4.x. A new version will be issued for master:

EZP-25038

If a session was started from the frontend (just use /#, it is impossible to log into PlatformUI without clearing the session cookie first. When it happens, REST will respond with a 401 because of a missing csrf token.

EZP-26179

This is the main issue fixed by this PR. It changes the behaviour of both refresh and delete session.

From a REST perspective, if either is used while a session has expired, they will now throw a 404, and delete the session cookie, instead of a 401 because of a missing csrf token.

From the UI, without this patch, if the session expires while on PlatformUI, it will be impossible to login again to the UI without deleting the session cookie. This can be tested together with ezsystems/platform-ui-bundle#668 on PlatformUI. Set sessions to expire quickly (session.gc_divisor = 1 + session.gc_maxlifetime = 10 will get sessions to expire after 10 seconds, and be garbage collected right away). Wait for the session to expire while logged into PlatformUI, and click on anything. Without the patches, you will get an error, and you will be unable to login again. With the patch, you will be redirected to the login screen, and be able to login again.

pcardiga · 2016-09-13T15:54:23Z

@bdunogier:
@miguelcleverti, on behalf of QA, is on it.
Meamwhile, following the new procedures, QA should also have a jira ticket in his kanban column and I see that both EZP-25038 and EZP-26179 are on development column. Can the main related issue be moved to QA?

miguelcleverti · 2016-09-14T15:08:39Z

@bdunogier

I was not able to reproduce the issue EZP-25038 only using the /# route but using the EventHandler provided in the jira issue I reproduced and tested it. Everything ok.

The issue EZP-26179 was tested using the ezsystems/platform-ui-bundle#668 as well and everything was ok. The only issue I had was not being able to make the garbage collector work after 10s, so I manually deleted the sessions. Doing this the response status return was a 404 instead of 401 and the PlatformUI redirected to the login.

These issues were tested on 1.4.x

bdunogier · 2016-09-15T14:39:17Z

I was not able to reproduce the issue EZP-25038 only using the /# route

Odd, this is how I trigger this case in the REST functional tests.

…n has expired According to the specifications, /api/ezp/v2/user/sessions/<id>/refresh must throw a 404 if the session doesn't exist / has expired. Because of the CSRF token check in the CsrfListener, a 401 is thrown because the csrf token verification fails, as the session has expired, and we don't have a stored token anymore. This changes the CsrfListener to skip this route, and explicitly checks in the refresh controller action if the token has expired.

bdunogier · 2016-09-16T10:27:21Z

The error is a 1.4 related one. Composer didn't install the tested branch, but 5.4.2-rc1. This means that platform wasn't set to basic auth, and tests try to run with basic auth, hence the failure on everything that requires permissions other than anonymous.

Tested locally, with a 1.4 install frm scratch and the PR's branch, and it works as expected.

Change needed after ezsystems/ezpublish-kernel#1769

Change needed after ezsystems/ezpublish-kernel#1769 Conflicts: .travis.yml

bdunogier force-pushed the ezp26179-rest_session_refresh_404-6.4 branch 2 times, most recently from d619cf8 to 1888fcd Compare September 7, 2016 15:59

dpobel reviewed Sep 8, 2016
View reviewed changes

bdunogier force-pushed the ezp26179-rest_session_refresh_404-6.4 branch 3 times, most recently from 19cb458 to f00ed9d Compare September 8, 2016 14:34

andrerom reviewed Sep 8, 2016
View reviewed changes

bdunogier force-pushed the ezp26179-rest_session_refresh_404-6.4 branch 2 times, most recently from 1fdd788 to 72494a4 Compare September 10, 2016 08:36

bdunogier force-pushed the ezp26179-rest_session_refresh_404-6.4 branch from b20337e to d0f758b Compare September 12, 2016 12:31

bdunogier force-pushed the ezp26179-rest_session_refresh_404-6.4 branch from d0f758b to 9206e22 Compare September 12, 2016 13:24

bdunogier added the Ready for review label Sep 12, 2016

andrerom reviewed Sep 13, 2016
View reviewed changes

bdunogier force-pushed the ezp26179-rest_session_refresh_404-6.4 branch from 9206e22 to 6e6e432 Compare September 13, 2016 15:15

bdunogier added Ready for QA and removed Ready for review labels Sep 13, 2016

bdunogier force-pushed the ezp26179-rest_session_refresh_404-6.4 branch from 6e6e432 to 4065815 Compare September 13, 2016 16:30

dpobel mentioned this pull request Sep 15, 2016

EZP-26179: UI Stability issues due to session loss ezsystems/PlatformUIBundle#668

Merged

3 tasks

Bertrand Dunogier added 4 commits September 16, 2016 11:14

EZP-26270: Use session auth in REST functional tests

84d20b5

EZP-26179: Added REST session functional coverage

e8bcfe8

Fix EZP-25038: error logging in with REST if session started earlier

01865fa

bdunogier force-pushed the ezp26179-rest_session_refresh_404-6.4 branch from 4065815 to 01865fa Compare September 16, 2016 09:14

bdunogier merged commit 37e75da into 6.4 Sep 16, 2016

bdunogier deleted the ezp26179-rest_session_refresh_404-6.4 branch September 16, 2016 10:32

bdunogier mentioned this pull request Sep 20, 2016

Refactored session rest actions to their own controller (EZP-26329) #1782

Merged

andrerom added a commit to ezsystems/ezplatform that referenced this pull request Sep 22, 2016

[Travis] Remove use of basic auth in REST functional tests

0dff2e3

Change needed after ezsystems/ezpublish-kernel#1769

andrerom added a commit to ezsystems/ezplatform that referenced this pull request Sep 27, 2016

[Travis] Remove use of basic auth in REST functional tests

6d39a15

Change needed after ezsystems/ezpublish-kernel#1769 Conflicts: .travis.yml

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix EZP-26179: REST session refresh must throw a 404 if session has expired #1769

Fix EZP-26179: REST session refresh must throw a 404 if session has expired #1769

bdunogier commented Sep 7, 2016 •

edited

Loading

andrerom commented Sep 8, 2016 •

edited

Loading

dpobel commented Sep 8, 2016

dpobel Sep 8, 2016

bdunogier Sep 8, 2016

dpobel commented Sep 8, 2016

bdunogier commented Sep 8, 2016 •

edited

Loading

bdunogier commented Sep 8, 2016

bdunogier commented Sep 8, 2016

andrerom Sep 8, 2016

bdunogier Sep 8, 2016

bdunogier commented Sep 8, 2016

andrerom commented Sep 12, 2016 •

edited

Loading

bdunogier commented Sep 12, 2016

bdunogier commented Sep 12, 2016

andrerom Sep 13, 2016

bdunogier Sep 13, 2016

bdunogier Sep 13, 2016

andrerom commented Sep 13, 2016 •

edited

Loading

bdunogier commented Sep 13, 2016 •

edited

Loading

pcardiga commented Sep 13, 2016

miguelcleverti commented Sep 14, 2016

bdunogier commented Sep 15, 2016

bdunogier commented Sep 16, 2016

		@@ -136,7 +136,8 @@ protected function isMethodSafe($method)
		*/
		protected function isLoginRequest($route)

Fix EZP-26179: REST session refresh must throw a 404 if session has expired #1769

Fix EZP-26179: REST session refresh must throw a 404 if session has expired #1769

Conversation

bdunogier commented Sep 7, 2016 • edited Loading

andrerom commented Sep 8, 2016 • edited Loading

dpobel commented Sep 8, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dpobel commented Sep 8, 2016

bdunogier commented Sep 8, 2016 • edited Loading

bdunogier commented Sep 8, 2016

bdunogier commented Sep 8, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bdunogier commented Sep 8, 2016

andrerom commented Sep 12, 2016 • edited Loading

bdunogier commented Sep 12, 2016

bdunogier commented Sep 12, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

andrerom commented Sep 13, 2016 • edited Loading

bdunogier commented Sep 13, 2016 • edited Loading

pcardiga commented Sep 13, 2016

miguelcleverti commented Sep 14, 2016

bdunogier commented Sep 15, 2016

bdunogier commented Sep 16, 2016

bdunogier commented Sep 7, 2016 •

edited

Loading

andrerom commented Sep 8, 2016 •

edited

Loading

bdunogier commented Sep 8, 2016 •

edited

Loading

andrerom commented Sep 12, 2016 •

edited

Loading

andrerom commented Sep 13, 2016 •

edited

Loading

bdunogier commented Sep 13, 2016 •

edited

Loading