Fix: No token (query) or authclient (session) found

[ArchBlood]

What kind of change does this PR introduce? (check at least one)

• Bugfix
• Feature
• Code style update
• Refactor
• Build-related changes
• Other, please describe:

Does this PR introduce a breaking change? (check one)

• Yes
• No

If yes, please describe the impact and migration path for existing applications:

The PR fulfills these requirements:

• It's submitted to the develop branch, not the master branch if no hotfix
• When resolving a specific issue, it's referenced in the PR's description (e.g. Fix #xxx[,#xxx], where "xxx" is the Github issue number)
• All tests are passing
• New/updated tests are included
• Changelog was modified

If adding a new feature, the PR's description includes:

• A convincing reason for adding this feature (to avoid wasting your time, it's best to open a suggestion issue first and wait for approval before working on it)

Other information:
Registering with an OAuth client can lead to token session issues, this addresses the issue.
fixes humhub-contrib/auth-facebook#4

[what-the-diff]

PR Summary

• Fix to Handle Absence of Token or Auth Client
  The update includes a solution for situations where no token or authentication client is found, reducing the chances of unexpected errors and ensuring smoother operation.

• User Validation Update in Registration Process
  A verification routine has been added to the registration process, preventing the creation of duplicate accounts. This leads to a more efficient use of resources by avoiding unnecessary data storage.

• Login Function for Registered Users
  A login functionality for existing users has been established within the registration module. This allows already registered users to access the system more conveniently, enhancing user experience.

• Mandatory Token or Auth Client Check
  A mandatory check for the availability of a token or an authentication client has been added. This increases security, as it ensures only authenticated users have access to the system.

[martincodeinc]

Good morning .

When using the updated registrationcontroller i get the following error message after submitting the registration form. This is with a normal registration through the login screen.

yii\base\UnknownPropertyException: Getting unknown property: humhub\modules\user\Module::showRegistrationForm in /var/www/vhosts/commotio.nl/protected/vendor/yiisoft/yii2/base/Component.php:154
Stack trace:
#0 /var/www/vhosts/commotio.nl/protected/vendor/yiisoft/yii2/di/ServiceLocator.php(77): yii\base\Component->__get('showRegistratio...')
#1 /var/www/vhosts/commotio.nl/protected/humhub/modules/user/controllers/RegistrationController.php(134): yii\di\ServiceLocator->__get('showRegistratio...')
#2 [internal function]: humhub\modules\user\controllers\RegistrationController->actionIndex()
#3 /var/www/vhosts/commotio.nl/protected/vendor/yiisoft/yii2/base/InlineAction.php(57): call_user_func_array(Array, Array)
#4 /var/www/vhosts/commotio.nl/protected/vendor/yiisoft/yii2/base/Controller.php(178): yii\base\InlineAction->runWithParams(Array)
#5 /var/www/vhosts/commotio.nl/protected/vendor/yiisoft/yii2/base/Module.php(552): yii\base\Controller->runAction('', Array)
#6 /var/www/vhosts/commotio.nl/protected/vendor/yiisoft/yii2/web/Application.php(103): yii\base\Module->runAction('user/registrati...', Array)
#7 /var/www/vhosts/commotio.nl/protected/vendor/yiisoft/yii2/base/Application.php(384): yii\web\Application->handleRequest(Object(humhub\components\Request))
#8 /var/www/vhosts/commotio.nl/index.php(24): yii\base\Application->run()
#9 {main}

Martin.

[martincodeinc]

HumHub 1.15.4 community edition

[martincodeinc]

I guess this is a fix on your branch; mentioned code snippets do not appear in the main branch code of humhub; e.g $existingUser = User::findByEmail

[ArchBlood]

Please note that this is using the develop branch which is setup for v1.16.x

[ArchBlood]

See the following variables that were added in v1.16.x;

humhub/protected/humhub/modules/user/Module.php

Lines 162 to 194 in d216e22

	/**
	* Reduce filters based on already active filters
	* @var bool
	* @since 1.16
	*/
	public $peopleEnableNestedFilters = true;
	/**
	* Should the login form be displayed. This can be deactivated, e.g. to display only SSO providers.
	* With the parameter `?showLoginForm=1` the login form can still be displayed as a fallback.
	*
	* @since 1.16
	* @var bool
	*/
	public $showLoginForm = true;
	/**
	* Should the login form be displayed. This can be deactivated, e.g. to display only SSO providers.
	* With the parameter `?showLoginForm=1` the login form can still be displayed as a fallback.
	*
	* @since 1.16
	* @var bool
	*/
	public $showRegistrationForm = true;
	/**
	* Allow new user registrations from the following AuthClient IDs even if "User Registration" is deactivated.
	*
	* @since 1.16
	* @var string[]
	*/
	public $allowUserRegistrationFromAuthClientIds = [];

[ArchBlood]

As for a test I've ran one personally with the latest changes in the develop branch and see no issues currently;
The issue here was the else statement was always returning true on humhub-contrib/auth-facebook#4 which is indeed fixed by this P/R.

I guess this is a fix on your branch; mentioned code snippets do not appear in the main branch code of humhub; e.g $existingUser = User::findByEmail

$existingUser = User::findByEmail isn't used in the core User model this was an error on my part, so I fixed this by changing it to $existingUser = User::findIdentity($registration->getUser()->email);.

[ArchBlood]

@yurabakhtin when you have the time can you look over the P/R in case I made any mistakes?

[yurabakhtin]

It seems before run the "login" action we should check for User::STATUS_ENABLED as it was implemented here and as it is implemented on the auth controller, so user sees a message like "Your account is not approved yet!".

[ArchBlood]

I'm not sure I understand, currently User::STATUS_ENABLED is used on line 114 for checking the status of the user in question, should we make an attempt at checking before an attempt to login an existing user or during registration?

[yurabakhtin]

I mean what if a user is already registered so $existingUser !== null but the user is not enabled (disabled or not approved). Will be the user logged in automatically without error message like "Your account is disabled!"?

[ArchBlood]

I mean what if a user is already registered so $existingUser !== null but the user is not enabled (disabled or not approved). Will be the user logged in automatically without error message like "Your account is disabled!"?

I've tested with your concerns in mind, and can say that all this does is bypass the email steps and redirects the user to the account creation steps which if user approval is enabled then the user must wait till their account is approved, if the user email already exists they're able to connect their OAuth account and user account, the only issue that I'm seeing is that the even though the email is checked correctly the user is still redirected to the account creation step, so that is something to look into. 🤔

[ArchBlood]

I'd like to clarify the following;

• $existingUser redirects to > account creation > second click of oauth client button > logs in the user
• $existingUser !== null redirects to > account creation > normal registration process

[ArchBlood]

From my tests if the registered user exists then they're logged in as normal

Did you test if the registered user has a "Disabled" or "Unapproved" status? I think user should be restricted for log in action with such statuses, isn't?

From my testing the user isn't created through this method it bypasses the process of requiring the go through the email token to get to the account creation section as stated, once the account creation form is filled out you're given the following screen;

Till the account is approved the user can't login if they're a new user, if the auth client doesn't properly handle defaultNormalizeUserAttributeMap() then no email will be displayed when the user registers their account, here's an example of what happens with approval needed by an admin;

The only issue is, if the registering user tries logging in they're redirected back to the account creation page, which I've not found a way of fixing yet... 🤔

[yurabakhtin]

The only issue is, if the registering user tries logging in they're redirected back to the account creation page, which I've not found a way of fixing yet... 🤔

I think if user is registered but it cannot be logged in by some reason (maybe it is disabled or not approved) then user should see a message about the reason as it is displayed when user is logged in by login/pass - https://github.com/humhub/humhub/blob/master/protected/humhub/modules/user/controllers/AuthController.php#L287-L292

[ArchBlood]

I think if user is registered but it cannot be logged in by some reason (maybe it is disabled or not approved) then user should see a message about the reason as it is displayed when user is logged in by login/pass - https://github.com/humhub/humhub/blob/master/protected/humhub/modules/user/controllers/AuthController.php#L287-L292

That is where the original issue came into play, too many else statements ended up causing the following to always return as true

else {	
            Yii::warning('Registration failed: No token (query) or authclient (session) found!', 'user');	
            Yii::$app->session->setFlash('error', 'Registration failed.');	
            return $this->redirect(['/user/auth/login']);
}

[yurabakhtin]

ok, if you think it works as expected now then I could approve it, because I cannot test it on my local server now.
I will test this later on community server because I have similar issue with my account here https://github.com/humhub/humhub-internal/issues/258.

[ArchBlood]

ok, if you think it works as expected now then I could approve it, because I cannot test it on my local server now. I will test this later on community server because I have similar issue with my account here https://github.com/humhub/humhub-internal/issues/258.

Maybe the issue could be more simpler than we think? Looking at how we're getting the token and how it's validated could also be the reason why the error is happening? There are so many other possibilities that could be the root cause of it;

humhub/protected/humhub/modules/user/services/InviteRegistrationService.php

Lines 41 to 44 in ef41e8a

	public function isValid(): bool
	{
	return ($this->getInvite() !== null);
	}

I've tested to see if $token is correct with the following and it is correct but I'm not sure if the validation is correct in the if statement $inviteRegistrationService->isValid();

// Log the token value
$token = Yii::$app->request->get('token');
Yii::info('Token value: ' . var_export($token, true), 'debug');

if ($token) {
    $inviteRegistrationService = new InviteRegistrationService($token);
    Yii::info('Token is present, checking validity.', 'debug');
    if (!$inviteRegistrationService->isValid()) {
        Yii::error('Invalid registration token!', 'user');
        throw new HttpException(404, 'Invalid registration token!');
    }
    $inviteRegistrationService->populateRegistration($registration);
} else {
    Yii::info('Token is not present.', 'debug');
}

if (Yii::$app->session->has('authClient')) {
    $authClient = Yii::$app->session->get('authClient');
    Yii::info('AuthClient found in session: ' . var_export($authClient, true), 'debug');
    $this->handleAuthClientRegistration($authClient, $registration);
} else {
    Yii::info('AuthClient not found in session.', 'debug');
}

if (!$token && !Yii::$app->session->has('authClient')) {
    Yii::warning('Registration failed: No token (query) or authClient (session) found!', 'user');
    Yii::$app->session->setFlash('error', 'Registration failed.');
    return $this->redirect(['/user/auth/login']);
}

[luke-]

@ArchBlood Unfortunately, I have only now had time to look into the error in more detail.

The error 'Registration failed: No token (query) or authclient (session) found!' could also occur in the context of AuthClients if:

• The AuthClient does not return enough attributes for a complete automatic registration (e.g. user name or surname missing) and need to provided by the user
• and: The class of the AuthClient is not serializable

---

If, for example, an AuthClient cannot be serialized because of an API, the following method can be implemented to remove non-serializable properties beforehand. https://github.com/humhub/humhub/blob/master/protected/humhub/modules/user/authclient/BaseClient.php#L42-L49
Ideally, of course, the AuthClient would not have to be serializable, but there is currently no other solution.

---

Unfortunately, your current PR is difficult to review & accept because it involves major changes and the process as a whole is very complex.

Due to the removed else conditions, registrations without token or previously set AuthClient are now also possible.

What specific problem are you trying to solve? Or what is the problem with the current implementation?

[ArchBlood]

@ArchBlood Unfortunately, I have only now had time to look into the error in more detail.

The error 'Registration failed: No token (query) or authclient (session) found!' could also occur in the context of AuthClients if:

• The AuthClient does not return enough attributes for a complete automatic registration (e.g. user name or surname missing) and need to provided by the user
• and: The class of the AuthClient is not serializable

---

If, for example, an AuthClient cannot be serialized because of an API, the following method can be implemented to remove non-serializable properties beforehand. https://github.com/humhub/humhub/blob/master/protected/humhub/modules/user/authclient/BaseClient.php#L42-L49
Ideally, of course, the AuthClient would not have to be serializable, but there is currently no other solution.

---

Unfortunately, your current PR is difficult to review & accept because it involves major changes and the process as a whole is very complex.

Due to the removed else conditions, registrations without token or previously set AuthClient are now also possible.

What specific problem are you trying to solve? Or what is the problem with the current implementation?

The else statement was just relocated within the handleAuthClientRegistration() method directly to remove the condition where it's returning true even though the token & AuthClient are present.

As for the possibility of the issue being because of attributes, this would require some looking into as this issue shouldn't affect authclient modules that are handling attributes correctly, and if they're not then that means all authclient modules currently in the marketplace are affected which needs placed on a priority list. 🤔

The only reason I say that it probably isn't related to attributes is that then authclients that are still in the core would be affected as well which doesn't seem to be the case.

[luke-]

@ArchBlood Unfortunately, I have only now had time to look into the error in more detail.
The error 'Registration failed: No token (query) or authclient (session) found!' could also occur in the context of AuthClients if:

• The AuthClient does not return enough attributes for a complete automatic registration (e.g. user name or surname missing) and need to provided by the user
• and: The class of the AuthClient is not serializable

If, for example, an AuthClient cannot be serialized because of an API, the following method can be implemented to remove non-serializable properties beforehand. https://github.com/humhub/humhub/blob/master/protected/humhub/modules/user/authclient/BaseClient.php#L42-L49
Ideally, of course, the AuthClient would not have to be serializable, but there is currently no other solution.

Unfortunately, your current PR is difficult to review & accept because it involves major changes and the process as a whole is very complex.
Due to the removed else conditions, registrations without token or previously set AuthClient are now also possible.
What specific problem are you trying to solve? Or what is the problem with the current implementation?

The else statement was just relocated within the handleAuthClientRegistration() method directly to remove the condition where it's returning true even though the token & AuthClient are present.

I understand that the else has been moved, but with that it now allows registration without prior set AuthClients.
The else condition prevented this case before.

We really need to understand why the AuthClient was not set before.

As for the possibility of the issue being because of attributes, this would require some looking into as this issue shouldn't affect authclient modules that are handling attributes correctly, and if they're not then that means all authclient modules currently in the marketplace are affected which needs placed on a priority list. 🤔

The only reason I say that it probably isn't related to attributes is that then authclients that are still in the core would be affected as well which doesn't seem to be the case.

The non-serializability of AuthClient does not affect all AuthClients. The only one I am currently aware of is SAML, as it works with anonymous functions that cannot be serialized.