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

Easier session handling or more info about It #4236

Closed
nesjett opened this issue Aug 24, 2020 · 4 comments
Closed

Easier session handling or more info about It #4236

nesjett opened this issue Aug 24, 2020 · 4 comments
Assignees
Labels
type: bug A problem that should not be happening

Comments

@nesjett
Copy link
Contributor

nesjett commented Aug 24, 2020

Motivation

I'm strugling with class2 to understand how session handling works, how to properly login/logout to build an interface for an API that will use e107 user system.

Proposed Solution

Refactoring of session handling or a point in the right direction on how sessions work/dependencies so I can maybe implement It by myself and do a pull request.

Right now class2 is messy and definetly not well explained, many things happen behind the scenes in "misterious" classes that are loadded from everywhere

Example

Using common post for user login to any URL works well, but getting the code that is aparently being used to login

e107::getUser()->login($name, $password, $autologin)

Is not working as expected, as in case the password is wrong, you still get getUser()->isUser() as true, and getUser()->getData() returns still all data of the username provided, like if the session was properly inited..

This means that somehwere else, e107 is performing a cleanup if the userLookup was wrong, but I really can't find this behaviour...

Greetings,

  • Nesjett
@nesjett nesjett added the type: enhancement An improvement or new feature request label Aug 24, 2020
@nesjett
Copy link
Contributor Author

nesjett commented Aug 24, 2020

My findings so far:
Calling (user_model.php login() )

getUser()->login() 

Is not really checking if the login is successfull or not, as It is actually calling

new userlogin()->login()

Which actually performs the real login attempt, but Ignore It's results. The code so you understand better:

In user_model.php you will find this:

final public function login($uname, $upass_plain, $uauto = false, $uchallange = false, $noredirect = true)
	{
		if($this->isUser()) return false;

		$userlogin = new userlogin();
		$userlogin->login($uname, $upass_plain, $uauto, $uchallange, $noredirect);
		
		$userdata  = $userlogin->getUserData(); 
		
		$this->setSessionData(true)->setData($userdata);
		
		e107::getEvent()->trigger('user_login', $userdata); 	

		return $this->isUser();
	}

Notice the $userlogin->login() line, where it executes the function but ignores It's results.

Conclusion

Despite this extrange behaviour, e107 login system seems to work properly when used from within the class2.php script, but not if you attempt using the getUser()->login() method "standalone"

@Moc
Copy link
Member

Moc commented Aug 24, 2020

@Deltik can you provide some insight in this?

@Deltik
Copy link
Member

Deltik commented Aug 25, 2020

@nesjett: I am able to reproduce your issue with this test:

diff --git a/e107_tests/tests/unit/e_user_modelTest.php b/e107_tests/tests/unit/e_user_modelTest.php
index fba624ced..e613a4fcf 100644
--- a/e107_tests/tests/unit/e_user_modelTest.php
+++ b/e107_tests/tests/unit/e_user_modelTest.php
@@ -377,7 +377,15 @@
 
                }
 */
+               /**
+                * @see https://github.com/e107inc/e107/issues/4236
+                */
+               public function testUserLoginWrongCredentialsNotUser()
+               {
+                       $user = e107::getUser();
+                       $user->login("e107", "DefinitelyTheWrongPassword");
 
-
-
+                       $this->assertFalse($user->isUser());
+                       $this->assertEmpty($user->getData());
+               }
        }

It's hard to imagine that this is intended behavior because the result is counterintuitive. I will try to make this test pass.

@Deltik Deltik added type: bug A problem that should not be happening and removed type: enhancement An improvement or new feature request topic: documentation labels Aug 25, 2020
@Deltik Deltik self-assigned this Aug 25, 2020
Deltik added a commit to Deltik/e107 that referenced this issue Aug 25, 2020
@Deltik
Copy link
Member

Deltik commented Aug 25, 2020

@Moc and @CaMer0n: Breaking change pull request ready for you to review: #4238

CaMer0n added a commit that referenced this issue Sep 17, 2020
Fixes #4236 Don't populate e_user_model as a logged in user if the password is wrong
@Moc Moc moved this to Backlog in Documentation update Oct 30, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
type: bug A problem that should not be happening
Projects
Status: Backlog
Development

No branches or pull requests

3 participants