Skip to content

Add HelperWithoutCredentials for call-time-credential Keycloak login patterns - #105

Merged
github-actions[bot] merged 1 commit into
developfrom
shawn.hsu/issue-1391/fix-keycloak-credential-check
Sep 17, 2026
Merged

github-actions[bot] merged 1 commit into
developfrom
shawn.hsu/issue-1391/fix-keycloak-credential-check

Conversation

@ambersun1234-bigstack

@ambersun1234-bigstack ambersun1234-bigstack commented Sep 17, 2026

Copy link
Copy Markdown
Contributor

What type of PR is this?

/kind bug

What this PR does / why we need it

Helper.SetKeycloakClient (called internally by NewHelper) requires Options.Username+Options.Password or Options.ClientID+Options.ClientSecret to already be set, or it returns an error before ever building the underlying gocloak.Client. That's the right precondition for LoginAdmin and LoginServiceAccount, which both read those same Options fields at call time.

It's the wrong precondition for Helper.CheckLoginUser(username, password, client ClientCredentials), whose whole point is to take login credentials as call-time parameters instead of pre-configured Options. A caller that builds a Helper via NewHelper with only network-location options (scheme/ip/port/path/realm) and supplies credentials later via CheckLoginUser's own client argument could never get past NewHelper, since Options never carried any credentials in that call pattern.

This PR adds HelperWithoutCredentials as a fully independent type for that call pattern, with its own constructor (NewHelperWithoutCredentials) and its own CheckLoginUser method, rather than reusing Helper or threading a bool through SetKeycloakClient. Helper/NewHelper/SetKeycloakClient stay byte-for-byte unchanged. Because HelperWithoutCredentials has no LoginAdmin or LoginServiceAccount methods, a caller that builds one can't accidentally try to log in with Options-level credentials that were never required to construct it — that misuse is a compile error here instead of a runtime one, which a shared struct (with credential-requiring methods still attached) couldn't have prevented.

Which issue(s) this PR fixes

Fixes bigstack-oss/cubecmp#1391

Special notes for your reviewer

This is the root-cause fix for cubecmp's self-service change-password endpoint returning 500 on every call regardless of whether the submitted password was correct. cubecmp's own dependency bump + switching connection.go's CheckLoginUser wrapper to use NewHelperWithoutCredentials + end-to-end re-verification on dev-cmp will follow in a separate cubecmp PR once this is merged and released.

Additional documentation


@ambersun1234-bigstack ambersun1234-bigstack self-assigned this Sep 17, 2026
@ambersun1234-bigstack
ambersun1234-bigstack force-pushed the shawn.hsu/issue-1391/fix-keycloak-credential-check branch from a4caecf to 23e9787 Compare September 17, 2026 09:06
@ambersun1234-bigstack ambersun1234-bigstack changed the title Let SetKeycloakClient build a client from network-location options alone Add NewHelperWithoutCredentials for call-time-credential Keycloak login patterns Sep 17, 2026
…patterns

Helper.SetKeycloakClient (called internally by NewHelper) requires
Options.Username/Password or Options.ClientID/ClientSecret to already be
set, or it returns an error before ever building the underlying
gocloak.Client. That's the right precondition for LoginAdmin and
LoginServiceAccount, which both read those same Options fields at call
time.

It's the wrong precondition for Helper.CheckLoginUser(username, password,
client ClientCredentials), whose whole point is to take login credentials
as call-time parameters instead of pre-configured Options. A caller that
builds a Helper via NewHelper with only network-location options
(scheme/ip/port/path/realm) and supplies credentials later via
CheckLoginUser's own client argument could never get past NewHelper,
since Options never carried any credentials in that call pattern.

Add HelperWithoutCredentials as a fully independent type for that call
pattern, with its own constructor (NewHelperWithoutCredentials) and its
own CheckLoginUser method, rather than reusing Helper or threading a bool
through SetKeycloakClient. Helper/NewHelper/SetKeycloakClient stay
byte-for-byte unchanged. Because HelperWithoutCredentials has no
LoginAdmin or LoginServiceAccount methods, a caller that builds one can't
accidentally try to log in with Options-level credentials that were never
required to construct it -- that misuse is a compile error here instead
of a runtime one, which a shared struct (with credential-requiring
methods still attached) couldn't have prevented.

This is the root-cause fix for cubecmp's self-service change-password
endpoint returning 500 on every call regardless of whether the submitted
password was correct (bigstack-oss/cubecmp#1391).

Signed-off-by: Shawn Hsu <shawn.hsu@bigstack.co>
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
@ambersun1234-bigstack
ambersun1234-bigstack force-pushed the shawn.hsu/issue-1391/fix-keycloak-credential-check branch from 23e9787 to e23fba1 Compare September 17, 2026 09:11
@ambersun1234-bigstack ambersun1234-bigstack changed the title Add NewHelperWithoutCredentials for call-time-credential Keycloak login patterns Add HelperWithoutCredentials for call-time-credential Keycloak login patterns Sep 17, 2026

@lizzy-liang-bigstack lizzy-liang-bigstack left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Approved to keep things moving! I haven't done a deep dive into the logic since it's outside my domain, so please make sure to get a final check from the domain expert. Just approving to unblock the pipeline for now 👍🏻

@ambersun1234-bigstack ambersun1234-bigstack added the done Merge the pull request label Sep 17, 2026
@github-actions
github-actions Bot merged commit e23fba1 into develop Sep 17, 2026
6 checks passed
@github-actions
github-actions Bot deleted the shawn.hsu/issue-1391/fix-keycloak-credential-check branch September 17, 2026 14:49
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

done Merge the pull request

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants