-
-
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
Add authx_login() API for backend script authentication (alt) #22292
Conversation
As mentioned in civicrm#22239 (comment), there can be an exception: ``` Error: Call to a member function id() on bool in ...\CRM\Utils\System\Drupal8.php on line 359 #0 ...\ext\authx\Civi\Authx\Authenticator.php(380) ``` The function signature specifies `@return int|null`. Various callers appear to expect this, and that seems to be how it behaves on D7/WP.
Before: If the authenticator encounters an error, it sends a response document. After: The authenticator encoutners an error, it may either: - (Default) Send a response document - (Option) Emit an exception
… use by backend/service scripts
(Standard links)
|
For the type error, I step-debugged and what happens is line 222 happens, so it never reaches line 252, so line 76 in authx.php returns null. It's drupal 9.3, civi master, php 7.4. It's windows but that shouldn't matter here. There's no extensions (except the stock ones enabled by default and of course authx). I don't see anything unusual in civicrm.settings.php except maybe
|
@demeritcowboy Aaaah, ok. That makes sense. I can reproduce it by choosing the same user for both login mechanisms ( I guess that circumstance is not an error (expected post-condition is met) - but it's also not quite right (it's redundant to specify both So I've pushed up a patch 322c4d0 which resolves the TypeError; instead, it emits a warning and constructs a placeholder |
Looks good! I also tried a couple other things just to see if I understood, e.g.
Although even with 'require', it did still let me "log in" as a user who was blocked in the cms which felt wrong. But maybe it worked that way before too in other flows so not going to hold this up on that. |
Overview
Adds a function to login as a user. It is cross-platform; accepts contact ID, user ID, or username; and it updates relevant services (eg Civi session; Drupal
$user
; MySQL@civicrm_user_id
).(This is a dependency for https://lab.civicrm.org/dev/core/-/issues/1304. @demeritcowboy this replaces #22239; it generally the same functionality, but some commits have been squashed, and some signatures have changed. In particular,
authx_login()
accepts more inputs -cred: string
orprincipal: array
combined withflow: string
and/oruseSession: bool
. Ex: Settingflow=>script
means that it will use the settingauthx_script_user
to decide whether user-accounts are required.)Before
Not available
After
This shares much of its implementation with the login processes used by authx for JWT+ApiKey+Password authentication.
Like the JWT/key/password implementations, this has cross-platform E2E testing.
Technical Details
The new use-case is like this: you have some background worker processes. These processes need to execute some tasks on behalf of a user. This is similar to
authx
's existing login functionality in some ways (eg you want it to be portable; to integrate with login/session mechanics in the CMS; and to work with a range of user-accounts); but it also differs in an important way (eg there is no authentication credential presented by a user; the decision is made by a background agent).The main changes here:
Civi\Authx\Authenticator::auth()
so that it can accept either (a) unvalidated credentials (string $cred
; e.g. username-password/JWT/API-key) or (b) validatedarray $principal
(e.g. validatedint $contactId
orint $userId
).authx_login()
. This callsCivi\Authx\Authenticator::auth()
with some defaults that are more useful for custom scripts.A quick way to see it in action is with
cv
,drush
, orwp-cli
. In each of these commands, we start Civi in different ways (cv ev
,drush ev
, etc) and try to login.(Note @demeritcowboy - I tested most of the above in
drupal-clean
w/php80+php74 anddrupal8-clean
w/php74. I couldn't reproduce the type error mentioned here. If you still get the error, then I'll need more precise description of the environment.)There are some existing alternatives, but each has issues.
CRM_Utils_System::loadBootStrap(...)
and pass username/password. However, in this function, the authentication and bootstrap processes are tightly coupled -- and some of its expectations are wonky.\Drupal::service('account_switcher')->switchTo(...)
orwp_set_current_user()
. Obviously not portable.cv --user
,drush --user
, orwp --user
. However, these require you pick the target principal/user before launching the script.CRM_Core_Config::singleton()->userSystem->loadUser(...)
. However, this only workswith a username (not contactId/userId); the incantation is very internal; and test coverage is unclear.