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

AuthX - Extended authentication support (portable and router-friendly) #19590

Merged
merged 25 commits into from
Mar 3, 2021

Conversation

totten
Copy link
Member

@totten totten commented Feb 13, 2021

Overview

This pull request introduces an extension, authx, which broadly improves support for HTTP authentication on any CiviCRM route. For example:

## Enable passwords and API keys for use in the `Authorization:` header
cv en authx
cv ev 'Civi::settings()->set("authx_header_cred",["pass","api_key","jwt"]);'

## Send API request with password authentication
curl 'http://demouser:demopass@dmaster.bknix:8001/civicrm/ajax/rest?entity=System&action=get'

## Send API request with api_key authentication
curl -H 'Authorization: Bearer MYKEY' 'http://dmaster.bknix:8001/civicrm/ajax/rest?entity=System&action=get'

Some key features:

  • CMSs: Integrates with authentication and session APIs in Backdrop, Drupal 7, Drupal 8/9, Joomla, and WordPress
  • Credentials: Accepts several types of credentials:
    • Username/Password (Basic {user:pass})
    • Contact api_key (Bearer {token})
    • JSON Web Token (Bearer {token})
  • Sessions: Sessions are optional, depending on how you make the request.
    • Ex: Session-based auth may be appropriate for a long-running agent or for facilitating a browser-based flow
    • Ex: Stateless (non-session) auth may be appropriate for REST integrations or web-hooks
  • Flows: Credentials may be submitted in different ways:
    • (Stateless) Parameter (?_authx=<CRED>)
    • (Stateless) Common Header (Authorization:<CRED>)
    • (Stateless) Alternate Header (X-Civi-Auth:<CRED>)
    • (Session) Explicit Login (/civicrm/authx/login, /civicrm/authx/logout)
    • (Session) Auto-Session (?_authx=<CRED>&authxSes=1)
  • Configurable: Depending on the deployment (use-cases, security posture, etc), one may enable or disable any credential-type and/or flow-type.
  • Testing: Civi\Authx\AllFlowsTest is an E2E test which covers every permutation of credentials+flows on every CMS.

The extension should help with several problems:

  • To support self-service links, Afform needs support for hyperlinking with a signed token. (civicrm/my-form?_authx=MY_JWT)
  • APIv4 is available for AJAX -- but is not currently available for REST (server-to-server). If you enable authx, then the existing AJAX route (civicrm/ajax/api4/ENTITY/ACTION) should be usable for REST purposes.
  • APIv3 REST does not have a supported end-point URL on Drupal 8. If you enable authx, then the existing AJAX route (civicrm/ajax/rest) should be usable for REST purposes. (dev/core#2077)

Before

A handful of end-points (extern/rest.php, extern/cron.php) are able to accept requests based on an api_key. However, the code which implements this is extremely convoluted. Consequently, it is difficult to fix or improve.

After

Any CiviCRM route can accept an authentication token.

Comments

Why do this as a core extension?

  • On one hand, this should be a shared basis for other core features (e.g. REST and Afform hyperlinks).
  • On the other hand, it is sensitive functionality, and it will probably require a few policy options (vis-a-vis which authentication modes are allowed / when / for which users). It may take a few cycles to shake-out expectations on the policy side. Making it optional for interim seems prudent.

Technical Details

There's more information about the intended list of features in ext/authx/README.md.

These bits would be good additions, but I think it's mergable without them:

  • Admin UI: Configuration matrix
  • Strict interoperability for APIv3 REST: Allow existing integrations to change end-point URL from extern/rest.php to civicrm/ajax/rest without changing logic.
    • If the route is civicrm/ajax/rest, then accept ?api_key=XXX as an alias for ?_authx=Bearer+XXX.
    • site_key enforcement: Some admins may currently rely on site_key to distinguish between users who are or are not allowed to use REST-y dataflows. For JWT, the extra grant is implicit. However, for api_key and password based auth it may be appropriate to have an option for site_key enforcement.

I've been able to successfully run AllFlowsTest locally on every CMS. However, there were a few quirks/caveats. I don't think they're blockers for this PR.

  • Drupal 8:
    • drush cc router: When you first enable the extension, D8's routing table remains stale. Clear it.
  • WordPress:
    • sendsExcessCookies: It sometimes sends a Set-Cookie: in responses which don't merit a cookie.
    • Authorization:: To support common Authorization: headers, one must add a line to.htaccess. But if you're happy with the other flows, then don't worry about it. Other flows work out-of-the-box.
      SetEnvIf Authorization "(.*)" HTTP_AUTHORIZATION=$1
      
    • Related bugs: WordPress - If we hit a "permission denied" error, return HTTP 403 #19608
  • Joomla: It's a known-issue that Civi-Joomla support is not as good. It needed several incidental bug fixes, and it's harder to run, but it eventually passed.

@civibot
Copy link

civibot bot commented Feb 13, 2021

(Standard links)

@totten
Copy link
Member Author

totten commented Feb 23, 2021

I've had some success using this for authenticated email links in Afform (#19660).

Removing the "(WIP)" flag.

There are some ideas in the description that haven't been done (e.g. a configuration UI and strict APIv3 REST interop). However, I don't think these are actually needed for the immediate goal of Afform/token support. They would matter more for achieving another goals (e.g. D8 REST), but I'm happy to kick that down to another issue/PR.

@totten
Copy link
Member Author

totten commented Feb 26, 2021

Based on the last discussion, I've fully extracted the Joomla bugfixes as a set of parallel PRs so that they can be reviewed on a separate timeline. In no particular order, they are:

Additionally, the GuzzleMiddleware bit is available as a separate PR #19678.

@seamuslee001
Copy link
Contributor

@totten can you rebase this please given that the core PRs have been mostly been merged

totten added 18 commits March 2, 2021 11:37
This significantly trims down the `auth()` method and rearranges as three
methods. A data object is passed between the three methods. The main method:

```
    if ($principal = $this->checkCredential($tgt)) {
      $tgt->setPrincipal($principal);
    }
    $this->checkPolicy($tgt);
    $this->login($tgt);
```

This arrangement lays the groundwork for implementing more varied policies.
For example, we could have a policy where the ability to login via
username/password/api_key is dictated by the user's role or permissions.
@totten
Copy link
Member Author

totten commented Mar 2, 2021

@seamuslee001 Sure. Rebased.

<releaseDate>2021-02-11</releaseDate>
<version>1.0</version>
<develStage>alpha</develStage>
<compatibility>
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@totten should we be hiding this extension from the GUI at this point or not?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@seamuslee001 My vote would be to show it, but I don't feel super strongly in the first release with it.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeh I guess the question I have is how supported is this but in any case I think its probably ok to show it

@seamuslee001
Copy link
Contributor

I note that 95% of this PR is all the extension and the only changes to core code are very minimal and are only about a new hook / event and creating a fake session which is only called from within the extension so I think this is all very safe to be added merging

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants