Tprod 296 Authentication refactoring needed for Symfony 5 #11057
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
We'll see what this breaks
cla-bot
bot
added
the
cla-signed
…ere is no need for authorization New feature of Symfony 4.4
… different shapes
…ate as there is no need for extending it
…y so many tests are failing
…t's breaking the tests
…ng mail from config
Codecov Report
@@ Coverage Diff @@
## 5.x #11057 +/- ##
============================================
+ Coverage 48.99% 49.15% +0.15%
Complexity 35399 35399
============================================
Files 2147 2147
Lines 105609 105637 +28
============================================
+ Hits 51744 51921 +177
+ Misses 53865 53716 -149
|
RCheesley
previously approved these changes
May 27, 2022
There was a problem hiding this comment.
I checked with the regular user login but unfortunately between the instructions in the linked PR and the developer docs I was not able to fathom how to test the OAuth2 login or SSO. So someone will need to check both of those (and we should improve our docs on this!)
biozshock
approved these changes
Sep 8, 2022
There was a problem hiding this comment.
Form login and the oAuth2 worked as expected.
POST https://mautic.mautic/oauth/v2/token
Accept: application/json
Content-Type: application/json
{ "grant_type": "client_credentials", "client_id": "<redacted>", "client_secret": "<redacted>" }
HTTP/1.1 200 OK
...
{
"access_token": "ODdmNzk2ODBmYTlkM2YzMzViZDQxNjI4ZDNmYTQ0YjRmNmZlOTNkZDI5MjYyMzA4MzhlZjQwMWRlZTM3NjAxNg",
"expires_in": 3600,
"token_type": "bearer",
"scope": null
}
GET https://mautic.mautic/api/contacts
Accept: application/json
Authorization: Bearer ODdmNzk2ODBmYTlkM2YzMzViZDQxNjI4ZDNmYTQ0YjRmNmZlOTNkZDI5MjYyMzA4MzhlZjQwMWRlZTM3NjAxNg
HTTP/1.1 200 OK
...
{
"total": "14",
"contacts": {
"29": {
"isPublished": true,
.....
escopecz
added
ready-to-commit
escopecz
added a commit
to escopecz/mautic
that referenced
this pull request
Sep 13, 2022
* Disabling the cookie service mocking for tests as it causes problems We'll see what this breaks * Functional tests for Oauth2 * The lazy anonymous mode prevents the session from being started if there is no need for authorization New feature of Symfony 4.4 * Removing deprecated interface from AuthenticationListener * Adding option to skip setting mock services in a test * Fixing test that was not asserting anyting useful * CS fixes * Fixing the flag condition * Backing up the strict type for providerKey as it seems it can come in different shapes * Making the class final and properties for AuthenticationListener private as there is no need for extending it * CS fix * Fixing several tests that were missing the table prefix * Trying to revert the change in Abstract test case to see if that's why so many tests are failing * CS fix * Oops, I meant to leave this line uncommented * CS fix * Trying if the lazy value is breaking the tests * Trying to run the new test in a new process * Yes! New process worked. Adding back what was removed to find out what's breaking the tests * Improving tests to print the response for debugging * Adding response content to the asssertion checks for debugging * Documenting the BC break about AuthenticationListener * Trying without enableReboot * Finally figured out why it was trying to install Mautic. It was missing mail from config
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Description:
I started with this by looking what is the AuthenticationListener used for and found last changes coming from #9837 so I got an impression it is used for Oauth2 and wrote a test for it. It turns out it's not that related and the AuthenticationListener that needed refactoring is actually used for user login and SSO. Anyway, the test for Oauth2 is useful to have.
The changes needed to get rid of the deprecated ListenerInterface are actually nicely described in https://stackoverflow.com/questions/56438177/deprecated-the-listenerinterface-turn-your-listeners-into-callables-instead and if I'd follow that instead of trying to figure out the usage from git history and the code then it would save a lot of time.
This PR also changes configuration for the "anonymous" security option from
trueto'lazy'. That is a new option since Symfony 4.4 described asSo I was hoping it will stop creating session cookies on those routes but it behaves the same as before.
Steps to test this PR: