Open adamfranco opened 11 years ago
I don't know (much) the phpCAS client, though I hope my comment can help.
Refactoring the large phpCAS sources is a great idea, I think it makes sense to separate things according to the different versions of the protocol from a pure source code perspective and as the client configuration is done by protocol version.
The Java CAS client is "organized in a similar way".
Nonetheless, there is something special about "proxy" I'd like to mention. When you talk about CAS_Client_Cas20Proxy
, I understand that you mean requesting proxy granting tickets by using the pgtUrl
parameter. I'm not sure to understand which class(es) will validate proxy tickets.
In the Java CAS client :
Cas20ServiceTicketValidator
class validates service tickets on CAS server and can request proxy granting tickets if the proxyCallbackUrl
property is defined : see https://github.com/Jasig/java-cas-client/blob/master/cas-client-core/src/main/java/org/jasig/cas/client/validation/Cas20ServiceTicketValidator.javaCas20ProxyTicketValidator
class validates service and proxy tickets and can request proxy granting tickets if the proxyCallbackUrl
property is defined. In this class, the proxy list must be defined or if any proxy is accepted : see https://github.com/Jasig/java-cas-client/blob/master/cas-client-core/src/main/java/org/jasig/cas/client/validation/Cas20ProxyTicketValidator.java.+1 for refactoring Cas_Client
.
I always had this on my todo list for the last couple of years. I actually created a 2.0-future branch where i jotted down a first very rough outline on an idea. However i'm not sure if simple inheritance by protocol for the client is the right way to go. The SimpleCAS client from Brett uses another approach that has a really nice touch to it and it has pretty much stuck in my head. What i especially like is how a similar pattern might enable us to separate authentication code flow from protocol specific implementation details and also split out most of the helper/feature code (html, single sign-out, parsing etc.)
Your idea also makes sense but is not as modular and extendable as we should try to be in my view. I'm just thinking about the many features/protocols we have and i believe it makes sense to have them completely separate from the core workings of the client code. Especially thinking about #10 i think such a design would make more sense than a simple inheritance model.
So my rough idea would be to have a core client that uses different CAS protocol implementations that inherit protocols where possible and implement interfaces for specific advanced features (single sign out, proxy?) No idea if there is another pattern which we could use to achieve such an abstraction.
Given the direction of comments in this issue, this is probably the place to continue discussing what a new public API might look like as I think that will drive the direction of refactoring the CAS_Client.
Here is a first stab at one API design -- I'll try to think of a few more to give us ideas to ponder:
Pseudo-code:
// General configuration is in controller.
$controller = new \PhpCas\Controller()
// Optional injection of a custom protocol handler could be a class or object, usually not used.
->registerProtocolClient(CAS_VERSION_2_0_proxy, '\My\Impl')
// Normal configuration.
->useProtocol(CAS_VERSION_2_0_proxy)
->setManageSessionId(true)
->setSessionHandler($mySessionHandler)
->setDebug('/var/log/phpcas.log');
// Protocol-specific configuration is in the client and will vary
// depending on the needs of the protocol
$controller->getProtocolClient()
->setHost('login.example.edu')
->setPort('443')
->setPath('/cas/')
->allowProxyChain(new \PhpCas\Protocol\Cas\ProxyChain(array(
'https://app.example.com/'
)));;
// Basic authentication flow interaction is against the controller.
$controller->forceAuthentication();
$userId = $controller->getUserId();
$attributes = $controller->getAttributes();
if ($_POST['logout'] && $controller->isAuthenticated()) {
$controller->logout();
}
// Should protocol-specific functions (like proxy calls) be made against the controller
// or protocol client?
$request = $controller->getProtocolClient()
->getProxiedService(PHPCAS_PROXIED_SERVICE_HTTP_GET);
$request->setUrl('http://otherApp.example.com/something')
$request->send();
print $request->getBody();
The CAS_Client is currently a giant omnibus hunk of code weighing in at 3600 lines with 1123 statements (counted with
grep -P ';|{' source/CAS/Client.php | wc -l
). It has been discussed in other issues that the size and complexity of this class are problems. I'm opening this issue as a place to discuss options for refactoring theCAS_Client
into smaller pieces that are easier to understand and work with.My first stab at a refactoring the
CAS_client
was to pull all of the code used to proxy access to services (when using the CAS 2.0 protocol) into a separateCAS_Proxy
class. This reduced theCAS_client
to 2800 lines/862 statements. See the branch at https://github.com/Jasig/phpCAS/tree/Split_client_and_proxy for these changes. While these changes were certainly helpful, the client still has mess of different functions for each of the different protocols we support and complex switch or if/else statements to redirect the execution flow based on which protocol is in use.Here's an initial proposal for refactoring the
CAS_Client
:CAS_Client
becomes an abstract class that provides the overall flow-control for authentication as shared by all of the protocols as well as common functions such as the following sections from the client:CAS_Client_Cas10
class extends theCAS_Client
and adds support for validating CAS 1.0 tickets.CAS_Client_Cas20
class extends theCAS_Client
and adds support for validating CAS 2.0 tickets and reading of attributes from CAS 2.0 responses.CAS_Client_Cas20Proxy
class extends theCAS_Client_Cas20
and adds support for proxying to other back-end services.CAS_Client_Saml11
class extends theCAS_Client
and adds support for validating SAML 1.1 tickets and reading attributes from SAML responses.phpCAS::client()
andphpCAS::proxy()
methods would be responsible for instantiating the proper client object based on the protocol passed.The big question: Does dividing up the client based on protocol seem like a reasonable option or can you think of a better way to slice it up?