Skip to content

Conversation

@rullzer
Copy link
Member

@rullzer rullzer commented Nov 21, 2018

Fixes #12568
Since the clearing of the execution context causes another reload. We
should not do the redirect_uri handling as this results in redirecting
back to the logout page on login.

Signed-off-by: Roeland Jago Douma [email protected]

@rullzer rullzer added the 3. to review Waiting for reviews label Nov 21, 2018
@rullzer rullzer added this to the Nextcloud 16 milestone Nov 22, 2018
@nickvergessen
Copy link
Member

I dislike this patch a bit, because if you are on a page which is publicly accessible, it loads that page, instead of redirecting to the login form.

In my head the following should have worked:

	public function showLoginForm(string $user = null, string $redirect_url = null): Http\Response {
		if ($this->session->exists('clearingExecutionContexts')) {
			$this->session->remove('clearingExecutionContexts');

			$response = new RedirectResponse($this->urlGenerator->linkToRouteAbsolute('core.login.showLoginForm'));
			$response->addHeader('Clear-Site-Data', '"cache", "storage", "executionContexts"');
			return $response;
		}

But while your request is on /login firefox reloads the last loaded page, which is still the previous page, so it also would reload the public call.

@rullzer
Copy link
Member Author

rullzer commented Nov 22, 2018

Do you have a logout button on a public page?

@nickvergessen
Copy link
Member

Well on Talk the same URL is used for a room independent of your login state.

@MorrisJobke
Copy link
Member

@rullzer @nickvergessen What is the status here? Continue it or close it?

@rullzer
Copy link
Member Author

rullzer commented Feb 4, 2019

Let me have another look. I might have a more elegant way

@rullzer
Copy link
Member Author

rullzer commented Feb 4, 2019

@nickvergessen can you verify that this seems fixed with the latest firefox?

@rakekniven
Copy link
Member

No, it is still the same with ff 65.

@nickvergessen
Copy link
Member

Same here, still broken with FF 65.0

@rullzer rullzer force-pushed the fix/12568/special_handling_of_logout branch from 9e1061d to 689adc8 Compare February 5, 2019 21:37
@rullzer
Copy link
Member Author

rullzer commented Feb 5, 2019

Ok this is now a bit more elegant I think.

@nickvergessen please check it out

Fixes #12568
Since the clearing of the execution context causes another reload. We
should not do the redirect_uri handling as this results in redirecting
back to the logout page on login.

This adds a simple middleware that will just check if the
ClearExecutionContext session variable is set. If that is the case it
will just redirect back to the login page.

Signed-off-by: Roeland Jago Douma <[email protected]>
@rullzer rullzer force-pushed the fix/12568/special_handling_of_logout branch from 689adc8 to 60e5a5e Compare February 6, 2019 10:29
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.

That works perfectly.

Copy link
Member

@MorrisJobke MorrisJobke left a comment

Choose a reason for hiding this comment

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

Tested and works 👍

@MorrisJobke MorrisJobke merged commit a1aa6ee into master Feb 6, 2019
@MorrisJobke MorrisJobke deleted the fix/12568/special_handling_of_logout branch February 6, 2019 14:39
@MorrisJobke
Copy link
Member

Backport?

@rullzer
Copy link
Member Author

rullzer commented Feb 6, 2019

Fine by me. It is mainly an added middleware. So should not cause issues.

@MorrisJobke
Copy link
Member

/backport to stable15

@backportbot-nextcloud
Copy link

The backport to stable15 failed. Please do this backport manually.

haslersn added a commit to haslersn/nextcloud-oidc-login that referenced this pull request Sep 14, 2023
See https://github.com/nextcloud/server/blob/b085803c0bfe8c568e5710525e49d5f6378833b6/core/Controller/LoginController.php#L99
and following lines.

Also note that setting `clearingExecutionContexts` is no longer required,
because it had to do with the executionContexts feature which is no longer
used by nextcloud since nextcloud/server#16310.
Furthermore, with the behavior introduced in
nextcloud/server#12573, setting
`clearingExecutionContexts` breaks our logout redirects, because the
middleware subsequently (after the logout redirect) returns another
redirects to `/login?clear=1`.
haslersn added a commit to haslersn/nextcloud-oidc-login that referenced this pull request Sep 14, 2023
See https://github.com/nextcloud/server/blob/b085803c0bfe8c568e5710525e49d5f6378833b6/core/Controller/LoginController.php#L99
and following lines.

Also note that setting `clearingExecutionContexts` is no longer required,
because it had to do with the executionContexts feature which is no longer
used by nextcloud since nextcloud/server#16310.
Furthermore, with the behavior introduced in
nextcloud/server#12573, setting
`clearingExecutionContexts` breaks our logout redirects, because the
middleware subsequently (after the logout redirect) returns another
redirects to `/login?clear=1`.
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

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants