websockify icon indicating copy to clipboard operation
websockify copied to clipboard

TokenRedis plugin improvements

Open javicacheiro opened this issue 2 years ago • 10 comments

Various improvements to the TokenRedis plugin maintaining backwards compatibility:

  • Avoid creating a new redis connection in each lookup, instead use redis connection pool (each Redis instance automatically creates its own connection pool, so we just have to create the instance in the init method, instead of creating a new instance each time the lookup method is called).
  • Support for password protected redis servers
  • Support to select the redis database to use
  • Support both for json and text format tokens (json seems a bit of an overhead in most cases)
  • Verification of the token decoding and return None in case of a decoding error
  • Eliminate the dependency with simplejson module since the json module in the standard lib is more than enough for our basic usage of using the loads
  • Additional tests to verify all the functionality

javicacheiro avatar Apr 19 '22 17:04 javicacheiro

Integration testing already performed against a password protected redis server.

javicacheiro avatar Apr 19 '22 17:04 javicacheiro

Thanks for the contribution! But could you split this to several commits. One for each thing you'd like to fix.

CendioOssman avatar Apr 22 '22 08:04 CendioOssman

Yes, of course, I was not sure the best way but this was also my initial thinking.

Do you want me to create different PR for each of them so then you can choose which ones to merge?

javicacheiro avatar Apr 22 '22 09:04 javicacheiro

Let's start with everything here. Hopefully everything is okay. :)

CendioOssman avatar Apr 22 '22 11:04 CendioOssman

Done!

javicacheiro avatar Apr 22 '22 11:04 javicacheiro

Sorry for the delays. This is on my to-do list, but I'm a bit too busy with other things at the moment. I will get to this eventually, though. :)

CendioOssman avatar May 24 '22 14:05 CendioOssman

No problem! In the meanwhile I have added the possibility you mentioned for empty options when using default values.

javicacheiro avatar May 25 '22 09:05 javicacheiro

I did a rebase to integrate the fix for the new jwcrypto versions so tests do not break. I hope to not have created a mess.

javicacheiro avatar May 27 '22 12:05 javicacheiro

Now is hard to see what are the commits specific to this PR.

Support for empty options is here:

https://github.com/novnc/websockify/pull/518/commits/8eeef0f6d77be3e6e8174a04a521a29b7a4c74cb

javicacheiro avatar May 27 '22 12:05 javicacheiro

I'm afraid something did indeed go wrong when rebasing. Many other commits are suddenly included. Could you try again? git rebase -i <upstream master branch> and filtering the resulting list should do the right thing.

CendioOssman avatar Aug 18 '22 09:08 CendioOssman

Fixed the problem with the rebase.

Everything looks fine now and as, a side effect, we now have a rebase to the latest upstream. Really needed after so many months.

javicacheiro avatar Nov 29 '22 17:11 javicacheiro

Just a small commit history adjustment and I think it's ready for merging.

Thanks for your work!

Happy New Year!

Sorry for the delay, the history is now adjusted and it looks much better without the back and forth.

When this PR is merged I will create the new one with the connection pool part based on this code.

javicacheiro avatar Jan 19 '23 16:01 javicacheiro