This PR was merged into the 5.1-dev branch.
Discussion
----------
[Security] Added LDAP support to Authenticator system
| Q | A
| ------------- | ---
| Branch? | master
| Bug fix? | no
| New feature? | yes
| Deprecations? | no
| Tickets | -
| License | MIT
| Doc PR | -
The last missing authenticator in the new system 🎉
I have no experience with LDAP at all and I didn't succeed in setting up a server locally. So I can't test whether this works, but the unit test works (and also tested in a real app, while adding a `dd()` call in the listener).
---
I want to share with you the current state of Security LDAP, how this PR implements it and a possible other solution (which I think I would prefer most). Is there anyone who can share their opinions on this? (hopefully @weaverryan and @csarrazi can share their opinion, as they have most experience on this topic)
1. **Current Solution: An LDAP authentication provider + duplicated `SecurityFactory` classes**
LDAP is done in one centralized authentication provider. This provider is configured by security factories for each core factory (e.g. `form_login` becomes `form_login_ldap`, `http_basic` becomes `http_basic_ldap`).
2. **Implementation in this PR: A listener is executed before the default `VerifyCredentialsListener`, to verify `PasswordCredentials`**
This listener must be configured for each specific authenticator wanting to use LDAP. This is a technique similar to (1). It's a bit difficult to use this for your own authenticator (you need to configure a custom listener service) and still needs the duplicated factory classes
3. **Proposal: Introduce a `LdapCredentials` class and always register a listener**
If an authentictor returns `LdapCredentials`, it'll be checked using the LDAP verification listener. This is the easiest for custom authenticators and would remove the duplicated factories, I can imagine `form_login` getting a new `ldap` sub option to configure the settings.
The main disadvantage (I think) is that we would need to make `LdapCredentials` configure all options: ldap service, dnString, searchDn, searchPassword & queryString. Especially passing around the ldap service seems a bit weird. The main questions here are: Is it weird to pass all these things in the `LdapCredentials`? And, do we really need to support having multiple LDAP configuration sets for different authenticators? Or can we e.g. add a global `security.ldap` configuration, that registers the listener for all authenticators returning `LdapCredentials`?
Commits
-------
20962e604a [Security] Added LDAP support to Authenticator system
This PR was squashed before being merged into the 5.1-dev branch.
Discussion
----------
[Messenger] Add option to stop the worker after a message failed
| Q | A
| ------------- | ---
| Branch? | master
| Bug fix? | no
| New feature? | yes
| Deprecations? | no
| Tickets | Fix#34414
| License | MIT
| Doc PR |
This pull request adds a new option to the `messenger:consume` command, to stop the worker after a specified amount of failed messages was handled by the worker.
Commits
-------
ea79206470 [Messenger] Add option to stop the worker after a message failed
This PR was squashed before being merged into the 5.1-dev branch.
Discussion
----------
[Translations] Throw exception if xFileLoader dependencies don't exist.
| Q | A
| ------------- | ---
| Branch? | master
| Bug fix? | no
| New feature? | yes
| Deprecations? | no
| Tickets | Fix#36658
| License | MIT
| Doc PR |
`XliffFileLoader` & `QtFileLoader` both require `XmlUtils::class` from the `Config` component. This PR throws a friendly exception is the `Config` component does not exist.
Original idea by @xabbuh was to throw the exception from the __constructor. This PR throws the exception from the `load()` method to be consistent with the `YamlFileLoader::class`. But that can easily be changed.
Commits
-------
627e476eb4 [Translations] Throw exception if xFileLoader dependencies don't exist.
This PR was squashed before being merged into the 5.1-dev branch.
Discussion
----------
[AmazonSqsMessenger] Use AsyncAws to handle SQS communication
| Q | A
| ------------- | ---
| Branch? | master
| Bug fix? | no
| New feature? | yes
| Deprecations? | no
| Tickets | /
| License | MIT
| Doc PR | /
Similar to #35992 this PR use AsyncAws to handle Sqs messages sent/receive
It move complexity of authentication/streaming outside Symfony while keeping HttpClient integration.
Commits
-------
7c4888eed1 [AmazonSqsMessenger] Use AsyncAws to handle SQS communication
This PR was merged into the 5.1-dev branch.
Discussion
----------
[Messenger] Fix messenger:failed:remove can not remove single message
| Q | A
| ------------- | ---
| Branch? | master
| Bug fix? | yes
| New feature? | no
| Deprecations? | no
| Tickets | Fix#36659
| License | MIT
| Doc PR | -
Fix this error:
```
count(): Parameter must be an array or an object that implements Countable
```
When calling `messenger:failed:remove` command from other code with single id
Commits
-------
e66cd97ec3 [Messenger] Fix messenger:failed:remove can not remove single message
This PR was merged into the 5.1-dev branch.
Discussion
----------
Add support of PHP8 static return type for withers
| Q | A
| ------------- | ---
| Branch? | master
| Bug fix? | no
| New feature? | yes
| Deprecations? | no
| Tickets | https://github.com/orgs/symfony/projects/1#card-35852557
| License | MIT
| Doc PR |
Commits
-------
04fdf05cff Add support of PHP8 static return type for withers
This PR was merged into the 5.1-dev branch.
Discussion
----------
[DI] allow loading and dumping tags with an attribute named "name"
| Q | A
| ------------- | ---
| Branch? | master
| Bug fix? | no
| New feature? | yes
| Deprecations? | no
| Tickets | -
| License | MIT
| Doc PR | -
This is a minor feature added for consistency: using PHP, we can already define tags with an attribute named `"name"`. But then, we cannot dump such definitions in YAML nor XML since we don't have a syntax to declare such tags in these formats.
I spotted this while looking at a dumped container: we already use an attribute named `"name"` on two tags: `cache.pool` and `workflow.definition`. Currently, the dumped XML is wrong because of this.
This PR enables the following new syntaxes (the current style still works as usual):
- in YAML, consistently with the new syntax for method calls:
```yaml
tags:
- cache.pool: { name: my_cache_pool }
```
- in XML:
```xml
<tag name="my_cache_pool">cache.pool</tag>
```
Commits
-------
b023e4cac3 [DI] allow loading and dumping tags with an attribute named "name"
This PR was merged into the 5.1-dev branch.
Discussion
----------
[HttpKernel] make kernels implementing `WarmableInterface` be part of the cache warmup stage
| Q | A
| ------------- | ---
| Branch? | master
| Bug fix? | no
| New feature? | yes
| Deprecations? | no
| Tickets | -
| License | MIT
| Doc PR | -
This allows your kernel to return extra classes to preload also (which was my main motivation for creating this PR actually.)
```php
// ...
use Symfony\Component\HttpKernel\CacheWarmer\WarmableInterface;
// ...
class Kernel ... implements ..., WarmableInterface
{
// ...
public function warmUp(string $cacheDir): array
{
// ...
return [
SomeClassToPreload::class,
AnotherClassClassToPreload::class,
$cacheDir.'/some-file-to-preload.php',
// ...
];
}
// ...
}
```
Commits
-------
649e530356 [HttpKernel] make kernels implementing `WarmableInterface` be part of the cache warmup stage
This PR was merged into the 5.1-dev branch.
Discussion
----------
[Form] deprecate not using a rounding mode
| Q | A
| ------------- | ---
| Branch? | master
| Bug fix? | yes
| New feature? | no
| Deprecations? | yes
| Tickets |
| License | MIT
| Doc PR |
Commits
-------
25ba1a241d deprecate not using a rounding mode
This PR was squashed before being merged into the 5.1-dev branch.
Discussion
----------
[Mailer] Use AsyncAws to handle SES requests
| Q | A
| ------------- | ---
| Branch? | master
| Bug fix? | no
| New feature? | yes
| Deprecations? | no
| Tickets | #33183, #35468 and #35037
| License | MIT
| Doc PR | TODO
alternative to #33326
This PR replace the native code to call AWS SES by the new [AsyncAws](https://github.com/async-aws/aws) project maintained by @Nyholm and me.
This removes complexity of signing request, and adds new features likes:
- authentication via .aws/config.ini, Instance profile, WebIdentity (K8S service account)
- usesignature V4 (the one recommanded by the Official SDK )
- fully compatible with API (uses the official AWS SDK interface contract to generate classes)
Because it's based on `symfony/http-client`, it's fully integrable with Symfony application.
Commits
-------
21243874bc [Mailer] Use AsyncAws to handle SES requests
This PR was merged into the 4.4 branch.
Discussion
----------
[Translation] Fix for translation:update command updating ICU messages
| Q | A
| ------------- | ---
| Branch? | 4.4
| Bug fix? | yes
| New feature? | no
| Deprecations? | no
| Tickets | Fix#36459
| License | MIT
If `translation:update` command executed with option `--domain=messages` – it ignore `messages-intl-icu` file and just create new `messages`
Method `TranslationUpdateCommand::filterCatalogue()` on `MessageCatalogue::all()` method to get all messages for domain
But `MessageCatalogue::all()` method disredard `intl-icu` domains and simply merge all.
[Translation] added $strict parameter for MessageCatalogueInterface::all() to be able to get only defined domain messages
[FrameworkBundle] modified translation:update command to respect intl-icu domain
Commits
-------
567cee5f02 [Translation] Fix for translation:update command updating ICU messages
This PR was merged into the 5.1-dev branch.
Discussion
----------
[Security] Removed anonymous in the new security system
| Q | A
| ------------- | ---
| Branch? | master
| Bug fix? | no
| New feature? | yes
| Deprecations? | no
| Tickets | -
| License | MIT
| Doc PR | tbd
This was one of the "Future considerations" of #33558:
> Drop the AnonymousToken and AnonymousAuthenticator: Anonymous authentication has never made much sense and complicates things (e.g. the user can be a string). For access control, an anonymous user has the same meaning as an un-authenticated one (null). This require changes in the AccessListener and AuthorizationChecker and probably also a new Security attribute (to replace IS_AUTHENTICATED_ANONYMOUSLY). Related issues: #34909, #30609
This new experimental system is probably a once-in-a-lifetime change to make this change. @weaverryan and I have had some brainstorming about this. Some reasons why we think it makes 100% sense to do this change:
* From a Security perspective, **a user that is not authenticated is similar to an "unknown" user**: They both have no rights at all.
* **The higher level consequences of the AnonymousToken are confusing and inconsistent**:
* It's hard to explain people new to Symfony Security that not being logged in still means you're authenticated within the Symfony app
* To counter this, some higher level APIs explicitly mark anonymous tokens as not being authenticated, see e.g. the [`is_authenticated()` expression language function](https://github.com/symfony/symfony/blob/master/src/Symfony/Component/Security/Core/Authorization/ExpressionLanguageProvider.php#L33-L37)
* The anonymous authentication resulted in the `IS_AUTHENTICATED` security attribute being removed from #35854, as there was no clear consensus on what its meaning should be
* **Spring Security, which is where this originated from, makes Anonymous a very special case**:
> Finally, there is an AnonymousAuthenticationFilter, which is chained after the normal authentication mechanisms and automatically adds an AnonymousAuthenticationToken to the SecurityContextHolder if there is no existing Authentication held there.
>
> Note that there is no real conceptual difference between a user who is “anonymously authenticated” and an unauthenticated user. Spring Security's anonymous authentication just gives you a more convenient way to configure your access-control attributes. Calls to servlet API calls such as getCallerPrincipal, for example, will still return null even though there is actually an anonymous authentication object in the SecurityContextHolder.
* Symfony uses AnonymousToken much more than "just for convience in access-control attributes". **Removing anonymous tokens allows us to move towards only allowing `UserInterface` users**: #34909
---
Removing anonymous tokens do have an impact on `AccessListener` and `AuthorizationChecker`. These currently throw an exception if there is no token in the storage, instead of treating them like "unknown users" (i.e. no roles). See #30609 on a RFC about removing this exception. We can also see e.g. the [Twig `is_granted()` function explicitly catching this exception](https://github.com/symfony/symfony/blob/master/src/Symfony/Bridge/Twig/Extension/SecurityExtension.php#L37-L52).
* **To make the changes in `AccessListener` and `AuthorizationChecker` BC, a flag has been added - default enabled - to throw an exception when no token is present** (which is automatically disabled when the new system is used). In Symfony 5.4 (or whenever the new system is no longer experimental), we can deprecate this flag and in 6.0 we can never throw the exception anymore.
* **`anonymous: lazy` has been deprecated in favor of `{ anonymous: true, lazy: true }`** This fixes the dependency on `AnonymousFactory` from the `SecurityExtension` and allows removing the `anonymous` option.
* **Introduced `PUBLIC_ACCESS` Security attribute** as alternative of `IS_AUTHENTICATED_ANONYMOUSLY`. Both work in the new system, the latter only triggers a deprecation notice (but may be usefull to allow switching back and forth between old and new system).
cc @javiereguiluz you might be interested, as I recently talked with you about this topic
Commits
-------
ac84a6c5d9 Removed AnonymousToken from the authenticator system
* Anonymous users are actual to unauthenticated users, both are now represented by no token
* Added a PUBLIC_ACCESS Security attribute to be used in access_control
* Deprecated "anonymous: lazy" in favor of "lazy: true"
This PR was squashed before being merged into the 5.1-dev branch.
Discussion
----------
[SecurityBundle] Fixed entry point service ID resolving and multiple guard entry points
| Q | A
| ------------- | ---
| Branch? | master
| Bug fix? | yes
| New feature? | no
| Deprecations? | no
| Tickets | -
| License | MIT
| Doc PR | n/a
@fabpot I am not able to reproduce [the error you reported](https://github.com/symfony/symfony/pull/36575#issuecomment-622272051) in any of my demo applications or in the tests introduced in this PR. The error indicates that no entry point is configured in your application, can you maybe try out this patch (given it now makes a hard error when more than one guard is used)? If it still doesn't work, can you maybe share your firewall configuration?
---
_build failures are unrelated_
Commits
-------
c75659350e Do not make AbstractFactory internal and revert method rename
6870a18803 Fixed entry point resolving and guard entry point configuration
This PR was merged into the 5.1-dev branch.
Discussion
----------
[Security] Renamed VerifyAuthenticatorCredentialsEvent to CheckPassportEvent
| Q | A
| ------------- | ---
| Branch? | master
| Bug fix? | no
| New feature? | no
| Deprecations? | no
| Tickets | Fix#36608
| License | MIT
| Doc PR | -
This event was named long before we introduced the concept of passports. Listeners on this event check the user, the credentials and any badges of the Security passport. I think `CheckPassportEvent` makes the most sense (more than `CheckCredentialsEvent`).
Also, I managed to break fabbot in the large PR. Just checked all new classes and added license headers in case they were missing (fabbot complained about most of them in this PR already).
Commits
-------
5ba4d1de86 Renamed VerifyAuthenticatorCredentialsEvent to CheckPassportEvent
This PR was squashed before being merged into the 3.4 branch (closes#36627).
Discussion
----------
[Validator] fix lazy property usage.
| Q | A
| ------------- | ---
| Branch? | 3.4
| Bug fix? | yes
| New feature? | no
| Deprecations? | no
| Tickets | Fix#36343
| License | MIT
| Doc PR |
This attempts to fix a large regression introduced in #36343, which broke recursing values returned from `getter` Constraints, because they are now wrapped in in a `LazyProperty`. The `LazyProperty` needs to be evaluated because some checks are done on the type of `$value`, i.e `is_array` etc... in `validateGenericNode`.
I'm concerned that the original PR didn't really add sufficient test coverage for the introduction of `LazyProperty`, and I'm not 100% sure that I've caught all the cases where the `instanceof` check are needed in this PR.
For the tests, I added the `@dataProvider getConstraintMethods` to every test that hit the problem area of code.
~~The only issue is that my fixed has broken the test introduced in #36343, `testGroupedMethodConstraintValidateInSequence`.~~
~~I think I need @HeahDude to help me work through this. Maybe there is a more simple solution, one that doesn't require doing `instanceof LazyPropery` checks in multiple places, because this feels very brittle.~~
EDIT: fixed that test.
Commits
-------
281861e788 [Validator] fix lazy property usage.
This PR was merged into the 3.4 branch.
Discussion
----------
Fix Form annotation
| Q | A
| ------------- | ---
| Branch? | 3.4
| Bug fix? | yes
| New feature? | no
| Deprecations? |no
| License | MIT
Symfony form component provide his own exception `OutOfBoundsException` which is implementing `ExceptionInterface`. Form are returning this custom `OutOfBoundsException`, but the phpDoc say it's an original `\OutOfBoundsException`, which is not extending the `ExceptionInterface`.
This can provide issue with static analysis tools.
Commits
-------
67b744929f Fix annotation
This PR was merged into the 4.4 branch.
Discussion
----------
[Form] provide a useful message when extension types don't match
| Q | A
| ------------- | ---
| Branch? | 4.4
| Bug fix? | no
| New feature? | no
| Deprecations? | no
| Tickets | Fix#36610
| License | MIT
| Doc PR |
Commits
-------
88d836643a provide a useful message when extension types don't match
This PR was merged into the 4.4 branch.
Discussion
----------
[Serializer] do not transform empty \Traversable to Array
| Q | A
| ------------- | ---
| Branch? | 4.4
| Bug fix? | yes
| New feature? | no <!-- please update src/**/CHANGELOG.md files -->
| Deprecations? | no <!-- please update UPGRADE-*.md and src/**/CHANGELOG.md files -->
| Tickets | na
| License | MIT
| Doc PR | na
Today, using `PRESERVE_EMPTY_OBJECTS` ([introduced in 4.0](f28e826627)), the JSON serialization of:
```php
<?php
$object = [];
$object['foo'] = new \ArrayObject();
$object['bar'] = new \ArrayObject(['notempty']);
$object['baz'] = new \ArrayObject(['nested' => new \ArrayObject()]);
```
Outputs:
```json
{"foo":[],"bar":["notempty"],"baz":{"nested":[]}}
```
Instead of the expected:
```json
{"foo":{},"bar":["notempty"],"baz":{"nested":{}}}
```
This issue comes from the Serializer that transforms `Traversable` to an Array [here](11a707200d/src/Symfony/Component/Serializer/Serializer.php (L159)). Also, the `AbstractObjectNormalizer` [doesn't support Traversable](11a707200d/src/Symfony/Component/Serializer/Normalizer/AbstractObjectNormalizer.php (L134)), but he allows to preserve empty objects.
I propose this patch where the fix doesn't transform a `Traversable` to an Array. I see another way to patch this in which we could allow empty Traversable in the `AbstractObjectNormalizer` (not sure it's better though). See attached [other-fix.patch](https://github.com/symfony/symfony/files/4539865/other-fix.log) to see the alternative patch.
Commits
-------
e5c20293fa Fix serializer do not transform empty \Traversable to Array
This PR was merged into the 5.1-dev branch.
Discussion
----------
Add missing port SQS Host Header request
| Q | A
| ------------- | ---
| Branch? | master
| Bug fix? | yes
| New feature? | no
| Deprecations? | no
| Tickets | /
| License | MIT
| Doc PR | /
When user provides a custom endpoint, the port is missing from the `Host` headers, leading to wrong URL when calling `getQueueUrl`
Commits
-------
41165beb48 Add missing port SQS Host Header request
This PR was merged into the 4.4 branch.
Discussion
----------
[Cache] Fixed not supported Redis eviction policies
| Q | A
| ------------- | ---
| Branch? | 4.4
| Bug fix? | yes
| New feature? | no
| Deprecations? | no
| Tickets | no
| License | MIT
| Doc PR | no
**Steps to reproduce:**
1. Define the following redis service on SymfonyCloud:
```
rediscache:
type: redis:5.0
size: S
configuration:
maxmemory_policy: allkeys-lru
```
2. Deploy the change
**Expected result:**
No redis cache will be populated
**Actual result:**
Following exception is thrown:
```
[2020-04-28T05:35:58.440403-04:00] php.CRITICAL: Uncaught Error: Return value of Symfony\Component\Cache\Adapter\RedisTagAwareAdapter::doSave() must be of the type array, bool returned {"exception":"[object] (TypeError(code: 0): Return value of Symfony\\Component\\Cache\\Adapter\\RedisTagAwareAdapter::doSave() must be of the type array, bool returned at /app/vendor/symfony/cache/Adapter/RedisTagAwareAdapter.php:100)"} []
```
Commits
-------
3d6e942da5 [Cache] Fixed not supported Redis eviction policies
This PR was merged into the 5.1-dev branch.
Discussion
----------
[FrameworkBundle][CacheWarmupCommand] Append files to preload
| Q | A
| ------------- | ---
| Branch? | master
| Bug fix? | yes
| New feature? | no
| Deprecations? | no
| Tickets | -
| License | MIT
| Doc PR | -
Files returned by the warmUp() method must be added to the preload file.
I also added checks to avoid empty preload calls in the preload file like this:
```
$classes = [];
Preloader::preload($classes);
```
Commits
-------
a82c7ab4c0 [FrameworkBundle][CacheWarmupCommand] Append files to preload