Make Saml2\Auth can accept a param $spValidationOnly
#510
Conversation
The user who uses this package can decide if they want to make `$spValidationOnly` to be true
|
I am very sorry I closed a pr, because I have never pushed a pr, this is my first ~ |
|
Is it convenient to ask what causes this PR not to be merged? |
lib/Saml2/Auth.php
Outdated
| * @throws OneLogin_Saml2_Error | ||
| */ | ||
| public function __construct($oldSettings = null) | ||
| public function __construct($oldSettings = null , $spValidationOnly = true) |
There was a problem hiding this comment.
From what I am seeing when looking at this, defaulting this to true will change the default functionality here, as before it would always be false. For backwards compatibility it would be best to set it to false
| $settingsDir = TEST_ROOT .'/settings/'; | ||
| include $settingsDir.'settings2.php'; | ||
| unset($settingsInfo['idp']); | ||
| $settings = new OneLogin_Saml2_Settings($settingsInfo,true); |
There was a problem hiding this comment.
It would be good to have a test here that validates that errors are found when this is false, and the test that is defining the default value shouldn't pass it in as that will help verify that the default doesn't change.
There was a problem hiding this comment.
Also, based on the error for the actions, it looks like the linter wants:
| $settings = new OneLogin_Saml2_Settings($settingsInfo,true); | |
| $settings = new OneLogin_Saml2_Settings($settingsInfo, true); |
There was a problem hiding this comment.
Thank you for telling me don't forget format code, I have fixed them
|
To merge this we just need to have the tests to pass, let me know if you are unable to view the details from the above actions that have failed. |
|
Thanks for your concern. I have fixed the errors and push it again. |
|
Hi , @bzvestey I can't know how to fix the error 'Could not validate timestamp: expired. Check system clock' , could you give me some help? |
|
I can try and take a look, but unfortunately I will not be able to do much time to until towards the end of next week. |
|
I think it will take me some time. I will fix it try my best. |
|
I have fixed it just now, I will push it as soon as possible, but I can't confirm if this repo master branch can run the tests passed, On my mac and my cloud server, it seems can't pass the tests. could you let me know the result of you running the test if it can be passed? |
|
Thank you for all of the time you put into this! |
ghost
mentioned this pull request
ghost
mentioned this pull request
2 participants
The user who uses this package can decide if they want to make
$spValidationOnlyto be true