Skip to content

Conversation

@rullzer
Copy link
Member

@rullzer rullzer commented Jan 23, 2019

It is a bit more general. If the only argument is the requesttoken. So a
get request will work with the CSRF protection. The route will be
generated without the parameters and the request token will be added at
the end.

Before the route could not properly be cached as the request token was
always changing. However now it can be cached saving precious cpu
cycles.

Noticed while looking at #13712. While #13712 is a general improvement for route generation this fixes the non caching of this specific route.

@MorrisJobke
Copy link
Member

I really don't like that this is handled in there. I would handle it directly in the logout URL handling - just request the URL without any parameter and manually add it then. This reduces unnecessary complexity in the reused method generate() and makes it easier to understand, because no magic like this is handled.

@rullzer
Copy link
Member Author

rullzer commented Jan 23, 2019

Then somebody with JS magic skills needs to tackle it

@MorrisJobke
Copy link
Member

Then somebody with JS magic skills needs to tackle it

Why? It's PHP as well - I would just do it in here:

public static function getLogoutUrl(\OCP\IURLGenerator $urlGenerator) {
$backend = self::findFirstActiveUsedBackend();
if ($backend) {
return $backend->getLogoutUrl();
}
$logoutUrl = $urlGenerator->linkToRouteAbsolute(
'core.login.logout',
[
'requesttoken' => \OCP\Util::callRegister(),
]
);
return $logoutUrl;
}

😉

@rullzer
Copy link
Member Author

rullzer commented Jan 23, 2019

Aaaaah right ok makes sense. Let me do that then...

@rullzer rullzer force-pushed the enh/also_cache_logout_route branch from 10a5d38 to 84fefb3 Compare January 23, 2019 09:22
Copy link
Member

@nickvergessen nickvergessen left a comment

Choose a reason for hiding this comment

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

🙈

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.

👍 🐘 🙈

@MorrisJobke
Copy link
Member

Still failing:

--- Expected
+++ Actual
@@ @@
-        'href' => 'https://example.com/logout'
+        'href' => 'https://example.com/logout?requesttoken=Wq1opm+k0/NnQAydww7painntbJdUuM6gyTGbzqAOUk=:NccK4zbBgdxWcViksE2QK1+OgdksEYBW1XCVNXTZUR4='

By requesting the plain logout url we allow it to be properly cached by
the caching router. We just add the requesttoken manually.

Signed-off-by: Roeland Jago Douma <[email protected]>
@rullzer rullzer force-pushed the enh/also_cache_logout_route branch from 3e9d93c to ebd9f30 Compare January 23, 2019 13:06
@rullzer rullzer merged commit 68c0fd3 into master Jan 23, 2019
@rullzer rullzer deleted the enh/also_cache_logout_route branch January 23, 2019 14:32
@danxuliu
Copy link
Member

The acceptance test failures seem legit; for me, logging out now (sometimes) results in an error page with

Access forbidden
CSRF check failed

@MorrisJobke
Copy link
Member

😢

@rullzer
Copy link
Member Author

rullzer commented Jan 23, 2019

The acceptance test failures seem legit; for me, logging out now (sometimes) results in an error page with

Access forbidden
CSRF check failed

Aaaah sometimes yes... OK I know what is going on... fix incomming

rullzer added a commit that referenced this pull request Jan 23, 2019
Followup of #13757

Signed-off-by: Roeland Jago Douma <[email protected]>
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.

7 participants