Outsource AbstractGrant::generateUniqueIdentifier

[crtl]

In League\OAuth2\Server\Grant\AbstractGrant I could not understand why generateUniqueIdentifier is implemented as it is.
There is no way to configure the length other than by extending AbstractGrant and override it, but then you have to reimplement all grants again.

I would suggest to define an interface and provide an default implementation with the current length of 40, which is then passed to the grant.

Example:

<?php
//src/Factory/IdentifierFactoryInterface.php
namespace League\OAuth2\Server\Factory;

interface IdentifierFactoryInterface {
  public function generateUniqueIdentifier(): string;
}

OR

Extending the repository interfaces to implement a generateUniqueIdentifier method but this would mix up concerns.

[lordrhodos]

@crtl I submitted a PR dealing with this code part here, but it will not be merged at the moment:

#987

If we can find more people in need of this we may get a chance to open up the discussion again ;-)

[Sephster]

It is good that there is a second person asking for this as it does add weight to the request. @crtl can you provide some more information as to why you want to modify this?

[crtl]

Ok.

So the OAuth2 does not define a length for client_id and client_secret:

From the OAuth2 RFC-6749, Section 2.2 Client Identifier:

The client identifier string size is left undefined by this
specification. The client should avoid making assumptions about the
identifier size. The authorization server SHOULD document the size
of any identifier it issues.

So the server should be able to specify its own length.
Viewing the source in AbstractGrant::generateUniqueIdentifier it shows us that this was intended because the method accepts a $length parameter.

The Problem

generateUniqueIdentifier is protected and therefore can/is only called by AbstractGrant which never passes a $length and nowhere has support for configuring it.

So when I want my identifiers to be of a diffrent length (for example 64) I have to:

1. Extend AbstractGrant -> MyAbstractGrant
2. Override generateUniqueIdentifier
3. Extend MyAbstractGrant for all GrantTypes I want to use

Step 3. will most likely result in copy/pasting the current grant implementations into the custom grants because @alexbilbie already did a geate job implementing those.

The Solution

1. Introduce an Interface IdentifierFactoryInterface
2. Pass an implementation of the interface to AbstractGrant
3. Call the factory method inside generateUniqueIdentifier

This way the implementation is backwards compatible because it can ship with a DefaultIdentifierFactory which generates an identifier with the current length of 40.
But everyone wishing to control the identifier generation can implement his/her own IdentifierFactoryInterface and pass it to the grant.

The Pull requests from @lordrhodos is the exact same solution I see.

[Sephster]

Thanks @crtl for your response. Sorry, I didn't phrase my question very well first time around. I suppose I'm more interested in why you want to change this.

@lordrhodos was looking to use UUIDs instead. Are you looking to do something along these lines? I suppose I'm just trying to establish the driving reason behind such a change.

I'm up for giving more control/customisability to the implementer, but want to make sure I understand the driving factors for such changes. As I said earlier, the fact two people have now requested something similar gives this proposal more weight. Thanks!

[crtl]

1. The length of 40 is not documented
2. When creating my schema I have to guess the length (maybe it changes) and I have no control over it.
3. I cant implement my own generation algorithm
4. It can also provide backwards compatibility for future releases.

I for example wanted a length of 64 (for whatever reason) but I couldnt change it without extending a dozen of classes and copy/paste.
Sure someone can argue but its working like it is. Then I would state that this is a library, developed not to be a final product.
@lordrhodos Is a good example. He wanted to use uuid, but couldn. Maybe he should/shouldnt, but should the library decide it?

[Sephster]

Agreed that this should be documented if it isn't already.

Can you give a reason why the current implementation is insufficient for your use case though? It really would help prioritise this proposed change if there is a demonstrative use case why this should be changed.

I understand that this change will give you more control but I am struggling to see why the current implementation would cause issues or be insufficient for anyone's needs.

I'm not averse to making this modification but I'm focussing on version 8 for now so wouldn't take this forward at this time unless there was a pressing need.

[lordrhodos]

I for example wanted a length of 64 (for whatever reason) but I couldnt change it without extending a dozen of classes and copy/paste.
Sure someone can argue but its working like it is. Then I would state that this is a library, developed not to be a final product.

@crtl I am backing this. I think the library should follow the current path to deliver the tools to implement an oauth2 and resource server supporting the defined (specs) oauth2 flow in a as easy as possible way, but also as flexible as possible. Flexibility for the unique identifier generation does not exist, which was the main reason for me tackling this issue.

@Sephster the flexibility may not be needed in version 8 if that is pressing right now, but given the implementer control how to generate the unique identifiers should be added in the long term. And if it will be added it should come in a flexible way and provide a default generation without breaking exisiting installations.

[crtl]

@Sephster For example I want my identifiers to consists of alnum characters from a-zA-Z0-9.

Now I do understand your point of view more.
In my opinion this is no breaking feature and the priority for me personally is not that high.
I just saw it in the code and thought that there is room for improvement because it breaks with the Single Responsibility Principle and the Seperation of Concerns (and its so clear).
From a feature/user perspective it works.
From a software design perspective its wrong.

[Sephster]

Thanks for the discussion guys. I think this is something we should look to do. I'm not sure on timings yet but will flag this as an improvement and reopen the original PR for another look. Cheers both for your contributions here!

[crtl]

Additional it sohuld noted that the current documentation for AccessTokenRepository and RefreshTokenRepository contains the following information for getIdentifier:

getIdentifier() : string this is randomly generated unique identifier (of 80+ characters in length) for the refresh token.

[Sephster]

I've checked and this is correct. The resultant identifier is 80 although I see how this can be confusing passing a length of 40.

[marc-mabe]

I also agree here that the unique identifier should be possible to customize in a simple way because:

• it's not defined by the spec (= requirement to be customizable)
• Needs to be stored
  • storages behave differently like for MySQL such long string as primary-key are suboptimal
• gets transferred (encoded/encrypted) ofer the network
  • means smaller sizes will increase network performance
• gets transferred in URLs like with implicit grant
  • browsers may have limitations for the size of URLs

Another thing I noticed is you try unique string generation multiple times before giving up, which I thing is great, but this only includes the unique identifier generation without any references to the storage. I mean even with the best unique string generator this way you can't make 100% sure the generated string will be unique. My suggestion would be to move that to the repositories to generate the string within the process of storing it. At this point you can retry multiple times including unique-check and it also opens the possibility to generate unique identifier on storage level if your store includes such a feature. (Just to clarify, I don't mean an auto incremental value here as this would be a security risk but e.g. an optimized UUID generation is often available on storage level with included unique check)

Also in my optinion generating a 40bytes random string is very extreme (UUID4 has 16) and then encoding it as hexadecimal is probably the most safe variant but there would be multiple other possibilities to reduce the size of such an encoded value without reducing uniqueness but this is just my opinion.