228 refaire la structure du code - #230
Conversation
…Repository to enhance dependency inversion and testability
…rgotPasswordController
…rgotPasswordController
…s to enhance dependency inversion
…' into 228-refaire-la-structure-du-code
…name handling for database constraints
There was a problem hiding this comment.
Pull request overview
Refactor structure “228” by introducing lightweight dependency injection and port-based abstractions (DIP), while also tightening password policy and improving import/dashboard UX.
Changes:
- Add a simple service container and wire it into the Router to support controller dependency injection.
- Introduce “Ports” interfaces for use cases and update repositories/use cases to depend on abstractions.
- Update import flow + dashboard/footer views, enforce stronger password rules (12 chars + upper/lower + special), and rename attempt user field to
user_id.
Reviewed changes
Copilot reviewed 40 out of 43 changed files in this pull request and generated 13 comments.
Show a summary per file
| File | Description |
|---|---|
| public/js/password-toggle.js | New shared password visibility toggle helper (used by login). |
| public/js/modules/import.js | More robust resourceId handling, chunk progress, and response/error parsing. |
| public/css/dashboard.css | Adds fixed dashboard footer styling. |
| index.php | Registers a DI container and attaches it to the router. |
| Core/Service/Container.php | New lightweight DI container for controller factories. |
| Core/Router/Router.php | Resolves controllers via container when available, fallback to new. |
| App/View/user/resource_details.php | Adjusts script loading + injects window.BASE_URL/RESOURCE_ID; footer updates. |
| App/View/user/dashboard.php | Ensures JS globals are set before dashboard-main; footer updates. |
| App/View/resources/details.php | Injects globals for import JS; footer updates. |
| App/View/auth/reset-password.php | Enforces stronger password requirements in UI + client-side checks. |
| App/View/auth/register.php | Enforces stronger password requirements in UI + client-side checks. |
| App/View/auth/login.php | Updates canonical URL, shows success message, uses external password toggle script. |
| App/Model/UserRepository.php | Implements new auth/registration ports; adjusts reset token persistence formatting. |
| App/Model/UseCase/RegisterUserUseCase.php | Depends on UserRegistrationPort; adds strong password validation. |
| App/Model/UseCase/Ports/UserRegistrationPort.php | New registration port. |
| App/Model/UseCase/Ports/UserFinderPort.php | New shared finder port. |
| App/Model/UseCase/Ports/UserAuthFinderPort.php | New login port. |
| App/Model/UseCase/Ports/ExerciseLookupPort.php | New exercise lookup port for imports. |
| App/Model/UseCase/Ports/ExerciseListReaderPort.php | New exercise list reader port. |
| App/Model/UseCase/Ports/ExerciseImporterPort.php | New exercise import persistence port. |
| App/Model/UseCase/Ports/ExerciseFinderPort.php | New base exercise finder port. |
| App/Model/UseCase/Ports/AttemptBulkInserterPort.php | New bulk insert port for attempts. |
| App/Model/UseCase/LoginUserUseCase.php | Depends on UserAuthFinderPort abstraction. |
| App/Model/UseCase/ListExercisesUseCase.php | Depends on ExerciseListReaderPort. |
| App/Model/UseCase/ImportExercisesUseCase.php | Depends on ExerciseImporterPort; normalizes/truncates imported fields. |
| App/Model/UseCase/ImportAttemptsUseCase.php | Depends on ports; normalizes/truncates imported attempt fields. |
| App/Model/ResourceRepositoryInterface.php | New interface consumed by ResourcesController. |
| App/Model/ResourceRepository.php | Implements interface; adjusts owner/share column naming; truncates resource name. |
| App/Model/ExerciseRepository.php | Implements multiple ports; truncates inputs; adjusts corrections join key. |
| App/Model/Entity/Resource.php | Updates schema comment. |
| App/Model/Entity/Attempt.php | Renames user to userId / user_id mapping. |
| App/Model/EmailService.php | Updates reset URL to new route format. |
| App/Model/AuthenticationServiceInterface.php | New auth service interface for controller-level DIP. |
| App/Model/AuthenticationService.php | Implements interface; adds first/last name getters; depends on SessionServiceInterface. |
| App/Model/AttemptRepository.php | Implements bulk insert port; switches to user_id column. |
| App/Controller/ResourcesController.php | Uses constructor DI via interfaces; removes direct session access. |
| App/Controller/LoginController.php | Documentation aligned with new port-based use case. |
| App/Controller/ImportController.php | Explicit JSON Content-Type header for import endpoints. |
| App/Controller/ForgotPasswordController.php | Uses isResetTokenValid(); enforces stronger passwords; adds success message on login. |
| App/Controller/DashboardApiController.php | Switches queries from user to user_id; adjusts corrections join key. |
| .vs/SAE/v17/.wsuo | Visual Studio workspace artifact added (should not be committed). |
Comments suppressed due to low confidence (3)
App/Controller/ResourcesController.php:136
- The
store()action handles sensitive state-changing operations (creating resources and sharing them) but does not implement any CSRF protection – it relies only on the session cookie andrequireAuth. An attacker can lure a logged-in teacher to a malicious page that silently issues a POST to this endpoint, creating or sharing resources in their name. Add CSRF tokens (server-generated, per-session/per-form values) that are validated before processing the request, and reject requests missing or with invalid tokens.
public function store(): void
{
$this->authService->requireAuth('/auth/login');
$email = $this->authService->getUserEmail();
if ($email === null) {
$this->redirect('/auth/login');
return;
}
$name = trim($this->getPost('name', ''));
$description = trim($this->getPost('description', ''));
if ($name === '') {
$this->redirect('/resources?error=Le+nom+de+la+ressource+est+obligatoire.');
return;
}
// Handle image upload
$imagePath = $this->handleImageUpload();
$resource = new \App\Model\Entity\Resource();
$resource->setOwnerMail($email);
$resource->setResourceName($name);
$resource->setDescription($description !== '' ? $description : null);
$resource->setImagePath($imagePath);
try {
$this->resourceRepository->save($resource);
// Sync sharing list
$sharedMails = $_POST['shared_teachers'] ?? [];
if (!empty($sharedMails) && is_array($sharedMails)) {
$this->resourceRepository->syncSharing($resource->getResourceId(), $sharedMails);
}
} catch (\Throwable $e) {
error_log('[ResourcesController::store] ' . $e->getMessage());
$this->redirect('/resources?error=' . urlencode('Erreur lors de la création : ' . $e->getMessage()));
return;
}
$this->redirect('/resources');
}
App/Controller/ResourcesController.php:243
- The
update()action performs privileged modifications (editing resource metadata and synchronizing sharing) but accepts POSTs based solely on the user’s session, with no CSRF token or origin protection. This allows a CSRF attack where a malicious site submits a crafted form to this URL, causing unintended edits or sharing changes for an authenticated teacher. Introduce CSRF protection by including a server-generated token in the edit form and verifying it on submission before applying any updates.
public function update(int $resourceId): void
{
$this->authService->requireAuth('/auth/login');
$email = $this->authService->getUserEmail();
if ($email === null) {
$this->redirect('/auth/login');
return;
}
try {
$resource = $this->resourceRepository->findById($resourceId);
} catch (\Throwable $e) {
error_log('[ResourcesController::update] findById: ' . $e->getMessage());
$resource = null;
}
if (!$resource) {
$this->redirect('/resources?error=Ressource+introuvable.');
return;
}
if ($resource->getOwnerMail() !== $email) {
$this->redirect('/resources?error=Action+non+autorisée.');
return;
}
$name = trim($this->getPost('name', ''));
$description = trim($this->getPost('description', ''));
if ($name === '') {
$this->redirect('/resources?error=Le+nom+de+la+ressource+est+obligatoire.');
return;
}
// Handle image upload (keep existing if no new file)
$newImagePath = $this->handleImageUpload();
$imagePath = $newImagePath ?? $resource->getImagePath();
$resource->setResourceName($name);
$resource->setDescription($description !== '' ? $description : null);
$resource->setImagePath($imagePath);
try {
$this->resourceRepository->save($resource);
// Sync sharing list
$sharedMails = $_POST['shared_teachers'] ?? [];
$this->resourceRepository->syncSharing($resourceId, is_array($sharedMails) ? $sharedMails : []);
} catch (\Throwable $e) {
error_log('[ResourcesController::update] save: ' . $e->getMessage());
$this->redirect('/resources?error=' . urlencode('Erreur lors de la mise à jour : ' . $e->getMessage()));
return;
}
$this->redirect('/resources');
}
App/Controller/ResourcesController.php:285
- The
delete()action deletes resources (and related exercises/attempts) purely based on an authenticated session, without any CSRF token or additional verification. An attacker can exploit this by tricking a logged-in teacher into loading a page that auto-submits a POST to this endpoint, resulting in unauthorized deletions. Protect this route with CSRF tokens (validated on POST) and consider requiring an explicit confirmation step or re-authentication for destructive actions.
public function delete(int $resourceId): void
{
$this->authService->requireAuth('/auth/login');
$email = $this->authService->getUserEmail();
if ($email === null) {
$this->redirect('/auth/login');
return;
}
try {
$resource = $this->resourceRepository->findById($resourceId);
} catch (\Throwable $e) {
error_log('[ResourcesController::delete] findById: ' . $e->getMessage());
$resource = null;
}
if (!$resource) {
$this->redirect('/resources?error=Ressource+introuvable.');
return;
}
if ($resource->getOwnerMail() !== $email) {
$this->redirect('/resources?error=Action+non+autorisée.');
return;
}
try {
$this->resourceRepository->delete($resourceId);
} catch (\Throwable $e) {
// Suppression échouée silencieusement — on redirige quand même
error_log('[ResourcesController::delete] ' . $e->getMessage());
}
$this->redirect('/resources');
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
You can also share your feedback on Copilot code review. Take the survey.
| if (!isset($this->factories[$id])) { | ||
| throw new \RuntimeException("No factory registered for: {$id}"); | ||
| } | ||
|
|
||
| $this->instances[$id] = ($this->factories[$id])($this); | ||
| return $this->instances[$id]; |
There was a problem hiding this comment.
Container::get() invoque la factory avec un argument ($this), mais Container::set() n’impose pas/mentionne cette signature. Avec une factory déclarée sans paramètre (ex. dans index.php), cela déclenchera un ArgumentCountError au runtime. Harmoniser l’API : soit appeler la factory sans argument, soit documenter/typer que la factory accepte le Container et mettre à jour les factories existantes en conséquence.
| <p>© <?= date('Y') ?> StudTraj - Tous droits réservés</p> | ||
| <ul class="footer-links"> | ||
| <li><a href="<?= BASE_URL ?>/index.php?action=mentions">Mentions légales</a></li> | ||
| <li><a href="<?= BASE_URL ?>/mentions-legales">Mentions légales</a></li> |
There was a problem hiding this comment.
Le lien footer pointe vers <?= BASE_URL ?>/mentions-legales, mais aucune route GET /mentions-legales n’est définie dans App/routes.php (seulement les routes /auth/*, /resources, /dashboard, etc.). Cela renverra une 404. Ajouter la route correspondante (et le contrôleur/vue) ou conserver l’ancienne URL tant que la route n’existe pas.
| <!DOCTYPE html> | ||
| <html lang="fr"> |
There was a problem hiding this comment.
Le fichier commence par un BOM (UTF-8) avant <!DOCTYPE html>. Même si ce template est HTML, ces octets invisibles peuvent polluer certaines réponses (et compliquer des header()/redirects si inclus ailleurs). Enregistrer le fichier en UTF-8 sans BOM.
| <!DOCTYPE html> | ||
| <html lang="fr"> | ||
| <head> |
There was a problem hiding this comment.
Le fichier commence par un BOM (UTF-8) avant <!DOCTYPE html>. Cela peut introduire des caractères parasites en sortie et, dans certains cas, déclencher des problèmes de headers si un redirect/header est émis avant/pendant le rendu. Enregistrer en UTF-8 sans BOM.
| <!DOCTYPE html> | ||
| <html lang="fr"> | ||
| <head> |
There was a problem hiding this comment.
Le fichier commence par un BOM (UTF-8) avant <!DOCTYPE html>. Comme pour les autres vues, supprimer le BOM (UTF-8 sans BOM) pour éviter des octets invisibles en sortie.
| <?php | ||
| if (!defined('BASE_URL')) { define('BASE_URL', ''); } | ||
| if (session_status() === PHP_SESSION_NONE) { session_start(); } |
There was a problem hiding this comment.
Le fichier commence par des caractères BOM invisibles (\uFEFF) avant <?php. Comme ce template appelle session_start() juste après, ces octets peuvent provoquer des erreurs « headers already sent » (cookie de session, redirections, etc.). Supprimer le BOM et s’assurer que le fichier est enregistré en UTF-8 sans BOM.
| <ul class="footer-links"> | ||
| <li><a href="<?= BASE_URL ?>/index.php?action=mentions">Mentions légales</a></li> | ||
| <li><a href="/mentions-legales">Mentions légales</a></li> | ||
| </ul> |
There was a problem hiding this comment.
Le lien est codé en dur en absolu (/mentions-legales) et ignore BASE_URL (alors que les autres liens utilisent BASE_URL). Si l’application est servie sous un sous-répertoire, ce lien cassera. De plus, aucune route /mentions-legales n’est définie dans App/routes.php, donc il renverra 404 tant qu’elle n’existe pas.
| <p>© <?= date('Y') ?> StudTraj - Tous droits réservés</p> | ||
| <ul class="footer-links"> | ||
| <li><a href="<?= BASE_URL ?>/index.php?action=mentions">Mentions légales</a></li> | ||
| <li><a href="<?= BASE_URL ?>/mentions-legales">Mentions légales</a></li> | ||
| </ul> |
There was a problem hiding this comment.
Le lien footer pointe vers <?= BASE_URL ?>/mentions-legales, mais aucune route GET /mentions-legales n’est déclarée dans App/routes.php. Sans route, ce lien renverra 404. Ajouter la route correspondante ou conserver l’ancienne URL jusqu’à disponibilité.
| $this->renderView('auth/login', [ | ||
| 'success_message' => 'Votre mot de passe a été réinitialisé avec succès. Vous pouvez vous connecter.', | ||
| ]); |
There was a problem hiding this comment.
Après un POST de réinitialisation, le contrôleur rend directement la vue de login au lieu de rediriger. Cela casse le pattern PRG et expose à une resoumission du POST au refresh (et potentiellement à des messages incohérents). Préférer un redirect vers /auth/login avec un mécanisme de message flash (session) ou un query param, puis afficher le message sur la page de login.
| $this->renderView('auth/login', [ | |
| 'success_message' => 'Votre mot de passe a été réinitialisé avec succès. Vous pouvez vous connecter.', | |
| ]); | |
| $message = urlencode('Votre mot de passe a été réinitialisé avec succès. Vous pouvez vous connecter.'); | |
| $this->redirect('/auth/login?success_message=' . $message); | |
| return; |
| // Initialize router | ||
| use Core\Router\Router; | ||
| use Core\Service\Container; | ||
|
|
There was a problem hiding this comment.
Les use (imports de namespace) apparaissent après du code exécutable (ob_start(), handlers, etc.). En PHP, les use doivent être déclarés en tête de fichier (après un éventuel namespace et avant toute instruction), sinon cela provoque une erreur de parsing. Déplacer ces use tout en haut, ou remplacer par des noms pleinement qualifiés (new \Core\Router\Router(), etc.).
No description provided.