Obviously will not be ported so this is just the 2.0.x branch.
Both the current version and ✨ Switch to OO approach Needs review version are D11 compatible after updating the info file.
Tests are added by ✨ Switch to OO approach Needs review
All I've done is use the more compact recently released version, and remove the testing of D9 since that's no longer supported by this module, so RTBC.
No need to deal with the warnings in this issue as they are fixed in the other issue as it is a big change.
Module does not have tests. Adding Gitlab-CI is a waste of resources.
Module does not have tests. Adding Gitlab CI would be a waste of resources.
Looks good. Injecting into a mirror repo allows the module to be required using composer.
Setting back to needs work.
Marking as fixed.
Tests all pass.
Also done:
- Move ConfigHelper from Configuration to module root namesapce
- Move CasServerServiceInterface into module Entity namespace
Caches need to rebuilt before this will take. An update has been included in hopes this will take of it.
Include links to change notices in issue summary. Link to core issue.
The flood system is a little confusing on expiration - there is no need for an additional setting to specifically dictate expiration as these are handled by the limit + window parameters for IP and Username. In simple terms the "expiration" is the "time window", and it's automatic.
For logging into a Drupal site, there are two flood identifiers - IP and IP+Username. As set out in the config form, both have a limit number and a time window.
On failed login, an event is registered for IP and possibly IP+Username (depending on if it is available ie. the user entered a valid username).
Also during login, the IP and IP+Username is checked to see if "Limit" number of attempts have happened inside "Time window".
If so many events have happened inside window, they are blocked. If not, they are allowed.
As such the expiration is dictated by the user - if they try enough to be blocked, and then don't try again until window expires for enough events that they no longer hit the limit, they are unblocked. But if continue to register new blocked events, they will continue to be blocked with a near infinite time being blocked.
Hence, Flood Control module to be able to manually unblock a user if they contact you about it. Otherwise, they need to wait at least "window" time from the first attempt to be allowed back in automatically. Expiry of events is automatic.
So, using the default from Drupal Core:
https://git.drupalcode.org/project/drupal/-/blob/11.x/core/modules/user/...
ip_limit: 50
ip_window: 3600
user_limit: 5
user_window: 21600
If a user attempts 6 bad sign-ins with a recognised username inside 6 hours, they are blocked for 6 hours from the first attempt. Unless of course they just keep on trying, at which point block expires 6 hours after they stop trying (or really 6 hours from the 5th previous attempt after stopping).
For an IP address it's the same except for different limit and window, plus they'd be using multiple or unrecognised usernames.
Pretty simple eh? Anyway, no need for an expiration setting as it's already built in. It's just not a set expiration, it's a sliding window.
I had one last try at it, but I just can't get the check of values from config to work. That's half the commits in this. At least the MR changes makes it simple to look at.
There is some kind of wild bug/sequencing which has nothing to do with what we're doing here which results in a call to BrowserTestBase::config() or \Drupal::configFactory()->get() returning stale data. When I added code to bypass the config system and check the values directly in the database, and then also get it via config, it works just fine for both. Drop the direct DB call and it then fails. As a result, I changed how those assertions are made from querying the config system, to getting the values from the returned form which has in turn pulled the values from config. Somehow the form can get up to date data, but the test instance cannot. I presume it's caching of some kind in the test instances vs making a request to the site which kinda makes sense except that it works 25 off lines above for the 'user.flood' settings check, and doing a DB query magically makes it work.
I ended up needing to fix way more tests than I thought would be needed. The fixing of tests could be separated out into a new issue. There is another tests related issue posted 📌 Add GitLab CI template Needs review which is to add the GitlabCI file which I did in this fork to allow testing pipelines to run in here. Testing & their fixes could be merged in before this issue is so that only the code related to this issue is in this MR. Or merging this all in will just make tests work too.
Once this or testing is merged, there's still a need for a new issue or two to deal with spelling, phpcs, eslint and phpstan errors/warnings.
Tests are passing in ✨ Make FloodUnblockManager generic Active where all of the tests have been fixed to ensure that functionality remains the same.
.gitlab-ci.yml file adds testing of previous major since D9 is still supported. Could probably add next minor testing. D11 not currently supported.
Need to revert those contact module removals. It was moved to core, not killed off. Oops. Will need to figure out why that test was failing instead of removing it.
Leaving on NW to also add tests that are missing for recently added functionality:
- ip whitelist
- using the 3x filters on unblock page
- actually unblocking an ip address
One of the testing nodes might be having issues.
The MR tests fail on "Previous Major" (Drupal 9) because the "flood" table does not exist in the database, but if the pipeline is run manually, the table is created. It's in core, so it should be installed every time. The current release passes tests triggered both manually and by the MR.
Successful pipeline run is here: https://git.drupalcode.org/issue/flood_control-3437860/-/pipelines/149294
With the goal of ensuring that all functionality should continue to work with this patch in place, there should really be tests included for the filtering changes introduced in 2.3.3 too.
It would make more sense to keep the solution and the background together over in ✨ Support external Flood Postponed: needs info , marking this as a duplicate. The code to add Redis (or whatever replaces it) is mostly complete in that issue, it just needs to be merged into the new layout. The code in the patches for FloodUnblockManagerBase in ✨ Make FloodUnblockManager generic Active seem to be based off the original issue. There are a pile of people who deserve credit for getting it to this point, plus more people who would be notified of the updates and changes in plan. I only ran into this due to the posts by Anybody and catch on that issue.
Returning to active.
Expand scope of this issue to include code cleanup, removal of deprecated code and worked around code.
Fix status.
ELC → created an issue.
2.1.x branch is available for Drupal 11. It is compatible at this time.
Target dev release branch.
This should be ported back to 8.x-2.x branch, or that branch should be marked as unsupported. This is a fairly important bug to have fixed in code being used in production sites.
I have been unable to reproduce this issue. When entering a username and bad password and hitting login, I am always presented with the username field still filed when told that the credentials do not work.
If this is still an issue, please provide steps to reproduce on a fresh install of 2 sites.
Curiosity - what's the reason for cloning request when getting the query parameters? Would it no be sufficient to clone the query parameters if they are an object, or if it returns an array that would be copied anyway? I'm expecting some pointed to wild kernel level explanation of avoiding bad outcomes :D Actually, it was done this way in both patches so you may not be in the know.
Otherwise the patch looks like it should do what it should.
MR contains everything that was in the patch. Thank you @aaron.ferris
All of the database related queries were redundant code now that settings for the module are kept in config instead. The code in hook_user_role_delete() is the only part that hadn't been replaced resulting in dangling config items.
Considering how many sites (some 40,000) this will be affecting, perhaps an emergency release is warranted? A lack of caching on search views would have brought down our site. Bumping to major.
The reason this is bugged is because of order of operations, fixed by the brackets. The cache type can be NULL in a number of situations, such as when the cache type is set in the default display and not overridden in the other displays.
https://www.php.net/manual/en/language.operators.precedence.php
$display['display_options']['cache']['type'] = 'search_api_tag';
$result = ($display['display_options']['cache']['type'] ?? '' === 'none');
print $result; // search_api_tag, which is TRUE in a condition
$display['display_options']['cache']['type'] = 'none';
$result = ($display['display_options']['cache']['type'] ?? '' === 'none');
print $result; // none, which is TRUE in a condition
$display['display_options']['cache']['type'] = NULL;
$result = ($display['display_options']['cache']['type'] ?? '' === 'none');
print $result; // false
This is because "??" is "right associative" and lower priority than "===" which is non-associative. In a condition, php tries to shortcut the decision. If it believes that it has a positive outcome, it will no longer execute the rest of the code. In this case, the parts are divided into $display['display_options']['cache']['type'] and '' === 'none' with "??" between. If the variable doesn't evaluate to NULL, then nothing on the right is checked. If the variable is NULL, then it evaluates '' === 'none' which is always false. All quite confusing.
Anyway, the brackets fixes it. The other option would be to evaluate the null coalescing operation into a variable and then use that in the condition.
All planned issues are in place for the 2.0.0 release.
Added planned release note.
Fixed or ignored all spelling errors.
De-assign.
De-assign.
Tests included in patch. Ready for next release.
Adds the role restriction feature:
✨ Restrict CAS log in to roles Needs review
An update keeps existing behaviour of all accounts being able to access all services.
--
Still @todo: Merge into 2.1.x branch
Test failure is caused by random label not being shown in options list.
Options list: (~o)>
Label is: (~o)>&{{
Fixed "Reponse -> Response" in ✨ Restrict CAS log in to roles Needs review .
This branch will not be Drupal 11 compatible. See 2.0.x or 2.1.x branches.
Updating remaining tasks.
This change was merged into the main repo that normal pipelines use correct?
If I run the pipeline using this merge branch as the source, it works as expected and phpstan (next major) runs fully and correctly:
_GITLAB_TEMPLATES_REPO=issue/gitlab_templates-3432156
_GITLAB_TEMPLATES_REF=3432156-fix-references
https://git.drupalcode.org/project/genpass/-/jobs/1126946
However, if I just run a pipeline normally, it still uses the wrong script includes that were fixed here. ie phpunit instead of phpstan.
https://git.drupalcode.org/project/genpass/-/jobs/1127095
Am I expecting something more than should be in place right now? Is this fix not working like it should?
I checked out one of the other modules that should be passing this test too and it too is failing with the same error:
https://git.drupalcode.org/project/config_inspector/-/jobs/1127414
There are no plans to make 8.x-1.x branch compatible with Drupal 11. Switching to 2.0.x branch which is current development branch.
Drupal 11 support will be added to a 2.1.x or 3.0.x branch as there are a number of changes currently in dev.
fjgarlin → credited ELC → .
Oh bother.
This is duplicate of #3432156: Wrong references in phpstan and nightwatch next major jobs. →
Well that's not useful. I can't do an issue fork as this issue is moved. Here's a patch instead.
Results after these changes have been made:
https://git.drupalcode.org/issue/genpass-3432265/-/jobs/1106684
Moving this issue over to Gitlab Templates as that is where it needs to be fixed.
Two bugs in the include.drupalci.main.yml introduced by https://git.drupalcode.org/project/gitlab_templates/-/commit/58524cf6b66...
The nightwatch (next major) and phpstan (next major) have both been accidentally merged with phpunit for their script which means they don't work.
eg. https://git.drupalcode.org/project/genpass/-/jobs/1106441
The code it's running doesn't have any of the variables set as it's trying to run phpunit in the phpstan environment, and thus immediately fails.
Fixes are to reference the correct parent script in those two jobs:
needs:
- "composer (next major)"
script:
- *amend-core-requirements-drupal-11
- - !reference [phpunit, script]
+ - !reference [phpstan, script]
variables:
_TARGET_PHP: $CORE_PHP_MAX
_TARGET_CORE: $CORE_MAJOR_DEVELOPMENT
@@ -751,7 +751,7 @@ nightwatch (next major):
- "composer (next major)"
script:
- *amend-core-requirements-drupal-11
- - !reference [phpunit, script]
+ - !reference [nightwatch, script]
variables:
_TARGET_PHP: $CORE_PHP_MAX
_TARGET_CORE: $CORE_MAJOR_DEVELOPMENT
Reverting the change to remove "before_script" makes next major work again. Changing the reference to the actual parent script also makes it work.
Possible copy'n'paste issue as all removals were converted to !reference [phpunit, script] which seems unusual for phpstan. And nightwatch. I would suspect both of these to be errors.
Seems that the #3431269: Yarn install fails in "composer (next major)" job → issue may have been masking the change from #3425959: Remove before_script in "next major" job → which is where the breakage really occurred.
It is not intended that the 2.0.x branch will support D11.
Please use the 2.1.x branch for Drupal 10 and Drupal 11 as it has breaking changes that will not work in Drupal 9, or with
The 2.1.x branch already supports D11.
Changing category and branch to match.
Enough time has passed. Merged both branches.
What are the blockers for this release?
This is also not excluded by the $settings['commerce_show_partner_banners'] = FALSE; of the latest release. At present it is hard-coded into the twig template with no toggle. The template could be extended to include the variable.
I have needed to patch this out as I am the contact person for my clients, not the community or maintainers. My clients are not the target audience for such advertising, I am, and I am well aware of my options.
If a small business was building a site on their own, I could see the utility of having such a link. They will have already run into much of the information already as part of seeking out the project and installing it, but these links are an ongoing resource as they use the site.
The ability to toggle it off even for those users is important though.
Please find patch with respects the commerce_show_partner_banners value in the twig template.
I hope you feeling better! Excellent news that you've fixed it, so I'll close this one off as fixed.
It should be noted that you don't need to bump your users over to the cas/login all the time, but it is necessary for the users to visit it as part of their use of SSO/CAS when trying to use the CAS Server to log into a service. There are timeouts involved too.
If users are log in directly to "cas/login" (and the site is configured to grant TGT), users will be granted a Ticket Granting Ticket (TGT) with the configured timeout. Users would then need to navigate to the site they want to log into within that timeout, and get redirected back to the "cas/login" with the service url from the site they're trying to log into. That's not a particularly straight forward sequence of events.
The following user flows work well for my applications:
Also, here's the reference manual for the CAS protocol:
https://apereo.github.io/cas/6.6.x/protocol/CAS-Protocol.html
Issue #3036892: invalid service error when passing parameters like returnto → is ready for review.
NB: It temporarily includes the patch from ✨ Restrict CAS log in to roles Needs review since they have conflicting changes.
If nobody gets back to any of this, I'll make 2.0.0-rc2 with these issues merged in.
Thank you for your efforts. I ended up going a different path, with some future changes in mind in handling the service url query parameters.
This is actually ready for review, but does include the contents of ✨ Restrict CAS log in to roles Needs review completely as the first commit - this obviously needs to be removed once that is merged first. My apologies for making this more difficult to review that it needs to be, but there are conflicting changes.
As the method signature is changing for ConfigHelper::matchServiceAgainstConfig(), it now deprecated in favour of ConfigHelper::loadServiceFromUri(). This both parses the service url and stores the results for later use. It seems folly to be running regex on the same string multiple times on the same request.
The service url is parsed using \Drupal\Component\Utility\UrlHelper::parse() to separate all of the parts. The path part is used for matching against the service entities, and the original url is used when redirected an authenticated user back to whence they came.
Could you please confirm that this will work for your situation?
Changing branch for fork and begin merging into current HEAD.
Tests have been fixed and added to, but a release is not warranted for just those.
Permissions on services are pending review:
✨
Restrict CAS log in to roles
Needs review
Haven't heard back, but going to assume you figured it and will be marking fixed in next week.
The most important part for an already logged in user to not be prompted for credentials again is including "gateway=true" parameter along side the service parameter when calling "cas/login"
Greater test coverage.
Needs review and confirmation on the anon user access check. This diverges from the original patch in many ways, but functionally the huge difference is that it doesn't access check anonymous users. Access checking anon users seems to have been a bug as it breaks CAS functionality.
NB Failed tests: These will need to be re-run once testing infrastructure is up and running again. Database server appears to be rejecting connections at present.
Patch prevent anon users from accessing login when providing a service which would have been a large breakage for most situations. Kept that check there, but wrapped it to only check for auth users, and then adding a new check during login pre-auth.
Fixed existing tests by adding the required permission but should review to see there are more to add permissions based tests to.
Update target branch. The 8.x-1.x branch is no longer supported.
Added tasks as this needs tests and an update for existing installs.
Bug discovered from unit testing: cas server site could not be in a sub-directory as the cookie path did not include the base path. Fix by setting cookie path using Url::fromUri('internal:/cas')->toString().
Have forked off 2.1.x branch which supports only D10 and D11, using PHP8.
Pushed CASResponseCookies event subscriber to 2.0.x branch prior to this to get tests working without cookie errors.
@see https://git.drupalcode.org/project/cas_server/-/commits/2.0.x?ref_type=h...
GitLabCI is showing errors. Did not encounter errors running offline. Here are the errors.
There were 3 failures:
1) Drupal\Tests\cas_server\Functional\UserActionControllerTest::testLogout
Failed asserting that a NULL is not empty.
/builds/project/cas_server/vendor/phpunit/phpunit/src/Framework/Constraint/Constraint.php:121
/builds/project/cas_server/vendor/phpunit/phpunit/src/Framework/Constraint/Constraint.php:55
/builds/project/cas_server/tests/src/Functional/UserActionControllerTest.php:123
/builds/project/cas_server/vendor/phpunit/phpunit/src/Framework/TestResult.php:728
2) Drupal\Tests\cas_server\Functional\UserLoginFormTest::testCorrectNoServiceWithTicketGrantingCookie
Failed asserting that a NULL is not empty.
/builds/project/cas_server/vendor/phpunit/phpunit/src/Framework/Constraint/Constraint.php:121
/builds/project/cas_server/vendor/phpunit/phpunit/src/Framework/Constraint/Constraint.php:55
/builds/project/cas_server/tests/src/Functional/UserLoginFormTest.php:158
/builds/project/cas_server/vendor/phpunit/phpunit/src/Framework/TestResult.php:728
3) Drupal\Tests\cas_server\Functional\UserLoginFormTest::testCorrectWithService
Failed asserting that a NULL is not empty.
/builds/project/cas_server/vendor/phpunit/phpunit/src/Framework/Constraint/Constraint.php:121
/builds/project/cas_server/vendor/phpunit/phpunit/src/Framework/Constraint/Constraint.php:55
/builds/project/cas_server/tests/src/Functional/UserLoginFormTest.php:208
/builds/project/cas_server/vendor/phpunit/phpunit/src/Framework/TestResult.php:728
FAILURES!
Tests: 40, Assertions: 283, Failures: 3.
The stack trace you have provided is from un-installing the module:
#9 /home/tesjanebiir22/public_html/core/lib/Drupal/Core/ProxyClass/Extension/ModuleInstaller.php(91): Drupal\Core\Extension\ModuleInstaller->uninstall()
And it's while attempting to write a drush log write out. Are you using the latest version of drush? And did you fully install the module before uninstalling it?
Seems to work for me.
Now that everyone can just include patches in composer, I've found participation in issues queues only happens to re-roll a patch when a significant enough change happens that the last patch made no longer cleanly applies. You'll also find people creating patches to the last release instead of HEAD, just so it can be used in composer. People just grab the patch and move on.
This branch is no longer supported.
This branch is no longer supported.
I've done some experimenting and this patch can be applied without hesitation. This change is required when using PHP 8.2. The deprecation notices slow a site down significantly.
The use of utf8_encode and utf8_decode is done on the resulting encrypted string. It is thus entirely superfluous to the function of the encryption, storage of the encrypted string, or retrieval and decryption.
There is an extra utf8_encode called on the decrypted email address but this an additional encode on a string that was previously perfectly fine inside Drupal. It should be considered a bug that this is being done - thankfully this deprecation has turned it up.
eg. the string it's working on
def50200a15257aa6ab390cae057c1228ba55db4c3129ccea7976554e9954147d3749f788ac1e6bc732738ae1fe3e42622d3d7f764ba0e582a6970684ad54beabae22b465411cacb23c12d49af58c5556d8eccb6ca380fcf83cc70f9ae59fdbb6c5ce126
The use of utf8_encode or utf8_decode is just being used on a hexadecimal string. Quite useless, and will never change the result.
That is a way better way to handle it. I had a bell going off in my head that what I written was missing something as the entity could still be deleted via code, and access handling was being done in forms. I knew better.
Small adjustment as the delete operation in the list builder is also taken care of by the new access handler.
Anyway, this works well now.
Looks like this one missed out on making into the next release. Updated MR to latest head.
Updated remaining tasks. Code ready for review.
I'm using this in a number live sites so I can get nice emails about updates.
Thank you Sidharth.
Looking at your video, I'm not entirely sure why you're being asked to log in twice - do you have a servie configured and SSO turned on for it? Once you're logged into the CAS Server Drupal site, you should not be asked to login again. The user you are using is a CAS user? You're not accidentally logging into two different accounts?
It shouldn't be necessary, but you could try logging into cas/login first to ensure you do have an SSO session (and make sure the service from Moodle is setup for it too).
The site stopping at the cas login on your return from Moodle means that you don't have an SSO when you arrive back there. It should be bouncing you back to the service immediately if you have an SSO session active, so clearly something is mis-configured or you're not keeping your cookie, or some other thing.
I know Moodle isn't Drupal (duh), but the CAS module for Drupal sites has an automatic gateway feature that if similarly implemented on Moodle would mean you also don't need to click "log into cas" on the Moodle site. A quick search would indicate that adding "gateway=true" to the link, or slightly modifying the code to ensure gateway is sent? Apparently it uses phpCAS which does have gateway capabilities. Very much out of scope here.
Changing this to support request because the feature you are asking for actually exists and should be working - not quite auto-submit, but it should be auto-confirming your SSO session and passing you back transparently to the end user.
Tests merged into 2.1.x and now passing.
2.0.x is limited to current and previous cores.
2.1.x is limited to current and next cores.
Leaving NR in case someone wants to have a look at it. Will merge into main branch if any of:
- If any other issue needs to run tests
- D11 releases or is imminent
- Someone else goes RTBC
- Enough time passes that I want to merge it so I don't forget about it (could be tomorrow, could be 6 months)
Re-jigged commits into ones that will be kept for the main branch. Squished a few and re-ordered them.
I've updated the the issue description into a FR.
For now I've just commented out the code with a patch to get the previous function working for users who have been complaining.
I have since had a request that it be possible to limit it to a number per order, so a similar setup as the other usage limits would be ideal. Setting to allow unlimited applications, or N applications per order.
Use case:
I have 2x promotions: "Fixed amount off the order subtotal"
- $50 off
- $20 off
Both require coupons. Unlimited uses per user, unlimited available.
I have 1000 unique codes on each which are given out. They each have single use set.
If a user has 4x $20 codes, they get $80 off their order.
If a user has 1x $50 code and 2x $20 codes, they get $90 off their order.
Added it as described in 🐛 Regression: Limit promotion (via coupon) to only apply once per order Active . Adding more detail now to clarify the use case.
The inclusion of this block of code has broken our workflow:
https://git.drupalcode.org/project/commerce/-/commit/4a8e073085981f3a0d6...
// The promotion might already be applied, make sure we don't apply the
// same promotion more than once.
if ($referenced_coupon->getPromotionId() == $coupon->getPromotionId()) {
$form_state->setErrorByName($coupon_code_path, $this->t('The provided coupon code cannot be applied to your order.'));
return;
}
The introduction of this block of code has broken functionality - I do want users to be able to apply multiple single use unique coupon codes to an order. In our case, each one gives up to $20 off the order. If a user uses 2 of them, they get $40 off etc. Trying to use the same coupon twice still rejects which is desired.
I can just remove this code for now, but it would be ideal if this was configurable in the usage limits: total allowed uses per order. I thought it was working just fine as it was.
I don't believe there should be bypassing of using the API - such as not using addAdjustment or removeItem. This spreads out the functionality instead of keeping it tight and controllable. Large duplication of code having to do the same thing all over the place and then remember to do recalc at the end.
Instead, should addAdjustment and removeItem be amended to use new addAdjustmentMultiple and removeItemMultiple, which are given an array of items to work with, after which it does the normal thing of $order->recalculateTotalPrice();. Much more in keeping with how it works now.
Test failures do not appear to be related to this issue. There do not appear to be any tests for the dashboard as yet.
Setting to NR for this issue, ignoring the failing tests.
Update issue summary.
Merged in as it's been a couple of months.
Already committed.
ELC → created an issue.
By definition, Single Sign On should not require users to enter their login information more than once. If they are being forced to do so, it may well be a configuration error.
Using the gateway feature on the CAS module (not CAS Server) should redirect users to the automatic CAS Server login, but the users do explicitly need to take this step if the automated redirect did show them logged in already.
Each new service (site) visited will require returning to the CAS Server to authenticate against the CAS Server for that given service. If the user is logged in using SSO this will be transparent and automatic already. They should not be logging in to the original service site, but instead being redirected to the caslogin on the CAS server.
Please provide step by step instructions on how to reproduce the situation you are trying to avoid.
Code regarding handling mail when default transport does not exist has already been changed enough to gracefully handle this happenstance.
This MR goes towards preventing the admin from deleting the default transport.