diff --git a/ProcessMaker/Http/Controllers/Api/UserController.php b/ProcessMaker/Http/Controllers/Api/UserController.php index d08e3374db..ea56691df4 100644 --- a/ProcessMaker/Http/Controllers/Api/UserController.php +++ b/ProcessMaker/Http/Controllers/Api/UserController.php @@ -35,6 +35,48 @@ class UserController extends Controller public $doNotSanitize = [ 'username', // has alpha_dash rule 'password', + 'firstname', // validated as plain text by User::rules() + 'lastname', // validated as plain text by User::rules() + 'title', // validated as plain text by User::rules() + ]; + + /** + * Fields accepted when a non-administrative user updates their own profile. + * + * @var array + */ + private const SELF_SERVICE_UPDATE_FIELDS = [ + 'username', + 'password', + 'firstname', + 'lastname', + 'title', + 'email', + 'address', + 'city', + 'state', + 'postal', + 'country', + 'phone', + 'fax', + 'cell', + 'timezone', + 'datetime_format', + 'status', + 'avatar', + 'preferences_2fa', + 'connected_accounts', + 'meta', + 'valpassword', + ]; + + /** + * Metadata fields accepted during a self-service profile update. + * + * @var array + */ + private const SELF_SERVICE_META_FIELDS = [ + 'disableRecommendations', ]; /** @@ -452,12 +494,22 @@ public function getPinnnedControls(User $user) */ public function update(User $user, Request $request) { - if (!Auth::user()->can('edit', $user)) { + $authenticatedUser = Auth::user(); + if (!$authenticatedUser->can('edit', $user)) { throw new AuthorizationException(__('Not authorized to update this user.')); } - $request->validate(User::rules($user)); $fields = $request->json()->all(); + $isSelfServiceUpdate = $this->authorizeSelfServiceUpdate($authenticatedUser, $user, $fields); + $rules = User::rules($user); + if ($isSelfServiceUpdate) { + $rules['meta'] = ['sometimes', 'array']; + $rules['meta.disableRecommendations'] = ['sometimes', 'boolean']; + } + $request->validate($rules); + if ($isSelfServiceUpdate) { + $fields = $this->normalizeSelfServiceMeta($user, $fields); + } if (isset($fields['password'])) { $fields['password'] = Hash::make($fields['password']); $fields['password_changed_at'] = Carbon::now()->toDateTimeString(); @@ -466,6 +518,7 @@ public function update(User $user, Request $request) session()->forget('login-error'); } $original = $user->getOriginal(); + $isLdapUser = $user->meta?->authenticationType === 'ldap'; $user->fill($fields); if (array_key_exists('cell', $fields)) { $response = $this->validateCellPhoneNumber($user, $fields['cell']); @@ -474,28 +527,24 @@ public function update(User $user, Request $request) } } if ($fields['email'] !== $original['email']) { + $ssoUser = $isLdapUser; if (class_exists(SsoUser::class)) { // Check if the user is an SSO user (including SAML) - $ssoUser = SsoUser::where('user_id', $user->id)->exists(); - - // Check if the user is an LDAP user - if (isset($user->meta?->authenticationType) && $user->meta->authenticationType === 'ldap') { - $ssoUser = true; - } - if ($ssoUser) { - return response([ - 'message' => __( - "The email can't be edited. This action is only available for SSO-synced users." - ), - 'errors' => [ - 'email' => [ - __( - "The email can't be edited. This action is only available for SSO-synced users." - ), - ], + $ssoUser = $ssoUser || SsoUser::where('user_id', $user->id)->exists(); + } + if ($ssoUser) { + return response([ + 'message' => __( + "The email can't be edited. This action is only available for SSO-synced users." + ), + 'errors' => [ + 'email' => [ + __( + "The email can't be edited. This action is only available for SSO-synced users." + ), ], - ], 422); - } + ], + ], 422); } if (!isset($fields['valpassword'])) { return response([ @@ -564,6 +613,59 @@ public function update(User $user, Request $request) return response([], 204); } + /** + * Authorize and constrain self-service profile updates. + */ + private function authorizeSelfServiceUpdate(User $authenticatedUser, User $targetUser, array $fields): bool + { + $isSelfServiceUpdate = $authenticatedUser->id === $targetUser->id + && !$authenticatedUser->is_administrator + && !$authenticatedUser->hasPermission('edit-users'); + + if (!$isSelfServiceUpdate) { + return false; + } + + if (!$authenticatedUser->hasPermission('edit-personal-profile')) { + throw new AuthorizationException(__('Not authorized to update this user.')); + } + + $disallowedFields = array_diff(array_keys($fields), self::SELF_SERVICE_UPDATE_FIELDS); + if ($disallowedFields !== []) { + throw new AuthorizationException(__('Not authorized to update one or more user fields.')); + } + + if (isset($fields['meta']) && is_array($fields['meta'])) { + $disallowedMetaFields = array_diff(array_keys($fields['meta']), self::SELF_SERVICE_META_FIELDS); + if ($disallowedMetaFields !== []) { + throw new AuthorizationException(__('Not authorized to update one or more user fields.')); + } + } + + return true; + } + + /** + * Merge self-service metadata into the persisted server-managed values. + */ + private function normalizeSelfServiceMeta(User $user, array $fields): array + { + $meta = (array) $user->meta; + if ( + array_key_exists('meta', $fields) + && array_key_exists('disableRecommendations', $fields['meta']) + ) { + if ($fields['meta']['disableRecommendations']) { + $meta['disableRecommendations'] = true; + } else { + unset($meta['disableRecommendations']); + } + } + $fields['meta'] = $meta ?: null; + + return $fields; + } + /** * Validate the phone number for SMS two-factor authentication. * diff --git a/ProcessMaker/Http/Middleware/ValidateEditUserAndPasswordPermission.php b/ProcessMaker/Http/Middleware/ValidateEditUserAndPasswordPermission.php index 416abdbb32..b9375fc620 100644 --- a/ProcessMaker/Http/Middleware/ValidateEditUserAndPasswordPermission.php +++ b/ProcessMaker/Http/Middleware/ValidateEditUserAndPasswordPermission.php @@ -15,8 +15,11 @@ public function handle(Request $request, Closure $next) { $user = $request->route('user'); $fields = $request->json()->all(); - if (($fields['username'] !== $user->getAttribute('username') || in_array('password', $fields)) && - !Auth::user()->hasPermission('edit-user-and-password') && !Auth::user()->is_administrator) { + $usernameChanged = array_key_exists('username', $fields) + && $fields['username'] !== $user->getAttribute('username'); + $passwordChanged = array_key_exists('password', $fields); + if (($usernameChanged || $passwordChanged) && + !Auth::user()->hasPermission('edit-user-and-password') && !Auth::user()->is_administrator) { throw new AuthorizationException(__('Not authorized to update the username and password.')); } diff --git a/ProcessMaker/Models/User.php b/ProcessMaker/Models/User.php index c13c133659..87a8ed4487 100644 --- a/ProcessMaker/Models/User.php +++ b/ProcessMaker/Models/User.php @@ -16,6 +16,7 @@ use ProcessMaker\Models\EmptyModel; use ProcessMaker\Notifications\ResetPassword as ResetPasswordNotification; use ProcessMaker\Query\Traits\PMQL; +use ProcessMaker\Rules\PlainText; use ProcessMaker\Rules\StringHasAtLeastOneUpperCaseCharacter; use ProcessMaker\Traits\Exportable; use ProcessMaker\Traits\HasAuthorization; @@ -184,9 +185,10 @@ public static function rules(self $existing = null) return [ // The following characters where not included in the regexp: & % ' " ? / 'username' /****/ => ['required', 'regex:/^[a-zA-Z0-9.!#$*+=^_`|~\-@]+$/', 'min:2', 'max:255', $unique], - 'firstname' /***/ => ['required', 'max:50'], - 'lastname' /****/ => ['required', 'max:50'], + 'firstname' /***/ => ['required', 'max:50', new PlainText()], + 'lastname' /****/ => ['required', 'max:50', new PlainText()], 'email' /*******/ => ['required', 'email'], + 'title' /*******/ => ['nullable', 'max:255', new PlainText()], 'birthdate' /***/ => ['nullable', 'date'], 'phone' /*******/ => ['nullable', 'regex:/^[+\.0-9x\)\(\-\s\/]*$/'], 'fax' /*********/ => ['nullable', 'regex:/^[+\.0-9x\)\(\-\s\/]*$/'], diff --git a/ProcessMaker/Rules/PlainText.php b/ProcessMaker/Rules/PlainText.php new file mode 100644 index 0000000000..f3a5fb76e2 --- /dev/null +++ b/ProcessMaker/Rules/PlainText.php @@ -0,0 +1,25 @@ + + '], + ['title', ''], + ['title', ''], + ]; + + foreach ($payloads as [$field, $markup]) { + $response = $this->apiCall( + 'PUT', + self::API_TEST_URL . '/' . $user->id, + $this->getSelfServiceUpdateData($user, [$field => $markup]) + ); + + $response->assertStatus(422)->assertJsonValidationErrors($field); + } + + $this->assertDatabaseHas('users', [ + 'id' => $user->id, + 'firstname' => 'Original First', + 'lastname' => 'Original Last', + 'title' => 'Original Title', + ]); + } + + public function testUserCreationRejectsProfileMarkup(): void + { + $response = $this->apiCall('POST', self::API_TEST_URL, [ + 'username' => 'four32745qa', + 'firstname' => '<img src=x onerror=alert(document.domain)>', + 'lastname' => 'FOUR-32745 QA', + 'title' => '', + 'email' => 'four32745qa@example.invalid', + 'status' => 'ACTIVE', + 'password' => $this->makePassword(), + ]); + + $response->assertStatus(422)->assertJsonValidationErrors(['firstname', 'title']); + $this->assertDatabaseMissing('users', ['username' => 'four32745qa']); + } + public function testDisableRecommendations() { RecommendationUser::factory()->create([ diff --git a/tests/unit/ProcessMaker/Rules/PlainTextTest.php b/tests/unit/ProcessMaker/Rules/PlainTextTest.php new file mode 100644 index 0000000000..ce414b2609 --- /dev/null +++ b/tests/unit/ProcessMaker/Rules/PlainTextTest.php @@ -0,0 +1,46 @@ +img src=x onerror=alert(document.domain)>', + '', + '', + '', + '', + '', + ]; + + foreach ($payloads as $payload) { + $validator = Validator::make(['value' => $payload], ['value' => [new PlainText()]]); + + $this->assertTrue($validator->fails(), $payload); + } + } + + public function testItAcceptsPlainText(): void + { + $values = [ + 'Éléazar Reséndez', + 'Research & Development', + 'VP < Sales', + '<img src=x>', + 'javascript:alert(document.domain)', + ]; + + foreach ($values as $value) { + $validator = Validator::make(['value' => $value], ['value' => [new PlainText()]]); + + $this->assertFalse($validator->fails(), $value); + } + } +}