Add Enabled Field to User

[0x46616c6b]

This PR introduces a new field for the User entity called "enabled". If it is set to false, the login should not work. This functionality is implemented in the UsersCheckPasswordCommand. The purpose of this field is to disallow not only spammers but also other users from logging in. To handle more complex database changes, the DoctrineMigrationsBundle is introduced. I have implemented a migration that adds the new field and also updates the existing spammers in the database to have enabled = false.

[codecov]

Codecov Report

Attention: Patch coverage is 25.80645% with 46 lines in your changes missing coverage. Please review.

Project coverage is 36.29%. Comparing base (aea4d13) to head (897a454).
Report is 184 commits behind head on main.

Files	Patch %	Lines
src/Repository/UserRepository.php	0.00%	15 Missing ⚠️
...c/Migrations/Factory/MigrationFactoryDecorator.php	0.00%	13 Missing ⚠️
src/Admin/UserAdmin.php	0.00%	12 Missing ⚠️
src/Security/UserChecker.php	66.66%	4 Missing ⚠️
src/Traits/EnableTrait.php	71.42%	2 Missing ⚠️

Additional details and impacted files

@@             Coverage Diff              @@
##               main     #517      +/-   ##
============================================
- Coverage     36.35%   36.29%   -0.06%     
- Complexity     1152     1166      +14     
============================================
  Files           186      189       +3     
  Lines          4627     4673      +46     
============================================
+ Hits           1682     1696      +14     
- Misses         2945     2977      +32     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[doobry-systemli]

Code changes look good to me. Regarding the architecture I'm a bit unsure whether replacing a spam flag with an enabled/disabled flag is a good idea.

First, our soft-delete already is "kind of" an enabled/disabled flag, just way more disruptive (as it purges all the user data).

Second, the spam flag makes it obvious why we disabled a user. If we merely have enabled/disabled in the future, there might be other reasons to disable them.

Third, a disabled user sounds like it's not functional at all any longer. But with the current implementation, it still receives mail and even can login to the web interface. So I wonder whether something like isSpammer or isDisabledMailSending would be a clearer implementation.

I'm not against this change, just writing down my thoughts.

[0x46616c6b]

Code changes look good to me. Regarding the architecture I'm a bit unsure whether replacing a spam flag with an enabled/disabled flag is a good idea.

First, our soft-delete already is "kind of" an enabled/disabled flag, just way more disruptive (as it purges all the user data).

Second, the spam flag makes it obvious why we disabled a user. If we merely have enabled/disabled in the future, there might be other reasons to disable them.

Third, a disabled user sounds like it's not functional at all any longer. But with the current implementation, it still receives mail and even can login to the web interface. So I wonder whether something like isSpammer or isDisabledMailSending would be a clearer implementation.

I'm not against this change, just writing down my thoughts.

Thanks for the feedback and I understand the points. The background to the change is the introduction of the Userli KeyCloak provider. This is where my idea comes from to implement the generic approach to cover the typical cases for KeyCloak/OIDC/SAML.

For our requirements, excluding spammers from sending email but not from login would then be implemented via a user attribute, which is what the application does, send email and/or user management. This enriches the token with this attribute and if it has a certain value, then the application decides what to do. The same principle could be implemented for "Multiplier".

For me, this PR is the first step in decoupling the different systems (user management, account management, ...). The next steps would then be the implementation of groups and attributes.

[t2d]

I understand the use case of providing this standard Keycloak attribute. I don't think it fully covers the spammer use-case, though.

Spammers SHOULD be enabled to log into userli (to receive messages from us), but MUST not be able to send mails.
To my knowledge, our spammer use case is also broken. Initially, we wanted spammers to be able to receive mails, but not send them. This is broken due to how the check-password-script and the chaining of postfix and dovecot work.

[t2d]

Suggested change

/** @deprecated */