From e23fba186caf0c244ac7c072900ec5e17c7958ec Mon Sep 17 00:00:00 2001 From: Shawn Hsu Date: Thu, 17 Sep 2026 16:41:26 +0800 Subject: [PATCH] fix(keycloak): add HelperWithoutCredentials for call-time-credential 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 Co-Authored-By: Claude Sonnet 5 --- pkg/keycloak/keycloak.go | 73 +++++++++++++++++++++++++ pkg/keycloak/keycloak_test.go | 100 ++++++++++++++++++++++++++++++++++ 2 files changed, 173 insertions(+) diff --git a/pkg/keycloak/keycloak.go b/pkg/keycloak/keycloak.go index 8f4e924..4a56adf 100644 --- a/pkg/keycloak/keycloak.go +++ b/pkg/keycloak/keycloak.go @@ -92,6 +92,79 @@ func NewHelper(opts ...Option) (*Helper, error) { return h, nil } +/* + * HelperWithoutCredentials is a keycloak client for call patterns like + * CheckLoginUser that take login credentials as call-time parameters rather + * than pre-configured Options. Unlike Helper, it has no LoginAdmin or + * LoginServiceAccount methods, so a caller 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. + */ +type HelperWithoutCredentials struct { + Client + Options +} + +func NewHelperWithoutCredentials(opts ...Option) (*HelperWithoutCredentials, error) { + initedOpts := initOptions(opts) + h := &HelperWithoutCredentials{Options: *initedOpts} + + if h.Options.Scheme == "" { + return nil, fmt.Errorf("keycloak scheme is empty") + } + + if h.Options.Ip == "" { + return nil, fmt.Errorf("keycloak ip is empty") + } + + if h.Options.Port == 0 { + return nil, fmt.Errorf("keycloak port is empty") + } + + if h.Options.Path == "" { + return nil, fmt.Errorf("keycloak path is empty") + } + + if h.Options.Realm == "" { + return nil, fmt.Errorf("keycloak realm is empty") + } + + h.Client = gocloak.NewClient(h.genKeycloakUrl()) + return h, nil +} + +func (h *HelperWithoutCredentials) genKeycloakUrl() string { + u := url.URL{} + u.Scheme = h.Options.Scheme + u.Host = fmt.Sprintf("%s:%d", h.Options.Ip, h.Options.Port) + u.Path = h.Options.Path + return u.String() +} + +func (h *HelperWithoutCredentials) CheckLoginUser(username, password string, client ClientCredentials) (*gocloak.JWT, error) { + if h.Options.TlsInsecureSkipVerify { + h.Client.RestyClient().SetTLSClientConfig(&tls.Config{InsecureSkipVerify: true}) + } + + ctx, cancel := context.WithTimeout(wait.CtxSeconds(10)) + defer cancel() + token, err := h.Client.Login( + ctx, + client.ID, + client.Secret, + h.Options.Realm, + username, + password, + ) + if err != nil { + // %w (not LoginAdmin's %s) so callers can errors.As into gocloak's *APIError + // to tell an invalid-credentials response apart from an unexpected failure. + return nil, fmt.Errorf("keycloak login failed: %w", err) + } + + return token, nil +} + func NewGlobalHelper(opts ...Option) error { var err error once.Do(func() { diff --git a/pkg/keycloak/keycloak_test.go b/pkg/keycloak/keycloak_test.go index 677fd56..0a51137 100644 --- a/pkg/keycloak/keycloak_test.go +++ b/pkg/keycloak/keycloak_test.go @@ -935,6 +935,106 @@ func TestSetKeycloakClient(t *testing.T) { } } +func TestNewHelperWithoutCredentials(t *testing.T) { + tests := []struct { + name string + opts []Option + expectedError string + }{ + { + name: "Should return an error if scheme is empty", + opts: []Option{Ip("keycloak"), Port(80), Path("auth"), Realm("master")}, + expectedError: "keycloak scheme is empty", + }, + { + name: "Should return an error if ip is empty", + opts: []Option{Scheme("http"), Port(80), Path("auth"), Realm("master")}, + expectedError: "keycloak ip is empty", + }, + { + name: "Should return an error if port is empty", + opts: []Option{Scheme("http"), Ip("keycloak"), Path("auth"), Realm("master")}, + expectedError: "keycloak port is empty", + }, + { + name: "Should return an error if path is empty", + opts: []Option{Scheme("http"), Ip("keycloak"), Port(80), Realm("master")}, + expectedError: "keycloak path is empty", + }, + { + name: "Should return an error if realm is empty", + opts: []Option{Scheme("http"), Ip("keycloak"), Port(80), Path("auth")}, + expectedError: "keycloak realm is empty", + }, + { + name: "Should succeed with no credentials set", + opts: []Option{Scheme("http"), Ip("keycloak"), Port(80), Path("auth"), Realm("master")}, + }, + { + name: "Should succeed even when credentials happen to be set", + opts: []Option{Scheme("http"), Ip("keycloak"), Port(80), Path("auth"), Realm("master"), Username("admin"), Password("admin")}, + }, + } + + for _, tc := range tests { + t.Run(tc.name, func(t *testing.T) { + h, err := NewHelperWithoutCredentials(tc.opts...) + + if tc.expectedError == "" { + require.NoError(t, err) + require.NotNil(t, h.Client) + } else { + require.EqualError(t, err, tc.expectedError) + require.Nil(t, h) + } + }) + } +} + +func TestCheckLoginUserWithHelperBuiltWithoutCredentials(t *testing.T) { + tests := []struct { + name string + username string + password string + client ClientCredentials + token *gocloak.JWT + expected *gocloak.JWT + }{ + { + name: "Should let CheckLoginUser use call-time credentials when NewHelperWithoutCredentials was built from network-location options alone", + username: "user", + password: "pass", + client: DefaultAdmin, + token: &gocloak.JWT{AccessToken: "access-token"}, + expected: &gocloak.JWT{AccessToken: "access-token"}, + }, + } + + for _, tc := range tests { + t.Run(tc.name, func(t *testing.T) { + h, err := NewHelperWithoutCredentials( + Scheme("http"), + Ip("keycloak"), + Port(80), + Path("auth"), + Realm("master"), + ) + require.NoError(t, err) + require.NotNil(t, h.Client) + + client := NewMockClient(t) + client.On("Login", mock.Anything, tc.client.ID, tc.client.Secret, "master", tc.username, tc.password). + Return(tc.token, nil) + h.Client = client + + got, err := h.CheckLoginUser(tc.username, tc.password, tc.client) + + require.NoError(t, err) + require.Equal(t, tc.expected, got) + }) + } +} + func TestHelperExecuteActionsEmail(t *testing.T) { errBoom := errors.New("boom")