Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
75 changes: 75 additions & 0 deletions xExtension-UnsafeAutologin/Controllers/authController.php
Original file line number Diff line number Diff line change
@@ -0,0 +1,75 @@
<?php
declare(strict_types=1);

class FreshExtension_auth_Controller extends FreshRSS_auth_Controller {
Copy link
Member

Choose a reason for hiding this comment

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

I am worried about the maintenance of this approach based on copying and overriding.
Would you be able to find another approach based on an extension hook (a new one)?
This way, it would both made this part more realistic to maintain and maybe also enable other custom login types

Copy link
Member Author

Choose a reason for hiding this comment

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

maybe also enable other custom login types

This isn't really a login type, but an extension of form login.
I'm aware of this TODO:

https://github.com/FreshRSS/FreshRSS/blob/128c375fc9de5119b1963d92dc9cd3e423053111/app/Models/Auth.php#L96-L98

Maybe some sort of hook should be put here?

https://github.com/FreshRSS/FreshRSS/blob/128c375fc9de5119b1963d92dc9cd3e423053111/app/Controllers/authController.php#L124-L125

Copy link
Member

@Alkarex Alkarex Oct 2, 2025

Choose a reason for hiding this comment

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

This isn't really a login type, but an extension of form login

Indeed. But maybe what about making it a new extension-defined login type?

Edit: and which could fall back to the native logins (e.g. Web form) if it does not have the required parameters to log in

Copy link
Member Author

@Inverle Inverle Oct 2, 2025

Choose a reason for hiding this comment

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

Maybe some sort of hook should be put here?

By the way I meant somewhere above not specifically in the POST if condition :p

Indeed. But maybe what about making it a new extension-defined login type?
Edit: and which could forward to the Web form login if it does not have the required parameters to log in

That would be a little more complicated than the current implementation.
I have considered this though

Copy link
Member

Choose a reason for hiding this comment

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

Copy link
Member Author

Choose a reason for hiding this comment

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

Ping @Alkarex

Copy link
Member

Choose a reason for hiding this comment

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

at least not if we want to keep full compatibility

What do you mean with full compatibility?

Could you tell a little bit what you have tried? I still hope that we can find an approach that does not require overloading and duplicated code

Copy link
Member Author

@Inverle Inverle Nov 21, 2025

Choose a reason for hiding this comment

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

What do you mean with full compatibility?

I meant:

unsafe autologin could be used for account switching

with two bookmarks for instance
Which duplicated code do you mean? this is mostly the same code as here https://github.com/FreshRSS/FreshRSS/blob/a7579e0cf5717e86fe954bd75f2625f8131120f7/app/Controllers/authController.php#L195

Copy link
Member

Choose a reason for hiding this comment

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

this is mostly the same code as here

Good point. I was still hoping for something a bit more stable to maintain when the original method evolves over time.

Copy link
Member Author

Choose a reason for hiding this comment

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

Could you tell a little bit what you have tried?

Implementing a hook in accessControl() as suggested, but then I realized it's not the correct place at all (read above).

And I believe placing a hook directly in formLoginAction() wouldn't be easier or harder to maintain than the current approach.

/**
* @throws Minz_ConfigurationException
*/
private static function redirectFormLogin(): void {
if (FreshRSS_Auth::hasAccess()) {
Minz_Request::forward(['c' => 'index', 'a' => 'index'], true);
return;
}
Minz_Request::forward(['c' => 'auth', 'a' => 'formLogin']);
}

/**
* @throws FreshRSS_Context_Exception
* @throws Minz_ConfigurationNamespaceException
* @throws Minz_ConfigurationException
* @throws Minz_PermissionDeniedException
*/
public function loginAction(): void {
if (FreshRSS_Context::systemConf()->auth_type !== 'form') {
parent::loginAction();
return;
}

$username = Minz_Request::paramString('u');
$password = Minz_Request::paramString('p', plaintext: true);

if ($username === '' || $password === '') {
self::redirectFormLogin();
return;
}

if (!FreshRSS_user_Controller::checkUsername($username) || !FreshRSS_user_Controller::userExists($username)) {
Minz_Request::bad(
_t('feedback.auth.login.invalid'),
['c' => 'index', 'a' => 'index']
);
return;
}

$config = get_user_configuration($username);

$s = $config->passwordHash ?? '';
$ok = password_verify($password, $s);

if ($ok) {
FreshRSS_Context::initUser($username);
FreshRSS_FormAuth::deleteCookie();
Minz_Session::regenerateID('FreshRSS');
Minz_Session::_params([
Minz_User::CURRENT_USER => $username,
'passwordHash' => $s,
'lastReauth' => false,
'csrf' => false,
]);
FreshRSS_Auth::giveAccess();

Minz_Translate::init(FreshRSS_Context::userConf()->language);

FreshRSS_UserDAO::touch();

Minz_Request::good(_t('feedback.auth.login.success'), ['c' => 'index', 'a' => 'index']);
return;
}

Minz_Log::warning('Unsafe password mismatch for user ' . $username, USERS_PATH . '/' . $username . '/' . 'log.txt');
Minz_Request::bad(
_t('feedback.auth.login.invalid'),
['c' => 'index', 'a' => 'index']
);
}
}
Loading