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

Specifying token class to be used #500

Closed
tigrang opened this issue Mar 27, 2016 · 12 comments
Closed

Specifying token class to be used #500

tigrang opened this issue Mar 27, 2016 · 12 comments

Comments

@tigrang
Copy link

tigrang commented Mar 27, 2016

I am using Doctrine to persist tokens with an entity that implements AccessTokenEntityInterface.

I have to create a new doctrine entity in persistNewAccessToken() and use setters/getters to copy over values from AccessTokenEntity that is passed and my doctrine entity in order for doctrine to work.

How does adding the ability to configure the class that is instantiated in issueAccessToken, issueRefreshToken, and issueAuthCode before its passed to persist function sound?

@alexbilbie
Copy link
Contributor

Hi,

Thanks for getting in touch. I'm away until the 8th April, but will get
back to you as soon as I'm home.

Please could you flesh out this issue as much as you can so I can think
about it whilst I'm travelling and can better answer you when I'm back?

Thanks,

Alex
On Mon, 28 Mar 2016 at 08:06, Tigran Gabrielyan [email protected]
wrote:

I am using Doctrine to persist tokens with an entity that implements
AccessTokenEntityInterface.

I have to create a new doctrine entity in persistNewAccessToken() and use
setters/getters to copy over values from AccessTokenEntity that is passed
and my doctrine entity in order for doctrine to work.

How does adding the ability to configure the class that is instantiated in
issueAccessToken, issueRefreshToken, and issueAuthCode before its passed
to persist function sound?


You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub
#500

@juliangut
Copy link
Contributor

Hi @tigrang, I'm facing the same issue here: composing Doctrine token entities pulling data from Legue\Oauth2\Server\Entities*Entity, and I assume this is going to be a recurrent issue because of the spread of Doctrine.

I'm thinking two options to avoid this extra step:

  • Making persistNewAccessToken, persistNewRefreshToken and persistNewAuthCode not only persist entities but create and return them by providing corresponding parameters
  • Injecting the entities' class names so the library creates them in issue* methods as you suggest, but somehow providing three class names (for access token, refresh token and auth code) seems like bloating it a bit

With the first option additionally the developer can be responsible of generating token identifiers instead of being restricted to what the library generates (even though using random_bytes is the correct way)

@frederikbosch
Copy link
Contributor

@tigrang @juliangut PR #497 adds the feature for access tokens I think. A new method is added to the AccessTokenRepository 'createNewToken'. This will instantiate the token in the repository so you will have the opportunity to return an object that can be persisted by Doctrine. We could also add createNewToken methods to the auth code and refresh token repository. I could extend the PR, if you like what I am proposing.

@tigrang
Copy link
Author

tigrang commented Mar 28, 2016

@frederikbosch having a method on the repository class is what I had in mind.

@juliangut
Copy link
Contributor

Please @frankdejonge add the methods to the repositories, I'll suggest though you change their signature as they don't need any parameter

public function getNewAccessToken();
public function getNewRefreshToken();
public function getNewAuthCode();

@frederikbosch
Copy link
Contributor

@juliangut Will do, and will also changes the signature. Will update the PR later today or tomorrow.

p.s. @frankdejonge You were mistakenly @-mentioned by @juliangut.

@juliangut
Copy link
Contributor

Ahhh! sorry for the missleading mention just typed @ and f and pressed enter 😞

@frederikbosch
Copy link
Contributor

Hehe, he is going to be super mad at you!

@lucadegasperi
Copy link
Contributor

Agree with @juliangut those methods are a very welcome addition and would allow for some great customizations as well as better framework integrations.

@frederikbosch
Copy link
Contributor

@lucadegasperi @juliangut New PR added with the requested methods.

@lucadegasperi
Copy link
Contributor

@frederikbosch awesome!

@alexbilbie
Copy link
Contributor

I believe this has now been resolved by #512

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

5 participants