Skip to content

Commit 13c4659

Browse files
christianbeeznestchristianbeeznest
committed
Security: add security.2fa_enable; hide 2FA UI & skip MFA when disabled
1 parent ee48eaf commit 13c4659

File tree

5 files changed

+151
-42
lines changed

5 files changed

+151
-42
lines changed

‎src/CoreBundle/Controller/AccountController.php‎

Lines changed: 36 additions & 33 deletions
Original file line numberDiff line numberDiff line change
@@ -114,42 +114,44 @@ public function changePassword(
114114

115115
if (!$user || !$user instanceof UserInterface) {
116116
$userId = $request->query->get('userId');
117-
// error_log("User not logged in. Received userId from query: " . $userId);
118117

119118
if (!$userId || !ctype_digit($userId)) {
120-
// error_log("Access denied: Missing or invalid userId.");
121119
throw $this->createAccessDeniedException('This user does not have access to this section.');
122120
}
123121

124122
$user = $userRepository->find((int) $userId);
125123

126124
if (!$user || !$user instanceof UserInterface) {
127-
// error_log("Access denied: User not found with ID $userId");
128125
throw $this->createAccessDeniedException('User not found or invalid.');
129126
}
130-
131-
// error_log("Loaded user by ID: " . $user->getId());
132127
}
133128

129+
// Global 2FA toggle: read either "security.2fa_enable" or fallback "2fa_enable"
130+
$twoFaEnabledGlobally = 'true' === $settingsManager->getSetting('security.2fa_enable', true);
131+
132+
// When rotating password (forced update), we also hide the 2FA widget
134133
$isRotation = $request->query->getBoolean('rotate', false);
135134

135+
// Build the form; expose 2FA fields only if globally enabled and not rotating the password
136136
$form = $this->createForm(ChangePasswordType::class, [
137137
'enable2FA' => $user->getMfaEnabled(),
138138
], [
139139
'user' => $user,
140140
'portal_name' => $settingsManager->getSetting('platform.institution'),
141141
'password_hasher' => $passwordHasher,
142-
'enable_2fa_field' => !$isRotation,
142+
'enable_2fa_field' => $twoFaEnabledGlobally && !$isRotation,
143+
'global_2fa_enabled' => $twoFaEnabledGlobally,
143144
]);
144145
$form->handleRequest($request);
145146

146147
$session = $request->getSession();
147148
$qrCodeBase64 = null;
148149
$showQRCode = false;
149150

150-
// Build QR code preview if user opts to enable 2FA but hasn't saved yet
151+
// Pre-generate QR preview only when 2FA is globally enabled and user opted-in but hasn't saved yet
151152
if (
152-
$form->isSubmitted()
153+
$twoFaEnabledGlobally
154+
&& $form->isSubmitted()
153155
&& $form->has('enable2FA')
154156
&& $form->get('enable2FA')->getData()
155157
&& !$user->getMfaSecret()
@@ -186,35 +188,40 @@ public function changePassword(
186188
if (!$csrfTokenManager->isTokenValid(new CsrfToken('change_password', $submittedToken))) {
187189
$form->addError(new FormError($this->translator->trans('CSRF token is invalid. Please try again.')));
188190
} else {
189-
$currentPassword = $form->get('currentPassword')->getData();
190-
$newPassword = $form->get('newPassword')->getData();
191-
$confirmPassword = $form->get('confirmPassword')->getData();
192-
$enable2FA = !$isRotation && $form->has('enable2FA')
193-
? $form->get('enable2FA')->getData()
191+
$currentPassword = $form->get('currentPassword')->getData();
192+
$newPassword = $form->get('newPassword')->getData();
193+
$confirmPassword = $form->get('confirmPassword')->getData();
194+
195+
// Only consider the user's 2FA intent if the global toggle is ON and not rotating
196+
$enable2FA = $twoFaEnabledGlobally && !$isRotation && $form->has('enable2FA')
197+
? (bool) $form->get('enable2FA')->getData()
194198
: false;
195199

196-
if ($enable2FA && !$user->getMfaSecret()) {
200+
// Handle 2FA activation (only when globally enabled)
201+
if ($twoFaEnabledGlobally && $enable2FA && !$user->getMfaSecret()) {
197202
$secret = $session->get('temporary_mfa_secret');
198-
$encryptedSecret = $this->encryptTOTPSecret($secret, $_ENV['APP_SECRET']);
199-
$user->setMfaSecret($encryptedSecret);
200-
$user->setMfaEnabled(true);
201-
$user->setMfaService('TOTP');
202-
$userRepository->updateUser($user);
203-
204-
$session->remove('temporary_mfa_secret');
205-
206-
$this->addFlash('success', '2FA activated successfully.');
203+
if ($secret) {
204+
$encryptedSecret = $this->encryptTOTPSecret($secret, $_ENV['APP_SECRET']);
205+
$user->setMfaSecret($encryptedSecret);
206+
$user->setMfaEnabled(true);
207+
$user->setMfaService('TOTP');
208+
$userRepository->updateUser($user);
209+
$session->remove('temporary_mfa_secret');
207210

208-
return $this->redirectToRoute('chamilo_core_account_home');
211+
$this->addFlash('success', $this->translator->trans('2FA activated successfully.'));
212+
return $this->redirectToRoute('chamilo_core_account_home');
213+
}
209214
}
210215

211-
if (!$isRotation && !$enable2FA && $user->getMfaEnabled()) {
216+
// Handle 2FA deactivation from the form (only visible if global is ON; safe to guard too)
217+
if ($twoFaEnabledGlobally && !$isRotation && !$enable2FA && $user->getMfaEnabled()) {
212218
$user->setMfaEnabled(false);
213219
$user->setMfaSecret(null);
214220
$userRepository->updateUser($user);
215-
$this->addFlash('success', '2FA disabled successfully.');
221+
$this->addFlash('success', $this->translator->trans('2FA disabled successfully.'));
216222
}
217223

224+
// Password change flow (unchanged)
218225
if ($newPassword || $confirmPassword || $currentPassword) {
219226
if (!$userRepository->isPasswordValid($user, $currentPassword)) {
220227
$form->get('currentPassword')->addError(new FormError(
@@ -228,15 +235,11 @@ public function changePassword(
228235
$user->setPlainPassword($newPassword);
229236
$user->setPasswordUpdatedAt(new DateTimeImmutable());
230237
$userRepository->updateUser($user);
231-
$this->addFlash('success', 'Password updated successfully.');
238+
$this->addFlash('success', $this->translator->trans('Password updated successfully.'));
232239

233-
// Re-login if the user was not logged
240+
// Re-login if the user was not logged in (edge case when rotating from link)
234241
if (!$this->getUser()) {
235-
$token = new UsernamePasswordToken(
236-
$user,
237-
'main',
238-
$user->getRoles()
239-
);
242+
$token = new UsernamePasswordToken($user, 'main', $user->getRoles());
240243
$tokenStorage->setToken($token);
241244
$request->getSession()->set('_security_main', serialize($token));
242245
}

‎src/CoreBundle/DataFixtures/SettingsCurrentFixtures.php‎

Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1326,6 +1326,11 @@ public static function getExistingSettings(): array
13261326
],
13271327
],
13281328
'security' => [
1329+
[
1330+
'name' => '2fa_enable',
1331+
'title' => 'Enable 2FA',
1332+
'comment' => "Add fields in the password update page to enable 2FA using a TOTP authenticator app. When disabled globally, users won't see 2FA fields and won't be prompted for 2FA at login, even if they had enabled it previously.",
1333+
],
13291334
[
13301335
'name' => 'filter_terms',
13311336
'title' => 'Filter terms',

‎src/CoreBundle/Form/ChangePasswordType.php‎

Lines changed: 12 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -47,6 +47,7 @@ public function buildForm(FormBuilderInterface $builder, array $options): void
4747
])
4848
;
4949

50+
// Show 2FA fields only when allowed by controller/options
5051
if ($options['enable_2fa_field']) {
5152
$builder->add('enable2FA', CheckboxType::class, [
5253
'label' => 'Enable two-factor authentication (2FA)',
@@ -72,7 +73,7 @@ public function buildForm(FormBuilderInterface $builder, array $options): void
7273
$newPassword = $form->get('newPassword')->getData();
7374
$confirmPassword = $form->get('confirmPassword')->getData();
7475
$enable2FA = $form->has('enable2FA')
75-
? $form->get('enable2FA')->getData()
76+
? (bool) $form->get('enable2FA')->getData()
7677
: false;
7778
$code = $form->has('confirm2FACode')
7879
? $form->get('confirm2FACode')->getData()
@@ -96,7 +97,8 @@ public function buildForm(FormBuilderInterface $builder, array $options): void
9697
}
9798
}
9899

99-
if ($form->has('confirm2FACode')) {
100+
// Guard 2FA validation behind the global toggle AND presence of the field
101+
if ($options['global_2fa_enabled'] === true && $form->has('confirm2FACode')) {
100102
if ($user->getMfaEnabled() || $enable2FA) {
101103
if (empty($code)) {
102104
$form->get('confirm2FACode')->addError(new FormError('The 2FA code is required.'));
@@ -131,13 +133,14 @@ public function buildForm(FormBuilderInterface $builder, array $options): void
131133
public function configureOptions(OptionsResolver $resolver): void
132134
{
133135
$resolver->setDefaults([
134-
'csrf_protection' => true,
135-
'csrf_field_name' => '_token',
136-
'csrf_token_id' => 'change_password',
137-
'enable_2fa_field' => true,
138-
'user' => null,
139-
'portal_name' => 'Chamilo',
140-
'password_hasher' => null,
136+
'csrf_protection' => true,
137+
'csrf_field_name' => '_token',
138+
'csrf_token_id' => 'change_password',
139+
'enable_2fa_field' => true,
140+
'global_2fa_enabled'=> true,
141+
'user' => null,
142+
'portal_name' => 'Chamilo',
143+
'password_hasher' => null,
141144
]);
142145
}
143146

‎src/CoreBundle/Migrations/Schema/V200/Version20250923224100.php‎

Lines changed: 96 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,96 @@
1+
<?php
2+
3+
4+
declare(strict_types=1);
5+
6+
namespace Chamilo\CoreBundle\Migrations\Schema\V200;
7+
8+
use Chamilo\CoreBundle\Migrations\AbstractMigrationChamilo;
9+
use Doctrine\DBAL\Schema\Schema;
10+
11+
final class Version20250923224100 extends AbstractMigrationChamilo
12+
{
13+
public function getDescription(): string
14+
{
15+
return 'Insert or update platform setting title/comment for security 2FA global toggle (2fa_enable).';
16+
}
17+
18+
public function up(Schema $schema): void
19+
{
20+
$settings = [
21+
[
22+
'name' => '2fa_enable',
23+
'title' => 'Enable 2FA',
24+
'comment' => "Add fields in the password update page to enable 2FA using a TOTP authenticator app. When disabled globally, users won't see 2FA fields and won't be prompted for 2FA at login, even if they had enabled it previously.",
25+
'category' => 'security',
26+
'default' => 'false',
27+
],
28+
];
29+
30+
foreach ($settings as $setting) {
31+
$variable = addslashes($setting['name']);
32+
$title = addslashes($setting['title']);
33+
$comment = addslashes($setting['comment']);
34+
$category = addslashes($setting['category']);
35+
$default = addslashes($setting['default']);
36+
37+
$sqlCheck = \sprintf(
38+
"SELECT COUNT(*) AS count
39+
FROM settings
40+
WHERE variable = '%s'
41+
AND subkey IS NULL
42+
AND access_url = 1",
43+
$variable
44+
);
45+
46+
$stmt = $this->connection->executeQuery($sqlCheck);
47+
$result = $stmt->fetchAssociative();
48+
49+
if ($result && (int) $result['count'] > 0) {
50+
$this->addSql(\sprintf(
51+
"UPDATE settings
52+
SET title = '%s',
53+
comment = '%s',
54+
category = '%s'
55+
WHERE variable = '%s'
56+
AND subkey IS NULL
57+
AND access_url = 1",
58+
$title,
59+
$comment,
60+
$category,
61+
$variable
62+
));
63+
$this->write(\sprintf('Updated setting: %s', $setting['name']));
64+
} else {
65+
$this->addSql(\sprintf(
66+
"INSERT INTO settings
67+
(variable, subkey, type, category, selected_value, title, comment, access_url_changeable, access_url_locked, access_url)
68+
VALUES
69+
('%s', NULL, NULL, '%s', '%s', '%s', '%s', 1, 0, 1)",
70+
$variable,
71+
$category,
72+
$default,
73+
$title,
74+
$comment
75+
));
76+
$this->write(\sprintf('Inserted setting: %s', $setting['name']));
77+
}
78+
}
79+
}
80+
81+
public function down(Schema $schema): void
82+
{
83+
$variables = ['2fa_enable'];
84+
85+
foreach ($variables as $variable) {
86+
$this->addSql(\sprintf(
87+
"DELETE FROM settings
88+
WHERE variable = '%s'
89+
AND subkey IS NULL
90+
AND access_url = 1",
91+
addslashes($variable)
92+
));
93+
$this->write(\sprintf('Removed setting: %s.', $variable));
94+
}
95+
}
96+
}

‎src/CoreBundle/Settings/SecuritySettingsSchema.php‎

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -43,6 +43,7 @@ public function buildSettings(AbstractSettingsBuilder $builder): void
4343
'anonymous_autoprovisioning' => 'false',
4444
'access_to_personal_file_for_all' => 'false',
4545
'password_rotation_days' => '0',
46+
'2fa_enable' => 'false',
4647
]);
4748

4849
$allowedTypes = [
@@ -80,6 +81,7 @@ public function buildForm(FormBuilderInterface $builder): void
8081
->add('anonymous_autoprovisioning', YesNoType::class)
8182
->add('access_to_personal_file_for_all', YesNoType::class)
8283
->add('password_rotation_days', TextType::class)
84+
->add('2fa_enable', YesNoType::class)
8385
;
8486

8587
$this->updateFormFieldsFromSettingsInfo($builder);

0 commit comments

Comments
 (0)