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

Logging out user is not committed immediately #2267

Closed
jpsjanne opened this issue Nov 1, 2019 · 8 comments
Assignees
Milestone

Comments

@jpsjanne
Copy link

@jpsjanne jpsjanne commented Nov 1, 2019

Describe the bug
Kirby()->user() returns a User object even if Kirby()->user()->logout() has been called.

Forum topic

To Reproduce
Steps to reproduce the behavior:

  1. Download and install starterkit-master
  2. Create a new page template called “logout” and a corresponding content page: content/logout/logout.txt
  3. Create an admin account in panel and log in
  4. Enter the newly created logout page localhost/logout

Text User is logged in appears in an empty page, even though user is logged out. Any snippet or plugin used to render the page, which relies on kirby()->user(), thinks that the user is still logged in.

site/controllers/logout.php

<?php

return function($site, $pages, $page) 
{
	$user = kirby()->user();

	if ($user) {
		$user->logout();
	}
};

site/templates/logout.php

<?php
if (kirby()->user()) {
	echo 'User is logged in';
} else {
	echo "No user object";
}
?>

Expected behavior

When entering localhost/logout and user is logged in, an empty page with text No user object should appear.

Kirby Version
Tested that problem exists in version 3.2.5 and 3.3.0rc-4
Problem does not exist in version 3.1.4.

Desktop (please complete the following information):

  • OS: Windows 10
  • Browser: Opera
  • Version: 63.0.3368.107
  • Server: XAMPP 7.3.7
@afbora

This comment has been minimized.

Copy link
Contributor

@afbora afbora commented Nov 1, 2019

I can reproduce too:

$user = kirby()->user('john@doe.com');
$user->loginPasswordless();
var_dump(kirby()->user());
$user->logout();
var_dump(kirby()->user()); // should be null but returns user data

The reason for this error is that the kirby()->user() method uses the Auth class to get current user, but the $user->logout() method doesn't use the Auth class, just clearing the session.
Thus, the $this->user cached variable in the Auth class is still full, returning user data.
On the other hand, the auth()->logout() method works correctly. Because this method also clears cache variables ($this->impersonate = null; $this->user = null;)

Cms/Auth::logout()

https://github.com/getkirby/kirby/blob/release/3.3.0/src/Cms/Auth.php#L339-L351

public function logout(): bool
{
    // stop impersonating
    $this->impersonate = null;

    // logout the current user if it exists
    if ($user = $this->user()) {
        $user->logout();
    }

    $this->user = null;
    return true;
}

As a result, the logout method in the User class should also clear the user cached variables in the Auth class.

@lukasbestle

This comment has been minimized.

Copy link
Contributor

@lukasbestle lukasbestle commented Nov 1, 2019

I agree that this is a bug.

Possible solution:

  • Move the lines that unset the $auth->impersonate and $auth->user properties to a new $auth->clearUser() method
  • Call that from the $user->logout() method with $this->kirby()->auth()->clearUser()
@lukasbestle lukasbestle self-assigned this Nov 1, 2019
@lukasbestle lukasbestle added this to the 3.3.1 milestone Nov 1, 2019
@distantnative

This comment has been minimized.

Copy link
Contributor

@distantnative distantnative commented Nov 1, 2019

@lukasbestle thought: call it flush to be closer to terms already used in Kirby?

@lukasbestle

This comment has been minimized.

Copy link
Contributor

@lukasbestle lukasbestle commented Nov 1, 2019

@distantnative Just flush() or flushUser()? I just saw that we'll also need a method to set the user, that would be setUser().

@distantnative

This comment has been minimized.

Copy link
Contributor

@distantnative distantnative commented Nov 1, 2019

@lukasbestle I think just flush could work - basically we are flushin everything that Auth is about - impersonate and user -, right?

lukasbestle added a commit that referenced this issue Nov 1, 2019
@lukasbestle

This comment has been minimized.

Copy link
Contributor

@lukasbestle lukasbestle commented Nov 1, 2019

@jpsjanne This bug is now fixed with #2269. The fix will be in Kirby 3.3.1, but you can try it out already if you want. :)

@jpsjanne

This comment has been minimized.

Copy link
Author

@jpsjanne jpsjanne commented Nov 1, 2019

Thanks everyone for the quick response! I'll just sit tight and wait for the update.

bastianallgeier added a commit that referenced this issue Nov 18, 2019
@bastianallgeier

This comment has been minimized.

Copy link
Contributor

@bastianallgeier bastianallgeier commented Nov 18, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
5 participants
You can’t perform that action at this time.