Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Check at least one server response #6

Closed
araab opened this issue Nov 10, 2014 · 3 comments
Closed

Check at least one server response #6

araab opened this issue Nov 10, 2014 · 3 comments

Comments

@araab
Copy link

araab commented Nov 10, 2014

If the connection to the server fails the responses array in the
ResponseCollection class is empty and the success function returns true.
Perhaps the default value of $success should be false and set to true if
at least one response was successful.

    public function success($first = false)
    {
        $success = false;
        if ($first === true) {
            // Sort them by timestamp, pop the first one and return pass/fail
            usort($this->responses, function($r1, $r2) {
                return $r1->getMt() > $r2->getMt();
            });
            $response = $this->responses[0];
            return $response->success();
        } else {
            foreach ($this->responses as $response) {
                if ($response->success() === false) {
                    return false;
                } else {
                    $success = true;
                }
            }
        }
        return $success;
    }
@enygma
Copy link
Owner

enygma commented Nov 11, 2014

Wouldn't that be correct? If it fails on success() it will return false. Otherwise the $success value starts out as false at the top there...

@araab
Copy link
Author

araab commented Nov 11, 2014

The code above is a suggestion how to change it.

The current function sets $success to true as default. This is the current function:

    public function success($first = false)
    {
        $success = true;
        if ($first === true) {
            // Sort them by timestamp, pop the first one and return pass/fail
            usort($this->responses, function($r1, $r2) {
                return $r1->getMt() > $r2->getMt();
            });
            $response = $this->responses[0];
            return $response->success();
        } else {
            foreach ($this->responses as $response) {
                if ($response->success() === false) {
                    return false;
                }
            }
        }
        return $success;
    }

Here an example with the current function.

$v = new \Yubikey\Validate($apiKey, $clientId, array("localhost:8888"));
$result = $v->check($otp);
print_r($result);
var_export($result->success());

You get the following output:

Yubikey\ResponseCollection Object
(
    [responses:Yubikey\ResponseCollection:private] => Array
        (
        )

    [position:Yubikey\ResponseCollection:private] => 0
)
true

$result->success() returns true because the $responses array is
empty and if ($response->success() === false) is never reached.

@enygma
Copy link
Owner

enygma commented Nov 13, 2014

Thanks for the suggestion - I've added it in 7a78243

@enygma enygma closed this as completed Nov 13, 2014
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants