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

Is that better to add an accessory function to get captcha's unique id? #69

Closed
demokn opened this issue May 16, 2019 · 2 comments
Closed

Comments

@demokn
Copy link

demokn commented May 16, 2019

Before I submit a pull request, just want to make sure whether it's necessary.

With usage of capture, we also need save the phrase to validate user input. Usually we may use session or a cache component. Well, if we choose to use cache, we also need a cache key for each capture. Thanks to the fingerprint property, we can simply use md5(json_encode($captcha->getFingerprint())) or md5(implode('', $captcha->getFingerprint())) to generate an unique key as the cache key.

As mentioned above, whether it's better to add an accessory function called getToken(), just like this:

    public function getToken()
    {
        if (empty($this->getFingerprint())) {
            throw new \Exception('Captcha token can only be accessed after built');
        }
        
        return md5(implode('', $this->getFingerprint()));
    }
@Gregwar
Copy link
Owner

Gregwar commented May 24, 2019

Hello

  1. Your method proposal will generate a lot of collision (1, 23 and 12, 3 will result in the same hash), but this can be easily fixed with a separator
  2. The fingerprint is actually really big, with default settings there is like 64 numbers which are not binary, leading to having two time the exact same image really unlikely (there is much more probability than md5 collide for instance)

However, I like the idea of naming the file according to your fingerprint in the special case where you would like to propose the same captcha multiple time to the same user for retry (which I don't really think is a good idea though)

@demokn
Copy link
Author

demokn commented May 26, 2019

Yeah, thank you for pointing out the shortcomings. I will close this issue and try to find other way.

@demokn demokn closed this as completed May 26, 2019
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