I took the part of the @mjgruta's patch from #19 that addresses the issue and created an MR out of it. It applies cleanly to the 8.x-1.x version of the module.
Note: The pipeline says there's a merge conflict but I can't see what's causing it. I've merged the 8.x-1.x branch into the views_field_view-3270751-3270751-error-call-to branch and git says there's nothing to commit/push. It's most likely that when I first opened the merge request I targeted the 7.x-1.x branch (because it's the default branch and I didn't look before creating it), which means the pipeline most likely ran under that context. If anyone has any advice how to resolve please let me know.
jsutta → created an issue.
Ah, I didn't realize that was the case. I certainly understand where you're coming from given that.
I like the idea of setting recovery codes as something that doesn't satisfy "User has configured TFA". Although I can see how it'd be more involved than other options, I think it might be the most satisfactory option. One other change I'd suggest is that we at least consider listing the default validation plugin first. In our case this would place TOTP above recovery codes and would alleviate most of the issues.
If this sounds good to you, I'd be happy to give it a try to make the changes.
Hi @cmlara, thank you for replying so quickly to this issue!
Answers to your feedback:
What if the user wishes to use a different plugin?
I definitely don't think we should force site owners to use one validation plugin over another. Rather, the idea here is to require end users to configure the default validation plugin for their accounts before they can configure secondary plugins. For us, that would mean requiring users to configure TOTP before they can configure recovery codes.
Can you clarify what happens from your testing?
I think I misspoke in the issue summary. Instead of creating invalid codes as I originally thought, it seems one of two things is happening:
- End users are creating recovery codes and not setting up TOTP
- End users are creating recovery codes first, then setting up TOTP, which means their default authentication method is recovery codes
In either situation, once they run out of recovery codes, they reach out to us for support thinking the functionality is broken. We could simply disable the recovery code validation plugin, but so far our leadership doesn't want to do that. So part of the motivation behind this change would be to steer end users to the default validation plugin first before allowing them to set up alternate validation methods.
In a way we actually would want the opposite, TFA skip counts only impact users when TFA is not configured for a user(disabled for the user) and is required for the role.
Site owners can configure the number of times end users can skip validation via the "Skip Validation" setting. I've thought about it a bit more and I think you're right, hiding this information would be less than ideal because it could remove urgency on the part of the end user in terms of needing to get TFA set up. So I think I'll update the merge request to remove that part. Once that's done I'll make sure to update the issue summary.
Merge request for 2.x is up. Patch for 1.x is attached.
jsutta → created an issue.
Thank you thank you thank you, @jeni_dc! I just spent a solid week wrestling with this and your comment helped me fix it. What a relief!
The latest commits apply cleanly and work on my end. Thank you @torfj!
I rebased the issue's merge request against the current 2.0.x branch and readded the changes from #27. A patch generated from the merge request applies cleanly against the latest release in my environment.
Just tried it out with Drupal 10.3.1 and version 3.0.0-alpha1 of this module, without the patch from this issue, and it works! Thank you for fixing it, and for following up here.
jsutta → created an issue. See original summary → .
Hello all,
I couldn't get the 10.3.x branch to apply, so I created a separate patch that applied in my local environment just now. This is just a band aid though, and I'm happy to investigate why the 10.3.x branch won't apply if needed.
Ah very true, I'd forgotten about Negate condition. I was able to test and verify that this is what I need. Thank you for your quick response @jurgenhaas, and for your work on this amazing functionality!
Attaching the following:
- A patch for the 1.1.x branch (because this is the branch I'm currently using)
- A patch for the 2.1.x branch
- A copy of the model that led to this issue. It listens for updates to user entities and, if a role change is detected, sends an email to the site admins, which it finds via a view with the machine name
userlist. Not sure if it has dependencies (like our user list view) that would prevent it from being imported... I'm happy to upload a simplified version if anyone's curious.
Sounds good and makes sense. Thank you for the quick response!
Closing as outdated.
Closing as outdated.
Closing as outdated.
To anyone interested, I've just released a new version (3.0.0) of the moodle_sso module. Please see the release notes → for a full list of changes. I also updated the README file, which will hopefully make how to configure it a little clearer.
To anyone interested, I've just released a new version (3.0.0) of the moodle_sso module. Please see the release notes → for a full list of changes. I also updated the README file, which will hopefully make how to configure it a little clearer.
@netw3rker I could have sworn I answered you back but I don't see my answer in the thread. Ah well, I must not have hit the Save button or something. With all that said, please forgive me for taking so long to respond to you!
Thank you so much for allowing me to take the reins! I've been bogged down by work but am setting up a new branch to incorporate our custom work. I just need to check the issue queue to make sure I don't step on anyone who's contributed potential solutions too.
@gaddman That would be fantastic! We're currently maintaining a custom version of the module, which is working very well in a Drupal 10/Moodle 4.1 site. I'd love to share this with the community.
Updated the patch from #2. Since you've moved all open issues to the 2.x branch, I made a version of the patch for version 2.x of the module. However, since I'm using version 8.x-1.x on my site I also included a version for that branch. I wasn't completely sure if the update hook is needed as it appears to just save the existing value over itself. However, that can be changed if needed.
New version of the patch attached. This version corrects an issue where the behavior triggered twice when the spacebar was pressed, but only once when the enter or mouse buttons were pressed.
Posting this in case it helps anyone else. I had to change the source_langcode and langcode values in the source key from und to en.
Regenerated the patch after updating PHP to version 8.2 and finding a couple issues I introduced unintentionally.
Patch attached.
This was my mistake. I added functionality in another patch that caused this issue. The other patch has been updated and is working now.
Attaching updated patch for the 2.1.0 version.
Patch attached.
I should specify that I'm not completely sure my approach is the right one. I'm absolutely open to any and all feedback, improvements, etc.
@apaderno Could I offer to co-maintain the module? Not sure if that would have the same requirements but thought I'd ask.
Highly unfortunate.
So given that, the only option for this module is to wait for someone with the proper permission to request to take over as maintainer? What if no one ever does?
It seems like my only other option would be to copy the code from this module, start a new module, and go from there. My goal is to avoid creating a copy of the module in my project because, while it would remove the "Unsupported module" error from my site's security report, it wouldn't benefit the community, which is always one of the aims of using a project like Drupal.
Do you have any thoughts or suggestions on a path forward?
@apaderno I just read the FAQ on applying for permission to opt in to security coverage → and I understand I can't do so since I didn't contribute most (in this case, any) of the code).
Given that, can we remove the module from coverage by the security advisory policy? My thought is that once I take over, my first task will be to release a Drupal 10-compatible, coding standards-compliant version. This would likely entail a big change to the code, which I can use to opt the module back in to coverage.
I finally had the chance to give Gin Everywhere a try last week. In my opinion it’s the way to go.
In fact I’d also suggest making Gin Everywhere one of the Gin ecosystem’s recommended modules.
I'd like to take over as a maintainer for this module. I'm currently working on a Drupal 10 site that will use it, and it'd be advantageous if this module were supported and covered by the Drupal security advisory policy.
I have not previously received security coverage opt-in permission, but am happy to go through the process to receive it.
Rerolled the patch and incorporated the change from https://www.drupal.org/project/media_thumbnails_pdf/issues/3414282 📌 Convert CMYK colors to RGB for generated thumbnail RTBC due to encountering CMYK/RGB colorspace issues on our own site.
I've attached a patch containing @is.manu's workaround from #9. I'm not sure if this is the correct solution, but it worked for me, which is good enough for now.
Patch attached.
That's odd, I specifically picked 6.1.x-dev to test against, but somehow it got set to 6.1.x?
Reposting yesterday's patch, targeted against the 6.1.x-dev branch. (Please let me know if I got this wrong, I'm still learning how to do this, and I'm happy to fix it so it's right.)
I also checked the test failure and it appears to be related to line 178 in tests/src/FunctionalJavascript/LinkFieldTest.php:
$assert_session->elementAttributeContains('xpath', '//article/div/div/div[2]/a', 'rel', "nofollow");
Meaning the test failure isn't related to this change, but to another test. However, it's possible something in this change broke the test, so if needed I can look into it further.
I wasn't able to get a patch generated from the merge request to install, so I rolled a patch using the contents of the merge request. Posting it here in case anyone runs into the same issue. All that said, the functionality provided by the merge request works perfectly.
Rerolled the patch against the latest version of the module.
Rerolled the patch for Drupal 10.2.x.
Rerolled the patch for Drupal 10.2.0.
Rerolled the patch after the latest release.
Attaching patch.
Attaching patch.
+1 for DDEV! I've used DDEV, Lando, Docksal, Acquia Dev Desktop (now discontinued), and Docker4Drupal. DDEV is by far the easiest to set up and maintain.
Agreed. I removed the patch and my site's working just fine. Most likely just user error on my part. We can close this issue.
Patch attached!
jsutta → created an issue.
Hello,
On the site I'm working on currently, we use the s3fs module to take over the public/private file system and store files in an S3 bucket, with the temporary file directory still local (see https://www.drupal.org/project/s3fs/issues/3325298 → for why the temporary directory should remain local).
As of today we're using Drupal 10.0.9, version 8.x-1.0-beta1 of Media Thumbnails, and version 2.0.0-beta2 of this module. On my site I have a requirement to generate thumbnails of PDF files as they're uploaded, which is where our use of this module originated. However, with ImageMagick's limitation around stream wrappers I had to find a solution for our site. Now that my patch is working and I've completed functional testing, I'm attaching it to this issue since it was created first and may be of use to others. It's based on a patch from the ImageMagick module's issue queue ( Allow ImageMagick to work with remote streamwrappers (s3) → ). The patch was originally written for Drupal 7, so I had to update it for Drupal 8/9/10.
Some notes from my experience setting up this module:
- Make sure to install ghostscript so ImageMagick can read PDFs (see https://www.php.net/manual/en/imagick.requirements.php for more information)
- If thumbnails aren't being generated and you see a message like this in the log:
cache resources exhausted `/tmp/magick-fCoRMvS5SPEH7_klIKyId3VgmNM24XZL346' @ error/cache.c/OpenPixelCache/4095, you need to increase ImageMagick's memory allocation. To do this, find ImageMagick'spolicy.xmlfile (for reference, mine is located at/etc/ImageMagick-6/policy.xml) and change the following settings, depending on your needs:<policy domain="resource" name="memory" value="256MiB"/>- I increased my value to 2GiB<policy domain="resource" name="disk" value="1GiB"/> - I increased my value to 2GiB
- If thumbnails aren't being generated and you see a message like this in the log:
attempt to perform an operation not allowed by the security policy `PDF' @ error/constitute.c/IsCoderAuthorized/421, you need to change ImageMagick's security policy to allow it to access PDFs (see https://stackoverflow.com/questions/52998331/imagemagick-security-policy...). To do this, find ImageMagick'spolicy.xmlfile (for reference, mine is located at/etc/ImageMagick-6/policy.xml) and uncomment the line that looks like this:<policy domain="module" rights="read|write" pattern="{PS,PDF,XPS}" />(the rights attribute may be set only to read; if it is, update it to read|write).
I've been doing some more work on this. I found that because my first patch (#6) was developed against version 2.0.1, it wouldn't apply to the latest 2.x-dev branch. I've developed and completed functional testing on two new patches:
- 3342951-8-2.0.1.patch: Developed against the module's 2.0.1 tag. Applies cleanly and functional testing completed.
- 3342951-8-2.x.patch: Developed against the 2.x-dev branch. Applies cleanly and functional testing completed.
As soon as I have time I'll circle back around to the unit testing.
Rerolled the patch from #6 against the latest dev branch after I saw the tests couldn't be done because the patch wouldn't apply. Hopefully this one works!
Hi @divyansh,
I've made a couple adjustments to your patch to add an optional message to be displayed to the user if their account is locked out. For the site I'm currently working on, we have to be able to:
- Lock a user account after a configurable number of attempts
- Unlock it after a configurable amount of time
- Display a message to the user in case their account is locked. (I know this part isn't necessarily best practice, but that's our requirement.)
I've attached my version of the patch to this comment in case you want to incorporate it or in case anyone else needs it. If it fails automated testing I'll do my best to fix them.
Rerolled the patch against the latest version of the 8.x-1.x branch.
For some reason the media edit and content translation edit forms weren't using the node form, so I added their routes to the route list, which seems to have fixed the problem. I regenerated the patch based on this change and have attached it to this comment.
Did some testing on this and the first patch worked for me in Drupal 10 with version 8.x-1.1 of the module installed. #5 seemed to have no effect on the module's behavior.
Uploading a new version of the patch that adds a check of the $widget_state variable to InlineParagraphsWidget.php's multipleElementValidate() function. The check must now validate before the rest of the function can execute.
Patch attached.
jsutta → created an issue.
Attaching patch.
#2 worked for me in Drupal 9.5.4 with Rules 8.x-3.0-alpha7. Because I have the Rules module installed I avoided #5 just in case based on the feedback from others.
The dev branch worked for us on Drupal 9.5.3. Just to be completely sure, after I installed the dev version of the module, I rebuilt caches via drush and rechecked the page where I saw the warning. The warning didn't appear, and I didn't see anything in the logs or status report page.
Adding a new version of the patch after my coworker pointed out that getPluginId will always return a string, whereas getPlugin returns a block. He suggested first making sure the block is an instance of Drupal\block\Entity\Block and modifying the if statement so that if the block exists and is an instance of TooltipBlockPluginInterface, it should evaluate to true. I also corrected the spelling of the word "instantiate" on line 45.
Patch attached.
#12 worked for me in Drupal 9.5.2 with Feeds 8.x-3.0-beta3. Thank you @carolpettirossi!