Skip to content

Commit aeffe22

Browse files
Crash--Crash--
committed
feat: Hint, use old_domain, then OIDC, then nothing
1 parent 22bd12e commit aeffe22

File tree

2 files changed

+118
-23
lines changed

2 files changed

+118
-23
lines changed

‎web/oidc/oidc.go‎

Lines changed: 28 additions & 13 deletions
Original file line numberDiff line numberDiff line change
@@ -51,7 +51,7 @@ func Start(c echo.Context) error {
5151
inst.Logger().WithNamespace("oidc").Infof("Start error: %s", err)
5252
return renderError(c, nil, http.StatusNotFound, "Sorry, the context was not found.")
5353
}
54-
u, err := makeStartURL(inst.Domain, c.QueryParam("redirect"), c.QueryParam("confirm_state"), "", conf)
54+
u, err := makeStartURL(inst, inst.Domain, c.QueryParam("redirect"), c.QueryParam("confirm_state"), "", conf)
5555
if err != nil {
5656
return renderError(c, nil, http.StatusNotFound, "Sorry, the server is not configured for OpenID Connect.")
5757
}
@@ -66,7 +66,7 @@ func StartFranceConnect(c echo.Context) error {
6666
inst.Logger().WithNamespace("oidc").Infof("StartFranceConnect error: %s", err)
6767
return renderError(c, nil, http.StatusNotFound, "Sorry, the context was not found.")
6868
}
69-
u, err := makeStartURL(inst.Domain, c.QueryParam("redirect"), c.QueryParam("confirm_state"), "", conf)
69+
u, err := makeStartURL(inst, inst.Domain, c.QueryParam("redirect"), c.QueryParam("confirm_state"), "", conf)
7070
if err != nil {
7171
return renderError(c, nil, http.StatusNotFound, "Sorry, the server is not configured for OpenID Connect.")
7272
}
@@ -81,7 +81,7 @@ func Sharing(c echo.Context) error {
8181
inst.Logger().WithNamespace("oidc").Infof("Start error: %s", err)
8282
return renderError(c, nil, http.StatusNotFound, "Sorry, the context was not found.")
8383
}
84-
u, err := makeSharingStartURL(inst.Domain, c.QueryParam("sharingID"), c.QueryParam("state"), conf, inst.ContextName)
84+
u, err := makeSharingStartURL(inst, c.QueryParam("sharingID"), c.QueryParam("state"), conf, inst.ContextName)
8585
if err != nil {
8686
return renderError(c, nil, http.StatusNotFound, "Sorry, the server is not configured for OpenID Connect.")
8787
}
@@ -96,7 +96,7 @@ func SharingPublic(c echo.Context) error {
9696
inst.Logger().WithNamespace("oidc").Infof("Start error: %s", err)
9797
return renderError(c, nil, http.StatusNotFound, "Sorry, the server is not configured for OpenID Connect.")
9898
}
99-
u, err := makeSharingStartURL(inst.Domain, c.QueryParam("sharingID"), c.QueryParam("state"), conf, contextName)
99+
u, err := makeSharingStartURL(inst, c.QueryParam("sharingID"), c.QueryParam("state"), conf, contextName)
100100
if err != nil {
101101
return renderError(c, nil, http.StatusNotFound, "Sorry, the server is not configured for OpenID Connect.")
102102
}
@@ -133,7 +133,7 @@ func BitwardenStart(c echo.Context) error {
133133
if err != nil {
134134
return renderError(c, nil, http.StatusNotFound, "Sorry, the context was not found.")
135135
}
136-
u, err := makeStartURL("", redirectURI, "", contextName, conf)
136+
u, err := makeStartURL(nil, "", redirectURI, "", contextName, conf)
137137
if err != nil {
138138
return renderError(c, nil, http.StatusNotFound, "Sorry, the server is not configured for OpenID Connect.")
139139
}
@@ -852,7 +852,20 @@ func getFranceConnectConfig(context string) (*Config, error) {
852852
return config, nil
853853
}
854854

855-
func makeStartURL(domain, redirect, confirm, oidcContext string, conf *Config) (string, error) {
855+
func getLoginHint(inst *instance.Instance) string {
856+
if inst == nil {
857+
return ""
858+
}
859+
if inst.OldDomain != "" {
860+
return inst.OldDomain
861+
}
862+
if inst.OIDCID != "" {
863+
return inst.OIDCID
864+
}
865+
return ""
866+
}
867+
868+
func makeStartURL(inst *instance.Instance, domain, redirect, confirm, oidcContext string, conf *Config) (string, error) {
856869
u, err := url.Parse(conf.AuthorizeURL)
857870
if err != nil {
858871
return "", err
@@ -868,8 +881,9 @@ func makeStartURL(domain, redirect, confirm, oidcContext string, conf *Config) (
868881
vv.Add("redirect_uri", conf.RedirectURI)
869882
vv.Add("state", state.id)
870883
vv.Add("nonce", state.Nonce)
871-
if domain != "" {
872-
vv.Add("login_hint", domain)
884+
loginHint := getLoginHint(inst)
885+
if loginHint != "" {
886+
vv.Add("login_hint", loginHint)
873887
}
874888
if conf.Provider == FranceConnectProvider {
875889
vv.Add("acr_values", "eidas1")
@@ -878,12 +892,12 @@ func makeStartURL(domain, redirect, confirm, oidcContext string, conf *Config) (
878892
return u.String(), nil
879893
}
880894

881-
func makeSharingStartURL(domain, sharingID, sharingState string, conf *Config, contextName string) (string, error) {
895+
func makeSharingStartURL(inst *instance.Instance, sharingID, sharingState string, conf *Config, contextName string) (string, error) {
882896
u, err := url.Parse(conf.AuthorizeURL)
883897
if err != nil {
884898
return "", err
885899
}
886-
state := newSharingStateHolder(domain, sharingID, sharingState, conf.Provider, contextName)
900+
state := newSharingStateHolder(inst.Domain, sharingID, sharingState, conf.Provider, contextName)
887901
if err = getStorage().Add(state); err != nil {
888902
return "", err
889903
}
@@ -894,8 +908,9 @@ func makeSharingStartURL(domain, sharingID, sharingState string, conf *Config, c
894908
vv.Add("redirect_uri", conf.RedirectURI)
895909
vv.Add("state", state.id)
896910
vv.Add("nonce", state.Nonce)
897-
if domain != "" {
898-
vv.Add("login_hint", domain)
911+
loginHint := getLoginHint(inst)
912+
if loginHint != "" {
913+
vv.Add("login_hint", loginHint)
899914
}
900915
if conf.Provider == FranceConnectProvider {
901916
vv.Add("acr_values", "eidas1")
@@ -1343,7 +1358,7 @@ func LoginDomainHandler(c echo.Context, contextName string) error {
13431358
if err != nil {
13441359
return renderError(c, nil, http.StatusNotFound, "Sorry, the context was not found.")
13451360
}
1346-
u, err := makeStartURL(r.Host, "", "", "", conf)
1361+
u, err := makeStartURL(nil, r.Host, "", "", "", conf)
13471362
if err != nil {
13481363
return renderError(c, nil, http.StatusNotFound, "Sorry, the server is not configured for OpenID Connect.")
13491364
}

‎web/oidc/oidc_test.go‎

Lines changed: 90 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -153,10 +153,20 @@ func TestOidc(t *testing.T) {
153153
assert.NotNil(t, redirectURL.Query().Get("response_type"))
154154
assert.NotNil(t, redirectURL.Query().Get("state"))
155155
assert.NotNil(t, redirectURL.Query().Get("scope"))
156-
// Verify login_hint is present with the instance domain
156+
// Verify login_hint logic: old_domain > OIDCID > nothing
157157
loginHint := redirectURL.Query().Get("login_hint")
158-
assert.NotEmpty(t, loginHint, "login_hint should be present in redirect URL")
159-
assert.Equal(t, testInstance.Domain, loginHint, "login_hint should contain the instance domain")
158+
expectedLoginHint := ""
159+
if testInstance.OldDomain != "" {
160+
expectedLoginHint = testInstance.OldDomain
161+
} else if testInstance.OIDCID != "" {
162+
expectedLoginHint = testInstance.OIDCID
163+
}
164+
if expectedLoginHint != "" {
165+
assert.NotEmpty(t, loginHint, "login_hint should be present in redirect URL")
166+
assert.Equal(t, expectedLoginHint, loginHint, "login_hint should match old_domain or OIDCID")
167+
} else {
168+
assert.Empty(t, loginHint, "login_hint should not be present when neither old_domain nor OIDCID is set")
169+
}
160170
})
161171

162172
// Get the login page, assert we have an error if state is missing
@@ -298,13 +308,49 @@ func TestOidc(t *testing.T) {
298308
require.Equal(t, "delegated-session-123", storedClient.OIDCSessionID)
299309
})
300310

301-
t.Run("LoginHintWithDomain", func(t *testing.T) {
311+
t.Run("LoginHintWithOIDCID", func(t *testing.T) {
302312
e := testutils.CreateTestClient(t, ts.URL)
303313

304314
onboardingFinished := true
305-
_ = lifecycle.Patch(testInstance, &lifecycle.Options{OnboardingFinished: &onboardingFinished})
315+
oidcID := "user-oidc-sub-123"
316+
// Set OIDCID without old_domain to test OIDCID priority
317+
_ = lifecycle.Patch(testInstance, &lifecycle.Options{
318+
OnboardingFinished: &onboardingFinished,
319+
OIDCID: oidcID,
320+
})
321+
322+
// Test that login_hint uses OIDCID when old_domain is not present
323+
u := e.GET("/oidc/start").
324+
WithHost(testInstance.Domain).
325+
WithRedirectPolicy(httpexpect.DontFollowRedirects).
326+
Expect().Status(303).
327+
Header("Location").Raw()
328+
329+
redirectURL, err := url.Parse(u)
330+
require.NoError(t, err)
331+
332+
loginHint := redirectURL.Query().Get("login_hint")
333+
// If old_domain exists from previous test, it should take priority
334+
// Otherwise, OIDCID should be used
335+
if testInstance.OldDomain != "" {
336+
assert.Equal(t, testInstance.OldDomain, loginHint, "login_hint should use old_domain when present (priority)")
337+
} else {
338+
assert.NotEmpty(t, loginHint, "login_hint should be present when OIDCID is set")
339+
assert.Equal(t, oidcID, loginHint, "login_hint should use OIDCID when old_domain is not present")
340+
}
341+
})
342+
343+
t.Run("LoginHintWithOldDomain", func(t *testing.T) {
344+
e := testutils.CreateTestClient(t, ts.URL)
345+
346+
onboardingFinished := true
347+
oldDomain := "old.example.com"
348+
_ = lifecycle.Patch(testInstance, &lifecycle.Options{
349+
OnboardingFinished: &onboardingFinished,
350+
OldDomain: oldDomain,
351+
})
306352

307-
// Test that login_hint is present when domain is provided
353+
// Test that login_hint uses old_domain when present (highest priority)
308354
u := e.GET("/oidc/start").
309355
WithHost(testInstance.Domain).
310356
WithRedirectPolicy(httpexpect.DontFollowRedirects).
@@ -315,15 +361,19 @@ func TestOidc(t *testing.T) {
315361
require.NoError(t, err)
316362

317363
loginHint := redirectURL.Query().Get("login_hint")
318-
assert.NotEmpty(t, loginHint, "login_hint should be present when domain is provided")
319-
assert.Equal(t, testInstance.Domain, loginHint, "login_hint should match the instance domain")
364+
assert.NotEmpty(t, loginHint, "login_hint should be present when old_domain is set")
365+
assert.Equal(t, oldDomain, loginHint, "login_hint should use old_domain when present (highest priority)")
320366
})
321367

322368
t.Run("LoginHintWithFranceConnect", func(t *testing.T) {
323369
e := testutils.CreateTestClient(t, ts.URL)
324370

325371
onboardingFinished := true
326-
_ = lifecycle.Patch(testInstance, &lifecycle.Options{OnboardingFinished: &onboardingFinished})
372+
oldDomain := "old.franceconnect.example.com"
373+
_ = lifecycle.Patch(testInstance, &lifecycle.Options{
374+
OnboardingFinished: &onboardingFinished,
375+
OldDomain: oldDomain,
376+
})
327377

328378
// Test that login_hint is present for FranceConnect flow
329379
u := e.GET("/oidc/franceconnect").
@@ -337,7 +387,37 @@ func TestOidc(t *testing.T) {
337387

338388
loginHint := redirectURL.Query().Get("login_hint")
339389
assert.NotEmpty(t, loginHint, "login_hint should be present for FranceConnect flow")
340-
assert.Equal(t, testInstance.Domain, loginHint, "login_hint should match the instance domain")
390+
assert.Equal(t, oldDomain, loginHint, "login_hint should use old_domain for FranceConnect")
391+
})
392+
393+
t.Run("LoginHintWithoutOldDomainOrOIDCID", func(t *testing.T) {
394+
e := testutils.CreateTestClient(t, ts.URL)
395+
396+
onboardingFinished := true
397+
_ = lifecycle.Patch(testInstance, &lifecycle.Options{
398+
OnboardingFinished: &onboardingFinished,
399+
})
400+
401+
// Test that login_hint behavior depends on current instance state
402+
// If old_domain or OIDCID exist from previous tests, they will be used
403+
// Otherwise, no login_hint should be present
404+
u := e.GET("/oidc/start").
405+
WithHost(testInstance.Domain).
406+
WithRedirectPolicy(httpexpect.DontFollowRedirects).
407+
Expect().Status(303).
408+
Header("Location").Raw()
409+
410+
redirectURL, err := url.Parse(u)
411+
require.NoError(t, err)
412+
413+
loginHint := redirectURL.Query().Get("login_hint")
414+
// Check current state - if neither old_domain nor OIDCID is set, login_hint should be empty
415+
if testInstance.OldDomain == "" && testInstance.OIDCID == "" {
416+
assert.Empty(t, loginHint, "login_hint should not be present when neither old_domain nor OIDCID is set")
417+
} else {
418+
// If one of them exists, it should be used (this tests the priority logic)
419+
assert.NotEmpty(t, loginHint, "login_hint should be present if old_domain or OIDCID is set")
420+
}
341421
})
342422

343423
t.Run("LoginHintWithoutDomain", func(t *testing.T) {

0 commit comments

Comments
 (0)