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 - validate if the user is blocked #27703

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

rubofvil
Copy link
Contributor

@rubofvil rubofvil commented Oct 2, 2023

Overview

Verify if the user is blocked in the CMS if want to use the API Rest by authx

Before

You can make a API call by webservice if the user is blocked.
With the configuration in civicrm/admin/setting/authx is required, like in the image.

image

Image of user blocked in drupal
image

After

Verify if the user is blocked(only drupal8/9, the other cases in ToDo) in the case that is blocked launch a exception;

Cannot login. User is blocked

Comments

Ref other MR with the deprecated way to call the API

#26185

@civibot
Copy link

civibot bot commented Oct 2, 2023

🤖 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 master label Oct 2, 2023
@totten
Copy link
Member

totten commented Oct 2, 2023

This seems like a good idea. From a skim, I'd be suspicious about breaking scenarios where the principal is a [contactId => 123, userId => NULL]. Easiest thing is probably to skip the check if userId===NULL.

(EDIT: userId===null indicates a visitor who is anonymous for purposes of UF-permissioning. Related notes about identity scenarios.)

Something to look out for: The automated PR test-runs use Drupal (not Drupal8), and there are some pre-existing failures on D8+ authx (esp testStatefulStatelessOverlap and testStatefulLoginAllowed). This means it takes extra steps to get the relevant test results. CiviCRM-Manual-Test can help. I ran it for drupal9-clean and phpunit-e2e phpunit-core-exts. Compare:

Comparing console logs, these things stand out to me:

  • Civi\Authx\AllFlowsTest::testMultipleStateless (pass on master; now fails on current PR revision)
  • Civi\Authx\AllFlowsTest::testJwtMiddleware (pass on master; now error Call to a member function isBlocked() on null)
  • Civi\Authx\AllFlowsTest::testCliServiceLogin (pass on master; now fails)
  • MockPublicFormTest::testAuthenticatedUrlToken_Plain (it was failing before, but I think this indicates a additional/secondary failure on Drupal8)
  • MockPublicFormTest::testAuthenticatedUrlToken_Html (it was failing before, but I think this indicates a additional/secondary failure on Drupal8)
  • MockPublicFormTest::testAuthenticatedLinkToken_Html(it was failing before, but I think this indicates a additional/secondary failure on Drupal8)

@@ -262,6 +262,10 @@ protected function login(AuthenticatorTarget $tgt) {
return !empty($a) && (string) $a === (string) $b;
};

if ($this->authxUf->getUserIsBlocked($tgt->userId)) {
Copy link
Member

Choose a reason for hiding this comment

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

In theory, maybe something like....

 if ($tgt->userId !== NULL && $this->authxUf->getUserIsBlocked($tgt->userId)) {

@rubofvil rubofvil force-pushed the dev-validate-authx-blocked-user branch from 030d51d to 4e79d05 Compare October 6, 2023 14:27
@totten
Copy link
Member

totten commented Oct 6, 2023

Requested new E2E test-run on D9: https://test.civicrm.org/job/CiviCRM-Manual-Test/103/ (currently queued)

@rubofvil rubofvil force-pushed the dev-validate-authx-blocked-user branch from 4e79d05 to a9abd36 Compare October 7, 2023 21:40
@rubofvil rubofvil force-pushed the dev-validate-authx-blocked-user branch from a9abd36 to f81a47f Compare October 7, 2023 21:41
@rubofvil
Copy link
Contributor Author

rubofvil commented Oct 7, 2023

Thx for the revision @totten !

This points aren't fixed yet.

  • MockPublicFormTest::testAuthenticatedUrlToken_Plain (it was failing before, but I think this indicates a additional/secondary failure on Drupal8)
  • MockPublicFormTest::testAuthenticatedUrlToken_Html (it was failing before, but I think this indicates a additional/secondary failure on Drupal8)
  • MockPublicFormTest::testAuthenticatedLinkToken_Html(it was failing before, but I think this indicates a additional/secondary failure on Drupal8)

The other points are fixed

Executed in https://test.civicrm.org/job/CiviCRM-Manual-Test/104/console

@mlutfy
Copy link
Member

mlutfy commented Nov 14, 2023

@rubofvil Just checking: from your last comment it was not clear if this PR was ready for a new review, or if there are issues that need fixing?

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