This issue plus others are fixed by the patch / merge request in 🐛 Login in Error Active so I am closing this one as a duplicate.
Tested and working. Nice.
Merged, thank you very much :-)
When you say `$config` do you mean `$options['config']` with the path to the openssl.conf file as described in https://www.php.net/manual/en/function.openssl-csr-new.php ?
There is support for database hints, I don't know if there still is PagerDefault or whether the pager query is pluggable. As far as I knwo Drupal still runs a count query (which is more terrible now that MyISAM is not the default storage engine on the preferred database) so in theory it would still be a good idea to not have to run those at all just to render a pager.
Oh, except I have no way to call that new method from an AwsClientFactory, I think. So it's not actually useful.
The library has been updated for PHP 8.4 compat - https://github.com/bshaffer/oauth2-server-php/releases/tag/v1.14.2
Does this still happen with the dev release without the patch applied?
We've previously hit "too many requests" in 🐛 Throttle code causes rate limit warnings Fixed on the API because the module made a request to check the quota (so it could calculate the send delay) before each email. In our case the problem didn't happen because there were too many actual *emails* being sent.
And found an issue in the Authorize form. The translations placeholders passed in the disclaimer in t() do not match the conditional that checks which ones are set and which not, so if some are NULL they end up breaking the form.
The oauth2-server-php library has a few issues with deprecations, which are addressed by https://github.com/bshaffer/oauth2-server-php/pull/1075
You can add https://patch-diff.githubusercontent.com/raw/bshaffer/oauth2-server-php/... to your composer.patches.json file.
Works as advertised, thanks @beloglazov91
greggles → credited cafuego → .
I live tested this on one of my Drupals yesterday, with the credential timeout set to 900s and a sleep(910); in the send() code :-) It took a bit of fiddling, as I expected the code to throw an `Aws\Sts\Exception\ExpiredTokenException` class but that never happened, it was an SesV2Exception instead with the error code it now checks for.
About 15 minutes after the gateway timeout from nginx I got the logs I expected *and* the amazon_ses test email.
How do you feel about adding a counter in the class to limit the number of retries?
Hmm, does this need to get ported to 3.1.x as well?
The whole point to using Symfony in Drupal is to avoid duplicating effort and then having to maintain more code. Although the package may be large, most isn't used and thus not loaded, so should not be a problem. Disk space is cheap, developer time is not ;-)
The issue is filed against the 7.x version of the module, so checking with Drupal 10 is not that helpful ;-)
This is not a module you use to allow password-less access to your site, this module allows you to use Drupal as a SSO provider, so users can use their Drupal credentials to access other sites.
To allow a script to crawl your Drupal, probably your best bet is a smaller custom module that can check a shared secret (token) and overrides the need for 2FA.
Merged, thanks!
Annoyingly, the test pipeline fails because the tests are (currently) set to use PHP 8.2 and they also default to Drupal 11 core now (which requires PHP 8.3) hurray!
I guess the question then becomes "will anything break when the exp field goes missing?"
Patch from #4 is committed. Thank you!
We can, I forgot to set the issue to fixed so the site didn't auto-close it. D'oh!
I will add a dissenting opinion, just to be helpful. Rather than be limited to us-ascii, Drupal should handle UTF8 throughout. That way, users and developers would be able to properly use Drupal in their local language.
Done, I think?
Patch committed, thanks!
All the tests are green, so I guess now the only question is "does it still do what it is supposed to do" ;-)
Hateful gitlab won't let me push to the feature branch for reasons I cannot fathom.
If you need to redirect the user to a specific URL, you should not hard-code that, but make it a config option.
In the mean time, a good first step is probably to simply check whether the user is blocked and *not* display the generic error if this is the case.
Drupal already shows "The username has not been activated" and the additional generic error message from openid_connect is confusing, as there was no error :-)
Re-opening, as I just ran into this as well.
It's not that overriding causes an issue, it's that the module allows you to configure an AAD client without a key (it is a not a required field) but then assumes a key exists during authentication and throws this error if it doesn't.
So either the code should be conditional, check if a key is configured before calling getValue() on it *or* make the key mandatory in the client config.
Gracias! :-)
Okay, I think I know where you're coming from.
I had a look at the PR and I *think* it's probably nicer to not throw an access denied exception but instead return a new 403 response:
return new BridgeResponse([], 403);
If you're going to add a new alter hook, you're going to have to add documentation for it in oauth2_server.api.php ;-)
The code and service for OAuth2DrupalAuthProvider both look fine to me.
@PrabuEla that backtrace looks like it might be an issue with bshaffer/oauth2-server-php. Which version of that library is installed?
I think this can be closed; the phpcs tests now run with PHP 8.2 and pass.
I did a wee bit of refactoring to use the request object instead of global $_GET but that's it. Merged, cheers :-)
I just hit this error with the 2.0.0 version. I did *not* use a `drush uli` login URL, but tried to login with my username, password and 2FA token.
This happens when I enable the module (and js_cookie) via config/core.extensions.yml and do not include any other config.
The remaining test fails are not my responsibility :-)
It's a bit of scope creep, but may it would be good to also add in support for the `DurationSeconds` and `ExternalId` parameters.
This should work, but by the time you have 50 clients, the permissions form would be a 100MB page load, so there are some potential practical obstacles ;-)
I have a similiar issue for my use case and what I've done is link a taxonomy vocabulary to my users. The taxonomy terms match sites that uses should be allowed to access. To grant access, I tag a user. To remove access, I untag them. A different vocabulary controls roles.
To achieve that, I've written a small custom module that maps the vocabularies to custom claims in `hook_oauth2_server_user_claims_alter()`. The client side of things can then check the claims and grant access and permissions based on that.
Fine, so with SESv2 is actually works as-is. Never mind.
It never ends, huh?
Re-done as MR (so the tests can run) and merged. Thank you!
This seems to have been fixed as part of the giant Drupal 10 compat PR.
I'm not sure how you ended up with so many rows in the `__scopes` tables; I have exactly 0 in both.
However, since the current cron hook *does* limit by the expires field on both oauth2_server_token and oauth2_server_authorization_code it is entirely reasonable to create an index on those fields.
All merged. Thanks!
Thanks for the patch @dpacassi. I'm merging the older 🐛 Oauth2Controller uses a LoggerChannelFactory argument instead of LoggerChannelFactoryInterface Active instead, they got in first ;-)
cafuego → made their first commit to this issue’s fork.
I've merged a large Drupal 10 compat patch set and this patch either needs a refactor or is no longer needed.
This patch no longer applies after I merged the large Drupal 10 auto-compat MR. It will need a re-factor (if you refactor it to a merge requests, we can run the tests! :-)
I am closing this as a duplicate of 🐛 Deprecated function: trim(): Passing null to parameter #1 ($string) of type string is deprecated in OAuth2DrupalAuthProvider->applies() Needs review . Also patches or merge requests should be against the latest dev branch, not against an (old) release tag.
Merged, thank you.
Nice, that does the trick. Thank you :-)
Merged, thank you.
Thanks everyone, merge request is merged.
Thanks @bojanz 🙏🏼
I've started seeing this on recent builds that used composer 2.7.0 or newer as root in a docker image.
In that case, the issue is that composer no longer runs plugins (like say the drupal-scaffold plugin) as root unless you set COMPOSER_ALLOW_SUPERUSER=1 in the environment.
This is in regards to the https://www.drupal.org/project/oauth2_server → project.
I contacted bojanz via the d.o contact form, but have not had a response.
Your patch definitely fixes the issue at hand. It still applies to newer versions of tome as well, so I expect they cause the same problem if not patched.
I'd try with newer versions of tome, but v1.12 won't install as-is in Drupal 9 and once I do make it install by bodging the info yaml, the drush commands seem to not get loaded.
I have a patch for a work-around, but I'm not sure if this isn't a bad idea :-)
class AmazonSesMailQueue extends QueueWorkerBase implements ContainerFactoryPluginInterface {
use HandlerTrait;
/**
* {@inheritdoc}
*/
public static function create(ContainerInterface $container, array $configuration, $plugin_id, $plugin_definition) {
$instance = new static(
$configuration,
$plugin_id,
$plugin_definition
);
// Only set the handler if queueing is enabled to avoid an error when
// trying to run without config.
$enabled = \Drupal::config('amazon_ses.settings')->get('queue');
if ($enabled) {
$instance->setHandler($container->get('amazon_ses.handler'));
}
return $instance;
}
I may need to add in a bit more work, the test tab also WSODs currently. May as well include that.
The error message about quota is a bit deceptive given it shows on the statistics tab, but the quota function is the one that fails, so ¯\_(ツ)_/¯