Skip to content
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 #22239

Closed
wants to merge 4 commits into from

Conversation

totten
Copy link
Member

@totten totten commented Dec 11, 2021

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. ping @seamuslee001)

Before

Not available

After

// Signature
function authx_login(array $principal, bool $useSession): array;

// Examples
authx_login(['contactId' => 202], FALSE);
authx_login(['userId' => 1], FALSE);
authx_login(['user' => 'admin'], FALSE);

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 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 authx_login() method basically uses the same login mechanics as the rest of authx, but it doesn't require specific credentials.

A quick way to see it in action is with cv, drush, or wp-cli. In each of these commands,

cv ev 'authx_login(["contactId"=>202],0); return civicrm_api3("Contact","get",["id"=>"user_contact_id"]);'

cv ev 'authx_login(["userId"=>1],0); return civicrm_api3("Contact","get",["id"=>"user_contact_id"]);'

cv ev 'authx_login(["user"=>"admin"],0); return civicrm_api3("Contact","get",["id"=>"user_contact_id"]);'

drush ev 'civicrm_initialize(); authx_login(["contactId"=>202],0); print_r(civicrm_api3("Contact","get",["id"=>"user_contact_id"]));'

wp eval 'civicrm_initialize(); authx_login(["contactId"=>202],0); print_r(civicrm_api3("Contact","get",["id"=>"user_contact_id"]));'

There are some existing alternatives, but each has issues.

  • Use 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.
  • Use CMS-specific APIs, eg \Drupal::service('account_switcher')->switchTo(...) or wp_set_current_user(). Obviously not portable.
  • Use cv --user, drush --user, or wp --user. However, these require you pick the target principal/user before launching the script.
  • Use 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.

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
@civibot
Copy link

civibot bot commented Dec 11, 2021

(Standard links)

@demeritcowboy
Copy link
Contributor

This seems ok just some questions:

  • What's the expected result when you give it a non-existent user? e.g. authx_login(['user' => 'nonexistent'], false); I get:
    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): CRM_Utils_System_Drupal8->getUfId('nonexistent')
  • Ditto what if you do something like cv ev --user=admin "authx_login(['userId' => '1'], false); (in this case the userid matches the --user). I get TypeError: Return value of authx_login() must be of the type array, null returned in ...\ext\authx\authx.php on line 64
  • I'm not clear on what $useSession does.

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.
@totten
Copy link
Member Author

totten commented Dec 21, 2021

@demeritcowboy Thanks. I've extended the tests to cover a few of the error scenarios - and added some more checks/exceptions along the way.

So far, on local D7 and WP, it throws the asserted AuthxException. For local D8, the test initially reproduced the error, but I think that reflects a bug in CRM_Utils_System_Drupal8::getUfId(). (The docblock indicate that it should return null, and the callers I see appear to expect that behavior.) So I pushed another to fix Drupal8::getUfId().

@demeritcowboy
Copy link
Contributor

Thanks. I didn't check the drupal function but yes it seems the right place to fix that.

A couple things still though:

(A)
I seem to be able to "log in" as a civi contact that does not have a corresponding user account. My expectation would be that it would fail. I put "log in" in quotes because while it seems to recognize me as a logged-in user for civi functions, it doesn't give me access to drupal functions, e.g.

cv ev "authx_login(['contactId' => 17], false); var_dump(civicrm_api3('Contact','get',['id'=>'user_contact_id'])); gives me data about contact 17

but

cv ev "authx_login(['contactId' => 17], false); var_dump(\Drupal::currentUser()->id());" gives 0.

(B)
I still get the type error when you have a --user that matches the params to authx_login, e.g. cv ev --user=admin "authx_login(['user' => 'admin'], false);" or cv ev --user=admin "authx_login(['userId' => 1], false);" gives TypeError: Return value of authx_login() must be of the type array, null returned in ...\ext\authx\authx.php on line 64

@totten
Copy link
Member Author

totten commented Dec 21, 2021

I'm not clear on what $useSession does.

Missed this in my previous reply. This flag decides whether the authenticated identity is stored in a persistent/cookie-based session -- alternative names might be isStateful or isPersistent or useCookie. See also: https://docs.civicrm.org/dev/en/latest/framework/authx/#flows.

For my current target use-case (with background/system/worker process running through pipe), it should not store a session/cookie.

However, based on experience with other auth APIs, I expect that some folks would look at authx_login() from a different POV/use-cases -- eg if you're trying to integrate SSO, you'd look for a function like authx_login(). But for SSO, it should store a session/cookie.

It seemed important to me that the developer calling authx_login() be intentional about whether to store identity persistently.

I seem to be able to "log in" as a civi contact that does not have a corresponding user account. My expectation would be that it would fail.

Authx has been treating this as a policy issue rather than a hardcoded issue. A couple relevant links:

  • https://docs.civicrm.org/dev/en/latest/framework/authx/#identity - There are use-cases for every permutation of "user exists/missing" and "contact exists/missing".
  • https://docs.civicrm.org/dev/en/latest/framework/authx/#settings - There are settings to decide whether user accounts are required. The default policy is like this:
    • By default, the only accepted credentials are JWTs. To accept passwords or API keys, the sysadmin must enable them.
    • By default, persistent/stateful/session flows require a user account (authx_login_user=required , authx_auto_user=required ).
    • By default, ephemeral/stateless/sessionless flows do not require a user account (authx_header_user=optional, authx_xheader_user=optional, authx_param_user=optional).

This all sort of raises the question - if someone calls authx_login(), then what flow are we to use? Do we abide by authx_login_user=required or authx_header_user=optional or...? The current PR fudges that a bit.

The target use-case right now (backend-y/system-y tasks) does not match any of the flows in https://docs.civicrm.org/dev/en/latest/framework/authx/#flows. I thought about adding a new flow and didn't... but the way it looks now, it probably should a distinct flow.

TLDR: I should probably rework this way that:

  1. Gives an explicit name for the auth flow (eg system or background or pipe)
  2. Applies policies to that (eg authx_system_user=optional or authx_system_user=required)

I still get the type error when you have a --user that matches the params to authx_login,

OK, I haven't looked at that yet - will try it today.

totten added a commit to totten/civicrm-core that referenced this pull request Dec 21, 2021
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.
@totten
Copy link
Member Author

totten commented Dec 22, 2021

Replaced by #22292. The signature is slightly different. The flow is now explicit option.

By setting flow, you can choose which policies to the current login. Ex: flow=>login will apply policies from authx_login_user (required). Ex: flow=>script will apply policies from authx_script_user (``)

@totten totten closed this Dec 22, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants