Skip to content

Commit 6f39b2b

Browse files
authored
fix: redirect to user details page when session is valid (#769)
closes #768
2 parents ccafe4d + b6a2322 commit 6f39b2b

File tree

6 files changed

+101
-49
lines changed

6 files changed

+101
-49
lines changed

internal/misc/http/helpers.go

Lines changed: 21 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -22,6 +22,27 @@ func CookiesToString(cookies []*http.Cookie) string {
2222
return strings.Join(ret, "; ")
2323
}
2424

25+
func FilterCookies(cookies []*http.Cookie, exclude ...string) []*http.Cookie {
26+
if len(exclude) == 0 {
27+
return cookies
28+
}
29+
30+
ret := make([]*http.Cookie, 0)
31+
32+
diff := make(map[any]bool)
33+
for _, deniedCookieName := range exclude {
34+
diff[deniedCookieName] = true
35+
}
36+
37+
for _, cookie := range cookies {
38+
if _, ok := diff[cookie.Name]; !ok {
39+
ret = append(ret, cookie)
40+
}
41+
}
42+
43+
return ret
44+
}
45+
2546
func GetUserClaims(i kratos_client.Identity, cr hydra_client.OAuth2ConsentRequest) map[string]interface{} {
2647
ret := make(map[string]interface{})
2748
// Export the user claims and filter them based on the requested scopes

pkg/kratos/handlers.go

Lines changed: 55 additions & 28 deletions
Original file line numberDiff line numberDiff line change
@@ -5,12 +5,14 @@ import (
55
"crypto/md5"
66
"encoding/base64"
77
"encoding/json"
8+
"errors"
89
"fmt"
910
"net/http"
1011
"net/url"
1112
"strconv"
1213
"time"
1314

15+
httpHelpers "github.com/canonical/identity-platform-login-ui/internal/misc/http"
1416
"github.com/go-chi/chi/v5"
1517
client "github.com/ory/kratos-client-go/v25"
1618

@@ -25,6 +27,7 @@ const RegenerateBackupCodesError = "regenerate_backup_codes"
2527
const SESSION_REFRESH_REQUIRED = "session_refresh_required"
2628
const KRATOS_SESSION_COOKIE_NAME = "ory_kratos_session"
2729
const LOGIN_UI_STATE_COOKIE = "login_ui_state"
30+
const SECURITY_CSRF_VIOLATION_ERROR = "security_csrf_violation"
2831

2932
type API struct {
3033
mfaEnabled bool
@@ -38,6 +41,12 @@ type API struct {
3841
logger logging.LoggerInterface
3942
}
4043

44+
type KratosErrorResponse struct {
45+
Error *client.GenericError `json:"error,omitempty"`
46+
RedirectBrowserTo string `json:"redirect_browser_to,omitempty"`
47+
RedirectTo string `json:"redirect_to,omitempty"`
48+
}
49+
4150
func (a *API) RegisterEndpoints(mux *chi.Mux) {
4251
mux.Post("/api/kratos/self-service/login", a.handleUpdateFlow)
4352
mux.Post("/api/kratos/self-service/login/id-first", a.handleUpdateIdentifierFirstFlow)
@@ -138,10 +147,17 @@ func (a *API) handleCreateFlow(w http.ResponseWriter, r *http.Request) {
138147
a.cookieManager.ClearStateCookie(w)
139148
}
140149
} else {
141-
response, cookies, err = a.handleCreateFlowNewSession(r, aal, returnTo, loginChallenge, refresh)
150+
response, cookies, err = a.handleCreateFlowNewSession(r, aal, returnTo, loginChallenge, refresh, session)
142151
}
143152

144153
if err != nil {
154+
// Propagate the KratosErrorResponse so frontend can handle it
155+
if kratosError, ok := parseGenericError(err); ok {
156+
w.WriteHeader(http.StatusBadRequest)
157+
_ = json.NewEncoder(w).Encode(kratosError)
158+
return
159+
}
160+
145161
a.logger.Errorf(err.Error())
146162
http.Error(w, err.Error(), http.StatusInternalServerError)
147163
return
@@ -167,15 +183,17 @@ func (a *API) handleCreateFlow(w http.ResponseWriter, r *http.Request) {
167183
_ = json.NewEncoder(w).Encode(response)
168184
}
169185

170-
func (a *API) handleCreateFlowNewSession(r *http.Request, aal, returnTo, loginChallenge string, refresh bool) (*client.LoginFlow, []*http.Cookie, error) {
186+
func (a *API) handleCreateFlowNewSession(r *http.Request, aal, returnTo, loginChallenge string, refresh bool, session *client.Session) (*client.LoginFlow, []*http.Cookie, error) {
171187
// redirect user to this endpoint with the login_challenge after login
172188
// see https://github.com/ory/kratos/issues/3052
173189

174190
cookies := r.Cookies()
175191

176-
// clear cookies if not aal2 and not a refresh request
177-
if !refresh && aal != "aal2" {
178-
cookies = filterCookies(cookies, KRATOS_SESSION_COOKIE_NAME)
192+
// clear cookies if not a refresh request, not aal2, and either:
193+
// - it's a hydra login (with loginChallenge)
194+
// - it's a kratos local login without a session
195+
if !refresh && aal != "aal2" && (session == nil || loginChallenge != "") {
196+
cookies = httpHelpers.FilterCookies(cookies, KRATOS_SESSION_COOKIE_NAME)
179197
}
180198

181199
flow, cookies, err := a.service.CreateBrowserLoginFlow(
@@ -187,7 +205,7 @@ func (a *API) handleCreateFlowNewSession(r *http.Request, aal, returnTo, loginCh
187205
cookies,
188206
)
189207
if err != nil {
190-
return nil, nil, fmt.Errorf("failed to create login flow, err: %v", err)
208+
return nil, nil, fmt.Errorf("failed to create login flow, err: %w", err)
191209
}
192210

193211
flow, err = a.service.FilterFlowProviderList(r.Context(), flow)
@@ -201,12 +219,26 @@ func (a *API) handleCreateFlowNewSession(r *http.Request, aal, returnTo, loginCh
201219
func (a *API) handleCreateFlowWithSession(r *http.Request, session *client.Session, loginChallenge string) (*BrowserLocationChangeRequired, []*http.Cookie, error) {
202220
response, cookies, err := a.service.AcceptLoginRequest(r.Context(), session, loginChallenge)
203221
if err != nil {
204-
return nil, nil, fmt.Errorf("failed to accept login request: %v", err)
222+
return nil, nil, fmt.Errorf("failed to accept login request: %w", err)
205223
}
206224

207225
return response, cookies, nil
208226
}
209227

228+
func parseGenericError(err error) (*KratosErrorResponse, bool) {
229+
var apiErr *client.GenericOpenAPIError
230+
if !errors.As(err, &apiErr) {
231+
return nil, false
232+
}
233+
234+
var resp KratosErrorResponse
235+
if err := json.Unmarshal(apiErr.Body(), &resp); err != nil {
236+
return nil, false
237+
}
238+
239+
return &resp, true
240+
}
241+
210242
func (a *API) returnToUrl(loginChallenge string) (string, error) {
211243
returnTo, err := url.JoinPath(a.baseURL, "/ui/login")
212244
if err != nil {
@@ -244,6 +276,15 @@ func (a *API) handleGetLoginFlow(w http.ResponseWriter, r *http.Request) {
244276

245277
flow, cookies, err := a.service.GetLoginFlow(r.Context(), flowId, r.Cookies())
246278
if err != nil {
279+
if kratosError, ok := parseGenericError(err); ok {
280+
if kratosError.Error.GetId() == SECURITY_CSRF_VIOLATION_ERROR {
281+
a.deleteKratosSession(w)
282+
}
283+
w.WriteHeader(http.StatusBadRequest)
284+
_ = json.NewEncoder(w).Encode(kratosError)
285+
return
286+
}
287+
247288
a.logger.Errorf("Error when getting login flow: %v\n", err)
248289
http.Error(w, "Failed to get login flow", http.StatusInternalServerError)
249290
return
@@ -298,12 +339,12 @@ func (a *API) handleGetRegistrationFlow(w http.ResponseWriter, r *http.Request)
298339
func (a *API) handleUpdateRegistrationFlow(w http.ResponseWriter, r *http.Request) {
299340
q := r.URL.Query()
300341
flowId := q.Get("flow")
301-
if flowId == "" {
302-
a.logger.Errorf("ID parameter not present")
303-
w.WriteHeader(http.StatusBadRequest)
304-
_ = json.NewEncoder(w).Encode("ID parameter not present")
305-
return
306-
}
342+
if flowId == "" {
343+
a.logger.Errorf("ID parameter not present")
344+
w.WriteHeader(http.StatusBadRequest)
345+
_ = json.NewEncoder(w).Encode("ID parameter not present")
346+
return
347+
}
307348

308349
body, err := a.service.ParseRegistrationFlowMethodBody(r)
309350
if err != nil {
@@ -1056,25 +1097,11 @@ func NewAPI(
10561097
}
10571098

10581099
func setCookies(w http.ResponseWriter, cookies []*http.Cookie, exclude ...string) {
1059-
for _, c := range filterCookies(cookies, exclude...) {
1100+
for _, c := range httpHelpers.FilterCookies(cookies, exclude...) {
10601101
http.SetCookie(w, c)
10611102
}
10621103
}
10631104

1064-
func filterCookies(cookies []*http.Cookie, exclude ...string) []*http.Cookie {
1065-
ret := []*http.Cookie{}
1066-
l1:
1067-
for _, c := range cookies {
1068-
for _, n := range exclude {
1069-
if c.Name == n {
1070-
continue l1
1071-
}
1072-
}
1073-
ret = append(ret, c)
1074-
}
1075-
return ret
1076-
}
1077-
10781105
func hash(plain string) string {
10791106
h := md5.New()
10801107
h.Write([]byte(plain))

pkg/kratos/service.go

Lines changed: 3 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -1112,6 +1112,8 @@ func (s *Service) ParseIdentifierFirstLoginFlowMethodBody(r *http.Request) (*kCl
11121112
if err := parseBody(r.Body, &body); err != nil {
11131113
return nil, cookies, err
11141114
}
1115+
1116+
cookies = httpHelpers.FilterCookies(cookies, KRATOS_SESSION_COOKIE_NAME)
11151117
return &body, cookies, nil
11161118
}
11171119

@@ -1190,13 +1192,7 @@ func (s *Service) ParseLoginFlowMethodBody(r *http.Request) (*kClient.UpdateLogi
11901192

11911193
// Remove session cookie if this is a 1FA method
11921194
if s.is1FAMethod(methodPayload.Method) {
1193-
filtered := make([]*http.Cookie, 0)
1194-
for _, c := range cookies {
1195-
if c.Name != KRATOS_SESSION_COOKIE_NAME {
1196-
filtered = append(filtered, c)
1197-
}
1198-
}
1199-
cookies = filtered
1195+
cookies = httpHelpers.FilterCookies(cookies, KRATOS_SESSION_COOKIE_NAME)
12001196
}
12011197

12021198
return &ret, cookies, nil

ui/tests/reset-password.spec.ts

Lines changed: 11 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -7,7 +7,7 @@ import { confirmMailCode, enterNewPassword } from "./helpers/password";
77

88
const USER_PASSWORD_NEW = "abcABC123!!!";
99

10-
test("reset password from oidc app", async ({ context, page }) => {
10+
test("reset password from oidc app", async ({ browser, context, page }) => {
1111
resetIdentities();
1212
await startNewAuthFlow(page);
1313

@@ -24,13 +24,17 @@ test("reset password from oidc app", async ({ context, page }) => {
2424
await expect(page.getByText("Account setup complete")).toBeVisible();
2525
await finishAuthFlow(page);
2626

27-
await startNewAuthFlow(page);
27+
// Start login in a new context as user is already authenticated within the current context
28+
const newContext = await browser.newContext();
29+
const newPage = await newContext.newPage();
2830

29-
await userPassLogin(page, USER_EMAIL, USER_PASSWORD);
30-
await expect(page.getByText("Incorrect username or password")).toBeVisible();
31+
await startNewAuthFlow(newPage);
3132

32-
await userPassLogin(page, USER_EMAIL, USER_PASSWORD_NEW);
33-
await enterTotpCode(page, totpSetupKey);
33+
await userPassLogin(newPage, USER_EMAIL, USER_PASSWORD);
34+
await expect(newPage.getByText("Incorrect username or password")).toBeVisible();
3435

35-
await finishAuthFlow(page);
36+
await userPassLogin(newPage, USER_EMAIL, USER_PASSWORD_NEW);
37+
await enterTotpCode(newPage, totpSetupKey);
38+
39+
await finishAuthFlow(newPage);
3640
});

ui/tests/use-backup-codes.spec.ts

Lines changed: 10 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -5,7 +5,7 @@ import { resetIdentities } from "./helpers/kratosIdentities";
55
import { userPassLogin } from "./helpers/login";
66
import { clickButton, verifyBackupCode } from "./helpers/backupCode";
77

8-
test("backup recovery code setup and usage", async ({ context, page }) => {
8+
test("backup recovery code setup and usage", async ({ browser, context, page }) => {
99
resetIdentities();
1010
await startNewAuthFlow(page);
1111
await userPassLogin(page);
@@ -25,11 +25,15 @@ test("backup recovery code setup and usage", async ({ context, page }) => {
2525

2626
await expect(page.getByText("Account setup complete")).toBeVisible();
2727

28-
await startNewAuthFlow(page);
29-
await userPassLogin(page);
28+
// Start login in a new context as user is already authenticated within the current context
29+
const newContext = await browser.newContext();
30+
const newPage = await newContext.newPage();
3031

31-
await clickButton(page, "Use backup code instead");
32-
await verifyBackupCode(page, backupCode);
32+
await startNewAuthFlow(newPage);
33+
await userPassLogin(newPage);
3334

34-
await finishAuthFlow(page);
35+
await clickButton(newPage, "Use backup code instead");
36+
await verifyBackupCode(newPage, backupCode);
37+
38+
await finishAuthFlow(newPage);
3539
});

ui/util/handleFlowError.ts

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -33,7 +33,7 @@ export const handleFlowError =
3333
return;
3434
case "session_already_available":
3535
// User is already signed in, let's redirect them to settings
36-
window.location.href = "./reset_password";
36+
window.location.href = "./manage_details";
3737
return;
3838
case "session_refresh_required":
3939
// We need to re-authenticate to perform this action

0 commit comments

Comments
 (0)