User Controller

[sveneld]

PR Type

Enhancement

---

Description

Migrate user management functionality from legacy PHP to Symfony framework:

• Added new UserController with proper CRUD operations and access control
• Created UserRepository to handle database operations
• Added new API endpoints for user management:
  • GET /api/user - List all users
  • GET /api/user/{userId} - Get single user
  • PUT /api/user/{userId} - Update user
• Updated admin UI with:
  • New user management template
  • Advanced search functionality
  • Improved form validation
  • Better error handling
• Removed legacy PHP functions and routes
• Added proper logging for admin actions
• Improved security with proper access control checks

---

Changes walkthrough 📝

	Relevant files
Enhancement	6 files UserController.php Add new User Controller with CRUD operations                          +119/-0  UserRepository.php Add User Repository for database operations                            +74/-0    admin.js Update admin JS to use new User API endpoints                        +116/-93 user.html.twig Add new user management template with search functionality +82/-0    actions-web.php Remove legacy user management functions                                    +0/-31    routes.php Add new User API routes                                                                    +9/-0
Additional files	8 files command.php +0/-18    BikeController.php +1/-1      ControllerEventListener.php +3/-0      ResponseEventListener.php +2/-0      fleet.html.twig +1/-1      index.html.twig +1/-38    base.html.twig +3/-3      messages+intl-icu.en.php +4/-3

---

💡 PR-Agent usage: Comment /help "your question" on any pull request to receive relevant information

[qodo-merge-pro-for-open-source]

CI Failure Feedback 🧐

(Checks updated until commit 722d7ea)

Action: phpcs (7.4)

Failed stage: Run PHP CodeSniffer [❌]

Failed test name: PHP_CodeSniffer

Failure summary: The PHP Code Sniffer (PHPCS) check failed due to code style violations in the file UserRepository.php: A critical error where the closing parenthesis and opening brace of a multi-line function declaration are not on the same line (line 68) A warning about line length exceeding 120 characters (line 71) The error is automatically fixable using PHPCBF (PHP Code Beautifier and Fixer).

Relevant error logs: 1: ##[group]Operating System 2: Ubuntu ... 168: - Locking symfony/asset (v5.4.45) 169: - Locking symfony/cache (v5.4.46) 170: - Locking symfony/cache-contracts (v2.5.4) 171: - Locking symfony/config (v5.4.46) 172: - Locking symfony/console (v5.4.47) 173: - Locking symfony/dependency-injection (v5.4.48) 174: - Locking symfony/deprecation-contracts (v2.5.4) 175: - Locking symfony/dotenv (v5.4.48) 176: - Locking symfony/error-handler (v5.4.46) ... 272: - Downloading symfony/var-exporter (v5.4.45) 273: - Downloading symfony/cache (v5.4.46) 274: - Downloading symfony/expression-language (v5.4.45) 275: - Downloading symfony/routing (v5.4.48) 276: - Downloading symfony/polyfill-php81 (v1.31.0) 277: - Downloading symfony/http-foundation (v5.4.48) 278: - Downloading symfony/event-dispatcher (v5.4.45) 279: - Downloading symfony/var-dumper (v5.4.48) 280: - Downloading symfony/error-handler (v5.4.46) ... 358: - Installing symfony/var-exporter (v5.4.45): Extracting archive 359: - Installing symfony/cache (v5.4.46): Extracting archive 360: - Installing symfony/expression-language (v5.4.45): Extracting archive 361: - Installing symfony/routing (v5.4.48): Extracting archive 362: - Installing symfony/polyfill-php81 (v1.31.0): Extracting archive 363: - Installing symfony/http-foundation (v5.4.48): Extracting archive 364: - Installing symfony/event-dispatcher (v5.4.45): Extracting archive 365: - Installing symfony/var-dumper (v5.4.48): Extracting archive 366: - Installing symfony/error-handler (v5.4.46): Extracting archive ... 409: shell: /usr/bin/bash -e {0} 410: env: 411: COMPOSER_PROCESS_TIMEOUT: 0 412: COMPOSER_NO_INTERACTION: 1 413: COMPOSER_NO_AUDIT: 1 414: ##[endgroup] 415: FILE: .../OpenSourceBikeShare/OpenSourceBikeShare/src/Repository/UserRepository.php 416: -------------------------------------------------------------------------------- 417: FOUND 1 ERROR AND 1 WARNING AFFECTING 2 LINES 418: -------------------------------------------------------------------------------- 419: 68 | ERROR | [x] The closing parenthesis and the opening brace of a 420: | | multi-line function declaration must be on the same line 421: 71 | WARNING | [ ] Line exceeds 120 characters; contains 128 characters 422: -------------------------------------------------------------------------------- 423: PHPCBF CAN FIX THE 1 MARKED SNIFF VIOLATIONS AUTOMATICALLY 424: -------------------------------------------------------------------------------- 425: Time: 2.06 secs; Memory: 12MB 426: ##[error]Process completed with exit code 2.

---

✨ CI feedback usage guide:

---

The CI feedback tool (/checks) automatically triggers when a PR has a failed check.
The tool analyzes the failed checks and provides several feedbacks:

• Failed stage
• Failed test name
• Failure summary
• Relevant error logs

In addition to being automatically triggered, the tool can also be invoked manually by commenting on a PR:

/checks "https://github.com/{repo_name}/actions/runs/{run_number}/job/{job_number}"

where {repo_name} is the name of the repository, {run_number} is the run number of the failed check, and {job_number} is the job number of the failed check.

Configuration options

• enable_auto_checks_feedback - if set to true, the tool will automatically provide feedback when a check is failed. Default is true.
• excluded_checks_list - a list of checks to exclude from the feedback, for example: ["check1", "check2"]. Default is an empty list.
• enable_help_text - if set to true, the tool will provide a help message with the feedback. Default is true.
• persistent_comment - if set to true, the tool will overwrite a previous checks comment with the new feedback. Default is true.
• final_update_message - if persistent_comment is true and updating a previous checks message, the tool will also create a new message: "Persistent checks updated to latest commit". Default is true.

See more information about the checks tool in the docs.

[qodo-merge-pro-for-open-source]

PR Reviewer Guide 🔍

Here are some key observations to aid the review process:

⏱️ Estimated effort to review: 4 🔵🔵🔵🔵⚪

🧪 No relevant tests

🔒 Security concerns SQL Injection: The UserRepository class uses string concatenation to build SQL queries with user input in findItem() and updateItem() methods. This is a serious security vulnerability that could allow attackers to manipulate the database queries. The queries should be rewritten using prepared statements or query builders. XSS: Several templates directly output user-provided data without proper escaping. This could allow cross-site scripting attacks. All user data should be properly escaped using Twig's built-in escaping. Privilege Escalation: The user update endpoint lacks proper validation of privilege levels and user limits, potentially allowing users to escalate their privileges through manipulated requests. Additional authorization checks should be added.

⚡ Recommended focus areas for review SQL Injection The findItem() and updateItem() methods concatenate user input directly into SQL queries without proper escaping or parameterization, making them vulnerable to SQL injection attacks WHERE users.userId=' . $userId )->fetchAssoc(); return $user; } public function updateItem(int $userId, string $username, string $email, string $number, int $privileges, int $userLimit) { $this->db->query( 'UPDATE users SET username="' . $username . '", mail="' . $email . '", number="' . $number . '", privileges=' . $privileges . ' WHERE userId=' . $userId ); $this->db->query( 'UPDATE limits SET userLimit=' . $userLimit . ' WHERE userId=' . $userId ); Input Validation The update() method performs minimal input validation but does not validate privilege levels or user limits, potentially allowing unauthorized privilege escalation $userName = $request->request->get('username'); $email = $request->request->get('email'); $number = $request->request->get('number'); $privileges = $request->request->getInt('privileges'); $userLimit = $request->request->getInt('userLimit'); XSS Vulnerability User data is rendered directly in the HTML without proper escaping, potentially allowing cross-site scripting attacks <label for="username" class="control-label">{{ 'Fullname:'|trans }}</label> <input type="text" name="username" id="username" class="form-control"/> </div> <div class="form-group"> <label for="email">{{ 'Email:'|trans }}</label> <input type="email" name="email" id="email" class="form-control"/> </div> {% if configuration.get('connectors')['sms'] %} <div class="form-group"> <label for="phone">{{ 'Phone number:'|trans }}</label> <input type="number" name="phone" id="phone" class="form-control"/> </div> {% endif %} <div class="form-group"> <label for="privileges">{{ 'Privileges:'|trans }}</label> <input type="number" name="privileges" id="privileges" class="form-control"/> </div> <div class="form-group"> <label for="limit">{{ 'Bike limit:'|trans }}</label> <input type="number" name="limit" id="limit" class="form-control"/> </div>

[qodo-merge-pro-for-open-source]

PR Code Suggestions ✨

Explore these optional code suggestions:

Category	Suggestion	Score
Security	Use parameterized queries to prevent SQL injection vulnerabilities The SQL queries in UserRepository are vulnerable to SQL injection attacks. Use parameterized queries with prepared statements instead of directly concatenating values into the SQL string. src/Repository/UserRepository.php [63-66] -$this->db->query( +$stmt = $this->db->prepare( 'UPDATE users - SET username="' . $username . '", mail="' . $email . '", number="' . $number . '", privileges=' . $privileges . ' - WHERE userId=' . $userId + SET username=?, mail=?, number=?, privileges=? + WHERE userId=?' ); +$stmt->bind_param('sssii', $username, $email, $number, $privileges, $userId); +$stmt->execute(); Apply this suggestion Suggestion importance[1-10]: 10 Why: Direct string concatenation in SQL queries creates critical security vulnerabilities through SQL injection attacks. Using parameterized queries is essential for security.	10
	Add comprehensive input validation and sanitization for user data The update method should sanitize and validate all input data before processing. Add proper validation for username format, email format, and privilege levels. src/Controller/Api/UserController.php [93-97] -$userName = $request->request->get('username'); -$email = $request->request->get('email'); -$number = $request->request->get('number'); +$userName = trim($request->request->get('username')); +if (strlen($userName) < 2 || strlen($userName) > 50) { + return $this->json(['error' => 'Invalid username length'], Response::HTTP_BAD_REQUEST); +} + +$email = filter_var($request->request->get('email'), FILTER_SANITIZE_EMAIL); +if (!filter_var($email, FILTER_VALIDATE_EMAIL)) { + return $this->json(['error' => 'Invalid email format'], Response::HTTP_BAD_REQUEST); +} + $privileges = $request->request->getInt('privileges'); -$userLimit = $request->request->getInt('userLimit'); +if ($privileges < 0 || $privileges > 5) { // Define valid privilege levels + return $this->json(['error' => 'Invalid privilege level'], Response::HTTP_BAD_REQUEST); +} Apply this suggestion Suggestion importance[1-10]: 9 Why: Current code lacks proper input validation, allowing potentially malicious or invalid data to be processed. Adding validation is critical for security and data integrity.	9
Possible issue	Validate entity existence before performing update operations The update method should validate that the user being modified exists before attempting updates. Add a check to verify the user exists and return a 404 response if not found. src/Controller/Api/UserController.php [71-77] public function update( $userId, Request $request, UserRepository $userRepository, Configuration $configuration, LoggerInterface $logger ): Response { + // ... existing permission checks ... + + $user = $userRepository->findItem((int)$userId); + if (!$user) { + return $this->json(['error' => 'User not found'], Response::HTTP_NOT_FOUND); + } Suggestion importance[1-10]: 8 Why: Missing existence check could lead to silent failures or inconsistent state. Adding 404 response for non-existent users improves API robustness and error handling.	8