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

Removed fancy showLogoutURL() #281

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Conversation

marknl
Copy link

@marknl marknl commented Jul 12, 2022

When running Racktables in a docker container, behind a nginx reversed proxy. The logout URL doesn't work. This is because there is no "server_name" available. This fixes that issue. Also, there's no need to generate such a "fancy" logout URL.

When running Racktables in a docker container, behind a nginx reversed proxy. The logout URL doesn't work. This is because there is no "server_name" available. 
This fixes that issue. Also, there's no need to generate such a "fancy" logout URL.
@infrastation
Copy link
Member

Thank you for reporting this problem. This change will likely not work, as far as it is possible to tell from the diff, could you double-check please?

If $_SERVER['SERVER_NAME'] is not defined in this context, perhaps something else could be used instead?

@marknl
Copy link
Author

marknl commented Jul 13, 2022

Why wouldn't it work? I'm currently running a Racktables docker container with nginx reversed proxy in front of it including this change. It works fine.

@sknolin
Copy link

sknolin commented Sep 15, 2022

Somewhat related -I'd like to modify the logout url to support OIDC logout. Using apache mod_auth_oidc works fine for login, but a custom logout url is needed. So maybe instead making this configurable is more generally useful?

@marknl
Copy link
Author

marknl commented Sep 22, 2022

Also, the way I did it will have the browser automatically add the current "server_name". No need to provide that through a php variable. I like the idea of @sknolin to make it completely configurable.

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

Successfully merging this pull request may close these issues.

3 participants