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.

Sign up for GitHub

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

Jump to bottom

(dev/core#4462) Afform - Fixes for page-token handling #31357

Merged
merged 7 commits into from
Oct 28, 2024
Merged

(dev/core#4462) Afform - Fixes for page-token handling #31357

merged 7 commits into from
Oct 28, 2024

Conversation

totten
Copy link
Member

@totten totten commented Oct 24, 2024 •
edited
Loading

Overview

This is a follow-up to #30585 (5.79.alpha) to address some of remaining issues (for 5.79.beta). It is part of https://lab.civicrm.org/dev/core/-/issues/4462. There is still more to do for 5.79 after this. (But I just wanted to keep the patchset from getting too big in one PR.)

ping @ufundo @eileenmcnaughton @aydun

Steps to Reproduce

  • Setup 5.79 on D7/BD.
  • Create an "Individual" form.
    • Form Settings: Placement: Enable "Message Tokens".
    • Form Settings: Permission: "Generic: Allow all users (including anonymous)"
    • Individual 1: Security: Form-Based
    • Individual 1: Autofill: Current User
    • Individual 1: Allowed Actions: Create=>yes, Update=>yes
    • Individual 1: Accept ID from URL: No
  • Send an email with the new token. The recipient should be someone has a User account.
  • Read the rendered email. Open the link in a new/private browser.

Before (circa 5.79.alpha)

The general idea is that the baseline HTML page is shown anonymously -- and then any AJAX subrequests will be authenticated (setting the active Civi Contact and active CMS User based on the token).

However, this runs into some snags:

  • Authx Bug: On the initial page-view, the site navigation looks anonymous. But it secretly initializes an authenticated session. This becomes obvious if you reload the page or manually open any other page.
    That defeats the whole point of 30585. The goal is to have a limited-use token that does not create a session.
  • First-Request Does Not Set Identity: The initial HTTP request doesn't set the active contact/user. That only happens in the AJAX subrequests. Consequently, it only works if the page is viewable by anonymous.
  • User-Loading Policy: You can kind of work-around problems by going to "System Settings > Authentication" and setting user-mapping policy for "HTTP X-Header" requests.
    • It requires work for site-builder to configure.
    • Those configuration steps are heavy-handed. They could impact other/unrelated customizations.
    • One size doesn't fit for all page-token use-cases. (CiviMail and cross-site IFRAMEs have some differing needs.)

After

This is an intermediate step/improvement.

The general idea is that you show the baseline HTML page (and its AJAX requests) as the target Contact and/or User.

Addressing those snags:

  • Authx Bug: Fixed. It no longer creates the unintended session.
  • First-Request Auth: It now sets the active Contact and/or User.
  • User-Loading Policy:
    • We still have the fundamental trade-offs in deciding whether to set active Contact and/or User.
    • But given that trade-off exists, you can distinguish policies for different use-cases.

Technical Details

Recall the basic mechanism of the page-level auth-token. You can take any afform and generate a signed token to view that one page.

https://example.com/civicrm/my-form?_aff=Bearer+XXXXXXXXX

For email hyperlinks (in the current work), message-tokens are used to generate these kinds of links. The policy is to set the active Contact but leave the anonymous User.

  • This should be closest analog to how "Profile+Checksum" behaves.
  • This means that the CMS will render nav-menus that are suitable to anonymous. (It's confusing to see non-functional nav-links for "New Individual" or "My Account" or "Logout".)
  • When configuring a form, you should set permissions suitable for anonymous users. (It's kinda hard to check the user-permissions if we don't load the user-accounts.)

For new/bespoke integrations (eg remote sites which embed IFRAMEs with auth-token links), it also defaults to "Contact-only". But:

  • If you enable "User" accounts for this case, it should work fine. (Navbars are already suppressed -- so no conflict there.

  • The integrator can decide whether to enable User account integration. They do this by putting a signed claim in the XXXXXXXX token:

    • Set JWT claim userMode=>ignore to disable user-loading.
    • Set JWT claim userMode=>optional or userMode=>require to enable user-loading.

@totten
authx - On Backdrop / D7, avoid accidental session setup
5f79c42 
I'm not entirely certain why we haven't noticed a problem with REST calls.
Maybe because most people don't care. Or maybe because REST calls use the
'print+exit' workflow (which might bypass this auto-save mechanism).
@totten
MixedFlowsTest - Add some more coverage
e8d1d86
@totten
(REF) Authenticator::checkPolicy() - Extract array to represent policy
b7595cb
@totten
(REF) Authenticator::checkPolicy() - Fire an event to alter active po…
54b2aab 
…licy

The code-style of this event (`checkPolicy()` and `CheckPolicyEvent`)
matches it closest sibling (`checkCredential()` and `CheckCredentialEvent`).
@totten
(dev/core#4462) For page-level auth, prefer to use CiviCRM-only ident…
b2fc5f3 
…ity.

When authenticating to Civi, you can set the active principal as "Contact" and/or "User".

If you set the user, then the web UI of the CMS is liable to render lots of widgets and
nav-items that are appropriate to logged-in users (like "My Account" or "Logout"). Fixing
this requires site-builder to take extra steps in their CMS.

OTOH, if you don't set the user, then -- in some
configurations/use-cases/add-ons -- you could find some events/listeners
don't run as expected.

This commi makes the default policy to be to -not- set the CMS user (since
that's more likely to work out-of-the-box and matches the approach of older
Profile/Checksum pattern). But you really need to switch, there is a way - tweak the token.

* If the auth-token has a signed claim `userMode=>ignore', then we'll ignore CMS user.
* If the auth-token has a signed claim `userMode=>optional', then we'll load CMS user (if available).
* If the auth-token doesn't explicitly say, then we use a default (`ignore`).
@totten
MockPublicFormTest - Re-enable assertion
bb0bc9d
@civibot civibot
Copy link

civibot bot commented Oct 24, 2024

🤖 Thank you for contributing to CiviCRM! ❤️ We will need to test and review this PR. 👷

Introduction for new contributors...
  • If this is your first PR, an admin will greenlight automated testing with the command ok to test or add to whitelist.
  • A series of tests will automatically run. You can see the results at the bottom of this page (if there are any problems, it will include a link to see what went wrong).
  • A demo site will be built where anyone can try out a version of CiviCRM that includes your changes.
  • If this process needs to be repeated, an admin will issue the command test this please to rerun tests and build a new demo site.
  • Before this PR can be merged, it needs to be reviewed. Please keep in mind that reviewers are volunteers, and their response time can vary from a few hours to a few weeks depending on their availability and their knowledge of this particular part of CiviCRM.
  • A great way to speed up this process is to "trade reviews" with someone - find an open PR that you feel able to review, and leave a comment like "I'm reviewing this now, could you please review mine?" (include a link to yours). You don't have to wait for a response to get started (and you don't have to stop at one!) the more you review, the faster this process goes for everyone 😄
  • To ensure that you are credited properly in the final release notes, please add yourself to contributor-key.yml
  • For more information about contributing, see CONTRIBUTING.md.
Quick links for reviewers...

➡️ Online demo of this PR 🔗

@civibot civibot bot added the 5.79 label Oct 24, 2024
@totten totten added the run-extended-tests Civibot should run comprehensive test suites with versions of UF/PHP/MySQL label Oct 25, 2024
@totten
Copy link
Member Author

totten commented Oct 25, 2024

This was discussed on call, and it sounded like we could merge. Would like to get this out of the way so we can work on other subparts of 4462.

I'm just gonna throw some more aggressive test-suites at it to make sure they aren't regressive. (Here's an example baseline for 5.79-rc.)

@ufundo
Copy link
Contributor

ufundo commented Oct 25, 2024

, it also defaults to "User-only"

Should that be "Contact-only" ?

@totten
Copy link
Member Author

totten commented Oct 25, 2024

Should that be "Contact-only" ?

Doh! Thanks, fixed.

@totten
Copy link
Member Author

totten commented Oct 25, 2024

The new tests are revealing a couple failures on other environments. Investigating...

@totten
Copy link
Member Author

totten commented Oct 28, 2024 •
edited
Loading

Woot. Compared to baseline test results, the PR fixes several pre-existing failures. drupal9-dev and drupal10-dev run green. wp-clean is reduced from 2x3 failures to 2x1 failure. Almost green all around.

@totten totten merged commit baf443d into civicrm:5.79 Oct 28, 2024
1 of 2 checks passed
@totten totten deleted the 5.79-afform-page3 branch October 28, 2024 20:55
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
5.79 run-extended-tests Civibot should run comprehensive test suites with versions of UF/PHP/MySQL
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@totten @ufundo