@dagmar Thank you for the catches! I've added the access dblog reports permission in the other locations where the access site reports was already being used, with one exception:
In modules/migrate_drupal_ui/tests/src/Functional/MigrateUpgradeExecuteTestBase.php: The user in this test is the root user (uid 1), so should implicitly have all permissions. With that in mind there's no need to set the specific permission.
@cmlara I'm so sorry about how long it took me to get this submitted!
I just pushed an update that does the following:
- Adds a setting called "Show default validation plugin first". If this setting is enabled, the default validation plugin will be shown first in lists of validation plugins (e.g., on the settings page, on the TFA setup page)
- Disables users' ability to set up validation plugins except for the default validation plugin if the default validation isn't set up. For example, if TOTP is the default validation plugin and recovery codes is also enabled, users must set up TOTP before they can set up recovery codes.
At this point I haven't added tests because I wanted to check in first to ensure you agree with the approach and see if you can think of anything I might have missed. One thing I think might be worth adding is a setting to control enabling/disabling this feature. Thoughts?
Hi @jeffschuler, thank you for getting back to me. I just checked it out and the patch worked like a charm for me.
@smustgrave I finally got the tests completed and all are now passing. Would you take another look at it and let me know if any other changes need to be made?
@smustgrave Thank you for the suggestions and I apologize it took me so long to circle back to this.
Here's a summary of the latest changes:
- Added an update hook to update the Watchdog view if it uses the default permissions (I assume we want to leave it alone if the site owner has customized it).
- Added test coverage for the update hook (the test coverage checks the permission on the Watchdog view before and after the update).
- Added test coverage to check if a user with the 'access site reports' permission can see the Reports menu item in the admin toolbar.
- Added test coverage to check if a user with the 'access dblog reports' permission can access the Recent log messages page.
This is the first time I've written these types of tests, so please double-check them and let me know if anything is missing or incorrect.
Also, I haven't addressed the test failures yet. I hope to do so within the next couple days.
@mradcliffe I finally had a chance to circle back to this to open the merge request. I went ahead and added the role attribute as suggested too.
+1 for a new release ASAP. We lost custom metadata for hundreds of pages and had to reconstruct it with a mixture of scripts and manual work.
I agree with that, since they're the one taking the action.
jsutta → created an issue.
Just made a slight change... I added a new $user variable whose value is set based on whether or not the getAccountName method is available. If it's available, it's set to $account->getAccountName(), and if not it's set using the current method, $account->getAccount()->name.
The change still addresses an issue that's only there because of the masquerade module, but could be a good compromise.
Hi all,
Sorry for the delay! I just had a chance to check and found that if I uninstall the masquerade module this issue doesn't occur when I log out (and the logout event is logged as expected).
Does that mean this is actually an issue with the masquerade module?
@chamilsabjeewa I had the same problem the other day. I created a 10.4.x branch and opened a merge request for it. You can get a working patch from there. It's working well for me so far on 10.4.2.
Added the change to the issue's fork.
jsutta → created an issue.
#13 worked for me as well with version 8.x-1.0-alpha6 of the module, PHP 8.2.26, and Drupal 10.4.2.
When I tried the version in the 2797583-11.x branch, the action failed to run on translations with an error stating "This value should not be null." I looked into this and was able to find that the translation was being validated even though its isValidationRequired flag was FALSE. Given this, my assumption is that the validation check doesn't always need to be run. However, if this is incorrect please let me know and we can reassess.
For now, I've pushed an update to the branch that checks the isValidationRequired flag and only performs the validation if it returns TRUE. Otherwise, it sets $violations to 0. It also checks to make sure $violations is an instance of EntityConstraintViolationList before attempting to use the count() method.
With this code in place, I'm able to use the action on both untranslated and translated nodes.
One more new version of the patch. I forgot to add dblog.permissions.yml to the previous versions.
Updated the patch as I missed a couple uses of the 'access site reports' permission.
Adding a patch for 10.3.x (that's our current branch) in case anyone else can use it.
RTBC +1!
We've been maintaining a local copy of this patch, updating it periodically when updates are released.
I tried to update the issue's MR to target the 6.0.x branch and rebase it, but couldn't change the target branch. Probably for the best in case others are using it. That being the case, I tried to create a new feature branch based on 6.0.x, but only the branches available in the fork are available to base the branch on. If anyone knows how to bring 6.0.x into the fork, or create a new fork in this same issue, please let me know and I'm happy to do the work. I've attached a patch that installs with the latest release (6.0.5).
jsutta → changed the visibility of the branch 3227245-entity-lookup-doesnt to active.
jsutta → changed the visibility of the branch 3227245-entity-lookup-doesnt to hidden.
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.