Wellington
Account created on 26 June 2008, over 16 years ago
#

Merge Requests

More

Recent comments

🇳🇿New Zealand RoSk0 Wellington

Also added dependency on EXIF extension - thought that would be good to have , but not worth a separate issue.

Will test and report back

🇳🇿New Zealand RoSk0 Wellington

Will do some more polishing myself.

🇳🇿New Zealand RoSk0 Wellington

Needs more work. See comments in the merge request.

🇳🇿New Zealand RoSk0 Wellington

PHPStan should be happy now.

🇳🇿New Zealand RoSk0 Wellington

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.

🇳🇿New Zealand RoSk0 Wellington

Two weeks passed.

Please publish tests and associated materials.

🇳🇿New Zealand RoSk0 Wellington

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.

🇳🇿New Zealand RoSk0 Wellington

rosk0 made their first commit to this issue’s fork.

🇳🇿New Zealand RoSk0 Wellington

Beautiful! Thank you.

🇳🇿New Zealand RoSk0 Wellington

There is one line that has to go, otherwise this is RTBC.

Thank you

🇳🇿New Zealand RoSk0 Wellington

Looks good , thank you.

Some code style could be fixed to make it perfect.

🇳🇿New Zealand RoSk0 Wellington

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++

🇳🇿New Zealand RoSk0 Wellington

Looks really good! Thanks Eric!

There are couple code style issues that could be fixed to make it perfect.

🇳🇿New Zealand RoSk0 Wellington

rosk0 made their first commit to this issue’s fork.

🇳🇿New Zealand RoSk0 Wellington

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.

🇳🇿New Zealand RoSk0 Wellington

I like the patch , but there is more works needs to happen here:

🇳🇿New Zealand RoSk0 Wellington

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.

🇳🇿New Zealand RoSk0 Wellington

Drupal 10 and 11 are the only supported version now.

🇳🇿New Zealand RoSk0 Wellington

Yes, and with Drupal 10 as well.

🇳🇿New Zealand RoSk0 Wellington

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.

🇳🇿New Zealand RoSk0 Wellington

Pinged @avpaderno on Slack about this. NW for now...

🇳🇿New Zealand RoSk0 Wellington

Yes Alberto, please add Steffen to maintainers - this module clearly needs more love than it currently gets.

🇳🇿New Zealand RoSk0 Wellington

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.

🇳🇿New Zealand RoSk0 Wellington

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.

🇳🇿New Zealand RoSk0 Wellington

Previous patch version fails to apply for some reason...

Attaching the patch from the same commit, but with code only changes.

🇳🇿New Zealand RoSk0 Wellington

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 .

🇳🇿New Zealand RoSk0 Wellington

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.

🇳🇿New Zealand RoSk0 Wellington

Tested on a real project - works.

🇳🇿New Zealand RoSk0 Wellington

Improved the merge request:

  • used correct types
  • updated form
  • added upgrade path
🇳🇿New Zealand RoSk0 Wellington

Small terminology and style improvement.

🇳🇿New Zealand RoSk0 Wellington

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

🇳🇿New Zealand RoSk0 Wellington

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.

🇳🇿New Zealand RoSk0 Wellington

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.

🇳🇿New Zealand RoSk0 Wellington

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.

🇳🇿New Zealand RoSk0 Wellington

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...

🇳🇿New Zealand RoSk0 Wellington

I haven't tested on 2.x ... My report was about 8.x-1.x as the current stable one.

🇳🇿New Zealand RoSk0 Wellington

As mentioned in #2 authsource is not required anymore.

🇳🇿New Zealand RoSk0 Wellington

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

  1. Access to constant DRUPALAUTH_EXTERNAL on an unknown class
  2. Access to constant DRUPALAUTH_EXTERNAL_USER_ID on an unknown class
🇳🇿New Zealand RoSk0 Wellington

authsource is not part of the configuration anymore.

🇳🇿New Zealand RoSk0 Wellington

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.

🇳🇿New Zealand RoSk0 Wellington

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.

🇳🇿New Zealand RoSk0 Wellington

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 the cweagans/composer-patches patchLevel configuration, 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.

🇳🇿New Zealand RoSk0 Wellington

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.

🇳🇿New Zealand RoSk0 Wellington

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.

🇳🇿New Zealand RoSk0 Wellington

Changes to merge request addresses #8 , if I understood this all correctly. Sorry in advance if not.

🇳🇿New Zealand RoSk0 Wellington

RoSk0 made their first commit to this issue’s fork.

🇳🇿New Zealand RoSk0 Wellington

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.

🇳🇿New Zealand RoSk0 Wellington

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.

🇳🇿New Zealand RoSk0 Wellington

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?

🇳🇿New Zealand RoSk0 Wellington

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.

🇳🇿New Zealand RoSk0 Wellington

RoSk0 created an issue.

🇳🇿New Zealand RoSk0 Wellington

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.

🇳🇿New Zealand RoSk0 Wellington

RoSk0 made their first commit to this issue’s fork.

🇳🇿New Zealand RoSk0 Wellington

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.

🇳🇿New Zealand RoSk0 Wellington

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
🇳🇿New Zealand RoSk0 Wellington

Added couple wording suggestions, but otherwise looks good to me!

+1 RTBC

🇳🇿New Zealand RoSk0 Wellington

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).

🇳🇿New Zealand RoSk0 Wellington

Needs rebase with conflict resolution.

Not postponed anymore as 📌 Move non-migration tests to Forum in preparation for deprecation Fixed have landed.

🇳🇿New Zealand RoSk0 Wellington

I went through the process today and updated that change record with the required steps.

🇳🇿New Zealand RoSk0 Wellington

Thanks!

🇳🇿New Zealand RoSk0 Wellington

The latest pipeline shows Guzzle 7.8.1 installed on Drupal core 10.2 no issues.

🇳🇿New Zealand RoSk0 Wellington

Great community spirit and effort! Thanks heaps!

🇳🇿New Zealand RoSk0 Wellington

Looks good. Should be merged.

🇳🇿New Zealand RoSk0 Wellington

RoSk0 made their first commit to this issue’s fork.

Production build 0.71.5 2024