AuthVolatile.php in SAML Authentication 8.3
Same filename and directory in other branches
Namespace
Drupal\samlauthFile
src/AuthVolatile.phpView source
<?php
namespace Drupal\samlauth;
/**
* EMPTY CLASS.
*
* After writing up all considerations, I concluded that this wasn't needed yet.
* So now this is just just a bunch of comments on plans for v4.x, for me to
* circle back to - which might as well be committed into 3.x for anyone
* curious about a future refactor. What I drafted so far:
* ---
* Something like the Auth class; can change methods/signatures at any time.
*
* Do not trust anything about this class to persist. I'm just creating it so I
* don't have to refactor all uses in SamlService at the same time.
*
* Use case / reason for its existence: I want to refactor at least
* processResponse() (i.e. the saml/acs path), in a way that requires us to not
* use Auth anymore. Other saml/* paths may follow but I can't make up my mind
* about that at the moment. (See "In the end" below.) Also, IF we do that... I
* don't want to do all this refactoring in one go.
*
* Besides this 'refactoring issue', we also want(ed) to refactor
* SamlService::reformatConfig() into a Settings subclass that does 'just in
* time' reading of cert/key values only when necessary. That is currently not
* possible IF we keep using the Auth class. So for now, we've postponed this
* and instead have added extra logic to SamlService::reformatConfig(); see
* comments there. We may extend that method (and/or move it in here) to return
* a Settings child class, if that ever becomes applicable.
*
* Bigger story / context: the Auth class is named the PHP Toolkit's "main
* class" but it doesn't fit some of the things we need to do and is hard to
* override. Also, taking a step back it doesn't seem clear in what it wants to
* be. On one hand it seems to be a service/helper class for all other callers
* to use, but:
* - It's rather rigid in the way it forces usage of its own Settings class.
* I'm not yet sure how to approach this / if this is an issue; see above &
* SamlService::reformatConfig() comments.
* - processResponse() and processSLO() have strange mechanics around error
* conditions, which makes error handling by callers more complicated then it
* needs to be. (Some errors throw an exception; some just set an error
* property so you have to call getErrors() afterwards - _and/or_
* isAuthenticated(); it isn't specified well that those two things are
* interdependent.)
* - It shields access from Response/LogoutResponse/LogoutRequest value objects
* and I need the Response object to pass around to event subscribers.
* On the other hand it seems to be a value object itself, that explicitly
* wants to replace/obsolete the use of Response/LogoutResponse/LogoutRequest
* objects by callers, because it copies all their properties into its own
* properties after processing. However I don't see why we'd need to use Auth
* instead of those other objects:
* - I'm not aware of any information inside those other objects that needs to
* be 'shielded' from callers.
* - The Auth class is only usable as a value object after first calling some
* 'processing' method inside the same object, which kind-of goes against the
* notion of a value object.
* - Even then, only part of its values (accessible by getters) will be
* populated and valid - depending on which thing we've processed (being an
* ACS Response / Logout Response / Logout Request). It feels like using the
* individual object would just be less confusing in that regard.
* Or maybe it just wants to be 'just an example class', which an implementer
* can learn from while building their own? What points into this direction is
* - the fact that Auth forces usage of its own Settings class combined with
* the fact that the Settings constructor has a second $spValidationOnly
* parameter which is not used by Auth.
* Then again, probably not, because
* - it is used in many examples in README.md
* - it is called 'the main class'
* - it contains too much useful logic we don't want to reimplement (mainly in
* login() / logout() I guess).
*
* In the end,
* - I don't want to use the Auth object to pass around to event subscribers
* during the ACS process; I want the Response object which is much more of
* a 'targeted' value object. That means I'll need to reimplement
* Auth::processResponse() - which is fine because it doesn't contain that
* much logic and makes error handling easier.
* - I'm not sure yet whether I want to reimplement Auth::login() / logout() /
* processSLO(). I'm alternating between "they contain valuable logic that we
* don't want to reimplement / we won't be able to compare our code to any
* future changes in the Auth object" and "reimplementing is easy / will
* actually make things clearer".
*
* Further notes:
* - When we reimplement processResponse(), many Auth methods (getters) will
* stop working completely, and we won't have a use for them. This means the
* code replacing Auth::processResponse() should (likely) not be in a child
* class of Auth. (So we're not bound to the fact that we cannot use a
* Settings child class for the new code coming out of that refactoring.)
*
* We still need to choose whether we want to
* - remove any use of the Auth object completely, so we can use a custom
* Settings child class that will do what we want; (Note if we go this route,
* we may want to consider whether we can open an upstream PR to the upstream
* library, to improve... anything that led us to this decision? And
* submitting such a PR should likely include looking at the code examples in
* README.md and checking whether our new approach could improve them?)
* - get a small PR accepted upstream that will allow passing a custom Settings
* child class into the Auth constructor, so we could use the same Settings
* child class(es) for passing into both Auth and whatever code will replace
* Auth::processResponse();
* - write extra logic outside a Settings child class, in some kind of factory
* method, to decide which cert/key values will be necessary, before
* instantiating Settings/Auth objects.
*
* These are too many considerations to oversee right now. In 8.x-3.3 /
* SamlService::reformatConfig() we've already been working toward the last
* point, because anything else isn't possible in the 3.x versions - and we
* didn't want to do a full refactor, removing all usage of the original Auth
* object (and then have to make a decision on all the above), just to
* implement alternative key / cert storage.
*/
class AuthVolatile {
}
Classes
Name | Description |
---|---|
AuthVolatile | EMPTY CLASS. |