apereo / apereo/phpCAS

Refactor CAS_Client, new public API?

Open
#62 3 comments 1 reaction 0 assignees View on GitHub

Nobody has claimed this yet.

API Change Enhancement
Dominant language
PHP
Stars
798
Forks
419
PR merge metrics
No merged PRs in 30d

Description

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 the CAS_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 separate CAS_Proxy class. This reduced the CAS_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:

  • The 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:
  • HTML output
  • Internationalization
  • Storage of CURL options
  • Configuration of ticket clearing and post-auth callbacks
  • Methods for supplying code-flow feedback to integrators.
  • Session handling
  • Authentication API methods (isAuthenticated(), forceAuthentication(), etc) and user/attribute access
  • redirection & logout
  • Misc helper functions like _readURL()
  • CAS_Client_Cas10 class extends the CAS_Client and adds support for validating CAS 1.0 tickets.
  • CAS_Client_Cas20 class extends the CAS_Client and adds support for validating CAS 2.0 tickets and reading of attributes from CAS 2.0 responses.
  • CAS_Client_Cas20Proxy class extends the CAS_Client_Cas20 and adds support for proxying to other back-end services.
  • CAS_Client_Saml11 class extends the CAS_Client and adds support for validating SAML 1.1 tickets and reading attributes from SAML responses.
  • The phpCAS::client() and phpCAS::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?

Contributor guide

No contributing guide indexed for this repository

First steps

  1. Read the whole issue, then the project's contributing guide.
  2. Comment on the issue to say you are picking it up — it saves two people doing the same work.
  3. Fork the repository and make your change on a branch.
  4. Open a pull request that references the issue number.

Research direction

Start by reading source/CAS/Client.php and the phpCAS::client() and phpCAS::proxy() entry points to understand the current protocol flow. Review the proposed CAS_Client, CAS_Client_Cas10, CAS_Client_Cas20, CAS_Client_Cas20Proxy, and CAS_Client_Saml11 split. Done would require agreeing on a design and implementing and validating the refactor across the supported protocols.

Written by the indexing model from the issue text.

Assessment

Tech stack
php
Domain
authentication
Issue type
Refactor
Difficulty
5/5
Estimated time
Over a week
Activity status
Stale
Clarity
Needs clarification
Newbie friendliness
25/100

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.