Manually tested locally with TCP/IP and executable modes - works as expected.
Not sure what GitLab doesn't like about my branch - I could merge it into the 2.x locally with no issues in fast-forward mode, also this is confirmed by the green tick mark ( if I understand this correctly ) next to the "Merge request must be rebased, because a fast-forward merge is not possible." check in the merge request overview.
I've clearly mixed some wires here - user mail field is case sensitive.
Sorry for the noise.
I believe this should be closed now - the use case of having multiple users with the same email in different capitalization was addressed in https://www.drupal.org/sa-core-2024-004 → .
I've just tested the behaviour on the Drupal 10.3.10 and PostgreSQL and it looks perfectly in line with various proposals in this issue:
- account registered for testEmail@test.com
- account registration attempted for testemail@test.com - failed with "Address already taken" message
- password reset for attempt for testemail@test.com sent a password to stored in the system testEmail@test.com
Didn't had a chance to test - they removed the bad data from LDAP.
Also added dependency on EXIF extension - thought that would be good to have , but not worth a separate issue.
Will test and report back
rosk0 → created an issue.
Back to NR
Will do some more polishing myself.
Needs more work. See comments in the merge request.
larowlan → credited rosk0 → .
PHPStan should be happy now.
Unfortunately it is impossible to to fix access check only for one route as the same custom access code is used all over.
Attempted to fix the bug by the smallest possible change.
Two weeks passed.
Please publish tests and associated materials.
I've traced the problem down to the encryption profile configuration - it was using a key of the wrong length. Encrypt module provides an API for the encryption plugins to prevent such issues. However this API wasn't used by this module.
I've removed everything that was not relevant and added that API usage to the merge request. N.B. These changes will not fix existing broken installation, but will prevent those from appearing. If you have a broken installation that gives you an error mentioned in the description you should create a 256 bits key first than navigate to the "Encryption profiles" page and update the profile used for the TFA module by selecting the correct 256 bits long key. With the changes from this merge request you wouldn't even be able to select a wrong key.
Changes have been tested manually and are working as expected.
Beautiful! Thank you.
There is one line that has to go, otherwise this is RTBC.
Thank you
Looks good , thank you.
Some code style could be fixed to make it perfect.
Looks good, apart from the fact that we need to duplicate schema and default configuration, but I guess this is a bigger problem and is out of scope here.
RTBC++
Looks good , nothing pops up as a red flag.
+1 to RTBC.
Looks really good! Thanks Eric!
There are couple code style issues that could be fixed to make it perfect.
Linking previous attempts to improve Drupal support in Dependency Track:
NW for :
- https://git.drupalcode.org/project/drupal/-/merge_requests/7894#note_381385
- tests mentioned in #14
jurgenhaas → credited rosk0 → .
Thanks Eric!
The value could only be string if the update function (drupalauth4ssp_post_update_convert_returnto_list_param) didn't run, but that is not a module problem and I would rather not add just-in-case code.
I like the patch , but there is more works needs to happen here:
- issue summary update ideally with details like in 🐛 Fix infinite loop on more than 20 zones Closed: duplicate
- tests showing with more than 20 zones it dies
This appears to be duplicate of 🐛 admin/config/services/cloudflare Slow to Load RTBC .
I'm honestly on a fence with this as here is better description and information to reproduce and the other issue is older and have way more elegant patch.
Drupal 10 and 11 are the only supported version now.
Yes, and with Drupal 10 as well.
This change is massive and needs updated merge request fir 2.0.x branch.
Issue summary must be updated as well because at the moment it is not clear how the daily tag invalidation limit could be reached , if there is no tag based invalidation involved. I have multiple projects using the module in URL purging mode and haven't seen API limits issues.
Pinged @avpaderno on Slack about this. NW for now...
Yes Alberto, please add Steffen to maintainers - this module clearly needs more love than it currently gets.
Marking as fixed as this was released in the 8.x-3.32 → and I was able to run updates without issues, status report shows nothing related as well - tested on the latest Drupal core 10.3.2.
Marking as fixed as this was released in the 8.x-3.32 → and fixed the issue for me - tested on the latest Drupal core 10.3.2.
Previous patch version fails to apply for some reason...
Attaching the patch from the same commit, but with code only changes.
Addressing feedback after initial user testing:
- showing message allowing a user to return to the service provide only after TFA was set up
- improving wording
- clean up session after TFA entry
Attaching patch form the latest version of the merge request https://git.drupalcode.org/project/drupalauth4ssp/-/merge_requests/10 .
Hi Elaman,
Thank you.
I will close your merge request as I already committed the fix on the 2.x branch, but I have credited you on the commit and in this issue.
Tested on a real project - works.
Improved the merge request:
- used correct types
- updated form
- added upgrade path
Found a problem with a fallback redirect. Raised a core issue 🐛 Using LocalRedirectResponse with "" URL in controller results in LogicException Active for it.
Updated merge request https://git.drupalcode.org/project/drupalauth4ssp/-/merge_requests/10 and attaching new patch.
RoSk0 → created an issue.
Patch for testing.
Updated the merge request to support rollout scenarios. Following are test cases I used:
## Test cases
### IDP first
- [x] TFA not required and not set up
- [x] registration
- [x] log in
- [x] TFA not required and set up - logged in using TFA
- [x] TFA required and not set up
- [x] password reset
- [x] log in
- [x] TFA required and set up - logged in using TFA
### SP first
- [x] TFA not required and not set up on SP1
- [x] TFA not required and set up - logged in using TFA on SP2
- [x] TFA required and not set up - logged in on SP1 and redirect to TFA setup as only one skip was allowed, saw `Your login in flow was interrupted to set up TFA` message. Set up TFA, clicked the link and landed on the SP1
- [x] TFA required and set up - logged in using TFA
There is still some work to be done here to properly support TFA - it already works fine when set up fully , including users, but during the rollout there will be time when users are already enforced to have TFA, but haven't set that up yet. During this period, if they are allowed to skip TFA, they will , until allowed number of times to skip is used.
Looking into how to support both, normal and rollout periods.
There is nothing stopping from updating SimpleSAMLphp to the latest in v2 branch.
I can tell for sure that this module works perfectly with the v2.2.2 SimpleSAMLphp version.
I've done a bit more digging and this is what I observe:
www-data@31b009f0b41c:~$ env|grep ^SENTRY
SENTRY_ENVIRONMENT=development
SENTRY_DSN=******************************************
SENTRY_RELEASE=test-local-release
www-data@31b009f0b41c:~$ drush status-report | grep 'Sentry release'
Sentry release Info test-local-release
Drupal status report still says
- Sentry DSN - NULL Sentry client key is not configured. No events will be sent to Sentry.
- Sentry environment - prod
I've set $settings['sa_core_2023_004_phpinfo_flags'] to INFO_ALL and now can clearly see the the problem comes from the Apache - Environment of phpinfo() output has SENTRY_DSN environment variable as well as $_ENV, but not $_SERVER.
At the same time, getenv('SENTRY_DSN') returns the correct value when executed via Apache.
It's clearly not a bug of this module, but rather a standard behaviour of the webserver in question.
Apache documentation mentions that on the first lines here https://httpd.apache.org/docs/current/env.html :
First, there are the environment variables controlled by the underlying operating system. These are set before the server starts. They can be used in expansions in configuration files, and can optionally be passed to CGI scripts and SSI using the PassEnv directive.
I think it would be good to add some notes to the documentation to mention this Apache behaviour, but I will leave it to your discretion.
Disregard previous message please - I got confused.
When deploying update to production containing Drupal 10.2.7 and Passoword Policy 4.0.1 I had password_policy_update_8302() ran successful . Than was my custom update hook to trigger password_policy_update_8302() again and that failed with Integrity constraint violation: 1062 Duplicate entry '0-0-0-x-default' for key 'user__field_pending_expire_sent.PRIMARY'.
Now I have this entity field definition mismatch warning on prod , but I'm not sure if that was project code or my mistake with updates...
I haven't tested on 2.x ... My report was about 8.x-1.x as the current stable one.
RoSk0 → created an issue.
Recategorising as this is not really a bug.
Needs work for #8 and rebase on the latest changes in the 2.x branch which now has GitLab CI configured together with the PHPStan.
PHPStan base line have all the warning that must be resolved, except those two
Access to constant DRUPALAUTH_EXTERNAL on an unknown classAccess to constant DRUPALAUTH_EXTERNAL_USER_ID on an unknown class
authsource is not part of the configuration anymore.
Thanks for the effort Team!
I've took the patch from #6 and modified it a bit. The most important change is that redirect to the service provider after login and TFA entry now works.
Updated code is in the https://git.drupalcode.org/project/drupalauth4ssp/-/merge_requests/10 merge request. For convenience attaching the patch with the changes here is well.
This was tested on D10.3, PHP 8.1 and Apache.
I would really appreciate if people can test the patch on their set ups as that would give better coverage, and provide feedback here sooner rather than later.
Please see my comment here https://www.drupal.org/project/drupalauth4ssp/issues/3453721#comment-156... 💬 Fatal exception with simplesamlphp 2.2.2: non-existent service "serializer.normalizer.object" Closed: works as designed with a patch that is already released in Symfony 7.1.1.
Temporary workaround , but still.
Symfony has already released the fix in v7.1.1. For prior versions I would recommend including the the patch with the big fix in your composer.json:
- add this to your patches file or section
"symfony/framework-bundle": { "Issue #57294: Issue with 'symfony/serializer' component on a clean Symfony installation": "https://github.com/symfony/symfony/commit/0625660d157f7f0fbeaa7486daf548270fc613cc.diff" } - and this
"symfony/framework-bundle": "-p5"to thecweagans/composer-patchespatchLevelconfiguration, so you have
"patchLevel": { "drupal/core": "-p2", "symfony/framework-bundle": "-p5" },
Strictly speaking this is not this module problem, or bug, so I'm closing this. Hopefully we will have 6.4.x release soon that would include that.
Adding related.
Overall, the MR looks good to me.
Postponing on ✨ Add has() method to ItemInterface Active based on https://git.drupalcode.org/project/feeds/-/merge_requests/169/diffs#note... .
No, it's not duplicate of 🐛 Error: Call to a member function getKeyValue() on null and Deprecated function: strlen(): Passing null to parameter #1 ($string) Active and different error messages proving that.
Reviewed and tested:
- #7 - addressed
- error message gone after applying the patch from the merge request
Thank you.
Re-running password_policy_update_8302() helped to remove entity/field definition mismatch warning on Drupal 10.2.7 , Passoword Policy 4.0.1 and MySQL 8.0.
Missing config filter is not the problem in my case - I have it required just to be properly uninstalled in production with a follow up removal from the code base.
Changes to merge request addresses #8 , if I understood this all correctly. Sorry in advance if not.
Not sure what status would be more appropriate here, but for anyone experiencing upgrade issues on PostgreSQL - patch from #1668644-59: PostgreSQL: Impossible to change a field to serial, bigserial, or numeric → works and fixes that issue.
Tested on PostgreSQL 12.
Thank you for the hard work Team! I couldn't tell how happy I am that this time it's not on me to debug another PostgreSQL bug!
Tested patch from #59 on a pretty sizable project that is using geofield module and where upgrade to the latest 2.5 version was blocked by this bug.
Works like a charm! Let's get this committed.
I've also faced it on multiple projects. In one case different website environments would react differently - one would run through deployment process without issue, another died immediately on the the attempt to put website into maintenance mode with drush state:set system.maintenance_mode 1... However not sure sure if that was maintenance mode as the only output in the log was
In DefaultFactory.php line 97:
Plugin (config_split:uat) instance class "Drupal\config_split\Plugin\Config
Filter\SplitFilter" does not exist.
To me this doesn't look like minor, if the standard automated update process fails. At the same time I would expect that changed $settings['deployment_identifier'] would tell container to ignore the cache and rebuild it self, effectively forgetting about Drupal\config_split\Plugin\ConfigFilter\SplitFilter. Not happening unfortunately.
Speaking about core, I faced this when was deploying Drupal core 10.2.6 update. Maybe that is related?
Hm, it looks like there was something wrong with my set up - symfony_mailer_lite_transport entity type was marked as not installed despite the fact that module was enabled. Not sure how that happen, but that is surely not this module fault or problem.
Ignore this and sorry for the noise.
Started new merge request targeting 11.x with:
- patch from #48
- suggestion from #54
- and links to #2883851: Add initial values support for configurable and multi-valued fields → from #36
RoSk0 → made their first commit to this issue’s fork.
Turned out that we are running this in production for a good five years now! So +1 for RTBC from me.
Created a merge request with the patch from #2.
I've already was burnt by the order once and somehow managed to forgot about it - order of the packages in the `allowed-packages` list is very important!
My libraries was ordered incorrectly in the top level composer.json:
"drupal-scaffold": {
"allowed-packages": [
"library/custom-settings",
"library/main-settings"
],
}
and it should be
"drupal-scaffold": {
"allowed-packages": [
"library/main-settings",
"library/custom-settings"
],
}
.
All of this is properly documented in the README.md "Allowed packages" section.
When I tried to replicate this issue on a pure core 10.3 , unfortunately I don't have access to PHP 8.3 to testing on Drupal 11, the behaviour was different - requiring main-settings library scaffolds settings.php as expected, however requiring custom-settings library scaffolds the settings-custom.php and completely ignores the main settings.php - there is even no mention of it in the output related to the custom-settings library ... Feels like that file is completely ignored.
However if I remove main-settings library and leave settings.php in place - custom-settings library appends to it as expected:
Scaffolding files for library/custom-settings:
- NOTICE Modifying existing file at [web-root]/sites/default/settings.php. Examine the contents and ensure that it came out correctly.
- Append to [web-root]/sites/default/settings.php from assets/settings-php-include.txt
Formatting
Looks good to me, thanks!
Fixed. Thanks!
Added couple wording suggestions, but otherwise looks good to me!
+1 RTBC
Thank you for your contribution.
The changes here looks good to me, but I will wait the SSP modules changes before I can accept these.
Note also that SimpleSAMLphp version 2.0.x is only supported till 2024-10-30, from https://simplesamlphp.org/security/ , so might want to look at 2.1.x compatibility (at least).
Thanks!
Needs rebase with conflict resolution.
Not postponed anymore as 📌 Move non-migration tests to Forum in preparation for deprecation Fixed have landed.
I went through the process today and updated that change record → with the required steps.
The latest pipeline shows Guzzle 7.8.1 installed on Drupal core 10.2 no issues.