Hi,
There can be a situation (at least I have) where storing access token in session is not a good idea.

Current situation in short:

  • I use password grant
  • I generate token on hook_user_login (so I have username, and plain password)
  • Access token uses for making API calls to REST server

Problem:
I use this module https://www.drupal.org/project/persistent_login,
one of his requirements is session.cookie_lifetime should be 0, it means the session is available "until the browser is closed".

When session is destroyed persistent_login will try to log in user again (depending on some module cookies/tokens).

In that case, session will be cleared, so I can't do any calls to REST server, because there is no access token anymore and I can't regenerate it, because I don't have a plain password which is required by password grant.

So, the user is got logged-out. To fix it, I need to store access token in $user->data or even in some custom table (doesn't matter for now).

Solution:
So, I'd like to improve Client class. My plan is:

1) Create StorageInterface with simple methods for reading, writing, deleting.
2) Create SessionStorage which will be used by default.
3) Provide the third argument (StorageInterface) in the construct.
4) patch is coming soon (no breaking changes or so).
5) profit :)

What do you think? Does it make sense? Any other ideas?

CommentFileSizeAuthor
#14 interdiff-2881022-13-14.txt1.67 KBApacheEx
#14 extend-oauth2-client-with-database-storage-2881022-14.patch5.6 KBApacheEx
#13 extend-oauth2-client-with-database-storage-2881022-13.patch5.21 KBdashohoxha
#12 extend-oauth2-client-with-database-storage-2881022-12.patch3.42 KBApacheEx
#7 extend-oauth2-client-with-database-storage-2881022-7.patch14.23 KBApacheEx
#5 extend-oauth2-client-with-token-operations-2881022-5.patch10.82 KBApacheEx

Comments

ApacheEx created an issue. See original summary.

ApacheEx’s picture

ApacheEx commented
Issue summary: View changes
dashohoxha’s picture

dashohoxha commented

I don't really understand the conflict between the module persistent_login and oauth2_client (I have never used it), but I trust your word about it and I'd like to help you solve this problem. However I would propose a different solution.

My proposed solution goes like this:

  1. Modify oauth2_client so that the parts related to storing and retriving the token can be overriden. This can be done by creating functions like OAuth2\Client::storeToken(), OAuth2\Client::loadToken(), etc. which will be protected methods. This will require a refactoring of the existing code and the default implementation will be based on using $_SESSION (as it currently does).
  2. Create a new module called oauth2_client_db (or something like this) which depends on oauth2_client. This module will just define a new class called OAuth2\DbClient which extends OAuth2\Client and overrides the methods OAuth2\Client::storeToken() and OAuth2\Client::loadToken() to use $user->data or a DB table instead of the $_SESSION.
  3. Whoever has problems with using session for storing the token, can install and enable the module oauth2_client_db besides oauth2_client and use the class OAuth2\DbClient instead of OAuth2\Client .

This seems to me better structured and more flexible. I can take care of the first step (maybe with a patch proposed by you), and you can take care and responsibility for the rest of the steps. What do you think?

ApacheEx’s picture

ApacheEx commented

@dashohoxha,
thanks for quick response.
I like your idea. Btw, I'm currently working on it and can add a patch for #1 soon if you don't mind.
Then, let's review it and proceed with other steps.

ApacheEx’s picture

ApacheEx commented
Status: Active » Needs review
StatusFileSize
new extend-oauth2-client-with-token-operations-2881022-5.patch10.82 KB

Here it is, a patch for #1 (there are also some Coding Standards fixes).

#2 is coming soon.

dashohoxha’s picture

dashohoxha commented

It looks good. But I am going to wait until you finish the rest and confirm that everything works as expected.

I am not an expert on Coding Standards, so I am going to trust you about that part. However I would prefer not to mix on the same patch the codind standards changes and the new feature. So, I will try to separate them into different commits (when the time comes). This is going to be beneficial to whomever is going to figure out from the commits what is going on.

ApacheEx’s picture

ApacheEx commented
StatusFileSize
new extend-oauth2-client-with-database-storage-2881022-7.patch14.23 KB

here it is, the patch includes #1 and #2 steps.

I have also tested that on an existing project. Works as expected.

ApacheEx’s picture

ApacheEx commented

@dashohoxha,
any updates?

dashohoxha’s picture

dashohoxha commented

I have been busy, but I will fix it soon.
By the way, I think that it is better that you manage oauth2_client_db as a separate module.

ApacheEx’s picture

ApacheEx commented

good, as I understand I should add oauth2_client_db as a separate module on drupal.org?
because it's already as separated module but placed inside oauth2_client module.

dashohoxha’s picture

dashohoxha commented

Yes, I think that it should be a separate module on drupal.org

ApacheEx’s picture

ApacheEx commented
StatusFileSize
new extend-oauth2-client-with-database-storage-2881022-12.patch3.42 KB

thanks, I will take care about this.
Here is a patch without oauth2_client_db and fixes in coding standards.

dashohoxha’s picture

dashohoxha commented
StatusFileSize
new extend-oauth2-client-with-database-storage-2881022-13.patch5.21 KB

I propose the attached patch which preserves the existing functionality of the module.
You will have to rewrite your code again, sorry for the inconvenience.
Also keep in mind that I haven't tested it yet, so maybe you will find there any stupid typo or bug.

ApacheEx’s picture

ApacheEx commented
StatusFileSize
new extend-oauth2-client-with-database-storage-2881022-14.patch5.6 KB
new interdiff-2881022-13-14.txt1.67 KB

I don't mind, anyway thank you.
I have tested and there were some little bugs. Here is the final patch (with your and my changes) and interdiff (my fixes).

ApacheEx’s picture

ApacheEx commented

alright, I created a module https://www.drupal.org/project/oauth2_client_db.

I'm waiting for your changes.

  • dashohoxha committed 9a16b0c on 7.x-1.x authored by ApacheEx 
    Issue #2881022 by ApacheEx, dashohoxha: Improve OAuth2\Client to allow...
dashohoxha’s picture

dashohoxha commented

I will do the cosmetic changes soon (coding standards).

  • dashohoxha committed 724ec63 on 7.x-1.x authored by ApacheEx 
    Issue #2881022 by ApacheEx, dashohoxha: Fix some coding standards.

  • dashohoxha committed 9a16b0c on 7.x-2.x authored by ApacheEx 
    Issue #2881022 by ApacheEx, dashohoxha: Improve OAuth2\Client to allow...
  • dashohoxha committed 724ec63 on 7.x-2.x authored by ApacheEx
    Issue #2881022 by ApacheEx, dashohoxha: Fix some coding standards.
dashohoxha’s picture

dashohoxha commented
Status: Needs review » Fixed
ApacheEx’s picture

ApacheEx commented
Status: Fixed » Closed (fixed)

once more, thank you. It can be closed.