(dev/core#4462) Afform - Add support for page-level authentication links #30585
Conversation
|
🤖 Thank you for contributing to CiviCRM! ❤️ We will need to test and review this PR. 👷 Introduction for new contributors...
Quick links for reviewers...
|
|
The wait() calls you tried look fine. Might need to step-debug it. |
|
api\v4\Authx\AuthxCredentialTest::testValidation /home/homer/buildkit/build/build-1/web/sites/all/modules/civicrm/ext/authx/Civi/Authx/Authenticator.php:166 |
|
Civi\Authx\CustomFlowsTest::testCliPipeUntrustedLogin /home/homer/buildkit/build/build-1/web/sites/all/modules/civicrm/Civi/Pipe/BasicPipeClient.php:132 |
|
|
||
| $extraFields = array_diff(array_keys($parsed), $routeInfo['allowFields']); | ||
| if (!empty($extraFields)) { | ||
| \Civi::log()->warning("Malformed request. Routes matching $regex only support these input fields: " . json_encode($routeInfo['allowFields'])); |
There was a problem hiding this comment.
I think "unauthorized" => "malformed" and "support" => "permit" might be clearer log message to show it's a permission-y issue
| } | ||
| } | ||
|
|
||
| // Actually, we may not need this? aiming for model where top page-request auth is irrelevant to subrequests... |
There was a problem hiding this comment.
is the idea that the page with the form on it could be public (or authenticated some other way) and the permission checks only happen on the AJAX calls?
sounds neat
There was a problem hiding this comment.
I actually don't see at the moment how the token would make it here on the initial request.
It's passed in _aff param and the only place that seems to go is to the onInvoke above?
If we want to use this doesn't it need to be picked up in the same way an _authx param is in Authenticator::on_civi_invoke_auth ?
There was a problem hiding this comment.
So currently if the form doesn't have anonymous access then I get access denied, even if I have a token.
I could get it working with:
diff --git a/ext/authx/Civi/Authx/Authenticator.php b/ext/authx/Civi/Authx/Authenticator.php
index d961b3a8a6..50cd5104bf 100644
--- a/ext/authx/Civi/Authx/Authenticator.php
+++ b/ext/authx/Civi/Authx/Authenticator.php
@@ -63,6 +63,10 @@ class Authenticator extends AutoService implements HookInterface {
_authx_redact(['_authx']);
}
}
+ if (!empty($params['_aff'])) {
+ $this->auth($e, ['flow' => 'param', 'cred' => $params['_aff'], 'siteKey' => $siteKey]);
+ _authx_redact(['_aff']);
+ }
}
/**
Which then meant it got picked up here.
There was a problem hiding this comment.
I also tried getting my head around the "independent top page request" model.
If I've understood correctly, I create a form where the form itself is anonymously accessible, but it behaves differently for a specific-user.
So I created an anonymous form to create some entities with Role based access, and then tried submitting with/without the _aff token. Without token - the entities don't get created as the anonymous user doesn't have permission. With the token, it works!
But I don't see the harm of authenticating the top level request using the token. And it's quite weird to have to remember that you have to make the form world-accessible; and maybe sometimes you don't want it to be, and then how do you provide that indepedent authentication?
In essence I think the Form-Based / User-Based access controls are already pretty hard to get one's head around, so seems like splitting the form page and form api authentication adds another dimension of complexity. So maybe to keep bundled for now?
There was a problem hiding this comment.
@ufundo Oh, very good point! The admin-experience (configuring perms) will be more forgiving if the token confers access to top-level page.
In fact, it is probably useful to be able to express the distinction between:
- "
civicrm/abcallows fully anonymous usage; it can be used with or without tokens" - "
civicrm/abcis not anonymous; but it can be used with tokens"
And there's no particular drawback. Yes, maybe someday we'd want to take a specific/anonymous form and export a static bundle of JS/HTML. But including auth here doesn't actually prevent that. Going down that path would require more/different QA, but the mechanisms can coexist.
I'll play with your patch to confirm (and hopefully include in the next push).
There was a problem hiding this comment.
It's still probably worthwhile to make this work, but I think I found trade-off regarding masquerade scenarios -- e.g. if you're logged in as contact 100 and click a link with ?_aff=JWT{...cid=200...}, then what happens? Where does it draw the line between the session-level-user and the page-level-user?
Intuitively, I think it plays a bit easier if the Afform JS/HTML content is essentially anonymous -- then we only need to think about the AJAX calls. And you can imagine different AJAX calls running with different identities. (For central page-content, the AJAX calls use the page-token; for peripheral page-content -- eg navbar -- the AJAX calls use your session-token.) Hard to imagine what you do with the top-level page if needs to simultaneously use permissions of both session-level-user and page-level-user.
Still, in either case, I think masquerade is tricky. If we want something nice, we have to put it back in the oven to bake longer...
Of course, "do nothing" is bad option. Right now, it abends and prints 1-line:
HTTP 401 Cannot login. Session already active.
Maybe in the meantime, a minimal improvement would be to show a regular HTML page with a message like:
<p>You are trying to view a page as another user:</p>
<center><a href="the-full-url-with-auth-token">https://example.com/civicrm/my-form</a></center>
<p>To continue as the other user, please right-click the link and open it in a private window.</p>It's not pretty, but it's more coherent...
There was a problem hiding this comment.
(This comment originally misplaced on a different subthread. Re-placing in correct context)
I added a WIP-commit for the idea of using _aff token to identity for the main page-view. (It moved a little bit to better respect module-boundaries, but still basically the same thing.) However, this technique is having an undesirable side-effect -- it causes the user-session to be authenticated. I'm not entirely sure why (and getting too sleep to dig into that). Hence the WIP-commit.
There was a problem hiding this comment.
e.g. if you're logged in as contact 100 and click a link with ?_aff=JWT{...cid=200...}, then what happens
As you say, currently you get an error.
To me this seems like a known, pre-existing bug: https://lab.civicrm.org/dev/core/-/issues/4464 (edit 4464 not 4463)
I def think it would be good to fix, but it seems outside of the scope of this PR.
Won't it affect the AJAX requests anyway? So a bit orthogonal to the question of which requests are in/out of the tokens authorising power?
|
A bit late to the party here ... but great to see this progressing. However, a setting at the global level seems rather limiting. On a complex site you may well have some forms that want page-level authentication and some that want session-level. How about making this a setting on the form instead? |
This only runs when you enable debugging for e2e tests (DEBUG=1).
Before: Separate tests for different permutations of token-name/output-media
After: One test checks consistency across token-names/output-media -- asserting
that they all point to the same URL.
Another test checks whether that URL behaves corectly.
…ge` or `session`)
… and page-level auth
…sion-level or page-level auth
totten
force-pushed
the
master-page-token
branch
from
53294bf
to
a345655
Compare
|
Rebased. Test(+functionality) had regressed due some intervening changes on master, but that's updated/fixed now. Included several (tho not all) discussion-points. |
|
@ufundo all good now? |
|
I believe the open points are:
|
|
@totten are the outstanding discussion points requiring resolution as part of this PR - or are they potential follow ups? |
|
IMHO, they can mostly be viewed as follow-ups. (But I'd allow a little more time for Ben or Aidan to feedback on that.) |
|
Currently on vacation, and tuned out from Civi but I have a nagging concern
about the global switch. (In other words, this might be completely
ill-founded - blame it on holiday brain!)
Suppose an admin creates an update form for contacts without logins. They
set the global switch to page-level auth and get things working. They don't
think much about other permissions because the form handles that.
Some time later, the admin encounters a different requirement and someone
suggests a form using session-level auth. They configure the form, update
the setting and (yay!) it works.
The chances are high that they don't realise this affects the earlier
update forms. Even if they do, a quick test shows the forms and tokens
still work as before.... but they don't. Those users now have an ongoing
session and potentially access far more than they did previously. And their
users may start getting "You're already logged in" errors.
Which is all a long-winded way of saying I'd prefer to make admins select
on every form what type to auth to use and not have a global setting.
If that makes sense, I'd opt for changing from global to per-form setting
now.
…On Tue, 10 Sept 2024, 12:17 am Tim Otten, ***@***.***> wrote:
IMHO, they can mostly be viewed as follow-ups. (But I'd allow a little
more time for Ben or Aidan to feedback on that.)
—
Reply to this email directly, view it on GitHub
<#30585 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AAU2QPP4T6HJOWBEPEXE3E3ZVY277AVCNFSM6AAAAABKE3FNQ2VHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDGMZZGM3TCMBUGM>
.
You are receiving this because you were mentioned.Message ID:
***@***.***>
|
|
|
||
| // \CRM_Core_Session::useFakeSession(); | ||
| // $params = ($_SERVER['REQUEST_METHOD'] === 'GET') ? $_GET : $_POST; | ||
| // $authenticated = \Civi::service('authx.authenticator')->auth($e, ['flow' => 'param', 'cred' => $params['_aff'], 'siteKey' => NULL]); |
There was a problem hiding this comment.
can't you pass useSession => FALSE here to make it stateless for the top level page load?
There was a problem hiding this comment.
Yeah, you would think so. I'm pretty sure I dipped a toe in that direction -- setting the explicit value and/or inspecting the effective value (with debugger). But while inspecting I realized that useSession=>FALSE is already the default.
It's particularly strange because StatelessFlowsTest is passing. (If the problem was on J/WP/SA, then this known quirk could explain the discrepancy. But I'm pretty sure my final testing was on D7 which should be more robust.)
Of course, it is worth double-checking the simple things again.
But this is why I'm expecting it needs a deeper trace of the session-management. (...Like maybe the FakeSession isn't superceding the UF's default session for some reason? Or something in UF-land is forcing the session to start?...)
|
My worry with the top-level-page auth thing is that out there in the world, the current behaviour is so intricate/subtle to the point of being unusable. I think there's a pretty high risk as-is of admins not even trying or getting cut by the subtlety at the first attempt, and never coming back. Having said that, I think it's functionally separate - so reasonable to split into a follow up PR. Just think it would be good to have that follow up before this makes it into any release. |
|
I leant towards II the same as you in terms of the setting @totten Though that does seem a valid concern in terms of the lifecycle of usage @aydun . I think on the flip side, having lots of settings is also error prone. Attempt to mitigate with a pretty serious health-warning on the global setting "changing this affects ALL your currently configured forms..." ? In terms of the lifecycle - how does it actually work? Is it for new tokens generated going forward (ie when new emails are sent)? Maybe if you have a tokenised link in an automated email that's going to slip through the net? Or a re-usable block in your email template or something like that. |
|
@aydun will you be at the sprint next week? Can we pick this up then? |
|
@ufundo I will - sure, would be good to talk about this.
…On Fri, 13 Sept 2024, 8:29 am ufundo, ***@***.***> wrote:
@aydun <https://github.com/aydun> will you be at the sprint next week?
Can we pick this up then?
—
Reply to this email directly, view it on GitHub
<#30585 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AAU2QPJ7D7T37LPUUMRO3NDZWKPABAVCNFSM6AAAAABKE3FNQ2VHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDGNBYGM2TIMBXHE>
.
You are receiving this because you were mentioned.Message ID:
***@***.***>
|
|
We discussed this a fair amount at Ashbourne - e.g. considering various migration paths. At one point, discussion moved toward using This led to another approach:
It's gonna take some massaging to the patch-set, and it may help to keep this branch for reference. So I think it's probably better to close this PR -- and submit other PRs(s) for that approach. |
Before+After ------------ * Up through 5.78-stable, `afform_core` included support for generating emails with form-links w/login-tokens. However, there was a long-standing request from multiple people to support form-links w/page-tokens. * During 5.79-alpha (civicrm#30585), we introduced page-tokens. We discussed more at the sprint, and there was a distinct feeling that page-tokens were more desirable (more approprate to more users/less foot-gunny). We agreed that login-tokens were hypothetically better for some, but no one in that discussion was eager to use them. So we moved login-tokens to a contrib extension. * But during 5.79-beta cycle, we got testing feedback from more people who were keen on login-tokens. * This PR re-introduces login-token support as a core-extension for 5.79-beta, which means that: * The default/typical mode of operation is based on page-tokens. * Using login-tokens is a little bit of work, but not as much.
Before+After ------------ * Up through 5.78-stable, `afform_core` included support for generating emails with form-links w/login-tokens. However, there was a long-standing request from multiple people to support form-links w/page-tokens. * During 5.79-alpha (civicrm#30585), we introduced page-tokens. We discussed more at the sprint, and there was a distinct feeling that page-tokens were more desirable (more approprate to more users/less foot-gunny). We agreed that login-tokens were hypothetically better for some, but no one in that discussion was eager to use them. So we moved login-tokens to a contrib extension. * But during 5.79-beta cycle, we got testing feedback from more people who were keen on login-tokens. * This PR re-introduces login-token support as a core-extension for 5.79-beta, which means that: * The default/typical mode of operation is based on page-tokens. * Using login-tokens is a little bit of work, but not as much.
Overview
When you create a custom form and enable email-token support, it will generate authenticated hyperlinks with session-level authentication (clicking the link logs you into a session). However, page-level authentication (granting access to one specific page) is also useful.
For more discussion about the differences, see: https://lab.civicrm.org/dev/core/-/issues/4462. (That issue is focused more on email-driven workflows. However, the same mechanism can also be incorporated into cross-site workflows -- where a remote site wants to grant limited access to display a specific form for a specific user.)
Before
Sending an email with a token like
{afform.myFormUrl}will always generate a URL with a session-level authentication token, e.g.After
There is a setting ("Administer > System Settings > Form Core") which determines whether to prefer page-level authentication or session-level authentication.
Sending an email with a token like
{afform.myFormUrl}will generate a URL with a token. The token will look like either:or
Comments
wait(2000). Maybe someone with more Mink experience (like @demeritcowboy) can figure a better technique?$.ajaxSetup()).