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

Migrate lua-resty-session to 4.0.3 #478

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

balajiv113
Copy link

Fixes #464

Signed-off-by: Balaji Vijayakumar <[email protected]>
@darjeeling
Copy link

@bodewig IMHO I think this works well. how about it?
@balajiv113 nice job.

@darjeeling
Copy link

@balajiv113 I think you missed some point of session:save

@balajiv113
Copy link
Author

@darjeeling could you give more context on the issues that you are facing ??

I ran the current changes with the integration tests that was there. All were passing

@lordgreg
Copy link

Hi there. I have few crazy days behind me figuring out the issue with "request to the redirect_uri path but there's no session state found". Turns out two versions of lua-resty-session are installed (see more info here revomatico/kong-oidc#34).

Is there anything open here or can the PR be merged?

@brafales
Copy link

Any updates on this or ETA would be greatly appreciated. As @lordgreg pointed out, this would fix a compatibility issue with Kong version 3 which is currently preventing us from using the https://github.com/revomatico/kong-oidc plugin.

@lukeyeager
Copy link

I see a warning when I try to use this:

2023/07/13 16:25:57 [warn] 7#7: *1 [lua] _G write guard:12: __newindex(): writing a global Lua variable ('session_present') which may lead to race conditions between concurrent requests, so prefer the use of 'local' variables
stack traceback:
  /usr/local/openresty/luajit/share/lua/5.1/resty/openidc.lua:1461: in function 'authenticate'
  /etc/nginx/lua/utils.lua:33: in function 'do_auth'
  access_by_lua(nginx.conf:99):3: in main chunk, client: REDACTED, server: REDACTED, request: "GET / HTTP/1.1", host: "REDACTED", referrer: "REDACTED"

@nikita-kuznetsov
Copy link

nikita-kuznetsov commented Jul 28, 2023

I found a bug with function is_session. Session v4 cause the type of session.start being nil. To make function woks properly, need to check state property instead of start
I used following implementation.

local function is_session(o)
return o ~= nil and o.state and o.state ~= "closed"
end

@oldium
Copy link

oldium commented Aug 5, 2023

I see a warning when I try to use this:

2023/07/13 16:25:57 [warn] 7#7: *1 [lua] _G write guard:12: __newindex(): writing a global Lua variable ('session_present') which may lead to race conditions between concurrent requests, so prefer the use of 'local' variables
stack traceback:
  /usr/local/openresty/luajit/share/lua/5.1/resty/openidc.lua:1461: in function 'authenticate'
  /etc/nginx/lua/utils.lua:33: in function 'do_auth'
  access_by_lua(nginx.conf:99):3: in main chunk, client: REDACTED, server: REDACTED, request: "GET / HTTP/1.1", host: "REDACTED", referrer: "REDACTED"

You need to add session_present to local variables:

local session, session_present

in function openidc.authenticate.

@oldium
Copy link

oldium commented Aug 5, 2023

I found a bug with function is_session. Session v4 cause the type of session.start being nil. To make function woks properly, need to check state property instead of start I used following implementation.

local function is_session(o)
return o ~= nil and o.state and o.state ~= "closed"
end

It would be better to check some actually called function, like save:

local function is_session(o)
  return o ~= nil and o.save and type(o.save) == "function"
end

@oldium
Copy link

oldium commented Aug 6, 2023

Access token refresh cannot work, session:refresh() is not the same as previous session:regenerate().

@oldium
Copy link

oldium commented Aug 6, 2023

I reworked the approach (I used session:get and session:set to work with session data) and tried to address few missing bits (session in session_or_opts, token refresh, is_session check) here (untested yet, so take it as work-in-progress). I will create PR as soon as I test it.

@bodewig
Copy link
Collaborator

bodewig commented Aug 8, 2023

Unfortunately I myself completely lack the time to understand what has changed between 3 and 4 and how to migrate this library right now. I'm very grateful for your effort here.

A very cursory glance over lua-resty-session 4.x a few weeks (months?) ago convinced me that migrating would mean more than the changes made initially in this PR and then life interfered.

@oldium
Copy link

oldium commented Aug 11, 2023

All issues mentioned here are fixed in #489. It uses session:get and session:set API instead of old-API simulation (simulated by calling session:set_data and manipulating the passed-in table afterwards).

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.

Migrate lua-resty-session to v4
8 participants