Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Possible fix for #7294 #7305

Merged
merged 4 commits into from Mar 14, 2020
Merged

Possible fix for #7294 #7305

merged 4 commits into from Mar 14, 2020

Conversation

@TonyBogdanov
Copy link
Contributor

TonyBogdanov commented Mar 6, 2019

When the configuration is updated a ConfigEvent is dispatched carrying both the original form submission data and a normalized version. In the normalized version files are represented by UploadedFile instances.

The event is being picked up by the audit logger which attempts to store it in the database by serializing the configuration into a string. According to symfony/symfony-docs#8180 UploadedFile instances cannot be serialized and throw an exception, which possibly what is happening in #7294.

With the above PR I am passing the normalized data through a filter which replaces all UploadedFile instances (recursively) with their contents (maybe the content is not the best idea?) when being passed to the event, allowing for the whole thing to be further serialized.

Please be sure you are submitting this against the staging branch.

Q A
Bug fix? Yes
New feature? No
Automated tests included? No
Related user documentation PR URL
Related developer documentation PR URL
Issues addressed (#s or URLs) #7294
BC breaks? No
Deprecations? No

Steps to reproduce the bug:

  1. Try to upload an IDP metadata file under Configuration / User/Authentication Settings.
TonyBogdanov added 4 commits Mar 6, 2019
When the configuration is updated a `ConfigEvent` is dispatched carrying both the original form submission data and a normalized version. In the normalized version files are represented by `UploadedFile` instances.

The event is being picked up by the audit logger which attempts to store it in the database by serializing the configuration into a string. According to symfony/symfony-docs#8180 `UploadedFile` instances cannot be serialized and throw an exception, which possibly what is happening in #7294.

With the above PR I am passing the normalized data through a filter which replaces all `UploadedFile` instances (recursively) with their contents (maybe the content is not the best idea?) when being passed to the event, allowing for the whole thing to be further serialized.
@YosuCadilla

This comment has been minimized.

Copy link

YosuCadilla commented Mar 7, 2019

Same issue here, applying this PR manually seems to work fine.

@alanhartless alanhartless added this to the 2.16.0 milestone Mar 12, 2019
@npracht npracht modified the milestones: 2.16.0, 2.15.2 Mar 28, 2019
@npracht npracht added the Code Review label Apr 4, 2019
@npracht npracht added this to Ready to Test (confirmation) in Mautic 2 Apr 4, 2019
@kuzmany kuzmany modified the milestones: 2.15.2, 2.16.0 May 20, 2019
@kuzmany

This comment has been minimized.

Copy link
Contributor

kuzmany commented May 20, 2019

Moved to next version.
Need more time for code review and tests

@npracht npracht moved this from Ready to Test (confirmation) to Ready to Test (first time) in Mautic 2 Aug 15, 2019
@mautibot

This comment has been minimized.

Copy link

mautibot commented Dec 18, 2019

This pull request has been mentioned on Mautic Community Forums. There might be relevant details there:

https://forum.mautic.org/t/unable-to-save-sso-configuration-with-updated-federationmetadata-xml/12058/2

@J-Light

This comment has been minimized.

Copy link

J-Light commented Dec 19, 2019

Had the same issue discussions in the link by @mautibot . Appying PR fixed the issue.

@luk4s
luk4s approved these changes Mar 10, 2020
Copy link

luk4s left a comment

Checked on 2.16.0 and worked 👍

@npracht npracht added this to Ready to test in Mautic 2 Mar 10, 2020
@RCheesley

This comment has been minimized.

Copy link
Member

RCheesley commented Mar 10, 2020

Moving to RTC given the two successful tests mentioned above. I have left the Code Review label in case there is a need to review before it is merged (I don't know if it is required or not)

@RCheesley RCheesley moved this from Ready to test to Ready to Commit (passed testing) in Mautic 2 Mar 10, 2020
Copy link
Member

dennisameling left a comment

As SAML can be quite tricky, I decided to also test it with the G Suite IDP, just to be sure. Fix works! Thanks a lot @TonyBogdanov! 😄

@dennisameling dennisameling merged commit a5b9ecc into mautic:staging Mar 14, 2020
2 checks passed
2 checks passed
Scrutinizer Analysis: 1 new issues, 1 updated code elements – Tests: passed
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
Mautic 2 automation moved this from Ready to Commit (passed testing) to Merged Mar 14, 2020
@mautibot

This comment has been minimized.

Copy link

mautibot commented Mar 20, 2020

This pull request has been mentioned on Mautic Community Forums. There might be relevant details there:

https://forum.mautic.org/t/announcing-mautic-2-16-1-beta/13438/1

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Mautic 2
  
Merged
You can’t perform that action at this time.