Problem/Motivation
It's possible to have access to create or edit a registration, but anything you do on the registration form will lead to a validation error. This is obviously a hostile UX.
This is because RegistrationAccessHandler and RegisterAccessCheck only checks that the host is configured for registration, not enabled for registration; but the validation cares about enabled too.
Steps to reproduce
Disable registration for a host by unchecking the Enable box on the settings edit page for the host.
Masquerade as an ordinary user already registered.
Try to edit your registration.
Proposed resolution
The access and validation need to align more closely.
Remaining tasks
User interface changes
API changes
Data model changes
Issue fork registration-3464134
Show commands
Start within a Git clone of the project using the version control instructions.
Or, if you do not have SSH keys set up on git.drupalcode.org:
Comments
Comment #2
jonathanshawI don't have a specific proposal for this yet, I'm just filing this issue to flag this as a @todo.
I find the intertwined and often not DRY logic for access, HostEntityInterface::isEnabledForRegistration(), HostEntityInterface::isUserRegistered(), and RegistrationConstraintValidator rather scary. I suspect that a fundamental refactor is needed in this area. It's as if this module has done an absolutely fantastic job of growing to handle all kinds of use cases, but the growth has got to the point where it shows that one of the existing pillars needs an overhaul.
I'm actively thinking about this and hope to make a proposal.
Comment #3
jonathanshawComment #4
jonathanshawWidened this to include creating registrations too.
Comment #5
jonathanshawIf we can do #3464717: HostEntity::isUserRegistered() and isEmailRegistered() don't consider edge cases then my question becomes:
could we make use of these new HostEntity methods in RegisterAccessCheck and RegistrationAccessControlHandler?
One possible concern is that they may be expensive, and so performance when viewing a list of hosts might degrade. I'm tempted to think we should do it anyway, rely on the render caching, and add more specific custom caching if we need to.
Comment #6
john.oltman commentedComment #7
john.oltman commentedThere are a number of things in play here:
* Refactoring to share more code between access and validation - I like this idea but let's set this aside since https://www.drupal.org/project/registration/issues/3464736 covers it
* Registration is disabled after the form has already been displayed, e.g. capacity is reached while a new registration form is being filled in - yes, bad UX since there will be an error no matter what the user inputs, but generally, you cannot rely on access control to avoid constraint checks for this reason
* Some validation is dependent on submitted values, e.g. in many cases the registrant is unknown until submit - some checks can only occur in validation, for example the "allow multiple registrations for the same user" check
That leaves us with:
* A non-administrator attempts to edit a registration after registration is disabled, and the disabling occurs before the Edit tab is rendered - we can do something about this one; currently only "update" permission is checked in access control - the constraint validator has a special check for this use case. I think the validator is correct in doing this check, but it could be added to access control as well.
Comment #8
john.oltman commentedNew registrations are checked by RegisterAccessCheck - although it only checks the main status switch in the settings, that should generally be accurate, and the form displays a message instead of the fields when it isn't. So going to leave that use case alone.
Comment #9
john.oltman commentedComment #13
john.oltman commented