HNNW/core
Watch
28
2
Fork
You've already forked core
 0
Code Issues Pull requests 8 Releases Activity Actions

team-1/development #35

Open
rani-harcharan wants to merge 28 commits from team-1/development into develop
pull from: team-1/development
merge into: HNNW:develop
Conversation 2 Commits 28 Files changed 31 +2126 -232
rani-harcharan commented 2026-10-05 12:24:37 +00:00
Member
Copy link
No description provided. 
Reviewed-on: #16
Reviewed-on: #22
Reviewed-on: #26
rileydeman requested changes 2026-10-12 12:52:54 +00:00
rileydeman left a comment
Copy link

Goed werk aan de RBAC-basis. De Permission-enum met maskFor()/in() is overzichtelijk, de voters controleren echt per object (eigenaar of actieve participant met rol), en het is fijn dat er unit-tests bij de voters zitten. Ook goed dat dd() en de /dev/set-user-route weg zijn, en dat het loginformulier nu een echte CSRF-token meestuurt.

Dit moet nog aangepast worden:

Moet opgelost worden

  1. config/packages/security.yaml: de firewall gebruikt provider: account_provider, maar die provider staat niet onder providers:. Daar staan alleen users_in_memory en app_user_provider. In commit 06d9a6c stond hij er nog, maar bij "Resolve merge conflicts" (4e5a868) is hij weggevallen. Daardoor compileert de container niet en draait de hele applicatie niet. Zet hem terug (account_provider: { id: App\Security\AccountUserProvider }) en haal de ongebruikte providers weg.
  2. Na het inloggen wordt account_id niet meer in de sessie gezet, maar AccountsController (Account Centre) leest dat op 8 plekken nog met $session->get('account_id'). Een ingelogde gebruiker krijgt in het Account Centre dus steeds "Je moet ingelogd zijn" en wordt naar de login gestuurd. Zet die plekken om naar $this->getUser() (dat is het hele doel van deze PR). Of zet account_id tijdelijk ook in AccountAuthenticator::onAuthenticationSuccess() en spreek af wie het later opruimt.
  3. Er is geen entry point: AccountAuthenticator implementeert geen AuthenticationEntryPointInterface en er is geen form_login. Een niet-ingelogde bezoeker op /org, /admin of een projectpagina krijgt daardoor een 401 in plaats van een redirect naar de login. Er is ook geen errors/401.html.twig of errors/500.html.twig, dus hij ziet alleen de platte tekst "Er is iets misgegaan.". Voeg start() toe aan AccountAuthenticator, met een redirect naar auth.login.
  4. ProjectsController::detail() vereist nu ProjectVoter::VIEW, net als chat, participants, reviews, tasks, updates en workPackages. VIEW krijgen alleen de eigenaar en actieve participants met een rol die VIEW heeft. Een bezoeker die zich wil aanmelden kan de projectpagina dus niet meer zien. Inschrijven op een taak (enroll) vereist bovendien PackageTaskVoter::EDIT, dus een participant met alleen VIEW kan zich niet inschrijven. Is dat bewust? Zo niet: houd detail (en eventueel apply) openbaar voor zichtbare projecten en gebruik VIEW voor het inschrijven.
  5. Er is een merge-conflict met develop in src/Entity/Project/WorkPackage.php. develop heeft dezelfde bug (packageTasks vs workPackageTasks) al opgelost in 040225e. Merge develop in je branch en houd één $workPackageTasks-property over.
  6. .env.local.example is verwijderd, terwijl de README.md er nog naar verwijst (cp .env.local.example .env.local). Nieuwe ontwikkelaars kunnen hun omgeving dan niet meer opzetten. Zet deze file terug.
  7. config/packages/framework.yaml: error_controller wordt voor de hele app vervangen door ErrorController::show. Daardoor ziet geen enkel team in dev nog de Symfony-debugpagina met de stacktrace. Verder is errors/404.html.twig leeg (een lege 404-pagina) en bestaat errors/500.html.twig niet. Gebruik de standaardmanier: templates/bundles/TwigBundle/Exception/error403.html.twig (en error404/error.html.twig). Haal dan error_controller en ErrorController weg.

Verbeterpunten
8. AuthController::register(): bij een fout geef je errors.general mee, maar register.html.twig toont die nergens. De gebruiker ziet dus geen melding. Gebruik addFlash('error', ...) of toon errors.general in de template. De inspringing in dat catch-blok klopt ook niet.
9. WorkPackageVoter en PackageTaskVoter kopiëren de logica van ProjectVoter met losse getallen (& 1, & 2, & 4). Gebruik $role->hasPermission(ProjectRole::PERMISSION_VIEW) enz., of laat ze via AccessDecisionManagerInterface doorvragen aan ProjectVoter voor het bijbehorende project. Dan staat de regel op één plek. Je hebt nu ook twee permission-systemen naast elkaar (de Permission-enum en ProjectRole::PERMISSION_*). Overweeg de projectrechten ook in de enum te zetten.
10. PanelController::modifyProject() controleert naast ProjectVoter::EDIT ook ORG_MANAGE_PROJECTS, maar de sub-pagina's (matching, participants, tasks, …) doen dat niet. Wie de hoofdpagina niet mag openen, kan de sub-pagina's dus wel openen. Kies één regel en pas die overal toe, bijvoorbeeld in getProjectAndCheckEditAccess().
11. Kleine opruimpunten: de inspringing in ProfileController (getAccount()/index()) is kapot, updateLocation() heeft een ongebruikte $em, en getProjectAndCheckViewAccess() geeft object terug in plaats van Project. In veel bestanden ontbreekt nu de newline aan het eind. Zet ook een beschrijving in de PR, met de vermelding dat je bin/console app:rbac:seed-roles moet draaien.

Let op: PR #15 (team 6) wijzigt security.yaml, AuthController::login() en login.html.twig ook, maar met form_login, een chain-provider en logout via account.signoff. Dat zijn twee verschillende login-implementaties: stem af welke het wordt voordat een van beide gemerged wordt. PR #28 (team 5) hernoemt ProjectParticipantRepository::findActiveParticipant() naar findActiveParticipantsByProjectAndProfile(). Daarop leunen jullie voters, dus als #28 eerst gemerged wordt, breken ze. PR #12 (team 3) wijzigt ook de enroll-actie in ProjectsController.

Goed werk aan de RBAC-basis. De `Permission`-enum met `maskFor()`/`in()` is overzichtelijk, de voters controleren echt per object (eigenaar of actieve participant met rol), en het is fijn dat er unit-tests bij de voters zitten. Ook goed dat `dd()` en de `/dev/set-user`-route weg zijn, en dat het loginformulier nu een echte CSRF-token meestuurt. Dit moet nog aangepast worden: **Moet opgelost worden** 1. `config/packages/security.yaml`: de firewall gebruikt `provider: account_provider`, maar die provider staat niet onder `providers:`. Daar staan alleen `users_in_memory` en `app_user_provider`. In commit `06d9a6c` stond hij er nog, maar bij "Resolve merge conflicts" (`4e5a868`) is hij weggevallen. Daardoor compileert de container niet en draait de hele applicatie niet. Zet hem terug (`account_provider: { id: App\Security\AccountUserProvider }`) en haal de ongebruikte providers weg. 2. Na het inloggen wordt `account_id` niet meer in de sessie gezet, maar `AccountsController` (Account Centre) leest dat op 8 plekken nog met `$session->get('account_id')`. Een ingelogde gebruiker krijgt in het Account Centre dus steeds "Je moet ingelogd zijn" en wordt naar de login gestuurd. Zet die plekken om naar `$this->getUser()` (dat is het hele doel van deze PR). Of zet `account_id` tijdelijk ook in `AccountAuthenticator::onAuthenticationSuccess()` en spreek af wie het later opruimt. 3. Er is geen entry point: `AccountAuthenticator` implementeert geen `AuthenticationEntryPointInterface` en er is geen `form_login`. Een niet-ingelogde bezoeker op `/org`, `/admin` of een projectpagina krijgt daardoor een 401 in plaats van een redirect naar de login. Er is ook geen `errors/401.html.twig` of `errors/500.html.twig`, dus hij ziet alleen de platte tekst "Er is iets misgegaan.". Voeg `start()` toe aan `AccountAuthenticator`, met een redirect naar `auth.login`. 4. `ProjectsController::detail()` vereist nu `ProjectVoter::VIEW`, net als `chat`, `participants`, `reviews`, `tasks`, `updates` en `workPackages`. VIEW krijgen alleen de eigenaar en actieve participants met een rol die VIEW heeft. Een bezoeker die zich wil aanmelden kan de projectpagina dus niet meer zien. Inschrijven op een taak (`enroll`) vereist bovendien `PackageTaskVoter::EDIT`, dus een participant met alleen VIEW kan zich niet inschrijven. Is dat bewust? Zo niet: houd `detail` (en eventueel `apply`) openbaar voor zichtbare projecten en gebruik VIEW voor het inschrijven. 5. Er is een merge-conflict met `develop` in `src/Entity/Project/WorkPackage.php`. `develop` heeft dezelfde bug (`packageTasks` vs `workPackageTasks`) al opgelost in `040225e`. Merge `develop` in je branch en houd één `$workPackageTasks`-property over. 6. `.env.local.example` is verwijderd, terwijl de `README.md` er nog naar verwijst (`cp .env.local.example .env.local`). Nieuwe ontwikkelaars kunnen hun omgeving dan niet meer opzetten. Zet deze file terug. 7. `config/packages/framework.yaml`: `error_controller` wordt voor de hele app vervangen door `ErrorController::show`. Daardoor ziet geen enkel team in dev nog de Symfony-debugpagina met de stacktrace. Verder is `errors/404.html.twig` leeg (een lege 404-pagina) en bestaat `errors/500.html.twig` niet. Gebruik de standaardmanier: `templates/bundles/TwigBundle/Exception/error403.html.twig` (en `error404`/`error.html.twig`). Haal dan `error_controller` en `ErrorController` weg. **Verbeterpunten** 8. `AuthController::register()`: bij een fout geef je `errors.general` mee, maar `register.html.twig` toont die nergens. De gebruiker ziet dus geen melding. Gebruik `addFlash('error', ...)` of toon `errors.general` in de template. De inspringing in dat `catch`-blok klopt ook niet. 9. `WorkPackageVoter` en `PackageTaskVoter` kopiëren de logica van `ProjectVoter` met losse getallen (`& 1`, `& 2`, `& 4`). Gebruik `$role->hasPermission(ProjectRole::PERMISSION_VIEW)` enz., of laat ze via `AccessDecisionManagerInterface` doorvragen aan `ProjectVoter` voor het bijbehorende project. Dan staat de regel op één plek. Je hebt nu ook twee permission-systemen naast elkaar (de `Permission`-enum en `ProjectRole::PERMISSION_*`). Overweeg de projectrechten ook in de enum te zetten. 10. `PanelController::modifyProject()` controleert naast `ProjectVoter::EDIT` ook `ORG_MANAGE_PROJECTS`, maar de sub-pagina's (`matching`, `participants`, `tasks`, …) doen dat niet. Wie de hoofdpagina niet mag openen, kan de sub-pagina's dus wel openen. Kies één regel en pas die overal toe, bijvoorbeeld in `getProjectAndCheckEditAccess()`. 11. Kleine opruimpunten: de inspringing in `ProfileController` (`getAccount()`/`index()`) is kapot, `updateLocation()` heeft een ongebruikte `$em`, en `getProjectAndCheckViewAccess()` geeft `object` terug in plaats van `Project`. In veel bestanden ontbreekt nu de newline aan het eind. Zet ook een beschrijving in de PR, met de vermelding dat je `bin/console app:rbac:seed-roles` moet draaien. Let op: PR #15 (team 6) wijzigt `security.yaml`, `AuthController::login()` en `login.html.twig` ook, maar met `form_login`, een chain-provider en logout via `account.signoff`. Dat zijn twee verschillende login-implementaties: stem af welke het wordt voordat een van beide gemerged wordt. PR #28 (team 5) hernoemt `ProjectParticipantRepository::findActiveParticipant()` naar `findActiveParticipantsByProjectAndProfile()`. Daarop leunen jullie voters, dus als #28 eerst gemerged wordt, breken ze. PR #12 (team 3) wijzigt ook de `enroll`-actie in `ProjectsController`.
patrick requested changes 2026-10-12 13:30:55 +00:00
patrick left a comment
Copy link

Tijdens mijn testen heb ik geen fouten gevonden, maar ik heb nog de volgende opmerkingen over de code.

  1. Bij src/Module/Main/Controller/ProfileController.php waarom bestaat er een getAccount() functie die alleen $this->getUser() returned. Je kan $this->getUser() direct gebruiken inplaats van de functie.
  2. Bij src/Module/Main/Controller/ProjectsController.php en src/Module/Org/Controller/PanelController.php kan de gesimplificeerd met gebruik van parameterconversion(https://symfony.com/doc/current/routing.html#parameter-conversion). Verder moeten projecten, etc een 404 error terug geven wanneer een onbevoegde gebruiker toegang tot een project probeert te krijgen, om te voorkomen dat ze er achter kunnen komen of bepaalde projecten bestaan. Dit ziet er dan als volgend uit:

#[Route('/projects/modify/{id}/matching', name: 'modifyProject.matching', methods: ['GET'])]'
// onbevoegde gebruikers krijgen een 404
#[IsGranted(ProjectVoter::VIEW,'project','Project niet gevonden','404')]
// Als een gebruiker wel toegang heeft maar niet mag aanpassen krijgen ze een 403
#[IsGranted(ProjectVoter::EDIT,'project')]
public function modifyProjectMatching(
// symfony bind autmatisch het id aan project en haalt het op
// als je en slug gebruikt inplaats van id kan je in de route {slug:project} gebruiken om de slug te binden
Project $project,
): Response { '
'return $this->render('pages/org/projects/modify-matching.html.twig', [
'id' => $project->getId(),
'project' => $project,
]);
}

Tijdens mijn testen heb ik geen fouten gevonden, maar ik heb nog de volgende opmerkingen over de code. 1. Bij src/Module/Main/Controller/ProfileController.php waarom bestaat er een getAccount() functie die alleen $this->getUser() returned. Je kan $this->getUser() direct gebruiken inplaats van de functie. 2. Bij src/Module/Main/Controller/ProjectsController.php en src/Module/Org/Controller/PanelController.php kan de gesimplificeerd met gebruik van parameterconversion(https://symfony.com/doc/current/routing.html#parameter-conversion). Verder moeten projecten, etc een 404 error terug geven wanneer een onbevoegde gebruiker toegang tot een project probeert te krijgen, om te voorkomen dat ze er achter kunnen komen of bepaalde projecten bestaan. Dit ziet er dan als volgend uit: `#[Route('/projects/modify/{id}/matching', name: 'modifyProject.matching', methods: ['GET'])]'` `// onbevoegde gebruikers krijgen een 404` `#[IsGranted(ProjectVoter::VIEW,'project','Project niet gevonden','404')]` ` // Als een gebruiker wel toegang heeft maar niet mag aanpassen krijgen ze een 403` ` #[IsGranted(ProjectVoter::EDIT,'project')]` `public function modifyProjectMatching(` `// symfony bind autmatisch het id aan project en haalt het op` `// als je en slug gebruikt inplaats van id kan je in de route {slug:project} gebruiken om de slug te binden` `Project $project,` `): Response { '` `'return $this->render('pages/org/projects/modify-matching.html.twig', [` ` 'id' => $project->getId(),` ` 'project' => $project,` `]);` ` }`
This pull request has changes conflicting with the target branch.
  • src/Entity/Project/WorkPackage.php
View command line instructions

Manual merge helper

Use this merge commit message when completing the merge manually.

Checkout

From your project repository, check out a new branch and test the changes.
git fetch -u origin team-1/development:team-1/development
git switch team-1/development
Sign in to join this conversation.
Reviewers
No reviewers
rileydeman
patrick
Labels
Clear labels
No items
No labels
Milestone
Clear milestone
No items
No milestone
Assignees
Clear assignees
No assignees
3 participants
Notifications
Due date
The due date is invalid or out of range. Please use the format "yyyy-mm-dd".

No due date set.

Dependencies

No dependencies set

Reference
HNNW/core!35
No description provided.