Skip to content

feat: support post_logout_redirect_uri config in openid-connect plugin#6455

Merged
spacewander merged 7 commits into
apache:masterfrom
starsz:feat_oidc_logout_uri
Mar 2, 2022
Merged

feat: support post_logout_redirect_uri config in openid-connect plugin#6455
spacewander merged 7 commits into
apache:masterfrom
starsz:feat_oidc_logout_uri

Conversation

@starsz

@starsz starsz commented Feb 26, 2022

Copy link
Copy Markdown
Contributor

What this PR does / why we need it:

Hi, this PR is used to support post_logout_redirect_uri config in openid-connect plugin.
So that we can redirect the uri when request the logout path of openid-connect plugin.
Fix: #6345, #6362

Since the lua-resty-openidc has support post_logout_redirect_uri already, we can just add post_logout_redirect_uri config in our plugin.
Refer: https://github.com/zmartzone/lua-resty-openidc/blob/3aac462f8206b8553cd1f34bb99b1788efbc2140/lib/resty/openidc.lua#L1331-L1339

Pre-submission checklist:

  • Did you explain what problem does this PR solve? Or what new features have been added?
  • Have you added corresponding test cases?
  • Have you modified the corresponding document?
  • Is this PR backward compatible? If it is not backward compatible, please discuss on the mailing list first

@starsz
starsz marked this pull request as draft February 26, 2022 01:37
@starsz
starsz marked this pull request as ready for review February 26, 2022 01:45
| realm | string | optional | "apisix" | | Realm used for the authentication |
| bearer_only | boolean | optional | false | | Setting this `true` will check for the authorization header in the request with a bearer token |
| logout_path | string | optional | "/logout" | | |
| post_logout_redirect_uri | string | optional | | | URL want to redirect when request logout_path

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

please align the table

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

OK. Let me fix it.

Comment thread t/plugin/openid-connect.t Outdated
"uri": "/*"
}]],
[[{
"node": {

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We don't need to check the response data of this request, right?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we should check the response data of this request like other route-creating tests.
Why we don't need to check the response?

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, we check the response status code but not the data. The data is mostly an echo of the input.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let me remove it.

Comment thread t/plugin/openid-connect.t Outdated
end
end

-- Get the final URI out of the Location response header. This should be the original URI that was requested.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The similar Lua snipper is repeated in multiple tests. Could we refactor it?

@starsz starsz Feb 28, 2022

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi, @spacewander.

The similar Lua snipper is repeated in multiple tests.

Yes, they are repeated. But I have no idea about refactoring it, can you give me some examples or docs?

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

my $extra_init_by_lua = <<_EOC_;

You can define helper functions in extra_init_by_lua

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed. Please review it again.

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.

bug: apisix openid plugin - how the logout path does work ?

3 participants