Add RegistrationInterface

[matthewslouismarie]

This is a very simple pull request. I've only added an interface for Registration. This, at least in my case, was the class that needed an interface with the highest priority (since it is the object persisted in the database).

Interfaces should be added to the other classes of the library as well. I'm just wondering if it's worth the effort right now considering the new WebAuthn API coming up soon. It might just be better to wait for the new API to be finalised. I believe I read in the WebAuthn API that registrations made with the new API will not be backward compatible, and that they had an additional field (maybe credential id), maybe it was a mechanism for the user agent to prevent the user to register the same U2F device twice…

I wasn't sure if the interface should return the binary or non-binary versions of the strings. However, since ECPublicKeyTrait already has a getPublicKey() which returns a binary value, I was obliged to make the interface return binary value, and I added a new method getPublicKeyBinary() just for consistency with the rest (in the rest of the code, a method that returns a binary value is suffixed with Binary, but not getPublicKey ECPublicKeyTrait).

Also, maybe the quality of the code could be improved with a tool such as PHP CS Fixer to enforce a specific coding style and standards. In particular, it can check that declare(strict_types=1); is at the beginning of every file. I saw this statement was on some files but not on others. Also, some functions do not have a return type. Finally, you might want to take a look at Extremely Defensive PHP. It's a set of coding guidelines, you might not agree with them though! :P

[coveralls]

Coverage decreased (-1.1%) to 98.155% when pulling 19848c9 on matthewslouismarie:master into 9c06b4b on Firehed:master.

[Firehed]

Thanks! I'll look over this in more detail when I get home. I want to read up more on the WebAuthn stuff.

Re: standards - I agree and did quite a bit of cleanup over the weekend but there's room for more improvement as it was only a basic first pass. I set up PHPCS with only PSR-2, and generally welcome improvements (though in a separate PR, preferably)

[Firehed]

I like this change a lot - it's straightforward and should make storage easier for library users to manage.

I spotted some additional minor improvements that tie in nicely and one style preference if you're up for it. If not I can take care of them but it may take a bit longer just due to other stuff I'm working on.

Thanks a lot for helping out!

[Firehed]

I like the consistency that this naming change adds. It may make sense to do the same with setPublicKey in the future, but let's keep that out of the discussion for now.

Would you mind updating the couple of usages in Server (and this trait's corresponding unit test) to use the new method as well?

[Firehed]

Since this method is being deprecated, I think it's preferable to return $this->getPublicKeyBinary(); instead of using the same implementation - it makes it clearer that the method is being renamed.

It also may be worth firing a trigger_error(E_USER_DEPRECATED, ...); as well, to be proactive. Thoughts?

[matthewslouismarie]

Not sure about trigger_error(E_USER_DEPRECATED, ...). Doesn't sound too different from removing the method altogether :p Certain static analysis tools such as scrutinizer warn of the use of deprecated functions, I wonder if PHP Storm does the same.

Although the advantage of trigger_error(E_USER_DEPRECATED, ...) is that the error and the reasons / fix is made explicit to the developer but you still give him the possibility to ignore it. I suppose I should do it then, you confirm?

[Firehed]

Left over from something? I don't see it being used.

[matthewslouismarie]

Oops! Removed.

[Firehed]

There are a couple of typehints in Server that would also benefit from being updated to accept the interface. Would you mind updating those too?

[matthewslouismarie]

Would have thought GitHub would have notified me of your comments! Glad you like the changes. I've updated the pull request with your comments.
In the phpDoc, I sometimes changed "Registration" for registration, since saying "object implementing RegistrationInterface" is a bit long, and because "RegistrationInterface" is already mentionned in the @param or in the @return, so it doesn't sound like it's needed.
Since getPublicKey() is not tested anymore, the test coverage decreased, coveralls is not clever enough to ignore deprecated functions. :P

[Firehed]

Thanks a lot! I'll tweak the coveralls setting (it's default behavior is insanely aggressive, and I always forget to do this when first configuring it) since that's not a meaningful regression.

As far as the trigger_error thing goes - my general philosophy is that users shouldn't have to rely on specific tools whenever possible - so if you're using a tool that detects @deprecated that's great, but you shouldn't be overly surprised if you're not.

Overall, looks great - thank you so much for your contribution :)

[matthewslouismarie]

Cool! Do you want me to add trigger_error?

[Firehed]

Yeah, that would be great - after that I'm totally happy landing this :)

[matthewslouismarie]

Done! Made me realise I was still using getPublicKey() somewhere.
Although at this point, we might even remove getPublicKey() altogether, which wouldn't lower the test coverage, and semantically version the new change. :p

[Firehed]

This all looks great - thanks again for contributing!