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

decouple unique identifier generation #987

Open
wants to merge 5 commits into
base: master
Choose a base branch
from

Conversation

lordrhodos
Copy link

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.

@lordrhodos lordrhodos changed the title decouple unique identifier generator decouple unique identifier generation Jan 19, 2019
@Sephster
Copy link
Member

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
Copy link
Author

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:

#596 (comment)

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
Copy link
Member

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
Copy link
Contributor

As suggested in #991 (comment)_ 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

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

Successfully merging this pull request may close these issues.

3 participants