Move all parameter validation out of the phpCAS class or make the phpCAS testable

[adamfranco]

Migrated from PHPCAS-127

Description

The phpCAS class should only validate that the client exists all other input validation and sequence validation should be done in the CAS_Client where it is testable.

Alternatively, the phpCAS class could be made testable by allowing resetting of its variables.

Joachim Fritschi added a comment - 06/Sep/11 1:52 PM - edited

Ok sounds like a plan. But if it's possible don't move the validation code directly into the CAS_Cclient class. I would really like to start stripping this class down. 3000 lines is just a bit to much for my taste. Maybe we can put this in some external Validator class?

[jfritschi]

I have opened up a branch and started putting together a new validation class. Any comments are highly welcome. This is crrently a proof of concept but works pretty well in my test environment.

The idea is currently to have the validation code external and have some kind of toggle to enable all the validation with one switch. I'm currently using the nomal debug switch since the full validation really only makes sense during development and debugging. There is little values to validate the config of the phpCAS integration during every single call.

This whole activity will probably also be extended later to include the work around the issue #57 because this is pretty much related.

[jfritschi]

@adamfranco Do you any comments? Don't know if the current state fits your requirements. In my view it fits and we can of course introduce manual overrides to the class if it's still needed. Otherwise i believe that unless you enable DEBUG during any test the validation code should not even be executed and interfere with any of your tests.

[adamfranco]

Hi @jfritschi, Sorry for taking a while to get back to this.

I question the utility of turning off validation, since we really want phpCAS to fail with an error when inputs are wrong. For instance, if the user passes the string 'cas.example.com' as the $server_port argument when an integer is expected. If validation is only happening when DEBUG mode is on then users are more likely to get strange failures that make less sense. Likewise if the user doesn't turn on DEBUG, but tries to call proxy methods after not initializing the client as a proxy things will fail, but maybe not in an obvious way.

The more I look at this, the more I think that the validation should really all be happening in the client. While it will certainly add more lines to the CAS_Client, these validation steps are pretty critical and having them in the client will ensure that they are always run.

I'll take a short stab at an implementation that throws InvalidArgumentExeptions from within the client to see if that can be done without becoming overly verbose. :-)

[jfritschi]

I understand your argument and i did consider it. I came to the following conclusion: Since phpCAS is statically configured once in the php code it does not make sense to validate the parameters for every user call. If someone is having problems or is developing a phpCAS integration i would expect him to use the debug option until he has a solid config.

We also have the option to use another flag or have an explicit "opt out" of the validation if that is more appropriate in your view. I have not done any benchmarks but i''m pretty certain that all the debug_backtrace calls etc. draw a lot of performance. That's why i would really like to have at least the possibility to disable them.

We can of course hook the validation into the CAS_Client. I don't think that makes much of a difference whether we hook the CAS_Util_Validation class into the phpCAS or CAS_Client class. My goal is simply to make it as modular as possible and not add real functional code into the client but only hooks.

[adamfranco]

I disagree with the assumption about static configuration. In my own usage of phpCAS I've run into several cases where this isn't true. For example the Drupal CAS module allows configuration of phpCAS via a GUI. In general I feel its better to fail early due to strict input validation in all cases rather than require users to take action to do validation. Validation not happening in select cases gives me nightmares. :-)

Its not fully baked, but I've put a branch at https://github.com/Jasig/phpCAS/commit/7_parameter_validation_in_client that moves the validation into the client. It does the validation directly without using reflection or debug_backtrace, so there shouldn't be a performance penalty over the old validation routines. (Please ignore commit c8472b3 as I pushed it before noticing that it included TABs.)

Let me know if this seems like a reasonable direction to go. If so, I can flesh it out some more tomorrow.

[jfritschi]

I have not seen many implementations in php that do that much type checking etc. While your argument with drupal is true it's still "admin defined" and permanently configured. It's not something that changes with every user call. That's why i see little reason to have it activated on every call. There should at least be some toggle to switch it off if we want to have it activated as a default. Your implementation is fine but there are 2 things that really bug me. We are bloating the Cas_Client again and we just replicate the old user defined copy&paste error messages all over the place. While i see the benefit in moving the actual valdiation from phpCAS to the Cas_Client i think this proposal has some room for improvement and here are some suggestions/questions from my side:

• What do you think about inheriting standard error classes from for example CAS_OutOfSequenceException for any standard cases with statically defined error messages like CAS_OutOfSequenceException_Proxy?
• How about moving the actual check and throwing outside of the CAS_Client core methods and that we don't just replicate things like !is_proxy() .... all over the class``` but have one (private?) method or external class that does these checks?
• Any particular reason you don't like the use of reflection? I thought it was a good way of automatically checking all parameters and also validating the docs with very little code. I even discovered a few bugs this way. (For example some of the validation code did not check for array before but suddenly did. Or the code docs mentions wrong types for $server_port) While there might be a perfomance downside it was really strict and provides a very quick way to check everthing with already existing info and not duplicate things. It enforces strict coding for both us and integrators with little effort to include it in any function.
• I would like to have a toggle to disable all checks on purpose. For the lack of a better idea disable_internal_checks()

I guess some hybrid between your and my proposal would be the best solution in my view. :smile:

[adamfranco]

• What do you think about inheriting standard error classes from for example CAS_OutOfSequenceException for any standard cases with statically defined error messages like CAS_OutOfSequenceException_Proxy?

This sounds like a fine idea.

• How about moving the actual check and throwing outside of the CAS_Client core methods and that we don't just replicate things like !is_proxy() .... all over the class, but have one (private?) method or external class that does these checks?

:+1: This is a good idea. I think we'll need two sequence-checking methods and the proxy check, since the proxy check is used with both the before authentication and after authentication checks:

protected function _validateSuccessfulAuthenticationSequence () {
    // Sequence validation
    if (!$this->wasAuthenticationCalled())
        throw new CAS_OutOfSequenceException('this method should only be called after phpCAS::forceAuthentication() or phpCAS::isAuthenticated()');
    if (!$this->wasAuthenticationCallSuccessful())
        throw new CAS_OutOfSequenceException('authentication was checked (by ' . $this->getAuthenticationCallerMethod() . '() at ' . $this->getAuthenticationCallerFile() . ':' . $this->getAuthenticationCallerLine() . ') but the method returned false');
}

protected function _validateBeforeAuthenticationSequence () {
    if ($this->wasAuthenticationCalled())
        throw new CAS_OutOfSequenceException('this method should only be called before ' . $this->getAuthenticationCallerMethod() . '() (called at ' . $this->getAuthenticationCallerFile() . ':' . $this->getAuthenticationCallerLine() . ')');
}

protected function _validateIsProxy () {
    if (!$this->isProxy())
        throw new CAS_OutOfSequenceException('this method should only be called after phpCAS::proxy()');
}

This would add at most two lines to each function in the CAS_Client that has a sequencing constraint.

I suggest that we consider the implementation of these sequence/proxy validation steps separately from any implementation of parameter validation. As you mention, we may be overly strict on the type checking of arguments. The sequence checking is rather critical though, since many methods may return wrong results of the correct type if used in the wrong order.

• Any particular reason you don't like the use of reflection?
• I would like to have a toggle to disable all checks on purpose. For the lack of a better idea disable_internal_checks()

I'm fine with the usage of reflection, but my hesitance comes because I thought that you were wanting to disable validation to get around slow performance of the reflection implementation. If an implementation isn't usable in production, that sounds like a negative mark against that way of doing it. :wink: Conversely, if the validation is sufficiently fast, I can't think of a case why disabling validation would be useful.

All that said, I think you're right that validating the types of every parameter may be a little excessive (at least compared to most PHP code out there). Type coercion should take care of converting strings to ints and ints to booleans. I guess the big benefit of checking the argument types is to use the type-checking as a proxy for validating the order of arguments. For example if the integrator types:

phpCAS::client(CAS_VERSION_2_0, 'login.example.edu', '/cas/', 443);

instead of

 phpCAS::client(CAS_VERSION_2_0, 'login.example.edu', 443, '/cas/');

they could immediately get an exception that the arguments are bad instead of getting their browser redirected to an invalid URL (or a CURL error). Actually, an enhancement would be to do additional checking that the URLs passed are valid URLs and not just arbitrary strings.

I guess overall I'd really like to keep the sequence validation in the CAS_Client and am more open to alternatives for the argument types.

P.S. On the topic of the CAS_Client being unwieldy due to its size, we might have success splitting it into two classes, one for the base client and a child class that adds the proxy support.

[jfritschi]

:+1: for all of your suggestions.

I think it makes sense to diffentiate between normal runtime checks that are essential for the client working proper and preventing wrong use which might even create unwanted security holes etc. Those should be lean and also serve a destinct purpose. All the other debug or additional validation should either be attached to debug mode or have some other toggle (default on is probably better). I'm always thinking of java assertions :smile: I think they really serve a purpose during development/debugging but should not impeed overall performance if you run a bigger site. I'm always thinking about the worst case gateway implementation with checkAuthenication() on a big website where every request hits phpCAS code.

I think i will also open an issue because i would really like to have some performance metrics in the future where we know how the code is developing over time and how this may impact our userbase on the perfomance side. Would be a nice feature in my view.

Regarding your P.S: i will answer in the other issue.

[jfritschi]

@adamfranco I have worked further on your branch and implemented some of the things discussed above. I also ironed out some bugs that i disovered while testing. Currently a couple of unit tests are failing that need to be fixed. I have also not started looking at the parameter validation. I have focused on the sequence check and streamlining them.

[jfritschi]

What was the reason to introduce the protected _getUser() and _hadAttribute() functions. I somehow fail to see the point?

[jfritschi]

I tracked down the error with the phpunit tests. They detected a bug that i fixed in the previous commit

[jfritschi]

I have merged the branch into master.