-
-
Notifications
You must be signed in to change notification settings - Fork 824
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#2077) Make 'civicrm/ajax/rest' interoperable with 'extern/rest.php' parameters #19727
Conversation
(Standard links)
|
The concept here looks great :-) I like the idea of Header based authentication since it's much, much easier to work with :-) I'll dive into some proper in-depth testing of this next week! |
This will need documentation updates to cover how the |
@totten test fails look related here |
test this please |
0a616c3
to
d458b6b
Compare
Test Result (2 failures / ±0) |
Yup, the test failures are related. Basically, the tests pass if the site has The trick is that we don't want to disable CSRF protection completely - but we do want a carve-out which allows REST-style authentication to be an adequate substitute. This requires some related work - e.g. next step/pre-req is #20026 |
d458b6b
to
1f13ee7
Compare
Hi @totten, I just used this PR to make the rest endpoint on Drupal9 working. I had to make to changes in the code. In the LegacyRestAuthenticator class the login function has the following signature:
I added a hack in on line 41 of auth.php
The ajax rest function checks if cal comes from Ajax. This line simulates this. (and I had to replace the drupal 8 |
This seems a blocker to me for using the REST API on Drupal 8/9 environments with the AuthX extension, since many webhook-issuing applications don't provide configurable HTTP headers. I thought this would have been resolved with #20026, although @totten stated there:
|
0a38d14
to
b8f084a
Compare
Rebased to address some merge-conflicts and to fix compatibility with phpunit8.
Oh, good point. Added that fix.
Quite right! I've added 69af731 + b8f084a to carve-out another way to get past the CSRF protection -- by supplying some valid secret as a bespoke param (ie The test now passes for me locally. For
🤞 Fingers crossed for the test to pass in CI as well. |
This provides more detailed debug information, esp when using the environment variable (DEBUG).
…LHttpRequest requirement Docblocks indicate the theory behind which styles are allowed and which are prohibited.
b8f084a
to
bd42f5c
Compare
Rebased to fix recent MCs. Tests pass. Updated labels (add Documentation links:
|
@totten I'm just waiting for you to patch the APIv4 endpoint and we'll get this merged. |
_Overview_: `civicrm/ajax/api4` and `returnJsonResponse()` inspect the web-request to see if it comes via AJAX/REST. If so, the call is allowed and formatted as JSON. The patch refines the test. _Before_: `X-Requested-With:` signals that a call is AJAX/REST. _After_: `X-Requested-With:` still signals that a call is AJAX/REST. Additionally, if `authx` is enabled, then some requests will be treated as AJAX/REST based on how they are authenticated (ie `xheader`/`X-Civi-Auth:` and `param`/`?_authx=` are AJAX/REST).
24de1f1
to
8d5feab
Compare
@colemanw I've updated the PR and description in a couple ways:
|
Looks good @totten - let's merge this before the RC branch |
Overview (updated)
This addresses a gap with APIv3 REST access on Drupal 8/9. Specifically, it updates an existing AJAX interface to satisfy the traditional REST contract.
The general approach involves two main changes:
legacyrest
. This looks for?key=...&api_key=...
. Accept this oncivicrm/ajax/rest
andcivicrm/ajax/api4/*
.X-Requested-With:
. If a request has a strong/secret/custom input like?api_key=
,?_authx=
, orX-Civi-Auth:
, then we do not needX-Requested-With:
.See also:
ping @seamuslee001 @MikeyMJCO
Before
If you have a remote app and wish to call APIv3 via REST, the call looks like this:
However, this is not generally available on D8/D9 because the script
extern/rest.php
is not available/accessible/bootable.Alternatively, following #19590, one can call APIv3 like this:
These two entry-points are based on the same controller (
CRM_Utils_REST
), so they mostly accept the same options (egentity=Contact&action=get&json=1
). However, they are not quite interoperable, so any external script/app/integration needs to tuned to match the different authentication parameters.extern/rest.php
only accepts credentials as thecivicrm_contact.api_key
(?api_key=...
).civicrm/ajax/rest
accepts credentials via query (?_authx=...
) or header (Authorization:
). This may be API key, username/password, or JWT. However, it does not accept?api_key
.After
The
civicrm/ajax/rest
can be used as a replacement forextern/rest.php
. It accepts the authentication via?api_key=...
.The E2E REST tests have been expanded -- instead one of one test for
extern/rest.php
, we have:E2E_Extern_BaseRestTest
- Common set of assertions/test-cases describing the REST contractE2E_Extern_LegacyRestTest
- Runs the common assertions usingextern/rest.php
(for environments that support it)E2E_ExternAuthxRestTest
- Runs the common assertions usingcivicrm/ajax/rest
+authx
(for all environments)Technical Details (updated)
extern/rest.php
requires?site_key
as a way to ensure that this user is approved for REST access.civicrm/ajax/rest
does not currently require this. However, there will be another PR momentarily to allow this requirement.extern/rest.php
accepts the APIv3 entity+action in two ways:?entity=...&action=...
. This is supported by both.?q=civicrm/{$entity}/{$action}
. This notation is not supported oncivicrm/ajax/rest
and would be problematic. Fortunately, I don't think it's used much, so...legacyrest
uses two top-level HTTP params (?key=...&api_key=...
). Of course, it's entirely possible that current and future routes have (or will have) their own uses for?key
and?api_key
. (This is difficult to check across-the-board.) To avoid conflicts,legacyrest
will only activatecivicrm/ajax/rest
andcivicrm/ajax/api4
-- because this doesn't produce a new conflict.Here are a few examples that I used for my own testing: