Skip to content

feat: openid-connect plugin: Support extra session config options, remove unused option#12051

Closed
arthurdarcet wants to merge 1 commit intoapache:masterfrom
arthurdarcet:patch-1
Closed

feat: openid-connect plugin: Support extra session config options, remove unused option#12051
arthurdarcet wants to merge 1 commit intoapache:masterfrom
arthurdarcet:patch-1

Conversation

@arthurdarcet
Copy link

Description

In the openid-connect plugin:

  • Remove the session.cookie.lifetime option: that option doesn't exist in the lua-resty-session lib, and has no impact
  • allow extra properties to support all the possible config options supported by the underlying lib

Needed because we need to change the cookie domain to set it to the base tld, and not the subdomain of the initial request, so that different product can share auth status.

Without that cookie_domain option, each subdomain needs to trigger an auth flow which are extra un-needed redirections for the user, and more importantly, if one subdomain is set to "unauth_action = pass", the user will be logged out on that domain until they explicitly log in even if they are actually already logged in on other subdomains.

Change is backward compatible because the allowed extra props allow the removed option to be still present.

Fixes
#12050
#12028

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)

@dosubot dosubot bot added size:S This PR changes 10-29 lines, ignoring generated files. plugin labels Mar 13, 2025
@arthurdarcet arthurdarcet changed the title openid-connect plugin: Support extra session config options, remove unused option feat: openid-connect plugin: Support extra session config options, remove unused option Mar 13, 2025
@arthurdarcet
Copy link
Author

I wasn't reading the right version of the code for the resty.session lib - sorry for the noise.

cookie.lifetime is supported, and the cookie domain can be configured by setting the session.cookie.domain conf option of the openid-connect plugin (which is allowed even if not documented).

I think the session options could be improved in that plugin, but that's not a blocker for us anymore (for instance, to set a custom cookie name, i would need to set the session.name option on the plugin, but the schema doesn't allow that (even though the underlying lib supports it). I would change this to accept anything in the session key, and document it only with a link to the exact version of the resty.session lib that is bundled to be both exhaustive and exact.

@mengxzh
Copy link

mengxzh commented Mar 14, 2025

@arthurdarcet
Sorry, How custom config session using redis in this plugin at version 3.10

@arthurdarcet
Copy link
Author

@arthurdarcet Sorry, How custom config session using redis in this plugin at version 3.10

afaics, you can't. You will need to write a custom plugin that can delegate to the openid-connect plugin after setting a few global vars that the underlying resty.session plugin will pick up.

@mengxzh
Copy link

mengxzh commented Mar 24, 2025

@arthurdarcet You mean i should custom plugin which is priority higher than openid-connect? And then i should set global vars that oidc-connect could read it?

@arthurdarcet
Copy link
Author

@mengxzh this isn't really a support forum. I meant that you need to write a custom plugin to replace the existing openid-connect plugin, but can do that with very little custom code by importing the existing plugin into your new plugin

@mengxzh
Copy link

mengxzh commented Mar 24, 2025

@arthurdarcet Why lift this limitaion as your code? As a custom config feature, it will be flexible.

@arthurdarcet
Copy link
Author

@arthurdarcet Why lift this limitaion as your code? As a custom config feature, it will be flexible.

yes, feel free to open another PR to remove the additionalProps: false or to document all the possible options

@mengxzh
Copy link

mengxzh commented Mar 24, 2025

@arthurdarcet Why lift this limitaion as your code? As a custom config feature, it will be flexible.

yes, feel free to open another PR to remove the additionalProps: false or to document all the possible options

Thanks for your help :)

@fernando-arellano
Copy link

fernando-arellano commented May 20, 2025

hello @arthurdarcet can you confirm how you were able to configure the domain.

  - config:
      access_token_in_authorization_header: true
      bearer_only: false
      client_id: ...
      client_secret: ...
      discovery: ...
      redirect_after_logout_uri: http://localhost:4200
      redirect_uri: http://localhost:4200/login/redirect
      scope: openid
      session:
        cookie:
          domain: example.com
        secret: "1234567890123456"
      use_jwks: true
    enable: true

but the cookie domain seems to be ignored after a successful login.

@arthurdarcet
Copy link
Author

Yes, this works for me:

      config:
        session:
          secret: xxx
          cookie:
            domain: xxx
            lifetime: 240
        scope: "openid profile email"
        required_scopes: [ email, profile ]
        unauth_action: auth
        redirect_uri: /callback
        discovery: xxx

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

plugin size:S This PR changes 10-29 lines, ignoring generated files.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants