Philippe Jausions [2008-03-14 01:36 UTC] Seems to be doing its thing.
Couple of remarks:
* In verify() use explode() instead of split()
* In the beginning of generate() you are mixing double " and single quotes '
* Add doblock for the constant declaration
* It would seem cleaner to use defined() + constant() instead of just @constant()
* In generate() I would throw an exception if $identity and/or $service are not well-formed URIs
Michael Gauthier [2008-03-14 01:53 UTC] I second Philippe's comments.
You should make the exception class name more specific like Auth_MircoID_BadAlgorithmException or Auth_MircoID_AlgorithmNotFoundException.
Unit tests would be nice and not too difficult since it is such a small class.
David Jean Louis [2008-03-14 09:07 UTC] Looks good, same remarks as Phillipe + I am not sure "Auth" is the appropriate category as the package only compute/verify a text format, maybe be Crypt::MicroID or Text::MicroID would be better ?
Ken Guest [2008-03-14 10:16 UTC] Nice work, but I did find some issues:
1// typo alert: I ran the tests and found the second one failed - this is due to a '$microid' variable being used in the MicroID::verify method. This should be $microID. There are a few other places where there is a $microid variable being used. Be consistent and choose one name and use it everywhere!
2// I also found some of the indentation is inconsistent in the verify method.
3/ Unless required for variable expansion, I would use just single quotes.
4// Regarding the exception class; I agree with Michael and for this reason: If you name the exception class to something more specific it'll be possible for people using your module to write cleaner code for handling thrown exceptions based on the class name rather than checking an error code (which you would then have to define, document etc etc).
So, picking up on what Philippe suggested, you might have:
Auth_MircoID_AlgorithmNotFoundException
Auth_MircoID_IdentityUriNotValidException
Auth_MircoID_ServiceUriNotValidException
I'd suggest using Validate::uri and possibly Validate::email to determine whether the given URIs are valid rather than duplicating code.
Well done so far and good luck with the rest of the proposal process!
Chuck Burgess [2008-03-14 13:09 UTC] I'm inclined to agree about this going into Crypt_ rather than Auth_, since I don't see any interaction with an external service.
Can't add much to the good list of previous comments, other than the CS looks pretty good from the start.
Kurt Wilms [2008-03-20 22:30 UTC] Validate::uri() is for RFC 2396, while MicroID uses RFC 3986 for it's URI's. Thus, Validate::uri() will not be used.
Till Klampaeckel [2008-03-21 15:12 UTC] I am just wondering, how "secure" is this? From what I see anyone could forge a microid easily and make believe the site is someone else'.
I'm just curious what you think.
|
