search

hslavich / OneloginSamlBundle

OneLogin SAML Bundle for Symfony
MIT License
149 stars 94 forks source link

Remove the private $attributes property #183

Closed pluk77 closed 2 years ago

pluk77 commented 2 years ago

Fixes #181. Remove the private $attributes property as it exists in the Passport and a getAttributes() function has just been merged into the Symfony 5.4 branch

a-menshchikov commented 2 years ago

@pluk77 thanks for PR, but it makes sense only for Symfony 5.4 projects. Hence, you need to fix up composer.json too.

I could to merge it when 5.4 will be released.

pluk77 commented 2 years ago

You are right, it can only be done in combination with a dependency update. Perhaps there is a way to detect if the method exists in Passport and if not, define it.

I have not done that before but I guess that in only an option if you do not want to move the minimum version of SF to 5.4

a-menshchikov commented 2 years ago

Symfony 5.3 will be out of date in a month after 5.4 release, so we have no reason to stay with 5.3.

a-menshchikov commented 2 years ago

@pluk77 hello. I have made some changes in my own branch. I have decided to completely remove SamlPassport class in favor on use of custom badge. You can explore this question a bit better in symfony/symfony/pull/42181

I will be grad if you would be mind to test it on your environment. If this change is well for you, this PR might be closed. Anyway I really thank you for your contribution. :+1:

pluk77 commented 2 years ago

Its a pleasure. I will test the new 5.4 branch on our application and let you know the outcome.

pluk77 commented 2 years ago

I can confirm I can get my solution to work with minor code changes. In my CheckPassportEvent subscriber I now have to get the attributes from the Badge instead of the passport, after which I set my final value on the passport itself.

Well done for making your code even more generic and easier to interact with.

pluk77 commented 2 years ago

FYI, I see the method AuthenticatorInterface::createAuthenticatedToken() will be deprecated as well in 5.4