Make processor aware of assertion types #240
Conversation
src/SAML2/Assertion/Processor.php
Outdated
| } elseif ($assertion instanceof Assertion) { | ||
| $decrypted->add($assertion); | ||
| } else { | ||
| throw new InvalidAssertionException('The assertion is neither of type: EncryptedAssertion nor Assertion.'); |
There was a problem hiding this comment.
Nitpick: using neither ... nor over a colon looks weird.
Suggestions:
"The assertion is neither of type EncryptedAssertion nor of type Assertion."
"The assertion must be of type: EncryptedAssertion or Assertion"
There was a problem hiding this comment.
It looks correct to me, and makes the method behave conforming to how the docblock that describes that it will. But would prefer if Tim or Jaime could comment whether this is the way to go.
It should also be merged to master so it will not break in 5.x again.
MKodde
force-pushed
the
bugfix/prevent-decryption-of-unencrypted-assertions
branch
2 times, most recently
from
473b420 to
9933693
Compare
|
@tvdijen the unit tests seem somewhat fragile. Autoload problems occurred after adding the ProcessorTest. Only, they did not occur on my tests but on some other test cases. They where caused by me using a data provider. Removing the provider and using the Is this something you are aware of? Or am I doing something wrong here? |
Both the Encrypted and regular Assertion classes should be passable in the decryptAssertion method. If not, you would never be able to process a SAML Response consisting of regular Assertion objects. The test merely verifies the behaviour that was changed in this commit. No additional processor tests where added. But that should be simple enough in the future. For some odd reason using a data provider for the processor_correctly_encrypts_assertions caused issues in other tests. The test suite does not seem to be fully idempotent.
MKodde
force-pushed
the
bugfix/prevent-decryption-of-unencrypted-assertions
branch
from
9933693 to
151a418
Compare
|
I haven't really paid much attention to this part of the library.. Jaime and I mostly worked on the classes in the XML-dir.. |
|
Hi Tim, on our OpenConext phpunit tests we never have issues with Mockery mocks. Maybe a side by side comparison of the PHPUnit/PSR autoloading will give some insights. Although i did not find a problem in plain sight yesterday. Regarding hoisting this change to master; what would you like me to do? Open a new PR or simply cherry pick these commits on master? |
|
Cherry-picking is fine, just make sure to use the correct namespaces for the Assertion/EncryptedAssertion classes.. They have been changed. |
|
Opened #242 to get these changes into master |
Both the Encrypted and regular Assertion classes should be passable in the decryptAssertion method. If not, you would never be able to process a SAML Response consisting of regular Assertion objects.
The test merely verifies the behavior that was changed in this commit. No additional processor tests where added. But that should be simple enough in the future.