decouple unique identifier generation

[lordrhodos]

I am working on an implementation using level 4 Uuids as unique identifiers for the tokens and auth code. My first approach was to create a custom Grant overriding the generateUniqueIdentifier function from AbstractGrant, but implementing the second grant I felt the urge to decouple the identifier generation.

This PR:

• moves the identifier generation from AbstractGrant::generateUniqueIdentifier to IdentifierGeneratorInterface::generateUniqueIdentifier
• adds a default IdentifierGenerator containing the moved method from the AbstractGrant
• allows to pass a custom generator in the AuthorizationServers constructor (optional). If it is not set the default generator is used instead and set on the Grant inside the AbstractGrant::enableGrantType function
• thus should be backwards compatible

Discussion My first idea was to inject the IdentifierGeneratorInterface into the AbstractGrant constructor, to be sure it would alwas be set when calling $this->identifierGenerator->generateUniqueIdentifier(), but after looking closer at the existing code I realized that common dependencies used by the different grant types where set in the AbstractGrant::enableGrantType function, so I followed that pattern. I am not sure this is the preferred way, so may need some feedback on this.

[Sephster]

Hey @lordrhodos - thanks for your PR here. I've had a wee read around this and I'm not sure this would be something we would include in the package. It is a fairly big change and alters interfaces so would need to be included in the next major version.

I am also unclear as to what benefits this has. To my knowledge, our use of random_bytes doesn't have any disadvantages to using UUIDs, and given the default length we are using is 40, it is highly improbable we will encounter a clash. I believe it is actually even less likely that UUID v4.

If you could detail why this change would be beneficial, I will take this on board but I think it is likely this will not be merged in. Thanks for your efforts with this regardless.

[lordrhodos]

Hi @Sephster, thx for having a look. To be honest I had started implementing some entities (Grant, Scope, etc.) already when I recognized that the identifiers for the tokens and the auth codes are being created by the library. I wanted to assure they all are handled the same way, so I started overriding it per grant type.

While investigating the issue further I came across a comment from @alexbilbie considering changing the identifiers to use uuids:

https://github.com/thephpleague/oauth2-server/issues/596#issuecomment-226700794

But that said, the comment is 2,5 years old and maybe not reflecting the current evaluation of the topic as you point out. My main motivation is to gain flexible control over the identifier generation and not about how the default identifier should look like. I am by no means an expert on cryptography or would argue for a certain collision percentage regarding uniqueness of either random_bytes or level4 uuids. Just wanted to decouple the generation so the implementor gets some better control avoiding code duplication.

[Sephster]

Thanks for the explanation and thanks for pointing to the previous comment. From my readings, my understanding is that unless you have a specific requirement for interoperability with UUID, random_bytes is generally a better choice as it can have a larger key range, meaning less collissions, and is quicker as it is built into PHP.

I think the chance of someone requiring this change are slim at the moment given the reasons above. I would advise that unless you have specific need for UUID v4, you are probably best sticking with the existing implementation.

Because this is an edge case and would likely need to be added to a major version, I don't think I will merge this at this time. Thank you very much for your efforts with this though and your explanation.

If there are a number of people calling for this in the future, then I will revisit this and see about progressing it further.

[marc-mabe]

As suggested in https://github.com/thephpleague/oauth2-server/issues/991#issuecomment-481137464_ in my opinion it would make most sense to move unique identifier generation to the repositories (within storing the entities) because:

• possibility to check uniqueness 100%
• possibility to use optimized unique id generation of storage layer
  • I mean mainly UUID generation of storage layer if supported as some may include a highly optimized version with including unique check
  • I explicitly don't mean things like auto increment as that would be a security issue
• different unique id generation per entity