(dev/core#4462) Form Builder (etal) - Define generic permissions to assist with message-tokens #31386
Conversation
This is similar to the `*always allow*` and `*always deny*` in that it's a generic permission based on special logic. For example, suppose you're define a new form "About Me". It should not be available to the anonymous public, but it should be available to any logged-in users and/or anyone with authenticated hyperlink. Then set the required permission to `*authenticated*`.
This shouldn't change functionality. It's just switching the short-circuit so that we can add more branches/options in the same fcuntion.
|
🤖 Thank you for contributing to CiviCRM! ❤️ We will need to test and review this PR. 👷 Introduction for new contributors...
Quick links for reviewers...
|
…tion
Before:
* In `Standalone::permissionDenied()`, it prefers to do one of two things, either:
* Show a login page, or
* Do a status-bounce (put messages and redirect to another page, like the dashboard)
* But what if you have a user with stateless authentication? (As in some of the edge-cases from `MockPublicFormTest::testSpecialPermissions`?)
* The login page is wrong because they're already authenticated.
* The status-bounce is wrong because they won't get session messages.
After:
* In `Standalone::permissionDenied()`, it has an extra check -- for stateless auth, a full "Access denied" page
totten
added
the
run-extended-tests
|
Since this touches on users/permissions, I ran the extended tests. The failures regarding |
ext/afform/core/afform.php
Outdated
| if (!str_starts_with($permission, '@afform:') || strlen($permission) < 9) { | ||
| // Micro-optimization - this function may get hit a lot. | ||
| return; | ||
| // Micro-optimization - this function may get hit a lot. |
There was a problem hiding this comment.
the micro optimization has gone now right? so this comment should too?
There was a problem hiding this comment.
Yeah, I suppose that could be interpreted either way. (Does "micro-optimization" mean "returns early" or does it mean "uses peculiar string manipulation"?) I'll just trim the comment a bit.
| use GuzzleHttp\Psr7\Utils; | ||
|
|
||
| /** | ||
| * (TEST HELPER/EXPERIMENTAL) |
There was a problem hiding this comment.
why doesn't it live in the tests directory?
There was a problem hiding this comment.
Yeah, it's a test helper, but the helper is used by tests in two extensions (authx and afform). Classloading gets weirder if you try share code from the tests/ folder. (Slightly different, but similar idea - note how newer helpers tend to go in civicrm-core:Civi/Test/ rather than civicrm-core:tests/phpunit/CiviTest/. It's because the class-loading is clearer.)
|
Have been trying to |
|
Ah, my bad. I had
I think you flagged that before as an easy mistake to make. Having fixed that, it |
ufundo
added
the
merge ready
|
Code looks good (minor question and a trailing comment). I think it's mergeable. |
|
Thanks @ufundo. Updated those bits. Yes, agree about how the "Permission" UI is easy to misread (and that's a pre-existing problem). Other test failures are pre-existing/unrelated. |
Overview
Suppose you are creating a custom form. You wish to link to it from mailings/messages. You must define the permissions for reading the form. But which permissions should you use? You need to think about the roles/authorizations of every person in the email list.
Before
You can theoretically use permissions like
access CiviEventoraccess CiviCRM, but these aren't very useful for a list of email recipients.When mailing links to regular constituents, the only useful option is
*always allow*:That's OK sometimes, but it can also be too broad -- it means that anonymous users (who never received the email) can access it.
After
You have two more generic options:
Compare:
Technical Details
The patch-set looks a bit bigger than it should. This is mostly because the afform/authx test-suites needed some reorganization to develop tests for these things.