Skip to content

oauth2 add trailing slash variable#1127

Closed
PVTGoesen wants to merge 4 commits intoLibreBooking:developfrom
PVTGoesen:OAuth2-trailing-slash
Closed

oauth2 add trailing slash variable#1127
PVTGoesen wants to merge 4 commits intoLibreBooking:developfrom
PVTGoesen:OAuth2-trailing-slash

Conversation

@PVTGoesen
Copy link
Contributor

Some OAuth2 provider need a trailing slash in the authorization URL. This changes let you choose if you want to cut or keep the trailing slash in oauth2 authorize URL.

Add variable 'LB_AUTHENTICATION_OAUTH2_TRAILING_SLASH' to choose if to cut or keep the trailing slash in oauth2 authorize URL, this is needed for some providers.
Add variable 'LB_AUTHENTICATION_OAUTH2_TRAILING_SLASH' to choose if to cut or keep the trailing slash in oauth2 authorize URL, this is needed for some providers.
Add variable 'LB_AUTHENTICATION_OAUTH2_TRAILING_SLASH' to choose if to cut or keep the trailing slash in oauth2 authorize URL, this is needed for some providers.
@PVTGoesen
Copy link
Contributor Author

Hi,
I don't have any experience with "linter checks". Can anyone explain what I did wrong and how I can avoid failing these checks in the future?

Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

Adds a configuration toggle to control whether the OAuth2 authorization URL should keep or trim a trailing slash, to support providers that require /authorize/ vs /authorize.

Changes:

  • Introduces authentication.oauth2.trailing.slash config key (default false).
  • Documents the new setting in config.dist.php under authentication → generic OAuth2.
  • Updates LoginPresenter::GetOauth2Url() to conditionally rtrim() the configured authorize URL.

Reviewed changes

Copilot reviewed 3 out of 3 changed files in this pull request and generated 4 comments.

File Description
lib/Config/ConfigKeys.php Adds a new boolean config key for OAuth2 authorize URL trailing-slash handling.
config/config.dist.php Adds the new OAuth2 trailing-slash setting to the distributed config template.
Presenters/LoginPresenter.php Applies the new config flag when building the OAuth2 authorize URL.
Comments suppressed due to low confidence (1)

Presenters/LoginPresenter.php:406

  • This new behavior in GetOauth2Url() isn’t covered by the existing LoginPresenter tests. Please add tests for both cases (trailing-slash preservation enabled/disabled) to ensure the generated authorize URL is correct when the configured base URL ends with '/'.
    public function GetOauth2Url()
    {
        // Retrieve Oauth2 configuration values
        $trailingSlash = Configuration::Instance()->GetKey(ConfigKeys::AUTHENTICATION_OAUTH2_TRAILING_SLASH);
        if (!$trailingSlash){
            $baseUrl = rtrim(Configuration::Instance()->GetKey(ConfigKeys::AUTHENTICATION_OAUTH2_URL_AUTHORIZE), '/');
        }
        else {
            $baseUrl = Configuration::Instance()->GetKey(ConfigKeys::AUTHENTICATION_OAUTH2_URL_AUTHORIZE);
        }
        $clientId = Configuration::Instance()->GetKey(ConfigKeys::AUTHENTICATION_OAUTH2_CLIENT_ID);
        $redirectUri = $this->buildRedirectUri(Configuration::Instance()->GetKey(ConfigKeys::AUTHENTICATION_OAUTH2_REDIRECT_URI));

        $params = [
            'client_id' => $clientId,
            'redirect_uri' => $redirectUri,
            'scope' => 'openid email profile',
            'response_type' => 'code'
        ];

        $Oauth2Url = $baseUrl . '?' . http_build_query($params, '', '&', PHP_QUERY_RFC3986);
        return $Oauth2Url;

You can also share your feedback on Copilot code review. Take the survey.

Comment on lines +389 to +392
if (!$trailingSlash){
$baseUrl = rtrim(Configuration::Instance()->GetKey(ConfigKeys::AUTHENTICATION_OAUTH2_URL_AUTHORIZE), '/');
}
else {
Copy link

Copilot AI Mar 10, 2026

Choose a reason for hiding this comment

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

Code style is inconsistent with the rest of this file: if (!$trailingSlash){ is missing a space before {, and the else is on a new line (elsewhere it’s } else {). Please align the formatting to match the surrounding conventions.

Suggested change
if (!$trailingSlash){
$baseUrl = rtrim(Configuration::Instance()->GetKey(ConfigKeys::AUTHENTICATION_OAUTH2_URL_AUTHORIZE), '/');
}
else {
if (!$trailingSlash) {
$baseUrl = rtrim(Configuration::Instance()->GetKey(ConfigKeys::AUTHENTICATION_OAUTH2_URL_AUTHORIZE), '/');
} else {

Copilot uses AI. Check for mistakes.
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
@PVTGoesen
Copy link
Contributor Author

Hi,
the "Linters/lint" check still fails because of the initial commit name. Should I close this pull request and open a new one?

@JohnVillalovos
Copy link
Collaborator

Hi, the "Linters/lint" check still fails because of the initial commit name. Should I close this pull request and open a new one?

You can do that or you can do an "interactive rebase" and squash it down to one commit.

https://docs.github.com/en/get-started/using-git/using-git-rebase-on-the-command-line

@PVTGoesen
Copy link
Contributor Author

I will open a new pull request

@PVTGoesen PVTGoesen closed this Mar 17, 2026
@PVTGoesen PVTGoesen deleted the OAuth2-trailing-slash branch March 17, 2026 08:29
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.

3 participants