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

Allow undef for logout_uri and logout_redirect #157

Merged
merged 1 commit into from Apr 17, 2024

Conversation

abujeda
Copy link
Contributor

@abujeda abujeda commented Apr 16, 2024

logout_uri and logout_redirect default values are managed in ood-portal application.
Without the Optional[String] settings for these properties, it is not possible to remove the logout redirect from Apache.

There are cases, when multiple domains and OIDC are used, that a more complex logout redirect is required.
This can be now accomplished using the new custom_vhost_directives like so:

openondemand::custom_vhost_directives:
  - 'RewriteRule ^/logout$ /oidc?logout=https://%{literal("%")}{HTTP_HOST} [R=301,L]'

This is simply propagating the current domain to the OIDC identity provider for any domain used within OOD.

Although the Redirect directive created with the default values for logout_uri and logout_redirect do not affect the functionality of the RewriteRule as the custom_vhost_directives are printed first, it will create a cleaner config.

@abujeda
Copy link
Contributor Author

abujeda commented Apr 16, 2024

relates to #3442

@treydock
Copy link
Collaborator

You only need to change the datatype. You don't need to remove the defaults. Linters won't let Optional be used without default being undef I believe:

Variant[String[1], Undef]

Could try this too but I think linters will complain.

Optional[String[1]]

In Hiera you could set:

openondemand::logout_uri: ~

So you can force undef without removing the default from this module.

The reason I don't want to remove the defaults is I try to keep this module to have same defaults as OnDemand which helps with maintenance.

Comment on lines 299 to 300
Optional[String] $logout_uri = undef,
Optional[String] $logout_redirect = undef,
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
Optional[String] $logout_uri = undef,
Optional[String] $logout_redirect = undef,
Variant[String[1], Undef] $logout_uri = '/logout',
Variant[String[1], Undef] $logout_redirect = '/pun/sys/dashboard/logout',

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks - I have updated with the changes to the REFERENCE.md too

@treydock treydock closed this Apr 16, 2024
@treydock treydock reopened this Apr 16, 2024
@treydock treydock added the bugfix Something isn't working label Apr 16, 2024
@treydock treydock changed the title Removed default values for logout_uri and logout_redirect Allow undef for logout_uri and logout_redirect Apr 16, 2024
@treydock treydock merged commit fcda665 into OSC:master Apr 17, 2024
16 checks passed
@treydock
Copy link
Collaborator

This is released as 5.0.1.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bugfix Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants