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

Evolve oidc logout #4986

Closed
wants to merge 9 commits into from
Closed

Evolve oidc logout #4986

wants to merge 9 commits into from

Conversation

llomgui
Copy link

@llomgui llomgui commented Jan 24, 2024

Proposed changes

Make sure the IDP is triggered during a logout.

Checklist

Before creating a PR, run through this checklist and mark each as complete.

  • I have read the CONTRIBUTING doc
  • I have added tests that prove my fix is effective or that my feature works
  • I have checked that all unit tests pass after adding my changes
  • I have updated necessary documentation
  • I have rebased my branch onto main
  • I will ensure my PR is targeting the main branch and pulling from my branch from my own fork

Copy link

netlify bot commented Jan 24, 2024

👷 Deploy request for nginx-kubernetes-ingress pending review.

Visit the deploys page to approve it

Name Link
🔨 Latest commit 91a9c12

@llomgui llomgui marked this pull request as ready for review January 24, 2024 22:39
@llomgui llomgui requested a review from a team as a code owner January 24, 2024 22:39
@llomgui llomgui requested a review from a team as a code owner January 25, 2024 09:19
@github-actions github-actions bot added the documentation Pull requests/issues for documentation label Jan 25, 2024
@brianehlert
Copy link
Collaborator

Thank you @llomgui
We will take a look and get back to you.
We need to make sure your changes won't break anyone and are relatively generic to the core OIDC flow.

@pdabelf5
Copy link
Collaborator

Thank you @llomgui for submitting. Would it be possible for you to submit a GitHub issue to describe the situation you are trying to address. See CONTRIBUTING.md for a description of the issue submission process. There is also the scheduled community call where issues can be discussed.

@llomgui
Copy link
Author

llomgui commented Jan 26, 2024

Hello @pdabelf5,
I created this issue to describe the current situation.

Copy link
Contributor

@ADubhlaoich ADubhlaoich left a comment

Choose a reason for hiding this comment

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

LGTM on the docs side. Withholding from an approval since much of the PR is code-based.

@danielnginx danielnginx linked an issue Jan 29, 2024 that may be closed by this pull request
@danielnginx
Copy link
Collaborator

Hi @llomgui,

We reviewed your issue and the team recommendation is to get the contents of your PR and port it into the Open ID connect reference implementation

@llomgui
Copy link
Author

llomgui commented Jan 29, 2024

@danielnginx I will create a PR on this repository.

Do I have to close this one? I don't see any link with Kubernetes-ingress.

@llomgui
Copy link
Author

llomgui commented Jan 30, 2024

@danielnginx
Copy link
Collaborator

@llomgui thank you. Once that PR is merged in the nginx-openid-connect, files should get copied over to https://github.com/nginxinc/kubernetes-ingress/tree/main/internal/configs/oidc via a PR. You can close this PR for now.

@llomgui
Copy link
Author

llomgui commented Jan 31, 2024

@danielnginx The PR created on OIDC repository does not include operators changes.
This PR needs to be merged.

@danielnginx
Copy link
Collaborator

@llomgui you are right, we can leave this PR open. Once we get the nginx-openid-connect PR in we can update here.

@llomgui
Copy link
Author

llomgui commented Feb 23, 2024

@danielnginx Do you have any news on this PR?

@shaun-nx
Copy link
Contributor

shaun-nx commented Feb 28, 2024

@llomgui

As @danielnginx said, we can merge this PR as soon as your PR in the nginx-openid-connect repo is approved and merged.

If you need more visibility on PR, you can post about it in the public #nginx-users slack channel. The maintainers of the nginx-openid-connect repo frequently monitor that channel.

@github-actions github-actions bot added the go Pull requests that update Go code label May 13, 2024
@haywoodsh
Copy link
Contributor

haywoodsh commented Jul 22, 2024

Hello @llomgui, the nginx-openid-connect now includes support for OIDC logout, and our repository has been updated accordingly. We are keen to work with you to get this pull request merged. Could you align the OIDC files in your pull request with those in our kubernetes-ingress repository?
Additionally, as we do not have integration tests for OIDC, I was wondering if you have attempted to deploy our OIDC example with logout, and if so, could you please outline the steps you followed so that I can do some testing on my side as well?

@@ -243,6 +243,9 @@ func validateOIDC(oidc *v1.OIDC, fieldPath *field.Path) field.ErrorList {
if oidc.AuthEndpoint == "" {
return field.ErrorList{field.Required(fieldPath.Child("authEndpoint"), "")}
}
if oidc.LogoutEndpoint == "" {
Copy link
Contributor

Choose a reason for hiding this comment

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

I am not sure if logoutEndpoint and redirectPostLogout should be required fields. This will lead to a breaking change for current users looking to upgrade to the new release, as they will need to modify their OIDC configurations.

@danielnginx
Copy link
Collaborator

Hi @llomgui, we are closing this PR and our team is taking over the #4989 implementation. You can follow the progress in the issue, we are working on it in our current sprint https://github.com/orgs/nginxinc/projects/2

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
documentation Pull requests/issues for documentation go Pull requests that update Go code
Projects
Status: Done 🚀
Development

Successfully merging this pull request may close these issues.

Be logged out on IDP side
7 participants