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 - Retain authentication outcome/metadata #20026

Merged
merged 1 commit into from
Apr 26, 2021

Conversation

totten
Copy link
Member

@totten totten commented Apr 9, 2021

Overview

This improves the internal APIs for Authx. Authx supports additional ways to authenticate requests (e.g. passwords, API keys, JWTs). With this patchset, we have an internal way to see how the user was authenticated, e.g.

CRM_Core_Session::singleton()->get('authx')['credType']
CRM_Core_Session::singleton()->get('authx')['jwt']

Example Use-cases

  • Suppose you wish to generate a JWT with a limited scope that allows access a handful of APIs. The JWT is parsed during authentication, but you should preserve the scope so that it can be consulted during authorization. ("Did the scope authorize access to this specific entity?")
  • The end-point civicrm/ajax/rest may be used in different ways (e.g. for AJAX or for REST). When evaluating an AJAX request (with implicit/automatic identification based on session-cookie), you should enforce the full CSRF guards. When evaluating a REST request (submitted by a non-browser agent with an API key), the form of the credential may indicate that it's not a CSRF attack. (If an attacker can construct a cross-site request that includes a valid API key, then they've already compromised the account and don't need CSRF shananigans.) Enforcing CSRF may require discriminating based on authentication info.

(This patch does not address those example use-cases -- it merely retains data so that it's possible.)

Before

While processing an authentication request, authx creates a private/short-lived object containing details of the authentication process (credential-type, user ID, etc).

The object is lost/destroyed after we make a decision about authenticating.

After

As before, authx creates a private/short-lived object containing details of the authentication process (credential-type, user ID, etc).

However, with successful authentication, key metadata is now copied to the session. This includes credType, jwt claims, and more. However, it omits any literal credentials (e.g. passwords and API keys).

Technical Details

I originally tried to retain the data as an object (e.g. \Civi\Authx\AuthenticatorTarget or \Civi\Authx\Authentication). This proved problematic during deserialization. (We don't decide when to deserialize - and our classloader may not be available early enough). So instead the authx variable is a basic array.

@civibot
Copy link

civibot bot commented Apr 9, 2021

(Standard links)

@civibot civibot bot added the master label Apr 9, 2021
@eileenmcnaughton
Copy link
Contributor

@totten do you know who is most likely to review this? Is it @seamuslee001 ?

@totten
Copy link
Member Author

totten commented Apr 13, 2021

Yeah, it would be great if @seamuslee001 had any thoughts.

FWIW, I think this is pretty low-risk given that it's a recent extension and covered by several tests.

@seamuslee001
Copy link
Contributor

I think this PR looks fine to me the only question in my mind is should it be against the 5.37 or master branches I think probably master given that this is a new iteration of stuff rather than fixing a bug in Authx

@totten
Copy link
Member Author

totten commented Apr 26, 2021

Thanks @seamuslee001 Agree with master - since it's new/opt-in stuff. Also, it's a building-block toward other work (19727) so not super-useful in isolation.

@totten totten merged commit fb15984 into civicrm:master Apr 26, 2021
@totten totten deleted the master-authx-details branch April 26, 2021 23:47
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.

3 participants