-
-
Notifications
You must be signed in to change notification settings - Fork 825
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
(dev/core#4462) Afform - Add support for page-level authentication links #30585
Changes from all commits
f635355
b882ec6
41ec310
255659f
526bffb
8d1e2da
3d598ca
34b8398
6634157
0de9546
f27d3cc
ab79da0
33c3a7d
a345655
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,32 @@ | ||
<?php | ||
/* | ||
+--------------------------------------------------------------------+ | ||
| Copyright CiviCRM LLC. All rights reserved. | | ||
| | | ||
| This work is published under the GNU AGPLv3 license with some | | ||
| permitted exceptions and without any warranty. For full license | | ||
| and copyright information, see https://civicrm.org/licensing | | ||
+--------------------------------------------------------------------+ | ||
*/ | ||
|
||
use CRM_Afform_ExtensionUtil as E; | ||
|
||
/** | ||
* | ||
*/ | ||
class CRM_Afform_Utils { | ||
|
||
/** | ||
* Get a list of authentication options for `afform_mail_auth_token`. | ||
* | ||
* @return array | ||
* Array (string $machineName => string $label). | ||
*/ | ||
public static function getMailAuthOptions(): array { | ||
return [ | ||
'session' => E::ts('Session-level authentication'), | ||
'page' => E::ts('Page-level authentication'), | ||
]; | ||
} | ||
|
||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,216 @@ | ||
<?php | ||
|
||
namespace Civi\Afform; | ||
|
||
use Civi\Authx\CheckCredentialEvent; | ||
use Civi\Core\Event\GenericHookEvent; | ||
use Civi\Core\Service\AutoService; | ||
use Civi\Crypto\Exception\CryptoException; | ||
use Symfony\Component\EventDispatcher\EventSubscriberInterface; | ||
|
||
/** | ||
* Allow Afform-based pages to accept page-level access token | ||
* | ||
* Example: | ||
* - Create a JWT with `[scope => afform, afform => MY_FORM_NAME, sub=>cid:123]`. | ||
* This is defined to support "Afform.prefill" and "Afform.submit" on behalf of contact #123. | ||
* - Navigate to `civicrm/my-form?_aff=Bearer+MY_JWT` | ||
* - Within the page-view, each AJAX call sets `X-Civi-Auth: MY_JWT`. | ||
* | ||
* @service civi.afform.page_token | ||
*/ | ||
class PageTokenCredential extends AutoService implements EventSubscriberInterface { | ||
|
||
public static function getSubscribedEvents(): array { | ||
$events = []; | ||
$events['civi.invoke.auth'][] = ['onInvoke', 105]; | ||
$events['civi.authx.checkCredential'][] = ['afformPageToken', -400]; | ||
return $events; | ||
} | ||
|
||
/** | ||
* If you visit a top-level page like "civicrm/my-custom-form?_aff=XXX", then | ||
* all embedded AJAX calls should "_authx=XXX". | ||
* | ||
* @param \Civi\Core\Event\GenericHookEvent $e | ||
* @return void | ||
*/ | ||
public function onInvoke(GenericHookEvent $e) { | ||
$token = $_REQUEST['_aff'] ?? NULL; | ||
|
||
if (empty($token)) { | ||
return; | ||
} | ||
|
||
if (!preg_match(';^[a-zA-Z0-9\.\-_ ]+$;', $token)) { | ||
throw new \CRM_Core_Exception("Malformed page token"); | ||
} | ||
|
||
// FIXME: This would authenticate requests to the main page, but it also has the side-effect | ||
// of making the user login. | ||
|
||
// \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]); | ||
// _authx_redact(['_aff']); | ||
// if (!$authenticated) { | ||
// return; | ||
// } | ||
|
||
\CRM_Core_Region::instance('page-header')->add([ | ||
'callback' => function() use ($token) { | ||
$ajaxSetup = [ | ||
'headers' => ['X-Civi-Auth' => $token], | ||
|
||
// Sending cookies is counter-productive. For same-origin AJAX, there doesn't seem to be an opt-out. | ||
// The main mitigating factor is that AuthX calls useFakeSession() for our use-case. | ||
// 'xhrFields' => ['withCredentials' => FALSE], | ||
// 'crossDomain' => TRUE, | ||
]; | ||
$script = sprintf('CRM.$.ajaxSetup(%s);', json_encode($ajaxSetup)); | ||
return "<script type='text/javascript'>\n$script\n</script>"; | ||
}, | ||
]); | ||
} | ||
|
||
/** | ||
* If we get a JWT with `[scope=>afform, afformName=>xyz]`, then setup | ||
* the current fake-session to allow limited page-views. | ||
* | ||
* @param \Civi\Authx\CheckCredentialEvent $check | ||
* | ||
* @return void | ||
*/ | ||
public function afformPageToken(CheckCredentialEvent $check) { | ||
if ($check->credFormat === 'Bearer') { | ||
try { | ||
$claims = \Civi::service('crypto.jwt')->decode($check->credValue); | ||
$scopes = isset($claims['scope']) ? explode(' ', $claims['scope']) : []; | ||
if (!in_array('afform', $scopes)) { | ||
// This is not an afform JWT. Proceed to check any other token sources. | ||
return; | ||
} | ||
if (empty($claims['exp'])) { | ||
$check->reject('Malformed JWT. Must specify an expiration time.'); | ||
} | ||
if (empty($claims['sub']) || substr($claims['sub'], 0, 4) !== 'cid:') { | ||
$check->reject('Malformed JWT. Must specify the contact ID.'); | ||
} | ||
else { | ||
$contactId = substr($claims['sub'], 4); | ||
if ($this->checkAllowedRoute($check->getRequestPath(), $claims)) { | ||
$check->accept(['contactId' => $contactId, 'credType' => 'jwt', 'jwt' => $claims]); | ||
} | ||
else { | ||
$check->reject('JWT specifies a different form or route'); | ||
} | ||
} | ||
} | ||
catch (CryptoException $e) { | ||
if (strpos($e->getMessage(), 'Expired token') !== FALSE) { | ||
$check->reject('Expired token'); | ||
} | ||
|
||
// Not a valid AuthX JWT. Proceed to check any other token sources. | ||
} | ||
} | ||
|
||
} | ||
|
||
/** | ||
* When processing CRM_Core_Invoke, check to see if our token allows us to handle this request. | ||
* | ||
* @param string $route | ||
* @param array $jwt | ||
* @return bool | ||
* @throws \CRM_Core_Exception | ||
* @throws \Civi\API\Exception\UnauthorizedException | ||
*/ | ||
public function checkAllowedRoute(string $route, array $jwt): bool { | ||
$allowedForm = $jwt['afform']; | ||
|
||
$ajaxRoutes = $this->getAllowedRoutes(); | ||
foreach ($ajaxRoutes as $regex => $routeInfo) { | ||
if (preg_match($regex, $route)) { | ||
$parsed = json_decode(\CRM_Utils_Request::retrieve('params', 'String'), 1); | ||
if (empty($parsed)) { | ||
\Civi::log()->warning("Malformed request. Routes matching $regex must submit params as JSON field."); | ||
return FALSE; | ||
} | ||
|
||
$extraFields = array_diff(array_keys($parsed), $routeInfo['allowFields']); | ||
totten marked this conversation as resolved.
Show resolved
Hide resolved
|
||
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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think "unauthorized" => "malformed" and "support" => "permit" might be clearer log message to show it's a permission-y issue |
||
return FALSE; | ||
} | ||
|
||
if (empty($routeInfo['checkRequest'])) { | ||
throw new \LogicException("Route ($regex) doesn't define checkRequest."); | ||
} | ||
$checkRequest = $routeInfo['checkRequest']; | ||
if (!$checkRequest($parsed, $jwt)) { | ||
\Civi::log()->warning("Malformed request. Requested form does not match allowed name ($allowedForm)."); | ||
return FALSE; | ||
} | ||
|
||
return TRUE; | ||
} | ||
} | ||
|
||
// 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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I actually don't see at the moment how the token would make it here on the initial request. It's passed in If we want to use this doesn't it need to be picked up in the same way an There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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:
Which then meant it got picked up here. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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 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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @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:
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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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 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:
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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. (This comment originally misplaced on a different subthread. Re-placing in correct context) I added a WIP-commit for the idea of using There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
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? |
||
$allowedFormRoute = \Civi\Api4\Afform::get(FALSE)->addWhere('name', '=', $allowedForm) | ||
->addSelect('name', 'server_route') | ||
->execute() | ||
->single(); | ||
if ($allowedFormRoute['server_route'] === $route) { | ||
return TRUE; | ||
} | ||
|
||
return FALSE; | ||
} | ||
|
||
/** | ||
* @return array[] | ||
*/ | ||
protected function getAllowedRoutes(): array { | ||
// These params are common to many Afform actions. | ||
$abstractProcessorParams = ['name', 'args', 'fillMode']; | ||
|
||
return [ | ||
// ';civicrm/path/to/some/page;' => [ | ||
// | ||
// // All the fields that are allowed for this API call. | ||
// // N.B. Fields like "chain" are NOT allowed. | ||
totten marked this conversation as resolved.
Show resolved
Hide resolved
|
||
// 'allowFields' => ['field_1', 'field_2', ...] | ||
// | ||
// // Inspect the API-request and assert that the JWT allows these values. | ||
// // Generally, check that the JWT's allowed-form-name matches REST's actual-form-name. | ||
// 'checkRequest' => function(array $request, array $jwt): bool, | ||
// | ||
// ], | ||
|
||
';^civicrm/ajax/api4/Afform/prefill$;' => [ | ||
'allowFields' => $abstractProcessorParams, | ||
'checkRequest' => fn($request, $jwt) => ($request['name'] === $jwt['afform']), | ||
], | ||
';^civicrm/ajax/api4/Afform/submit$;' => [ | ||
'allowFields' => [...$abstractProcessorParams, 'values'], | ||
'checkRequest' => fn($request, $jwt) => ($request['name'] === $jwt['afform']), | ||
], | ||
';^civicrm/ajax/api4/Afform/submitFile$;' => [ | ||
'allowFields' => $abstractProcessorParams, | ||
'checkRequest' => fn($request, $jwt) => ($request['name'] === $jwt['afform']), | ||
], | ||
';^civicrm/ajax/api4/\w+/autocomplete$;' => [ | ||
'allowFields' => ['fieldName', 'filters', 'formName', 'ids', 'input', 'page', 'values'], | ||
'checkRequest' => fn($request, $jwt) => ('afform:' . $jwt['afform']) === $request['formName'], | ||
], | ||
// It's been hypothesized that we'll also need this. Haven't seen it yet. | ||
// ';^civicrm/ajax/api4/Afform/getFields;' => [ | ||
// 'allowFields' => [], | ||
// 'checkRequest' => fn($expected, $request) => TRUE, | ||
// ], | ||
]; | ||
} | ||
|
||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,27 @@ | ||
<?php | ||
use CRM_Afform_ExtensionUtil as E; | ||
|
||
return [ | ||
'afform_mail_auth_token' => [ | ||
'group_name' => 'Afform Preferences', | ||
'group' => 'afform', | ||
'name' => 'afform_mail_auth_token', | ||
'type' => 'String', | ||
'html_type' => 'select', | ||
'html_attributes' => [ | ||
'class' => 'crm-select2', | ||
], | ||
'pseudoconstant' => [ | ||
'callback' => 'CRM_Afform_Utils::getMailAuthOptions', | ||
], | ||
// Traditional default. Might be nice to change, but need to tend to upgrade process. | ||
'default' => 'session', | ||
'add' => '4.7', | ||
'title' => E::ts('Mail Authentication Tokens'), | ||
'is_domain' => 1, | ||
'is_contact' => 0, | ||
'description' => E::ts('How do authenticated email hyperlinks work?'), | ||
'help_text' => NULL, | ||
'settings_pages' => ['afform' => ['weight' => 10]], | ||
], | ||
]; |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,14 +1,8 @@ | ||
<!-- This example is entirely public; anonymous users may use it to submit a `Contact`, but they cannot view or modify data. --> | ||
<af-form ctrl="afform"> | ||
<af-entity data="{contact_type: 'Individual', source: 'Hello'}" url-autofill="1" security="FBAC" actions="{create: true, update: false}" type="Contact" name="me" label="Myself" /> | ||
<fieldset af-fieldset="me"> | ||
<legend class="af-text">Individual 1</legend> | ||
<div class="af-container"> | ||
<div class="af-container af-layout-inline"> | ||
<af-field name="first_name" /> | ||
<af-field name="last_name" /> | ||
</div> | ||
</div> | ||
<af-entity data="{contact_type: 'Individual', source: 'Mock Public Form'}" type="Contact" name="me" label="Myself" actions="{create: true, update: true}" security="FBAC" url-autofill="0" autofill="user" /> | ||
<fieldset af-fieldset="me" class="af-container" af-title="Myself"> | ||
<afblock-name-individual></afblock-name-individual> | ||
</fieldset> | ||
<button class="af-button btn-primary" crm-icon="fa-check" ng-click="afform.submit()">Submit</button> | ||
</af-form> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
can't you pass
useSession
=>FALSE
here to make it stateless for the top level page load?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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?...)