[Hockenba]

Description

Adds opt-in Redis-backed revocation for OpenID Connect cookie sessions.

Setting session.revocation to redis enables lua-resty-session's Redis revocation backend. session.revocation_fail_mode defaults to open and can be set to closed. Redis-backed session storage remains unchanged, and incompatible or incomplete configurations are rejected.

Session options remain a direct pass-through to lua-resty-session. The deprecated session.cookie.lifetime alias continues to write absolute_timeout to the shared configuration, so its warning is not emitted on every request.

This also upgrades lua-resty-session to 4.3.0-1 and lua-resty-openidc to1.9.1-1 and documents the feature in English and Chinese.

Which issue(s) this PR fixes:

N/A

Test plan

• Verify valid default-open, explicit open, explicit closed, and omitted revocation configurations.
• Verify invalid revocation backends and failure modes, missing Redis configuration, and incompatible Redis session storage are rejected.
• Verify login, authenticated access, logout, and replay of the pre-logout cookie redirects with status 302.
• Verified all three revocation test blocks locally: 9 of 9 assertions passed.

Checklist

• I have explained the need for this PR and the problem it solves
• I have explained the changes or the new features added to this PR
• I have added tests corresponding to this change
• I have updated the documentation to reflect this change
• I have verified that this change is backward compatible (If not, please discuss on the APISIX mailing list first)

[juzhiyuan]

Hello @Hockenba,

I did a quick review of this PR. It also depends on bungle/lua-resty-session#208, right?

Should we wait for the upstream PR to be merged first?

Update: This PR depends on bungle/lua-resty-session#208 status

https://the-asf.slack.com/archives/CUC5MN17A/p1783014722148709

[janiussyafiq]

Some comments that you can address or state your reasoning on why you think the current approaches should remain:

• I think it would be cleaner to expose the schema in this way rather than the current way since lua-resty-session also do it the same way instead nesting it within redis, WDYT?

"session": {
  "storage": "cookie",
  "redis": { "host": "127.0.0.1" },
  "revocation": "redis",
  "revocation_fail_mode": "closed"
}

• Docs comment: can add one section for usage example using this current feature (can refer how other example showcases this), and add docs for chinese version too
• Update PR desc to reflect the newest code submitted for review, some of them were stale
• IMO the tests were bloated and some are mere duplication from lua-resty-session upstream test which have already been covered, might consider rechecking again. e.g.
  • TEST 13-16 exercise lua-resty-session's fail-open/fail-closed behaviour through a stub revocation table and the library's private __set_ngx_header / __set_ngx_var hooks. That path is already covered upstream (spec/07-revocation-2_spec.lua); APISIX's contribution is one field, which TEST 2 already asserts.
  • TEST 13-17 accept skip: revocation not supported in response_body, so they pass without testing anything on lua-resty-session < 4.2.0. With the rockspec pinned to 4.2.0-1 the probe should go; a missing feature should fail, not skip (same reason check-test-code-style.sh bans --- SKIP).
  • The 12 schema/passthrough blocks differ by one or two fields each. t/plugin/openid-connect2.t (TEST 1) folds such variants into a test_cases table in one block; the same would bring this file down to a handful of blocks.
  • Four cases from the test plan have no test: default revocation_fail_mode (every block sets it explicitly), mode=storage on cookie, revocation_fail_mode without redis, invalid redis.mode.
  • etc. You might want to revisit this again

[Hockenba]

Some comments that you can address or state your reasoning on why you think the current approaches should remain:

• I think it would be cleaner to expose the schema in this way rather than the current way since lua-resty-session also do it the same way instead nesting it within redis, WDYT?

"session": {
  "storage": "cookie",
  "redis": { "host": "127.0.0.1" },
  "revocation": "redis",
  "revocation_fail_mode": "closed"
}

• Docs comment: can add one section for usage example using this current feature (can refer how other example showcases this), and add docs for chinese version too

• Update PR desc to reflect the newest code submitted for review, some of them were stale

• IMO the tests were bloated and some are mere duplication from lua-resty-session upstream test which have already been covered, might consider rechecking again. e.g.

  • TEST 13-16 exercise lua-resty-session's fail-open/fail-closed behaviour through a stub revocation table and the library's private __set_ngx_header / __set_ngx_var hooks. That path is already covered upstream (spec/07-revocation-2_spec.lua); APISIX's contribution is one field, which TEST 2 already asserts.
  • TEST 13-17 accept skip: revocation not supported in response_body, so they pass without testing anything on lua-resty-session < 4.2.0. With the rockspec pinned to 4.2.0-1 the probe should go; a missing feature should fail, not skip (same reason check-test-code-style.sh bans --- SKIP).
  • The 12 schema/passthrough blocks differ by one or two fields each. t/plugin/openid-connect2.t (TEST 1) folds such variants into a test_cases table in one block; the same would bring this file down to a handful of blocks.
  • Four cases from the test plan have no test: default revocation_fail_mode (every block sets it explicitly), mode=storage on cookie, revocation_fail_mode without redis, invalid redis.mode.
  • etc. You might want to revisit this again

You make a lot of great points. It is a little late in my time zone, so I'll refine this tomorrow based on your comments.

One thing: the Chinese documentation needs to the created and pushed to this PR by someone who actually knowns Chinese. I do not know a lick of Chinese and I am sure we want better than what Google Translate will spit out for me.

[janiussyafiq]

You make a lot of great points. It is a little late in my time zone, so I'll refine this tomorrow based on your comments.

One thing: the Chinese documentation needs to the created and pushed to this PR by someone who actually knowns Chinese. I do not know a lick of Chinese and I am sure we want better than what Google Translate will spit out for me.

Thanks for the fast reply. For Chinese translation u can ask your agent to do it on your behalf, it usually does it right the first time if the change is not so complex. If there's any issue then maintainers will give some feedback.

[Hockenba]

You make a lot of great points. It is a little late in my time zone, so I'll refine this tomorrow based on your comments.
One thing: the Chinese documentation needs to the created and pushed to this PR by someone who actually knowns Chinese. I do not know a lick of Chinese and I am sure we want better than what Google Translate will spit out for me.

Thanks for the fast reply. For Chinese translation u can ask your agent to do it on your behalf, it usually does it right the first time if the change is not so complex. If there's any issue then maintainers will give some feedback.

I wasn't sure how good agents are at English to Chinese translation. Sounds good. Will do.

I'll be spending my workday tomorrow refactoring and addressing your comments with either commits or follow up discussion (Eastern Standard Time). So be on the look out for that. Thanks.

[Hockenba]

You make a lot of great points. It is a little late in my time zone, so I'll refine this tomorrow based on your comments.
One thing: the Chinese documentation needs to the created and pushed to this PR by someone who actually knowns Chinese. I do not know a lick of Chinese and I am sure we want better than what Google Translate will spit out for me.

Thanks for the fast reply. For Chinese translation u can ask your agent to do it on your behalf, it usually does it right the first time if the change is not so complex. If there's any issue then maintainers will give some feedback.

I wasn't sure how good agents are at English to Chinese translation. Sounds good. Will do.

I'll be spending my workday tomorrow refactoring and addressing your comments with either commits or follow up discussion (Eastern Standard Time). So be on the look out for that. Thanks.

Done. Refactored it quite a bit. Cut out all the unnecessary bloat. Ready for review again.

[janiussyafiq]

Thanks for addressing my concerns, however few issues remain:

1. I ran the test locally and TEST 3 seems to fail. You can open up the PR against your fork and ensure CI green.
2. build_session_opts now does core.table.clone per request, so the session.cookie.lifetime is deprecated warning fires on every request for users still on the alias (previously once, because absolute_timeout was written back into the shared conf).
3. On the schema shape I still think an explicit session.revocation enum ({"redis"}) with revocation_fail_mode defaulting to open is the better contract. Every other session.* key is a 1:1 forward to lua-resty-session. This is the only derived one (by setting revocation field implicitly.
4. --- no_error_log [crit] on TEST 1 replaces the default [error] gate; nothing in the flow logs [error], so it only hides regressions. Drop it, and assert the replay is 302 rather than ~= 200.

[Hockenba]

Thanks for addressing my concerns, however few issues remain:

1. I ran the test locally and TEST 3 seems to fail. You can open up the PR against your fork and ensure CI green.
2. build_session_opts now does core.table.clone per request, so the session.cookie.lifetime is deprecated warning fires on every request for users still on the alias (previously once, because absolute_timeout was written back into the shared conf).
3. On the schema shape I still think an explicit session.revocation enum ({"redis"}) with revocation_fail_mode defaulting to open is the better contract. Every other session.* key is a 1:1 forward to lua-resty-session. This is the only derived one (by setting revocation field implicitly.
4. --- no_error_log [crit] on TEST 1 replaces the default [error] gate; nothing in the flow logs [error], so it only hides regressions. Drop it, and assert the replay is 302 rather than ~= 200.

1-4 should be addressed.

[janiussyafiq]

Thanks, 1-4 look good and openid-connect-revocation.t passes locally. One issue with the new default = "open" on revocation_fail_mode is that it is applied unconditionally, so every openid-connect route now carries "session":{"revocation_fail_mode":"open"} even with revocation disabled (this breaks t/plugin/openid-connect.t TEST 25 "Check defaults"). The intent is "open only when revocation is redis", and lua-resty-session already does that internally (defaults to open, only consulted on the revocation path). Please drop the schema default, document it as the default in the table, and adjust the TEST 2 assertions to expect nil. Also run the full t/plugin/openid-connect*.t set before pushing.

[Hockenba]

Thanks, 1-4 look good and openid-connect-revocation.t passes locally. One issue with the new default = "open" on revocation_fail_mode is that it is applied unconditionally, so every openid-connect route now carries "session":{"revocation_fail_mode":"open"} even with revocation disabled (this breaks t/plugin/openid-connect.t TEST 25 "Check defaults"). The intent is "open only when revocation is redis", and lua-resty-session already does that internally (defaults to open, only consulted on the revocation path). Please drop the schema default, document it as the default in the table, and adjust the TEST 2 assertions to expect nil. Also run the full t/plugin/openid-connect*.t set before pushing.

Should be all addressed now. I ran all the openid-connect tests, and they all passed. No idea why the CI tests failed, but none are related to my changes or even the openid-connect plugin.

[Copilot]

🟡 Changes recommended

Two exposed cache TTL options are unsupported by the pinned session library and are silently ignored.

Get a fresh assessment by requesting another Copilot review.

Review details

• Files reviewed: 5/5 changed files
• Comments generated: 1
• Review effort level: Balanced

[juzhiyuan]

Waiting for bungle/lua-resty-session#213

[Hockenba]

Yes. Still feel free to take a peak at to make sure that I implemented what was requested :)

[Hockenba]

@juzhiyuan @membphis Hey, the lua-resty-session maintainer told me he won’t cut a new release until he’s made a few more changes on top of my in-memory cache PR (which is already merged). He didn’t give a timeline, but he’s actively working on it. So this PR isn’t abandoned, just stalled for now. Once the new version is published, it’s ready to go after I update the version file here.

[Hockenba]

@juzhiyuan @membphis Ready for review. Bungle updated the version. We now have a in memory cache that sits in front of the redis revocation store.

[Hockenba]

This introduces a Redis lookup on every request that opens an existing cookie session when session.revocation is redis, not only on logout. For example, 100 requests using the same session within 3–5 seconds can generate roughly 100 Redis GETs.

Could we add a small, short-lived local cache keyed by the hashed session ID? On a cache miss, query Redis and cache the result for around 3–5 seconds. Logout should update or invalidate the local cache after writing the revocation marker. This keeps the implementation simple while avoiding dense repeated lookups for the same session. The TTL would be the maximum local stale window for a just-revoked session.

Done.