From dc88e6bf92e276583a8a9abe93bfb8d6e4f18368 Mon Sep 17 00:00:00 2001 From: natalia Date: Wed, 10 Jul 2024 13:08:53 +0200 Subject: [PATCH 1/4] feat: ui support for mfa flows > > Co-authored-by: David Edler Co-authored-by: Natalia Nowakowska --- ui/components/Flow.tsx | 2 +- ui/components/NodeInputSubmit.tsx | 6 +- ui/components/NodeInputText.tsx | 8 +- ui/pages/consent.tsx | 6 +- ui/pages/error.tsx | 9 ++- ui/pages/login.tsx | 24 ++++-- ui/pages/reset_complete.tsx | 13 ---- ui/pages/reset_email.tsx | 10 +-- ui/pages/reset_password.tsx | 36 +++++++-- ui/pages/setup_secure.tsx | 125 +++++++++++++++++++++++++++--- ui/util/handleFlowError.ts | 18 ++--- ui/util/replaceAuthLabel.ts | 30 +++++++ 12 files changed, 226 insertions(+), 61 deletions(-) delete mode 100644 ui/pages/reset_complete.tsx create mode 100644 ui/util/replaceAuthLabel.ts diff --git a/ui/components/Flow.tsx b/ui/components/Flow.tsx index 08e352923..bc8dd59f1 100644 --- a/ui/components/Flow.tsx +++ b/ui/components/Flow.tsx @@ -21,7 +21,7 @@ export type Values = Partial< | UpdateVerificationFlowBody >; -export type Methods = "oidc" | "password" | "code" | "link"; +export type Methods = "oidc" | "password" | "code" | "totp"; export interface Props { // The flow diff --git a/ui/components/NodeInputSubmit.tsx b/ui/components/NodeInputSubmit.tsx index adcdd3dee..dfa6ed66b 100644 --- a/ui/components/NodeInputSubmit.tsx +++ b/ui/components/NodeInputSubmit.tsx @@ -37,7 +37,11 @@ export const NodeInputSubmit: FC = ({ return ( + + {pwChanged === "success" && ( + + Password was changed successfully + + )} + {flow ? : } ); }; diff --git a/ui/util/handleFlowError.ts b/ui/util/handleFlowError.ts index 005f50988..5b4f2c9ab 100644 --- a/ui/util/handleFlowError.ts +++ b/ui/util/handleFlowError.ts @@ -1,5 +1,4 @@ import { AxiosError } from "axios"; -import { NextRouter } from "next/router"; import { Dispatch, SetStateAction } from "react"; import { toast } from "react-toastify"; @@ -13,7 +12,6 @@ interface KratosErrorResponse { // deal with errors coming from initializing a flow. export const handleFlowError = ( - router: NextRouter, flowType: | "login" | "registration" @@ -29,8 +27,8 @@ export const handleFlowError = window.location.href = err.response.data.redirect_browser_to; return; case "session_already_available": - // User is already signed in, let's redirect them home! - await router.push("/"); + // User is already signed in, let's redirect them to settings + window.location.href = "./reset_password"; return; case "session_refresh_required": // We need to re-authenticate to perform this action @@ -39,13 +37,13 @@ export const handleFlowError = case "session_inactive": // No session found, redirect the user to sign in resetFlow(undefined); - await router.push("/login"); + window.location.href = "./login"; return; case "self_service_flow_return_to_forbidden": // The flow expired, let's request a new one. toast.error("The return_to address is not allowed."); resetFlow(undefined); - await router.push("/" + flowType); + window.location.href = "./" + flowType; return; case "self_service_flow_expired": // The flow expired, let's request a new one. @@ -53,7 +51,7 @@ export const handleFlowError = "Your interaction expired, please fill out the form again.", ); resetFlow(undefined); - await router.push("/" + flowType); + window.location.href = "./" + flowType; return; case "security_csrf_violation": // A CSRF violation occurred. Best to just refresh the flow! @@ -61,12 +59,12 @@ export const handleFlowError = "A security violation was detected, please fill out the form again.", ); resetFlow(undefined); - await router.push("/" + flowType); + window.location.href = "./" + flowType; return; case "security_identity_mismatch": // The requested item was intended for someone else. Let's request a new flow... resetFlow(undefined); - await router.push("/" + flowType); + window.location.href = "./" + flowType; return; case "browser_location_change_required": // Ory Kratos asked us to point the user to this URL. @@ -78,7 +76,7 @@ export const handleFlowError = case 410: // The flow expired, let's request a new one. resetFlow(undefined); - await router.push("/" + flowType); + window.location.href = "./" + flowType; return; } diff --git a/ui/util/replaceAuthLabel.ts b/ui/util/replaceAuthLabel.ts new file mode 100644 index 000000000..2d8ec75a7 --- /dev/null +++ b/ui/util/replaceAuthLabel.ts @@ -0,0 +1,30 @@ +import { LoginFlow } from "@ory/client"; + +export const replaceAuthLabel = ( + flow: LoginFlow | undefined, +): LoginFlow | undefined => { + if (!flow) { + return flow; + } + return { + ...flow, + ui: { + ...flow?.ui, + nodes: flow?.ui.nodes.map((node) => { + if (node.meta.label?.text === "Use Authenticator") { + return { + ...node, + meta: { + ...node.meta, + label: { + ...node.meta.label, + text: "Sign in", + }, + }, + }; + } + return node; + }), + }, + }; +}; From 16df2793f5882cb0a3e4b264352b62235842c3ed Mon Sep 17 00:00:00 2001 From: natalia Date: Wed, 10 Jul 2024 13:09:29 +0200 Subject: [PATCH 2/4] feat: support mfa with totp method --- pkg/kratos/handlers.go | 100 +++++++++++++-------- pkg/kratos/handlers_test.go | 86 +++++++++++++++++- pkg/kratos/interfaces.go | 4 +- pkg/kratos/service.go | 175 +++++++++++++++++++++++++++++------- pkg/kratos/service_test.go | 133 +++++++++++++++++++-------- 5 files changed, 386 insertions(+), 112 deletions(-) diff --git a/pkg/kratos/handlers.go b/pkg/kratos/handlers.go index 1077eedaa..ffe839644 100644 --- a/pkg/kratos/handlers.go +++ b/pkg/kratos/handlers.go @@ -3,14 +3,13 @@ package kratos import ( "context" "encoding/json" - "fmt" "net/http" "net/url" "strconv" - "strings" - "github.com/canonical/identity-platform-login-ui/internal/logging" "github.com/go-chi/chi/v5" + + "github.com/canonical/identity-platform-login-ui/internal/logging" ) type API struct { @@ -72,7 +71,7 @@ func (a *API) handleCreateFlow(w http.ResponseWriter, r *http.Request) { returnTo, err := url.JoinPath(a.baseURL, "/ui/login") if err != nil { - a.logger.Fatal("Failed to construct returnTo URL: ", err) + a.logger.Errorf("Failed to construct returnTo URL: %v", err) } returnTo = returnTo + "?login_challenge=" + loginChallenge @@ -231,24 +230,28 @@ func (a *API) handleUpdateRecoveryFlow(w http.ResponseWriter, r *http.Request) { return } - flow, cookies, err := a.service.UpdateRecoveryFlow(context.Background(), flowId, *body, r.Cookies()) + flow, cookies, err := a.service.UpdateRecoveryFlow(r.Context(), flowId, *body, r.Cookies()) if err != nil { a.logger.Errorf("Error when updating recovery flow: %v\n", err) http.Error(w, err.Error(), http.StatusInternalServerError) return } - resp, err := json.Marshal(flow) - if err != nil { - a.logger.Errorf("Error when marshalling json: %v\n", err) - http.Error(w, "Failed to parse recovery flow", http.StatusInternalServerError) + if !flow.HasRedirectTo() && flow.HasError() { + a.logger.Errorf("Error when updating recovery flow: %v\n", flow) + w.WriteHeader(http.StatusInternalServerError) + _ = json.NewEncoder(w).Encode(flow) return } + setCookies(w, cookies) - // Kratos returns us a '422' response but we tranform it to a '200', - // because this is the expected behavior for us. + // Kratos '422' response maps to 200 OK, it is expected w.WriteHeader(http.StatusOK) - w.Write(resp) + _ = json.NewEncoder(w).Encode( + BrowserLocationChangeRequired{ + RedirectTo: flow.RedirectTo, + }, + ) } func (a *API) handleCreateRecoveryFlow(w http.ResponseWriter, r *http.Request) { @@ -280,22 +283,38 @@ func (a *API) handleCreateRecoveryFlow(w http.ResponseWriter, r *http.Request) { func (a *API) handleGetSettingsFlow(w http.ResponseWriter, r *http.Request) { q := r.URL.Query() - flow, cookies, err := a.service.GetSettingsFlow(context.Background(), q.Get("id"), r.Cookies()) + flow, response, err := a.service.GetSettingsFlow(context.Background(), q.Get("id"), r.Cookies()) if err != nil { a.logger.Errorf("Error when getting settings flow: %v\n", err) http.Error(w, "Failed to get settings flow", http.StatusInternalServerError) return } - resp, err := flow.MarshalJSON() - if err != nil { - a.logger.Errorf("Error when marshalling json: %v\n", err) - http.Error(w, "Failed to parse settings flow", http.StatusInternalServerError) + if response == nil { + resp, err := flow.MarshalJSON() + if err != nil { + a.logger.Errorf("Error when marshalling json: %v\n", err) + http.Error(w, "Failed to parse settings flow", http.StatusInternalServerError) + return + } + + w.WriteHeader(http.StatusOK) + w.Write(resp) + return + } + + // If aal1, redirect to complete second factor auth + if response.HasRedirectTo() { + a.logger.Errorf("Failed to get settings flow due to insufficient aal: %v\n", response) + w.WriteHeader(http.StatusForbidden) + _ = json.NewEncoder(w).Encode( + ErrorBrowserLocationChangeRequired{ + Error: response.Error, + RedirectBrowserTo: response.RedirectTo, + }, + ) return } - setCookies(w, cookies) - w.WriteHeader(http.StatusOK) - w.Write(resp) } func (a *API) handleUpdateSettingsFlow(w http.ResponseWriter, r *http.Request) { @@ -328,28 +347,43 @@ func (a *API) handleUpdateSettingsFlow(w http.ResponseWriter, r *http.Request) { } func (a *API) handleCreateSettingsFlow(w http.ResponseWriter, r *http.Request) { - returnTo, err := url.JoinPath(a.baseURL, "/ui/reset_complete") + returnTo, err := url.JoinPath(a.baseURL, "/ui/setup_complete") if err != nil { a.logger.Errorf("Failed to construct returnTo URL: ", err) http.Error(w, "Failed to construct returnTo URL", http.StatusBadRequest) } - flow, cookies, err := a.service.CreateBrowserSettingsFlow(context.Background(), returnTo, r.Cookies()) + flow, response, err := a.service.CreateBrowserSettingsFlow(context.Background(), returnTo, r.Cookies()) if err != nil { a.logger.Errorf("Failed to create settings flow: %v", err) http.Error(w, "Failed to create settings flow", http.StatusInternalServerError) return } - resp, err := flow.MarshalJSON() - if err != nil { - a.logger.Errorf("Error when marshalling json: %v\n", err) - http.Error(w, "Failed to marshal json", http.StatusInternalServerError) + if response == nil { + resp, err := flow.MarshalJSON() + if err != nil { + a.logger.Errorf("Error when marshalling json: %v\n", err) + http.Error(w, "Failed to marshal json", http.StatusInternalServerError) + return + } + w.WriteHeader(http.StatusOK) + w.Write(resp) + return + } + + // If aal1, redirect to complete second factor auth + if response.HasRedirectTo() { + a.logger.Errorf("Failed to create settings flow due to insufficient aal: %v\n", response) + w.WriteHeader(http.StatusForbidden) + _ = json.NewEncoder(w).Encode( + ErrorBrowserLocationChangeRequired{ + Error: response.Error, + RedirectBrowserTo: response.RedirectTo, + }, + ) return } - setCookies(w, cookies) - w.WriteHeader(http.StatusOK) - w.Write(resp) } func NewAPI(service ServiceInterface, baseURL string, logger logging.LoggerInterface) *API { @@ -368,11 +402,3 @@ func setCookies(w http.ResponseWriter, cookies []*http.Cookie) { http.SetCookie(w, c) } } - -func cookiesToString(cookies []*http.Cookie) string { - var ret = make([]string, len(cookies)) - for i, c := range cookies { - ret[i] = fmt.Sprintf("%s=%s", c.Name, c.Value) - } - return strings.Join(ret, "; ") -} diff --git a/pkg/kratos/handlers_test.go b/pkg/kratos/handlers_test.go index 348337d3b..cc3381c69 100644 --- a/pkg/kratos/handlers_test.go +++ b/pkg/kratos/handlers_test.go @@ -784,14 +784,14 @@ func TestHandleCreateSettingsFlow(t *testing.T) { mockLogger := NewMockLoggerInterface(ctrl) mockService := NewMockServiceInterface(ctrl) - redirect := "https://example.com/ui/reset_complete" + redirect := "https://example.com/ui/setup_complete" req := httptest.NewRequest(http.MethodGet, HANDLE_CREATE_SETTINGS_FLOW_URL, nil) values := req.URL.Query() req.URL.RawQuery = values.Encode() flow := kClient.NewSettingsFlowWithDefaults() - mockService.EXPECT().CreateBrowserSettingsFlow(gomock.Any(), redirect, req.Cookies()).Return(flow, req.Cookies(), nil) + mockService.EXPECT().CreateBrowserSettingsFlow(gomock.Any(), redirect, req.Cookies()).Return(flow, nil, nil) w := httptest.NewRecorder() mux := chi.NewMux() @@ -809,6 +809,43 @@ func TestHandleCreateSettingsFlow(t *testing.T) { } } +func TestHandleCreateSettingsFlowWithRedirect(t *testing.T) { + ctrl := gomock.NewController(t) + defer ctrl.Finish() + + mockLogger := NewMockLoggerInterface(ctrl) + mockService := NewMockServiceInterface(ctrl) + + redirect := "https://example.com/ui/setup_complete" + + redirectErrorBrowserTo := "https://some/path/to/somewhere" + redirectFlow := new(BrowserLocationChangeRequired) + redirectFlow.RedirectTo = &redirectErrorBrowserTo + + req := httptest.NewRequest(http.MethodGet, HANDLE_CREATE_SETTINGS_FLOW_URL, nil) + values := req.URL.Query() + req.URL.RawQuery = values.Encode() + + flow := kClient.NewSettingsFlowWithDefaults() + mockService.EXPECT().CreateBrowserSettingsFlow(gomock.Any(), redirect, req.Cookies()).Return(flow, redirectFlow, nil) + mockLogger.EXPECT().Errorf(gomock.Any(), gomock.Any()).Times(1) + + w := httptest.NewRecorder() + mux := chi.NewMux() + NewAPI(mockService, BASE_URL, mockLogger).RegisterEndpoints(mux) + + mux.ServeHTTP(w, req) + + res := w.Result() + + if _, err := json.Marshal(flow); err != nil { + t.Fatalf("Expected error to be nil got %v", err) + } + if res.StatusCode != http.StatusForbidden { + t.Fatal("Expected HTTP status code 403, got: ", res.Status) + } +} + func TestHandleCreateSettingsFlowFailOnCreateBrowserSettingsFlow(t *testing.T) { ctrl := gomock.NewController(t) defer ctrl.Finish() @@ -816,7 +853,7 @@ func TestHandleCreateSettingsFlowFailOnCreateBrowserSettingsFlow(t *testing.T) { mockLogger := NewMockLoggerInterface(ctrl) mockService := NewMockServiceInterface(ctrl) - redirect := "https://example.com/ui/reset_complete" + redirect := "https://example.com/ui/setup_complete" req := httptest.NewRequest(http.MethodGet, HANDLE_CREATE_SETTINGS_FLOW_URL, nil) values := req.URL.Query() @@ -855,7 +892,7 @@ func TestHandleGetSettingsFlow(t *testing.T) { values.Add("id", id) req.URL.RawQuery = values.Encode() - mockService.EXPECT().GetSettingsFlow(gomock.Any(), id, req.Cookies()).Return(flow, req.Cookies(), nil) + mockService.EXPECT().GetSettingsFlow(gomock.Any(), id, req.Cookies()).Return(flow, nil, nil) w := httptest.NewRecorder() mux := chi.NewMux() @@ -881,6 +918,47 @@ func TestHandleGetSettingsFlow(t *testing.T) { } } +func TestHandleGetSettingsFlowWithRedirect(t *testing.T) { + ctrl := gomock.NewController(t) + defer ctrl.Finish() + + mockLogger := NewMockLoggerInterface(ctrl) + mockService := NewMockServiceInterface(ctrl) + + id := "test" + flow := kClient.NewSettingsFlowWithDefaults() + flow.SetId(id) + flow.SetState("show_form") + + req := httptest.NewRequest(http.MethodGet, HANDLE_GET_SETTINGS_FLOW_URL, nil) + values := req.URL.Query() + values.Add("id", id) + req.URL.RawQuery = values.Encode() + + redirectErrorBrowserTo := "https://some/path/to/somewhere" + redirectFlow := new(BrowserLocationChangeRequired) + redirectFlow.RedirectTo = &redirectErrorBrowserTo + + mockService.EXPECT().GetSettingsFlow(gomock.Any(), id, req.Cookies()).Return(flow, redirectFlow, nil) + mockLogger.EXPECT().Errorf(gomock.Any(), gomock.Any()).Times(1) + + w := httptest.NewRecorder() + mux := chi.NewMux() + NewAPI(mockService, BASE_URL, mockLogger).RegisterEndpoints(mux) + + mux.ServeHTTP(w, req) + + res := w.Result() + + if res.StatusCode != http.StatusForbidden { + t.Fatal("Expected HTTP status code 403, got: ", res.Status) + } + _, err := io.ReadAll(res.Body) + if err != nil { + t.Fatalf("Expected error to be nil got %v", err) + } +} + func TestHandleGetSettingsFlowFail(t *testing.T) { ctrl := gomock.NewController(t) defer ctrl.Finish() diff --git a/pkg/kratos/interfaces.go b/pkg/kratos/interfaces.go index a80297267..e72035c8c 100644 --- a/pkg/kratos/interfaces.go +++ b/pkg/kratos/interfaces.go @@ -27,10 +27,10 @@ type ServiceInterface interface { AcceptLoginRequest(context.Context, string, string) (*hClient.OAuth2RedirectTo, []*http.Cookie, error) CreateBrowserLoginFlow(context.Context, string, string, string, bool, []*http.Cookie) (*kClient.LoginFlow, []*http.Cookie, error) CreateBrowserRecoveryFlow(context.Context, string, []*http.Cookie) (*kClient.RecoveryFlow, []*http.Cookie, error) - CreateBrowserSettingsFlow(context.Context, string, []*http.Cookie) (*kClient.SettingsFlow, []*http.Cookie, error) + CreateBrowserSettingsFlow(context.Context, string, []*http.Cookie) (*kClient.SettingsFlow, *BrowserLocationChangeRequired, error) GetLoginFlow(context.Context, string, []*http.Cookie) (*kClient.LoginFlow, []*http.Cookie, error) GetRecoveryFlow(context.Context, string, []*http.Cookie) (*kClient.RecoveryFlow, []*http.Cookie, error) - GetSettingsFlow(context.Context, string, []*http.Cookie) (*kClient.SettingsFlow, []*http.Cookie, error) + GetSettingsFlow(context.Context, string, []*http.Cookie) (*kClient.SettingsFlow, *BrowserLocationChangeRequired, error) UpdateLoginFlow(context.Context, string, kClient.UpdateLoginFlowBody, []*http.Cookie) (*BrowserLocationChangeRequired, []*http.Cookie, error) UpdateRecoveryFlow(context.Context, string, kClient.UpdateRecoveryFlowBody, []*http.Cookie) (*BrowserLocationChangeRequired, []*http.Cookie, error) UpdateSettingsFlow(context.Context, string, kClient.UpdateSettingsFlowBody, []*http.Cookie) (*kClient.SettingsFlow, []*http.Cookie, error) diff --git a/pkg/kratos/service.go b/pkg/kratos/service.go index ad06ac31b..ecb481a0a 100644 --- a/pkg/kratos/service.go +++ b/pkg/kratos/service.go @@ -9,12 +9,13 @@ import ( "io" "net/http" + hClient "github.com/ory/hydra-client-go/v2" + kClient "github.com/ory/kratos-client-go" + "github.com/canonical/identity-platform-login-ui/internal/logging" httpHelpers "github.com/canonical/identity-platform-login-ui/internal/misc/http" "github.com/canonical/identity-platform-login-ui/internal/monitoring" "github.com/canonical/identity-platform-login-ui/internal/tracing" - hClient "github.com/ory/hydra-client-go/v2" - kClient "github.com/ory/kratos-client-go" ) const ( @@ -23,6 +24,7 @@ const ( InvalidRecoveryCode = 4060006 RecoveryCodeSent = 1060003 InvalidProperty = 4000002 + InvalidAuthCode = 4000008 ) type Service struct { @@ -44,7 +46,16 @@ type ErrorBrowserLocationChangeRequired struct { RedirectBrowserTo *string `json:"redirect_browser_to,omitempty"` } +func (e *BrowserLocationChangeRequired) HasError() bool { + return e.Error != nil +} + +func (e *BrowserLocationChangeRequired) HasRedirectTo() bool { + return e.RedirectTo != nil +} + type BrowserLocationChangeRequired struct { + Error *kClient.GenericError `json:"error,omitempty"` // Points to where to redirect the user to next. RedirectTo *string `json:"redirect_to,omitempty"` } @@ -53,13 +64,17 @@ type UiErrorMessages struct { Ui kClient.UiContainer `json:"ui"` } +type methodOnly struct { + Method string `json:"method"` +} + func (s *Service) CheckSession(ctx context.Context, cookies []*http.Cookie) (*kClient.Session, []*http.Cookie, error) { - ctx, span := s.tracer.Start(ctx, "kratos.FrontendApi.ToSession") + ctx, span := s.tracer.Start(ctx, "kratos.Service.ToSession") defer span.End() session, resp, err := s.kratos.FrontendApi(). ToSession(ctx). - Cookie(cookiesToString(cookies)). + Cookie(httpHelpers.CookiesToString(cookies)). Execute() if err != nil { @@ -94,7 +109,7 @@ func (s *Service) AcceptLoginRequest(ctx context.Context, identityID string, lc func (s *Service) CreateBrowserLoginFlow( ctx context.Context, aal, returnTo, loginChallenge string, refresh bool, cookies []*http.Cookie, ) (*kClient.LoginFlow, []*http.Cookie, error) { - ctx, span := s.tracer.Start(ctx, "kratos.FrontendApi.CreateBrowserLoginFlow") + ctx, span := s.tracer.Start(ctx, "kratos.Service.CreateBrowserLoginFlow") defer span.End() flow, resp, err := s.kratos.FrontendApi(). @@ -103,7 +118,7 @@ func (s *Service) CreateBrowserLoginFlow( ReturnTo(returnTo). LoginChallenge(loginChallenge). Refresh(refresh). - Cookie(cookiesToString(cookies)). + Cookie(httpHelpers.CookiesToString(cookies)). Execute() if err != nil { s.logger.Debugf("full HTTP response: %v", resp) @@ -129,7 +144,7 @@ func (s *Service) CreateBrowserRecoveryFlow(ctx context.Context, returnTo string return flow, resp.Cookies(), nil } -func (s *Service) CreateBrowserSettingsFlow(ctx context.Context, returnTo string, cookies []*http.Cookie) (*kClient.SettingsFlow, []*http.Cookie, error) { +func (s *Service) CreateBrowserSettingsFlow(ctx context.Context, returnTo string, cookies []*http.Cookie) (*kClient.SettingsFlow, *BrowserLocationChangeRequired, error) { ctx, span := s.tracer.Start(ctx, "kratos.Service.CreateBrowserSettingsFlow") defer span.End() @@ -138,22 +153,40 @@ func (s *Service) CreateBrowserSettingsFlow(ctx context.Context, returnTo string ReturnTo(returnTo). Cookie(httpHelpers.CookiesToString(cookies)). Execute() - if err != nil { + + // 403 means the user must be redirected to complete second factor auth + // in order to access settings + if err != nil && resp.StatusCode != http.StatusForbidden { s.logger.Debugf("full HTTP response: %v", resp) return nil, nil, err } - return flow, resp.Cookies(), nil + if err == nil { + return flow, nil, nil + } + + redirectResp := new(ErrorBrowserLocationChangeRequired) + err = unmarshalByteJson(resp.Body, redirectResp) + if err != nil { + s.logger.Debugf("Failed to unmarshal JSON: %s", err) + return nil, nil, err + } + + returnToResp := &BrowserLocationChangeRequired{ + RedirectTo: redirectResp.RedirectBrowserTo, + Error: redirectResp.Error, + } + return flow, returnToResp, nil } func (s *Service) GetLoginFlow(ctx context.Context, id string, cookies []*http.Cookie) (*kClient.LoginFlow, []*http.Cookie, error) { - ctx, span := s.tracer.Start(ctx, "kratos.FrontendApi.GetLoginFlow") + ctx, span := s.tracer.Start(ctx, "kratos.Service.GetLoginFlow") defer span.End() flow, resp, err := s.kratos.FrontendApi(). GetLoginFlow(ctx). Id(id). - Cookie(cookiesToString(cookies)). + Cookie(httpHelpers.CookiesToString(cookies)). Execute() if err != nil { s.logger.Debugf("full HTTP response: %v", resp) @@ -180,7 +213,7 @@ func (s *Service) GetRecoveryFlow(ctx context.Context, id string, cookies []*htt return flow, resp.Cookies(), nil } -func (s *Service) GetSettingsFlow(ctx context.Context, id string, cookies []*http.Cookie) (*kClient.SettingsFlow, []*http.Cookie, error) { +func (s *Service) GetSettingsFlow(ctx context.Context, id string, cookies []*http.Cookie) (*kClient.SettingsFlow, *BrowserLocationChangeRequired, error) { ctx, span := s.tracer.Start(ctx, "kratos.Service.GetSettingsFlow") defer span.End() @@ -189,12 +222,30 @@ func (s *Service) GetSettingsFlow(ctx context.Context, id string, cookies []*htt Id(id). Cookie(httpHelpers.CookiesToString(cookies)). Execute() - if err != nil { + + // 403 means the user must be redirected to complete second factor auth + // in order to access settings + if err != nil && resp.StatusCode != http.StatusForbidden { s.logger.Debugf("full HTTP response: %v", resp) return nil, nil, err } - return flow, resp.Cookies(), nil + if err == nil { + return flow, nil, nil + } + + redirectResp := new(ErrorBrowserLocationChangeRequired) + err = unmarshalByteJson(resp.Body, redirectResp) + if err != nil { + s.logger.Debugf("Failed to unmarshal JSON: %s", err) + return nil, nil, err + } + + returnToResp := &BrowserLocationChangeRequired{ + RedirectTo: redirectResp.RedirectBrowserTo, + Error: redirectResp.Error, + } + return flow, returnToResp, nil } func (s *Service) UpdateRecoveryFlow( @@ -210,6 +261,25 @@ func (s *Service) UpdateRecoveryFlow( Cookie(httpHelpers.CookiesToString(cookies)). Execute() + // if the flow responds with 400, it means a session already exists + if err != nil && resp.StatusCode == http.StatusBadRequest { + s.logger.Debugf("full HTTP response: %v", resp) + + redirectResp := new(ErrorBrowserLocationChangeRequired) + err := unmarshalByteJson(resp.Body, redirectResp) + if err != nil { + s.logger.Debugf("Failed to unmarshal JSON: %s", err) + return nil, nil, err + } + + returnToResp := &BrowserLocationChangeRequired{ + RedirectTo: redirectResp.RedirectBrowserTo, + Error: redirectResp.Error, + } + + return returnToResp, nil, nil + } + // If the recovery code was invalid, kratos returns a 200 response // with a 4060006 error in the rendered ui messages. // If the recovery code was valid, we expect to get a 422 response from kratos. @@ -242,10 +312,7 @@ func (s *Service) UpdateRecoveryFlow( return nil, nil, err } - // We trasform the kratos response to our own custom response here. - // The original kratos response contains an 'Error' field, which we remove - // because this is not a real error. - returnToResp := BrowserLocationChangeRequired{redirectResp.RedirectBrowserTo} + returnToResp := BrowserLocationChangeRequired{RedirectTo: redirectResp.RedirectBrowserTo} return &returnToResp, resp.Cookies(), nil } @@ -253,14 +320,14 @@ func (s *Service) UpdateRecoveryFlow( func (s *Service) UpdateLoginFlow( ctx context.Context, flow string, body kClient.UpdateLoginFlowBody, cookies []*http.Cookie, ) (*BrowserLocationChangeRequired, []*http.Cookie, error) { - ctx, span := s.tracer.Start(ctx, "kratos.FrontendApi.UpdateLoginFlow") + ctx, span := s.tracer.Start(ctx, "kratos.Service.UpdateLoginFlow") defer span.End() _, resp, err := s.kratos.FrontendApi(). UpdateLoginFlow(ctx). Flow(flow). UpdateLoginFlowBody(body). - Cookie(cookiesToString(cookies)). + Cookie(httpHelpers.CookiesToString(cookies)). Execute() // We expect to get a 422 response from Kratos. The sdk forces us to // make the request with an 'application/json' content-type, whereas Kratos @@ -284,7 +351,7 @@ func (s *Service) UpdateLoginFlow( // We trasform the kratos response to our own custom response here. // The original kratos response contains an 'Error' field, which we remove // because this is not a real error. - returnToResp := BrowserLocationChangeRequired{redirectResp.RedirectBrowserTo} + returnToResp := BrowserLocationChangeRequired{RedirectTo: redirectResp.RedirectBrowserTo} return &returnToResp, resp.Cookies(), nil } @@ -346,6 +413,8 @@ func (s *Service) getUiError(responseBody io.ReadCloser) (err error) { err = fmt.Errorf("inactive account") case InvalidProperty: err = fmt.Errorf("invalid %s", errorCodes[0].Context["property"]) + case InvalidAuthCode: + err = fmt.Errorf("invalid authentication code") default: err = fmt.Errorf("unknown error") s.logger.Debugf("Kratos error code: %v", errorCode) @@ -354,7 +423,7 @@ func (s *Service) getUiError(responseBody io.ReadCloser) (err error) { } func (s *Service) GetFlowError(ctx context.Context, id string) (*kClient.FlowError, []*http.Cookie, error) { - ctx, span := s.tracer.Start(ctx, "kratos.FrontendApi.GetFlowError") + ctx, span := s.tracer.Start(ctx, "kratos.Service.GetFlowError") defer span.End() flowError, resp, err := s.kratos.FrontendApi().GetFlowError(context.Background()).Id(id).Execute() @@ -432,13 +501,8 @@ func (s *Service) FilterFlowProviderList(ctx context.Context, flow *kClient.Logi } func (s *Service) ParseLoginFlowMethodBody(r *http.Request) (*kClient.UpdateLoginFlowBody, error) { - - type MethodOnly struct { - Method string `json:"method"` - } - // TODO: try to refactor when we bump kratos sdk to 1.x.x - methodOnly := new(MethodOnly) + methodOnly := new(methodOnly) defer r.Body.Close() b, err := io.ReadAll(r.Body) @@ -468,6 +532,18 @@ func (s *Service) ParseLoginFlowMethodBody(r *http.Request) (*kClient.UpdateLogi ret = kClient.UpdateLoginFlowWithPasswordMethodAsUpdateLoginFlowBody( body, ) + case "totp": + body := new(kClient.UpdateLoginFlowWithTotpMethod) + + err := parseBody(r.Body, &body) + + if err != nil { + return nil, err + } + ret = kClient.UpdateLoginFlowWithTotpMethodAsUpdateLoginFlowBody( + body, + ) + ret.UpdateLoginFlowWithTotpMethod.Method = "totp" // method field is empty for oidc: https://github.com/ory/kratos/pull/3564 default: body := new(kClient.UpdateLoginFlowWithOidcMethod) @@ -504,16 +580,49 @@ func (s *Service) ParseRecoveryFlowMethodBody(r *http.Request) (*kClient.UpdateR } func (s *Service) ParseSettingsFlowMethodBody(r *http.Request) (*kClient.UpdateSettingsFlowBody, error) { - body := new(kClient.UpdateSettingsFlowWithPasswordMethod) + methodOnly := new(methodOnly) - err := parseBody(r.Body, &body) + defer r.Body.Close() + b, err := io.ReadAll(r.Body) if err != nil { + return nil, errors.New("unable to read body") + } + + // replace the body that was consumed + r.Body = io.NopCloser(bytes.NewReader(b)) + + if err := json.Unmarshal(b, methodOnly); err != nil { return nil, err } - ret := kClient.UpdateSettingsFlowWithPasswordMethodAsUpdateSettingsFlowBody( - body, - ) + + var ret kClient.UpdateSettingsFlowBody + + switch methodOnly.Method { + case "password": + body := new(kClient.UpdateSettingsFlowWithPasswordMethod) + + err := parseBody(r.Body, &body) + + if err != nil { + return nil, err + } + ret = kClient.UpdateSettingsFlowWithPasswordMethodAsUpdateSettingsFlowBody( + body, + ) + case "totp": + body := new(kClient.UpdateSettingsFlowWithTotpMethod) + + err := parseBody(r.Body, &body) + + if err != nil { + return nil, err + } + + ret = kClient.UpdateSettingsFlowWithTotpMethodAsUpdateSettingsFlowBody( + body, + ) + } return &ret, nil } diff --git a/pkg/kratos/service_test.go b/pkg/kratos/service_test.go index 2d953a36b..36b7952f9 100644 --- a/pkg/kratos/service_test.go +++ b/pkg/kratos/service_test.go @@ -49,7 +49,7 @@ func TestCheckSessionSuccess(t *testing.T) { Header: http.Header{"Set-Cookie": []string{cookie.Raw}}, } - mockTracer.EXPECT().Start(ctx, "kratos.FrontendApi.ToSession").Times(1).Return(ctx, trace.SpanFromContext(ctx)) + mockTracer.EXPECT().Start(ctx, "kratos.Service.ToSession").Times(1).Return(ctx, trace.SpanFromContext(ctx)) mockKratos.EXPECT().FrontendApi().Times(1).Return(mockKratosFrontendApi) mockKratosFrontendApi.EXPECT().ToSession(ctx).Times(1).Return(sessionRequest) mockKratosFrontendApi.EXPECT().ToSessionExecute(gomock.Any()).Times(1).DoAndReturn( @@ -96,7 +96,7 @@ func TestCheckSessionFails(t *testing.T) { } mockLogger.EXPECT().Debugf(gomock.Any(), gomock.Any()).Times(1) - mockTracer.EXPECT().Start(ctx, "kratos.FrontendApi.ToSession").Times(1).Return(ctx, trace.SpanFromContext(ctx)) + mockTracer.EXPECT().Start(ctx, "kratos.Service.ToSession").Times(1).Return(ctx, trace.SpanFromContext(ctx)) mockKratos.EXPECT().FrontendApi().Times(1).Return(mockKratosFrontendApi) mockKratosFrontendApi.EXPECT().ToSession(ctx).Times(1).Return(sessionRequest) mockKratosFrontendApi.EXPECT().ToSessionExecute(gomock.Any()).Times(1).DoAndReturn( @@ -239,7 +239,7 @@ func TestCreateBrowserLoginFlowSuccess(t *testing.T) { Header: http.Header{"Set-Cookie": []string{cookie.Raw}}, } - mockTracer.EXPECT().Start(ctx, "kratos.FrontendApi.CreateBrowserLoginFlow").Times(1).Return(ctx, trace.SpanFromContext(ctx)) + mockTracer.EXPECT().Start(ctx, "kratos.Service.CreateBrowserLoginFlow").Times(1).Return(ctx, trace.SpanFromContext(ctx)) mockKratos.EXPECT().FrontendApi().Times(1).Return(mockKratosFrontendApi) mockKratosFrontendApi.EXPECT().CreateBrowserLoginFlow(ctx).Times(1).Return(request) mockKratosFrontendApi.EXPECT().CreateBrowserLoginFlowExecute(gomock.Any()).Times(1).DoAndReturn( @@ -305,7 +305,7 @@ func TestCreateBrowserLoginFlowFail(t *testing.T) { } mockLogger.EXPECT().Debugf(gomock.Any(), gomock.Any()).Times(1) - mockTracer.EXPECT().Start(ctx, "kratos.FrontendApi.CreateBrowserLoginFlow").Times(1).Return(ctx, trace.SpanFromContext(ctx)) + mockTracer.EXPECT().Start(ctx, "kratos.Service.CreateBrowserLoginFlow").Times(1).Return(ctx, trace.SpanFromContext(ctx)) mockKratos.EXPECT().FrontendApi().Times(1).Return(mockKratosFrontendApi) mockKratosFrontendApi.EXPECT().CreateBrowserLoginFlow(ctx).Times(1).Return(request) mockKratosFrontendApi.EXPECT().CreateBrowserLoginFlowExecute(gomock.Any()).Times(1).Return(nil, &resp, fmt.Errorf("error")) @@ -348,7 +348,7 @@ func TestGetLoginFlowSuccess(t *testing.T) { Header: http.Header{"Set-Cookie": []string{cookie.Raw}}, } - mockTracer.EXPECT().Start(ctx, "kratos.FrontendApi.GetLoginFlow").Times(1).Return(ctx, trace.SpanFromContext(ctx)) + mockTracer.EXPECT().Start(ctx, "kratos.Service.GetLoginFlow").Times(1).Return(ctx, trace.SpanFromContext(ctx)) mockKratos.EXPECT().FrontendApi().Times(1).Return(mockKratosFrontendApi) mockKratosFrontendApi.EXPECT().GetLoginFlow(ctx).Times(1).Return(request) mockKratosFrontendApi.EXPECT().GetLoginFlowExecute(gomock.Any()).Times(1).DoAndReturn( @@ -402,7 +402,7 @@ func TestGetLoginFlowFail(t *testing.T) { } mockLogger.EXPECT().Debugf(gomock.Any(), gomock.Any()).Times(1) - mockTracer.EXPECT().Start(ctx, "kratos.FrontendApi.GetLoginFlow").Times(1).Return(ctx, trace.SpanFromContext(ctx)) + mockTracer.EXPECT().Start(ctx, "kratos.Service.GetLoginFlow").Times(1).Return(ctx, trace.SpanFromContext(ctx)) mockKratos.EXPECT().FrontendApi().Times(1).Return(mockKratosFrontendApi) mockKratosFrontendApi.EXPECT().GetLoginFlow(ctx).Times(1).Return(request) mockKratosFrontendApi.EXPECT().GetLoginFlowExecute(gomock.Any()).Times(1).Return(nil, &resp, fmt.Errorf("error")) @@ -451,7 +451,7 @@ func TestUpdateLoginFlowSuccess(t *testing.T) { Body: io.NopCloser(bytes.NewBuffer(flowJson)), } - mockTracer.EXPECT().Start(ctx, "kratos.FrontendApi.UpdateLoginFlow").Times(1).Return(ctx, trace.SpanFromContext(ctx)) + mockTracer.EXPECT().Start(ctx, "kratos.Service.UpdateLoginFlow").Times(1).Return(ctx, trace.SpanFromContext(ctx)) mockKratos.EXPECT().FrontendApi().Times(1).Return(mockKratosFrontendApi) mockKratosFrontendApi.EXPECT().UpdateLoginFlow(ctx).Times(1).Return(request) mockKratosFrontendApi.EXPECT().UpdateLoginFlowExecute(gomock.Any()).Times(1).DoAndReturn( @@ -516,7 +516,7 @@ func TestUpdateLoginFlowFail(t *testing.T) { } mockLogger.EXPECT().Debugf(gomock.Any(), gomock.Any()).Times(1) - mockTracer.EXPECT().Start(ctx, "kratos.FrontendApi.UpdateLoginFlow").Times(1).Return(ctx, trace.SpanFromContext(ctx)) + mockTracer.EXPECT().Start(ctx, "kratos.Service.UpdateLoginFlow").Times(1).Return(ctx, trace.SpanFromContext(ctx)) mockKratos.EXPECT().FrontendApi().Times(1).Return(mockKratosFrontendApi) mockKratosFrontendApi.EXPECT().UpdateLoginFlow(ctx).Times(1).Return(request) mockKratosFrontendApi.EXPECT().UpdateLoginFlowExecute(gomock.Any()).Times(1).Return(nil, &resp, fmt.Errorf("error")) @@ -557,7 +557,7 @@ func TestGetFlowErrorSuccess(t *testing.T) { Header: http.Header{"K": []string{"V"}}, } - mockTracer.EXPECT().Start(ctx, "kratos.FrontendApi.GetFlowError").Times(1).Return(ctx, trace.SpanFromContext(ctx)) + mockTracer.EXPECT().Start(ctx, "kratos.Service.GetFlowError").Times(1).Return(ctx, trace.SpanFromContext(ctx)) mockKratos.EXPECT().FrontendApi().Times(1).Return(mockKratosFrontendApi) mockKratosFrontendApi.EXPECT().GetFlowError(ctx).Times(1).Return(request) mockKratosFrontendApi.EXPECT().GetFlowErrorExecute(gomock.Any()).Times(1).DoAndReturn( @@ -605,7 +605,7 @@ func TestGetFlowErrorFail(t *testing.T) { } mockLogger.EXPECT().Debugf(gomock.Any(), gomock.Any()).Times(1) - mockTracer.EXPECT().Start(ctx, "kratos.FrontendApi.GetFlowError").Times(1).Return(ctx, trace.SpanFromContext(ctx)) + mockTracer.EXPECT().Start(ctx, "kratos.Service.GetFlowError").Times(1).Return(ctx, trace.SpanFromContext(ctx)) mockKratos.EXPECT().FrontendApi().Times(1).Return(mockKratosFrontendApi) mockKratosFrontendApi.EXPECT().GetFlowError(ctx).Times(1).Return(request) mockKratosFrontendApi.EXPECT().GetFlowErrorExecute(gomock.Any()).Times(1).Return(nil, &resp, fmt.Errorf("error")) @@ -1003,6 +1003,39 @@ func TestParseLoginFlowPasswordMethodBody(t *testing.T) { } } +func TestParseLoginFlowTotpMethodBody(t *testing.T) { + ctrl := gomock.NewController(t) + defer ctrl.Finish() + + mockLogger := NewMockLoggerInterface(ctrl) + mockHydra := NewMockHydraClientInterface(ctrl) + mockKratos := NewMockKratosClientInterface(ctrl) + mockAuthz := NewMockAuthorizerInterface(ctrl) + mockTracer := NewMockTracingInterface(ctrl) + mockMonitor := monitoring.NewMockMonitorInterface(ctrl) + + flow := kClient.NewUpdateLoginFlowWithTotpMethodWithDefaults() + flow.SetMethod("totp") + + body := kClient.UpdateLoginFlowWithTotpMethodAsUpdateLoginFlowBody(flow) + + jsonBody, _ := body.MarshalJSON() + + req := httptest.NewRequest(http.MethodPost, "http://some/path", io.NopCloser(bytes.NewBuffer(jsonBody))) + + b, err := NewService(mockKratos, mockHydra, mockAuthz, mockTracer, mockMonitor, mockLogger).ParseLoginFlowMethodBody(req) + + actual, _ := b.MarshalJSON() + expected, _ := body.MarshalJSON() + + if !reflect.DeepEqual(string(actual), string(expected)) { + t.Fatalf("expected flow to be %s not %s", string(expected), string(actual)) + } + if err != nil { + t.Fatalf("expected error to be nil not %v", err) + } +} + func TestGetProviderNameWhenNotOidcMethod(t *testing.T) { loginFlow := &kClient.UpdateLoginFlowBody{} service := NewService(nil, nil, nil, nil, nil, nil) @@ -1473,6 +1506,39 @@ func TestParseSettingsFlowPasswordMethodBody(t *testing.T) { } } +func TestParseSettingsFlowTotpMethodBody(t *testing.T) { + ctrl := gomock.NewController(t) + defer ctrl.Finish() + + mockLogger := NewMockLoggerInterface(ctrl) + mockHydra := NewMockHydraClientInterface(ctrl) + mockKratos := NewMockKratosClientInterface(ctrl) + mockAuthz := NewMockAuthorizerInterface(ctrl) + mockTracer := NewMockTracingInterface(ctrl) + mockMonitor := monitoring.NewMockMonitorInterface(ctrl) + + flow := kClient.NewUpdateSettingsFlowWithTotpMethodWithDefaults() + flow.SetMethod("totp") + + body := kClient.UpdateSettingsFlowWithTotpMethodAsUpdateSettingsFlowBody(flow) + + jsonBody, _ := body.MarshalJSON() + + req := httptest.NewRequest(http.MethodPost, "http://some/path", io.NopCloser(bytes.NewBuffer(jsonBody))) + + b, err := NewService(mockKratos, mockHydra, mockAuthz, mockTracer, mockMonitor, mockLogger).ParseSettingsFlowMethodBody(req) + + actual, _ := b.MarshalJSON() + expected, _ := body.MarshalJSON() + + if !reflect.DeepEqual(string(actual), string(expected)) { + t.Fatalf("expected flow to be %s not %s", string(expected), string(actual)) + } + if err != nil { + t.Fatalf("expected error to be nil not %v", err) + } +} + func TestGetSettingsFlowSuccess(t *testing.T) { ctrl := gomock.NewController(t) defer ctrl.Finish() @@ -1490,37 +1556,32 @@ func TestGetSettingsFlowSuccess(t *testing.T) { cookie := &http.Cookie{Name: "test", Value: "test"} cookies = append(cookies, cookie) id := "id" + flow := kClient.NewSettingsFlowWithDefaults() request := kClient.FrontendApiGetSettingsFlowRequest{ ApiService: mockKratosFrontendApi, } - resp := http.Response{ - Header: http.Header{"Set-Cookie": []string{cookie.Raw}}, - } mockTracer.EXPECT().Start(ctx, "kratos.Service.GetSettingsFlow").Times(1).Return(ctx, trace.SpanFromContext(ctx)) mockKratos.EXPECT().FrontendApi().Times(1).Return(mockKratosFrontendApi) mockKratosFrontendApi.EXPECT().GetSettingsFlow(ctx).Times(1).Return(request) mockKratosFrontendApi.EXPECT().GetSettingsFlowExecute(gomock.Any()).Times(1).DoAndReturn( - func(r kClient.FrontendApiGetSettingsFlowRequest) (*kClient.SettingsFlow, *http.Response, error) { + func(r kClient.FrontendApiGetSettingsFlowRequest) (*kClient.SettingsFlow, *BrowserLocationChangeRequired, error) { if _id := (*string)(reflect.ValueOf(r).FieldByName("id").UnsafePointer()); *_id != id { t.Fatalf("expected id to be %s, got %s", id, *_id) } - if cookie := (*string)(reflect.ValueOf(r).FieldByName("cookie").UnsafePointer()); *cookie != "test=test" { - t.Fatalf("expected cookie string as test=test, got %s", *cookie) - } - return flow, &resp, nil + return flow, nil, nil }, ) - s, c, err := NewService(mockKratos, mockHydra, mockAuthz, mockTracer, mockMonitor, mockLogger).GetSettingsFlow(ctx, id, cookies) + s, r, err := NewService(mockKratos, mockHydra, mockAuthz, mockTracer, mockMonitor, mockLogger).GetSettingsFlow(ctx, id, cookies) if s != flow { t.Fatalf("expected flow to be %v not %v", flow, s) } - if !reflect.DeepEqual(c, resp.Cookies()) { - t.Fatalf("expected cookies to be %v not %v", resp.Cookies(), c) + if r != nil { + t.Fatalf("expected response to be nil not %v", r) } if err != nil { t.Fatalf("expected error to be nil not %v", err) @@ -1557,13 +1618,13 @@ func TestGetSettingsFlowFail(t *testing.T) { mockKratosFrontendApi.EXPECT().GetSettingsFlow(ctx).Times(1).Return(request) mockKratosFrontendApi.EXPECT().GetSettingsFlowExecute(gomock.Any()).Times(1).Return(nil, &resp, fmt.Errorf("error")) - f, c, err := NewService(mockKratos, mockHydra, mockAuthz, mockTracer, mockMonitor, mockLogger).GetSettingsFlow(ctx, id, cookies) + f, r, err := NewService(mockKratos, mockHydra, mockAuthz, mockTracer, mockMonitor, mockLogger).GetSettingsFlow(ctx, id, cookies) if f != nil { t.Fatalf("expected flow to be %v not %v", nil, f) } - if c != nil { - t.Fatalf("expected header to be %v not %v", nil, c) + if r != nil { + t.Fatalf("expected response to be %v not %v", nil, r) } if err == nil { t.Fatalf("expected error not nil") @@ -1591,31 +1652,28 @@ func TestCreateBrowserSettingsFlowSuccess(t *testing.T) { request := kClient.FrontendApiCreateBrowserSettingsFlowRequest{ ApiService: mockKratosFrontendApi, } - resp := http.Response{ - Header: http.Header{"Set-Cookie": []string{cookie.Raw}}, - } mockTracer.EXPECT().Start(ctx, "kratos.Service.CreateBrowserSettingsFlow").Times(1).Return(ctx, trace.SpanFromContext(ctx)) mockKratos.EXPECT().FrontendApi().Times(1).Return(mockKratosFrontendApi) mockKratosFrontendApi.EXPECT().CreateBrowserSettingsFlow(ctx).Times(1).Return(request) mockKratosFrontendApi.EXPECT().CreateBrowserSettingsFlowExecute(gomock.Any()).Times(1).DoAndReturn( - func(r kClient.FrontendApiCreateBrowserSettingsFlowRequest) (*kClient.SettingsFlow, *http.Response, error) { + func(r kClient.FrontendApiCreateBrowserSettingsFlowRequest) (*kClient.SettingsFlow, *BrowserLocationChangeRequired, error) { if rt := (*string)(reflect.ValueOf(r).FieldByName("returnTo").UnsafePointer()); *rt != returnTo { t.Fatalf("expected returnTo to be %s, got %s", returnTo, *rt) } - return flow, &resp, nil + return flow, nil, nil }, ) - f, c, err := NewService(mockKratos, mockHydra, mockAuthz, mockTracer, mockMonitor, mockLogger).CreateBrowserSettingsFlow(ctx, returnTo, cookies) + f, r, err := NewService(mockKratos, mockHydra, mockAuthz, mockTracer, mockMonitor, mockLogger).CreateBrowserSettingsFlow(ctx, returnTo, cookies) if f != flow { t.Fatalf("expected flow to be %v not %v", flow, f) } - if !reflect.DeepEqual(c, resp.Cookies()) { - t.Fatalf("expected cookies to be %v not %v", resp.Cookies(), c) + if r != nil { + t.Fatalf("expected response to be nil not %v", r) } if err != nil { t.Fatalf("expected error to be nil not %v", err) @@ -1642,20 +1700,23 @@ func TestCreateBrowserSettingsFlowFail(t *testing.T) { request := kClient.FrontendApiCreateBrowserSettingsFlowRequest{ ApiService: mockKratosFrontendApi, } + resp := http.Response{ + StatusCode: http.StatusNotFound, + } mockTracer.EXPECT().Start(ctx, "kratos.Service.CreateBrowserSettingsFlow").Times(1).Return(ctx, trace.SpanFromContext(ctx)) mockKratos.EXPECT().FrontendApi().Times(1).Return(mockKratosFrontendApi) mockKratosFrontendApi.EXPECT().CreateBrowserSettingsFlow(ctx).Times(1).Return(request) - mockKratosFrontendApi.EXPECT().CreateBrowserSettingsFlowExecute(gomock.Any()).Times(1).Return(nil, nil, fmt.Errorf("")) + mockKratosFrontendApi.EXPECT().CreateBrowserSettingsFlowExecute(gomock.Any()).Times(1).Return(nil, &resp, fmt.Errorf("")) mockLogger.EXPECT().Debugf(gomock.Any(), gomock.Any()).Times(1) - f, c, err := NewService(mockKratos, mockHydra, mockAuthz, mockTracer, mockMonitor, mockLogger).CreateBrowserSettingsFlow(ctx, returnTo, cookies) + f, r, err := NewService(mockKratos, mockHydra, mockAuthz, mockTracer, mockMonitor, mockLogger).CreateBrowserSettingsFlow(ctx, returnTo, cookies) if f != nil { t.Fatalf("expected flow to be %v not %v", nil, f) } - if c != nil { - t.Fatalf("expected cookies to be %v not %v", nil, c) + if r != nil { + t.Fatalf("expected response to be %v not %v", nil, r) } if err == nil { t.Fatalf("expected error not nil") From 37f6eac24f72685c7b04af9df6fb844d62d38371 Mon Sep 17 00:00:00 2001 From: natalia Date: Wed, 10 Jul 2024 13:09:46 +0200 Subject: [PATCH 3/4] ci: update kratos config to support mfa --- docker/kratos/kratos.yml | 10 ++++++---- 1 file changed, 6 insertions(+), 4 deletions(-) diff --git a/docker/kratos/kratos.yml b/docker/kratos/kratos.yml index 53aa1fa32..829b4bd82 100644 --- a/docker/kratos/kratos.yml +++ b/docker/kratos/kratos.yml @@ -22,10 +22,7 @@ identity: url: file:///etc/config/kratos/identity.schema.json selfservice: allowed_return_urls: - - http://localhost:4455/ui/login - - http://localhost:4455/ui/reset_email - - http://localhost:4455/ui/reset_password - - http://localhost:4455/ui/reset_complete + - http://localhost:4455/ui/ default_browser_return_url: http://localhost:4455/ui/ flows: @@ -43,12 +40,17 @@ selfservice: - hook: revoke_active_sessions settings: ui_url: http://localhost:4455/ui/reset_password + required_aal: highest_available registration: after: oidc: hooks: - hook: session methods: + totp: + enabled: true + config: + issuer: GoogleAuthenticator password: enabled: True config: From aacb0e9bfccac928ce07b68361d8c9825591dbfe Mon Sep 17 00:00:00 2001 From: natalia Date: Wed, 10 Jul 2024 13:42:30 +0200 Subject: [PATCH 4/4] refactor: settings handlers --- pkg/kratos/handlers.go | 47 ++++++++++++++++++------------------------ 1 file changed, 20 insertions(+), 27 deletions(-) diff --git a/pkg/kratos/handlers.go b/pkg/kratos/handlers.go index ffe839644..21a6ecbaa 100644 --- a/pkg/kratos/handlers.go +++ b/pkg/kratos/handlers.go @@ -290,21 +290,8 @@ func (a *API) handleGetSettingsFlow(w http.ResponseWriter, r *http.Request) { return } - if response == nil { - resp, err := flow.MarshalJSON() - if err != nil { - a.logger.Errorf("Error when marshalling json: %v\n", err) - http.Error(w, "Failed to parse settings flow", http.StatusInternalServerError) - return - } - - w.WriteHeader(http.StatusOK) - w.Write(resp) - return - } - // If aal1, redirect to complete second factor auth - if response.HasRedirectTo() { + if response != nil && response.HasRedirectTo() { a.logger.Errorf("Failed to get settings flow due to insufficient aal: %v\n", response) w.WriteHeader(http.StatusForbidden) _ = json.NewEncoder(w).Encode( @@ -315,6 +302,15 @@ func (a *API) handleGetSettingsFlow(w http.ResponseWriter, r *http.Request) { ) return } + + resp, err := flow.MarshalJSON() + if err != nil { + a.logger.Errorf("Error when marshalling json: %v\n", err) + http.Error(w, "Failed to marshal json", http.StatusInternalServerError) + return + } + w.WriteHeader(http.StatusOK) + w.Write(resp) } func (a *API) handleUpdateSettingsFlow(w http.ResponseWriter, r *http.Request) { @@ -360,20 +356,8 @@ func (a *API) handleCreateSettingsFlow(w http.ResponseWriter, r *http.Request) { return } - if response == nil { - resp, err := flow.MarshalJSON() - if err != nil { - a.logger.Errorf("Error when marshalling json: %v\n", err) - http.Error(w, "Failed to marshal json", http.StatusInternalServerError) - return - } - w.WriteHeader(http.StatusOK) - w.Write(resp) - return - } - // If aal1, redirect to complete second factor auth - if response.HasRedirectTo() { + if response != nil && response.HasRedirectTo() { a.logger.Errorf("Failed to create settings flow due to insufficient aal: %v\n", response) w.WriteHeader(http.StatusForbidden) _ = json.NewEncoder(w).Encode( @@ -384,6 +368,15 @@ func (a *API) handleCreateSettingsFlow(w http.ResponseWriter, r *http.Request) { ) return } + + resp, err := flow.MarshalJSON() + if err != nil { + a.logger.Errorf("Error when marshalling json: %v\n", err) + http.Error(w, "Failed to marshal json", http.StatusInternalServerError) + return + } + w.WriteHeader(http.StatusOK) + w.Write(resp) } func NewAPI(service ServiceInterface, baseURL string, logger logging.LoggerInterface) *API {