nic-6443 commented on code in PR #13178:
URL: https://github.com/apache/apisix/pull/13178#discussion_r3381620112
##########
apisix/plugins/openid-connect.lua:
##########
@@ -76,14 +104,59 @@ local schema = {
description = "the key used for the encrypt and HMAC
calculation",
minLength = 16,
},
+ cookie_name = {
+ type = "string",
+ description = "session cookie name",
+ },
+ cookie_path = {
+ type = "string",
+ description = "cookie path scope",
+ },
+ cookie_domain = {
+ type = "string",
+ description = "cookie domain scope",
+ },
+ cookie_secure = {
+ type = "boolean",
+ description = "if true, set the Secure cookie attribute",
+ },
+ cookie_http_only = {
+ type = "boolean",
+ description = "if true, set the HttpOnly cookie attribute",
+ },
+ cookie_same_site = {
+ type = "string",
+ enum = {"Strict", "Lax", "None", "Default"},
+ description = "SameSite cookie attribute",
+ },
+ idling_timeout = {
+ type = "integer",
+ description = "idling timeout in seconds",
+ },
+ rolling_timeout = {
+ type = "integer",
+ description = "rolling timeout in seconds",
+ },
+ absolute_timeout = {
+ type = "integer",
+ description = "absolute session lifetime in seconds",
+ },
cookie = {
type = "object",
+ description =
+ "Deprecated. Kept for backward compatibility with "
+ .. "the lua-resty-session 3.x schema. Use the flat "
+ .. "session.* options (cookie_name, absolute_timeout, "
+ .. "etc.) instead.",
properties = {
lifetime = {
type = "integer",
- description = "it holds the cookie lifetime in
seconds in the future",
- }
- }
+ description =
+ "Deprecated. Mapped to absolute_timeout at "
+ .. "runtime when absolute_timeout is not set.",
+ },
+ },
+ additionalProperties = false,
Review Comment:
Heads up — `TEST 9` in `openid-connect10.t` asserts exactly this rejection
(an unknown key under `session.cookie`), so when you drop `additionalProperties
= false` here it'll need to be removed too, otherwise it'll start failing.
##########
apisix/plugins/openid-connect.lua:
##########
@@ -33,6 +34,33 @@ local ngx_encode_base64 = ngx.encode_base64
local plugin_name = "openid-connect"
+-- Translate session config to lua-resty-session 4.x options.
+-- Most keys (cookie_name, cookie_path, *_timeout, etc.) are already named
+-- after lua-resty-session and pass straight through. The only translation
+-- is the legacy session.cookie.lifetime alias from the lua-resty-session 3.x
+-- schema, which is mapped to absolute_timeout when the latter is unset.
+local function build_session_opts(session_conf)
+ if not session_conf then
+ return nil
+ end
+ local opts = {}
+ for k, v in pairs(session_conf) do
+ if k ~= "cookie" then
+ opts[k] = v
+ end
+ end
+ local cookie = session_conf.cookie
+ if cookie and cookie.lifetime ~= nil then
+ if opts.absolute_timeout == nil then
+ opts.absolute_timeout = cookie.lifetime
+ core.log.warn("session.cookie.lifetime is deprecated; ",
+ "use session.absolute_timeout instead")
+ end
+ end
+ return opts
Review Comment:
One follow-on if you take this version: it no longer strips the `cookie` key
from the returned table, so `TEST 4` and `TEST 5` (which assert `cookie=nil`)
will fail — they should instead expect the `cookie` table to still be present.
Leaving it there is harmless since lua-resty-session 4.x only reads the keys it
knows. Also `local pairs = pairs` becomes unused and luacheck will flag it, so
that import can go.
##########
t/plugin/openid-connect10.t:
##########
@@ -0,0 +1,393 @@
+#
+# Licensed to the Apache Software Foundation (ASF) under one or more
+# contributor license agreements. See the NOTICE file distributed with
+# this work for additional information regarding copyright ownership.
+# The ASF licenses this file to You under the Apache License, Version 2.0
+# (the "License"); you may not use this file except in compliance with
+# the License. You may obtain a copy of the License at
+#
+# http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing, software
+# distributed under the License is distributed on an "AS IS" BASIS,
+# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+# See the License for the specific language governing permissions and
+# limitations under the License.
+#
+
+use t::APISIX 'no_plan';
+
+repeat_each(1);
+no_long_string();
+no_root_location();
+no_shuffle();
+
+add_block_preprocessor(sub {
+ my ($block) = @_;
+
+ if ((!defined $block->error_log) && (!defined $block->no_error_log)) {
+ $block->set_value("no_error_log", "[error]");
+ }
+
+ if (!defined $block->request) {
+ $block->set_value("request", "GET /t");
+ }
+});
+
+run_tests();
+
+__DATA__
+
+=== TEST 1: valid session with flat cookie_name, cookie_path, absolute_timeout
+--- config
+ location /t {
+ content_by_lua_block {
+ local plugin = require("apisix.plugins.openid-connect")
+ local ok, err = plugin.check_schema({
+ client_id = "a",
+ client_secret = "b",
+ discovery = "c",
+ session = {
+ secret = "jwcE5v3pM9VhqLxmxFOH9uZaLo8u7KQK",
+ cookie_name = "my_session",
+ cookie_path = "/app",
+ absolute_timeout = 7200,
+ }
+ })
+ if not ok then
+ ngx.say(err)
+ else
+ ngx.say("done")
+ end
+ }
+ }
+--- response_body
+done
+
+
+
+=== TEST 2: valid session with flat cookie_secure, cookie_same_site,
idling/rolling timeouts
+--- config
+ location /t {
+ content_by_lua_block {
+ local plugin = require("apisix.plugins.openid-connect")
+ local ok, err = plugin.check_schema({
+ client_id = "a",
+ client_secret = "b",
+ discovery = "c",
+ session = {
+ secret = "jwcE5v3pM9VhqLxmxFOH9uZaLo8u7KQK",
+ cookie_name = "oidc_session",
+ cookie_secure = true,
+ cookie_http_only = true,
+ cookie_same_site = "Strict",
+ cookie_domain = "example.com",
+ idling_timeout = 600,
+ rolling_timeout = 1800,
+ }
+ })
+ if not ok then
+ ngx.say(err)
+ else
+ ngx.say("done")
+ end
+ }
+ }
+--- response_body
+done
+
+
+
+=== TEST 3: backward-compatible session.cookie.lifetime still accepted
+--- config
+ location /t {
+ content_by_lua_block {
+ local plugin = require("apisix.plugins.openid-connect")
+ local ok, err = plugin.check_schema({
+ client_id = "a",
+ client_secret = "b",
+ discovery = "c",
+ session = {
+ secret = "jwcE5v3pM9VhqLxmxFOH9uZaLo8u7KQK",
+ cookie = {
+ lifetime = 3600,
+ }
+ }
+ })
+ if not ok then
+ ngx.say(err)
+ else
+ ngx.say("done")
+ end
+ }
+ }
+--- response_body
+done
+
+
+
+=== TEST 4: build_session_opts passes flat keys through untouched
+--- config
+ location /t {
+ content_by_lua_block {
+ local plugin = require("apisix.plugins.openid-connect")
+ local build = plugin._build_session_opts
+ local opts = build({
+ secret = "jwcE5v3pM9VhqLxmxFOH9uZaLo8u7KQK",
+ storage = "cookie",
+ cookie_name = "my_session",
+ cookie_path = "/app",
+ cookie_secure = true,
+ cookie_same_site = "Strict",
+ idling_timeout = 600,
+ rolling_timeout = 1800,
+ absolute_timeout = 7200,
+ })
+ ngx.say("cookie_name=", opts.cookie_name)
+ ngx.say("cookie_path=", opts.cookie_path)
+ ngx.say("cookie_secure=", tostring(opts.cookie_secure))
+ ngx.say("cookie_same_site=", opts.cookie_same_site)
+ ngx.say("idling_timeout=", opts.idling_timeout)
+ ngx.say("rolling_timeout=", opts.rolling_timeout)
+ ngx.say("absolute_timeout=", opts.absolute_timeout)
+ ngx.say("storage=", opts.storage)
+ ngx.say("cookie=", tostring(opts.cookie))
+ }
+ }
+--- response_body
+cookie_name=my_session
+cookie_path=/app
+cookie_secure=true
+cookie_same_site=Strict
+idling_timeout=600
+rolling_timeout=1800
+absolute_timeout=7200
+storage=cookie
+cookie=nil
+
+
+
+=== TEST 5: build_session_opts maps deprecated cookie.lifetime to
absolute_timeout
+--- config
+ location /t {
+ content_by_lua_block {
+ local plugin = require("apisix.plugins.openid-connect")
+ local build = plugin._build_session_opts
+ local opts = build({
+ secret = "jwcE5v3pM9VhqLxmxFOH9uZaLo8u7KQK",
+ cookie = {
+ lifetime = 7200,
+ }
+ })
+ ngx.say("absolute_timeout=", opts.absolute_timeout)
+ ngx.say("cookie=", tostring(opts.cookie))
+ }
+ }
+--- response_body
+absolute_timeout=7200
+cookie=nil
+--- error_log
+session.cookie.lifetime is deprecated
+
+
+
+=== TEST 6: build_session_opts returns nil for nil input
+--- config
+ location /t {
+ content_by_lua_block {
+ local plugin = require("apisix.plugins.openid-connect")
+ local build = plugin._build_session_opts
+ ngx.say(tostring(build(nil)))
+ }
+ }
+--- response_body
+nil
+
+
+
+=== TEST 7: build_session_opts works when cookie field is absent
+--- config
+ location /t {
+ content_by_lua_block {
+ local plugin = require("apisix.plugins.openid-connect")
+ local build = plugin._build_session_opts
+ local opts = build({
+ secret = "jwcE5v3pM9VhqLxmxFOH9uZaLo8u7KQK",
+ storage = "redis",
+ redis = { host = "127.0.0.1", port = 6379 },
+ })
+ ngx.say("secret=", opts.secret)
+ ngx.say("storage=", opts.storage)
+ ngx.say("redis.host=", opts.redis.host)
+ ngx.say("redis.port=", opts.redis.port)
+ }
+ }
+--- response_body
+secret=jwcE5v3pM9VhqLxmxFOH9uZaLo8u7KQK
+storage=redis
+redis.host=127.0.0.1
+redis.port=6379
+
+
+
+=== TEST 8: invalid type for cookie.lifetime (string instead of integer)
+--- config
+ location /t {
+ content_by_lua_block {
+ local plugin = require("apisix.plugins.openid-connect")
+ local ok, err = plugin.check_schema({
+ client_id = "a",
+ client_secret = "b",
+ discovery = "c",
+ session = {
+ secret = "jwcE5v3pM9VhqLxmxFOH9uZaLo8u7KQK",
+ cookie = {
+ lifetime = "invalid",
+ }
+ }
+ })
+ if not ok then
+ ngx.say(err)
+ else
+ ngx.say("done")
+ end
+ }
+ }
+--- response_body_like
+property "lifetime" validation failed: wrong type: expected integer, got
string.*
+
+
+
+=== TEST 9: unknown key under session.cookie is rejected
+--- config
+ location /t {
+ content_by_lua_block {
+ local plugin = require("apisix.plugins.openid-connect")
+ local ok, err = plugin.check_schema({
+ client_id = "a",
+ client_secret = "b",
+ discovery = "c",
+ session = {
+ secret = "jwcE5v3pM9VhqLxmxFOH9uZaLo8u7KQK",
+ cookie = {
+ cookie_secure = true,
+ }
+ }
+ })
+ if not ok then
+ ngx.say(err)
+ else
+ ngx.say("done")
+ end
+ }
+ }
+--- response_body_like
+.*additional property.*cookie_secure.*
+
+
+
+=== TEST 10: valid session with redis storage and flat cookie options
+--- config
+ location /t {
+ content_by_lua_block {
+ local plugin = require("apisix.plugins.openid-connect")
+ local ok, err = plugin.check_schema({
+ client_id = "a",
+ client_secret = "b",
+ discovery = "c",
+ session = {
+ secret = "jwcE5v3pM9VhqLxmxFOH9uZaLo8u7KQK",
+ storage = "redis",
+ redis = {
+ host = "127.0.0.1",
+ port = 6379,
+ },
+ cookie_name = "oidc_session",
+ absolute_timeout = 7200,
+ }
+ })
+ if not ok then
+ ngx.say(err)
+ else
+ ngx.say("done")
+ end
+ }
+ }
+--- response_body
+done
+
+
+
+=== TEST 11: absolute_timeout wins when both it and cookie.lifetime are set
+--- config
+ location /t {
+ content_by_lua_block {
+ local plugin = require("apisix.plugins.openid-connect")
+ local build = plugin._build_session_opts
+ local opts = build({
+ secret = "jwcE5v3pM9VhqLxmxFOH9uZaLo8u7KQK",
+ absolute_timeout = 1800,
+ cookie = {
+ lifetime = 7200,
+ }
+ })
+ ngx.say("absolute_timeout=", opts.absolute_timeout)
+ }
+ }
+--- response_body
+absolute_timeout=1800
+--- no_error_log
+session.cookie.lifetime is deprecated
+
+
+
+=== TEST 12: unknown key directly under session is rejected
+--- config
+ location /t {
+ content_by_lua_block {
+ local plugin = require("apisix.plugins.openid-connect")
+ local ok, err = plugin.check_schema({
+ client_id = "a",
+ client_secret = "b",
+ discovery = "c",
+ session = {
+ secret = "jwcE5v3pM9VhqLxmxFOH9uZaLo8u7KQK",
+ not_a_real_option = true,
+ }
+ })
+ if not ok then
+ ngx.say(err)
+ else
+ ngx.say("done")
+ end
+ }
+ }
+--- response_body_like
+.*additional property.*not_a_real_option.*
+
+
+
+=== TEST 13: invalid cookie_same_site value is rejected
+--- config
+ location /t {
+ content_by_lua_block {
+ local plugin = require("apisix.plugins.openid-connect")
+ local ok, err = plugin.check_schema({
+ client_id = "a",
+ client_secret = "b",
+ discovery = "c",
+ session = {
+ secret = "jwcE5v3pM9VhqLxmxFOH9uZaLo8u7KQK",
+ cookie_same_site = "bogus",
+ }
+ })
+ if not ok then
+ ngx.say(err)
+ else
+ ngx.say("done")
+ end
+ }
+ }
+--- response_body_like
+.*cookie_same_site.*
+session.cookie: both
Review Comment:
This test will fail as-is. The `session.cookie: both` line is leftover from
the earlier alias implementation and is never emitted now — and since this is a
schema-validation case, the response body is just the `cookie_same_site` enum
error. That second line should be dropped.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
