fix: read OIDC Prompt from the Prompt key, not the Ports key (#12557)#12559
Open
DeepDiver1975 wants to merge 1 commit into
Open
fix: read OIDC Prompt from the Prompt key, not the Ports key (#12557)#12559DeepDiver1975 wants to merge 1 commit into
DeepDiver1975 wants to merge 1 commit into
Conversation
|
Thanks for opening this pull request! The maintainers of this repository would appreciate it if you would create a changelog item based on your changes. |
AppConfig::loadOpenIdConfigFromSystemConfig() read the OpenID Connect Prompt value from OidcPortsKey instead of OidcPromptKey. As a result an administrator-provided Prompt in the system configuration was ignored and prompt was instead populated with the raw Ports string (e.g. "8080,8443"), which is not a valid OIDC prompt parameter. Since a valid system OIDC config fully replaces the theme config, the configured prompt was unreachable. Read prompt from OidcPromptKey, and add a regression test that exercises loadOpenIdConfigFromSystemConfig() and asserts prompt, ports and scopes are each sourced from their own key. The method is made public (mirroring configPath) so it is reachable from the test. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Signed-off-by: Thomas Müller <1005065+DeepDiver1975@users.noreply.github.com>
4c072f8 to
8c92873
Compare
modSpike
requested changes
Jun 22, 2026
| * Load the OpenID Connect configuration from a system QSettings instance. | ||
| * @param system the system configuration settings | ||
| * @return An OpenIdConfig object populated from the [OpenIDConnect] section. | ||
| * @internal kept public for testing purposes |
Contributor
There was a problem hiding this comment.
the tests should rely on the original public interfaces only - I do not agree to making any of the internal "helper" functions public as they should never be called by any impl - that is why they are private. This is basic encapsulation.
please refactor your new test to use the appconfig instance only, as that is what is and should continue to be used in real life.
you should be able to essentially do the same thing wrt setting up the test values in a fake system config, then check the values on a resulting appConfig instance.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Fixes #12557.
AppConfig::loadOpenIdConfigFromSystemConfig()read the OpenID ConnectPromptvalue fromOidcPortsKeyinstead ofOidcPromptKey. As a result, an administrator-providedPromptin the system configuration ([OpenIDConnect]section / Windows registry) was ignored, andpromptwas instead populated with the rawPortsstring (e.g."8080,8443"), which is not a valid OIDCpromptparameter. Since a valid system OIDC config fully replaces the theme config, the configured prompt was unreachable.Changes
promptfromOidcPromptKey.testLoadOpenIdConfig) that exercisesloadOpenIdConfigFromSystemConfig()and assertsprompt,ports, andscopesare each sourced from their own key.loadOpenIdConfigFromSystemConfig()public (mirroring the existingconfigPathtesting convention) so it is reachable from the test.Testing
AppConfigTestpasses locally (4 passed, 0 failed). The new test fails before the fix (prompt would equal"8080,8443") and passes after.Note
#12560 backports the same fix to the
7.1branch.🤖 Generated with Claude Code