Feat user management#40
Conversation
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 58231d8360
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
dominikletica
added
security
dominikletica
modified the milestones:
0.1-dev – Functional basic backend features,
First development preview release @0.5.0
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 91d616b0a9
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| if (null !== $this->policy->validateGroupDeleteSystem($group)) { | ||
| return WorkflowResult::blocked([$this->message(MessageKey::ACL_GROUP_APPLY_DELETE_BLOCKED, ['%group%' => $group->identifier()], ['group_uid' => $group->uid(), 'group' => $group->identifier()])]); |
There was a problem hiding this comment.
When _operation_live queues a group delete/update, the controller validates the requester only before the background process is started; if the group is raised above that actor’s access level (or the actor is downgraded) before the runner reaches this service, this path only checks system constraints and still mutates the group. Persist the requesting actor or expected access boundary with the job and re-run the same validateGroupDelete/validateGroupUpdate policy here before applying the operation.
Useful? React with 👍 / 👎.
| return; | ||
| } | ||
|
|
||
| $groups = $this->userGroups->groups($groupIdentifiers); |
There was a problem hiding this comment.
When an already-delivered invitation or registration link is submitted after one of its ACL groups has been deleted, groups() silently drops the missing identifier and this path still activates the account and consumes the token. That gives the user a different ACL assignment than the admin approved, often with the intended contextual group missing; compare the resolved groups with the token identifiers and reject or repair stale links before clearing/replacing memberships.
Useful? React with 👍 / 👎.
| ]; | ||
|
|
||
| if (true === $input->getOption('include-used')) { | ||
| $statuses[] = AccountTokenStatus::Used; |
There was a problem hiding this comment.
If an operator runs studio:account-tokens:cleanup --include-used after a user disputes a password change, the consumed SecurityReview token is expired by the original link TTL and is deleted along with other used tokens. The reviews list and reactivate/delete handlers require that used token to identify the unresolved dispute, so the inactive account can disappear from review with no admin recovery action; exclude used security-review tokens while their user is still inactive.
Useful? React with 👍 / 👎.
| return 'admin.groups.form.invalid'; | ||
| } | ||
|
|
||
| return null; |
There was a problem hiding this comment.
When the group configured as user.default_acl_group is edited to require a role above USER, this validator accepts the change even though settings validation only allows default registration groups at user level and defaultRegistrationGroup() later ignores groups whose minRole() > USER. That leaves public registrations silently created without the configured default ACL group; block this update or clear/repair the setting just as deletion of the default group is blocked.
Useful? React with 👍 / 👎.
| 'en' => $pending['name_en'], | ||
| 'de' => $pending['name_de'], | ||
| ]); | ||
| $group->changeMinRole($pending['min_role']); |
There was a problem hiding this comment.
When a group’s min_role is raised above some current members’ roles, this update keeps those users in the group, and AccessActor::fromUserAccount()/AccessRule::allows() still grants group-based access solely from the identifier without re-checking min_role. That leaves lower-role users retaining access through a group they could no longer be assigned; block the raise until affected memberships/tokens are repaired, or remove memberships that fall below the new floor.
Useful? React with 👍 / 👎.
| $errors[] = 'ui.user.profile.errors.email_in_use'; | ||
| } else { | ||
| try { | ||
| $user->changeEmail($newEmail); |
There was a problem hiding this comment.
When a user has a pending password-reset/security-review link sent to their old address and then changes the profile email here, the token remains pending and bound to the same account, so anyone with the old mailbox link can still reset or dispute the account after it moved to a new address. Revoke pending recovery tokens when the normalized email actually changes before flushing the profile update.
Useful? React with 👍 / 👎.
1 participant
Summary
Adds the user-management foundation and hardening slice: durable account tokens, invitation-first onboarding, registration modes, password reset, profile/password/API-key flows, account closure, deleted-account reactivation, Admin User/ACL management, review queue, ACL impact review, mailer-shaped message-log delivery, setup seed centralization, translation cache warmup, and APP_SECRET rotation handling for API keys.
The branch keeps the implementation modular: account tokens, delivery, ACL policy, lifecycle side effects, ACL apply operations, state markers, setup seed data, and mail-flow metadata live behind focused service boundaries instead of controller-only logic.
Testing
bin/phpunit: OK (679 tests, 4325 assertions)php bin/console lint:container --env=test: OKphp bin/console lint:twig templates/backend/admin/users --env=test: OKphp bin/console lint:yaml --parse-tags config/services.yaml translations/languages/en/admin.yaml translations/languages/de/admin.yaml --env=test: OKphp .codex/compare_translations.php: OKgit diff --check dev-latest...HEAD: OKDocumentation
README.md)dev/draft/*.md)dev/CLASSMAP.md)dev/WORKLOG.md)dev/manual/*.md/docs/*.md)Additional Checks
Review Notes
dev/draft/0.4.x-MailerDeliveryContract.md.