During login, 'User code' (event subscribers) lacks proper access to some data.
This has led to using attributes in a way that is not ideal, IMHO. SamlService::getAttributes() returns an array of attributes by name mixed in with copies of the same attributes by 'friendly name'. It is soon also going to be containing the NameId value. This is messy.
I want to be a able to instead, get an attribute's 'friendly name' by its 'machine name'. That way, we can display the friendly name in the UI of the attribute mapping screen, while saving its machine name in configuration. (I'm fairly sure that the people requesting support for friendly names are now storing those friendly names in mapping configuration, which... does not feel right to me.)
Challenge: the OneLogin\Saml2\Response class has no way of getting a friendly name <-> machine name mapping. (For no good reason.)
Below points could be split into separate issues if necessary?
1)
Pass a value object containing attributes, NameID and various getters, instead of an array with attributes. It'll have to be our own custom class that wraps either OneLogin\Saml2\Response or OneLogin\Saml2\Auth. Likely the former, because the latter does not seem to contain any significant info of its own.
(Or does that have some properties (getters) that should be hidden from generic user code? I don't think so, but best do a little check.)
2)
If so, our ACS code cannot use Auth::acs() anymore because that hides away the Response object. That's fine / maybe even better, though. acs() is a very thin layer of logic that's just in the way.
We may also strip Auth::sls() for consistency. What about Auth::login() / Auth::logout()? (If we strip those, we won't have to work around the 'Relaystate' that is set to /saml/login|out, in SamlController.)
What about maintainability? Is the layer of logic in those 4 methods so thin that we don't care if we copy all of it into SamlService? Or do we want to put those in a separate class to be able to better compare with the original Auth class?
3)
On the other hand... we really want to refactor SamlService::reformatConfig() into something that returns a child class of OneLogin\Saml2\Settings. At this moment we have to choose between: removing any use of Auth and stripping all 4 mentioned methods, and submitting an upstream PR to change Auth::__construct() to also accept a Settings class.
4)
Also... we want to ideally get rid of our own events and use refactored ones from a theoretical externalauth v2. (More info, still evolving, in 🌱 Plan for SAML Authentication 4.x Active comments.)
That doesn't mean we can't already work on this issue, though. The new events will be very similar to our current 2 events we want to refactor from using 'array $attributes' to 'Response $response', so the amount of temporary work we'll throw away again later, will be small.
(We may just want to wait with writing refined documentation on how event subscribers should be refactored in v4.)
Active
4.0
Code
Not all content is available!
It's likely this issue predates Contrib.social: some issue and comment data are missing.