From 6ca5730828404dfd27d5d88dbed015eac2c8125b Mon Sep 17 00:00:00 2001 From: Henning Perl Date: Thu, 27 Apr 2023 11:32:34 +0200 Subject: [PATCH 1/7] run Playwright in CI --- .github/workflows/ci.yaml | 122 ++++++++++++++++++++++++++++++++++---- 1 file changed, 110 insertions(+), 12 deletions(-) diff --git a/.github/workflows/ci.yaml b/.github/workflows/ci.yaml index de5757977da3..029300a82333 100644 --- a/.github/workflows/ci.yaml +++ b/.github/workflows/ci.yaml @@ -75,7 +75,7 @@ jobs: - uses: ory/ci/checkout@master with: fetch-depth: 2 - - uses: actions/setup-go@v2 + - uses: actions/setup-go@v4 with: go-version: "1.19" - run: go list -json > go.list @@ -168,7 +168,7 @@ jobs: sudo apt-get install -y moreutils gettext name: Install tools - name: Setup Go - uses: actions/setup-go@v3 + uses: actions/setup-go@v4 with: go-version: "1.19" @@ -210,16 +210,114 @@ jobs: NODE_UI_PATH: node-ui REACT_UI_PATH: react-ui CYPRESS_RECORD_KEY: ${{ secrets.CYPRESS_RECORD_KEY }} - # TODO(hperl): Enable this once the React Native app uses the new SDK - # - name: "Run Playwright tests" - # run: | - # cd test/e2e - # npm run playwright - # env: - # DB: ${{ matrix.database }} - # RN_UI_PATH: react-native-ui - # NODE_UI_PATH: node-ui - # REACT_UI_PATH: react-ui + - if: failure() + uses: actions/upload-artifact@v2 + with: + name: logs + path: test/e2e/*.e2e.log + + test-e2e-playwright: + name: Run Playwright end-to-end tests + runs-on: ubuntu-latest + needs: + - sdk-generate + services: + postgres: + image: postgres:9.6 + env: + POSTGRES_DB: postgres + POSTGRES_PASSWORD: test + POSTGRES_USER: test + ports: + - 5432:5432 + mysql: + image: mysql:5.7 + env: + MYSQL_ROOT_PASSWORD: test + ports: + - 3306:3306 + mailslurper: + image: oryd/mailslurper:latest-smtps + ports: + - 4436:4436 + - 4437:4437 + - 1025:1025 + env: + TEST_DATABASE_POSTGRESQL: "postgres://test:test@localhost:5432/postgres?sslmode=disable" + TEST_DATABASE_MYSQL: "mysql://root:test@(localhost:3306)/mysql?parseTime=true&multiStatements=true" + TEST_DATABASE_COCKROACHDB: "cockroach://root@localhost:26257/defaultdb?sslmode=disable" + strategy: + matrix: + database: ["postgres", "cockroach", "sqlite", "mysql"] + steps: + - uses: actions/setup-node@v3 + with: + node-version: 16 + - run: | + docker create --name cockroach -p 26257:26257 \ + cockroachdb/cockroach:v22.2.6 start-single-node --insecure + docker start cockroach + name: Start CockroachDB + - uses: ory/ci/checkout@master + with: + fetch-depth: 2 + - run: | + npm ci + cd test/e2e; npm ci + npm i -g expo-cli + name: Install node deps + - run: | + sudo apt-get install -y moreutils gettext + name: Install tools + - name: Setup Go + uses: actions/setup-go@v4 + with: + go-version: "1.19" + run: go build -tags sqlite,json1 . + + - name: Install selfservice-ui-react-native + uses: actions/checkout@v3 + with: + repository: ory/kratos-selfservice-ui-react-native + path: react-native-ui + - run: | + cd react-native-ui + npm install + + - name: Install selfservice-ui-node + uses: actions/checkout@v3 + with: + repository: ory/kratos-selfservice-ui-node + path: node-ui + - run: | + cd node-ui + npm install + + - name: Install selfservice-ui-react-nextjs + uses: actions/checkout@v3 + with: + repository: ory/kratos-selfservice-ui-react-nextjs + path: react-ui + - run: | + cd react-ui + npm ci + + - run: | + echo 'RN_UI_PATH='"$(realpath react-native-ui)" >> $GITHUB_ENV + echo 'NODE_UI_PATH='"$(realpath node-ui)" >> $GITHUB_ENV + echo 'REACT_UI_PATH='"$(realpath react-ui)" >> $GITHUB_ENV + + - name: "Set up environment" + run: test/e2e/run.sh --only-setup + - name: "Run Playwright tests" + run: | + cd test/e2e + npm run playwright + env: + DB: ${{ matrix.database }} + RN_UI_PATH: react-native-ui + NODE_UI_PATH: node-ui + REACT_UI_PATH: react-ui - if: failure() uses: actions/upload-artifact@v2 with: From 346e7ef8bdb8542d1224240b0ea55e761e8aa114 Mon Sep 17 00:00:00 2001 From: Henning Perl Date: Thu, 27 Apr 2023 13:30:14 +0200 Subject: [PATCH 2/7] add cleanup for session token exchangers --- persistence/sql/persister.go | 6 ++++++ persistence/sql/persister_cleanup_test.go | 16 ++++++++++++++++ .../sql/persister_sessiontokenexchanger.go | 19 +++++++++++++++++++ .../sessiontokenexchange/persistence.go | 2 ++ 4 files changed, 43 insertions(+) diff --git a/persistence/sql/persister.go b/persistence/sql/persister.go index c49216d3f7aa..9762823b1056 100644 --- a/persistence/sql/persister.go +++ b/persistence/sql/persister.go @@ -192,6 +192,12 @@ func (p *Persister) CleanupDatabase(ctx context.Context, wait time.Duration, old } time.Sleep(wait) + p.r.Logger().Println("Cleaning up expired session token exchangers") + if err := p.DeleteExpiredExchangers(ctx, currentTime, batchSize); err != nil { + return err + } + time.Sleep(wait) + p.r.Logger().Println("Successfully cleaned up the latest batch of the SQL database! " + "This should be re-run periodically, to be sure that all expired data is purged.") return nil diff --git a/persistence/sql/persister_cleanup_test.go b/persistence/sql/persister_cleanup_test.go index 4f32159c843a..31dc2d6a1f8f 100644 --- a/persistence/sql/persister_cleanup_test.go +++ b/persistence/sql/persister_cleanup_test.go @@ -139,3 +139,19 @@ func TestPersister_Verification_Cleanup(t *testing.T) { assert.Error(t, p.DeleteExpiredVerificationFlows(ctx, currentTime, reg.Config().DatabaseCleanupBatchSize(ctx))) }) } + +func TestPersister_SessionTokenExchange_Cleanup(t *testing.T) { + _, reg := internal.NewFastRegistryWithMocks(t) + p := reg.Persister() + currentTime := time.Now() + ctx := context.Background() + + t.Run("case=should not throw error on cleanup session token exchangers", func(t *testing.T) { + assert.Nil(t, p.DeleteExpiredExchangers(ctx, currentTime, reg.Config().DatabaseCleanupBatchSize(ctx))) + }) + + t.Run("case=should throw error on cleanup session token exchangers", func(t *testing.T) { + p.GetConnection(ctx).Close() + assert.Error(t, p.DeleteExpiredExchangers(ctx, currentTime, reg.Config().DatabaseCleanupBatchSize(ctx))) + }) +} diff --git a/persistence/sql/persister_sessiontokenexchanger.go b/persistence/sql/persister_sessiontokenexchanger.go index ed9647a61c57..845027e459e7 100644 --- a/persistence/sql/persister_sessiontokenexchanger.go +++ b/persistence/sql/persister_sessiontokenexchanger.go @@ -6,6 +6,7 @@ package sql import ( "context" "fmt" + "time" "github.com/gobuffalo/pop/v6" "github.com/gofrs/uuid" @@ -106,3 +107,21 @@ func (p *Persister) MoveToNewFlow(ctx context.Context, oldFlow, newFlow uuid.UUI return sqlcon.HandleError(conn.RawQuery(query, newFlow, oldFlow, p.NetworkID(ctx)).Exec()) } + +func (p *Persister) DeleteExpiredExchangers(ctx context.Context, at time.Time, limit int) error { + expiredAfter := at.Add(1 * time.Hour) + conn := p.GetConnection(ctx) + + //#nosec G201 -- TableName is static + err := conn.RawQuery(fmt.Sprintf( + "DELETE FROM %s WHERE id in (SELECT id FROM (SELECT id FROM %s c WHERE created_at <= ? and nid = ? ORDER BY created_at ASC LIMIT %d ) AS s )", + conn.Dialect.Quote(new(sessiontokenexchange.Exchanger).TableName()), + conn.Dialect.Quote(new(sessiontokenexchange.Exchanger).TableName()), + limit, + ), + expiredAfter, + p.NetworkID(ctx), + ).Exec() + + return sqlcon.HandleError(err) +} diff --git a/selfservice/sessiontokenexchange/persistence.go b/selfservice/sessiontokenexchange/persistence.go index 986d7bd1d2cb..b19be3998ad0 100644 --- a/selfservice/sessiontokenexchange/persistence.go +++ b/selfservice/sessiontokenexchange/persistence.go @@ -42,6 +42,8 @@ type ( UpdateSessionOnExchanger(ctx context.Context, flowID uuid.UUID, sessionID uuid.UUID) error CodeForFlow(ctx context.Context, flowID uuid.UUID) (codes *Codes, found bool, err error) MoveToNewFlow(ctx context.Context, oldFlow, newFlow uuid.UUID) error + + DeleteExpiredExchangers(context.Context, time.Time, int) error } PersistenceProvider interface { From a612d65debcef690c1bf08df5f6d10064dc6c6a0 Mon Sep 17 00:00:00 2001 From: Henning Perl Date: Thu, 27 Apr 2023 13:36:22 +0200 Subject: [PATCH 3/7] fixup: ci --- .github/workflows/ci.yaml | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/.github/workflows/ci.yaml b/.github/workflows/ci.yaml index 029300a82333..f18cd059d1ed 100644 --- a/.github/workflows/ci.yaml +++ b/.github/workflows/ci.yaml @@ -264,6 +264,7 @@ jobs: - run: | npm ci cd test/e2e; npm ci + npx playwright install --with-deps npm i -g expo-cli name: Install node deps - run: | @@ -273,7 +274,7 @@ jobs: uses: actions/setup-go@v4 with: go-version: "1.19" - run: go build -tags sqlite,json1 . + - run: go build -tags sqlite,json1 . - name: Install selfservice-ui-react-native uses: actions/checkout@v3 From 7efa9a45e01ede83077a0023c6a9c90d7aa99151 Mon Sep 17 00:00:00 2001 From: Henning Perl Date: Fri, 28 Apr 2023 13:48:34 +0200 Subject: [PATCH 4/7] fix: compatibility between OIDC+code and other flows This improves the compatibility between OIDC+code and other flows such as TOTP, settings, password auth. --- selfservice/flow/login/handler.go | 15 +++++++-------- selfservice/flow/login/hook.go | 2 +- selfservice/flow/registration/error.go | 2 +- selfservice/flow/registration/hook.go | 2 +- selfservice/hook/session_issuer.go | 13 +++++++++---- session/manager.go | 3 ++- session/manager_http.go | 7 ++++++- session/session.go | 9 +++++++++ 8 files changed, 36 insertions(+), 17 deletions(-) diff --git a/selfservice/flow/login/handler.go b/selfservice/flow/login/handler.go index 7af617126ef9..1b37d126c619 100644 --- a/selfservice/flow/login/handler.go +++ b/selfservice/flow/login/handler.go @@ -135,18 +135,17 @@ func (h *Handler) NewLoginFlow(w http.ResponseWriter, r *http.Request, ft flow.T return nil, nil, errors.WithStack(herodot.ErrBadRequest.WithReasonf("Unable to parse AuthenticationMethod Assurance Level (AAL): %s", cs.ToUnknownCaseErr())) } - if ft == flow.TypeAPI && r.URL.Query().Get("return_session_token_exchange_code") == "true" { - e, err := h.d.SessionTokenExchangePersister().CreateSessionTokenExchanger(r.Context(), f.ID) - if err != nil { - return nil, nil, errors.WithStack(herodot.ErrInternalServerError.WithWrap(err)) - } - f.SessionTokenExchangeCode = e.InitCode - } - // We assume an error means the user has no session sess, err := h.d.SessionManager().FetchFromRequest(r.Context(), r) if e := new(session.ErrNoActiveSessionFound); errors.As(err, &e) { // No session exists yet + if ft == flow.TypeAPI && r.URL.Query().Get("return_session_token_exchange_code") == "true" { + e, err := h.d.SessionTokenExchangePersister().CreateSessionTokenExchanger(r.Context(), f.ID) + if err != nil { + return nil, nil, errors.WithStack(herodot.ErrInternalServerError.WithWrap(err)) + } + f.SessionTokenExchangeCode = e.InitCode + } // We can not request an AAL > 1 because we must first verify the first factor. if f.RequestedAAL > identity.AuthenticatorAssuranceLevel1 { diff --git a/selfservice/flow/login/hook.go b/selfservice/flow/login/hook.go index 2859a8731502..18a31b3b2817 100644 --- a/selfservice/flow/login/hook.go +++ b/selfservice/flow/login/hook.go @@ -183,7 +183,7 @@ func (e *HookExecutor) PostLoginHook(w http.ResponseWriter, r *http.Request, g n trace.SpanFromContext(r.Context()).AddEvent(events.NewSessionIssued(r.Context(), s.ID, i.ID)) - if handled, err := e.d.SessionManager().MaybeRedirectAPICodeFlow(w, r, a, s.ID); err != nil { + if handled, err := e.d.SessionManager().MaybeRedirectAPICodeFlow(w, r, a, s.ID, g); err != nil { return errors.WithStack(err) } else if handled { return nil diff --git a/selfservice/flow/registration/error.go b/selfservice/flow/registration/error.go index 0c9fde5384d7..697a77439c2b 100644 --- a/selfservice/flow/registration/error.go +++ b/selfservice/flow/registration/error.go @@ -136,7 +136,7 @@ func (s *ErrorHandler) WriteFlowError( http.Redirect(w, r, f.AppendTo(s.d.Config().SelfServiceFlowRegistrationUI(r.Context())).String(), http.StatusFound) return } - if _, hasCode, _ := s.d.SessionTokenExchangePersister().CodeForFlow(r.Context(), f.ID); f.Type == flow.TypeAPI && hasCode { + if _, hasCode, _ := s.d.SessionTokenExchangePersister().CodeForFlow(r.Context(), f.ID); group == node.OpenIDConnectGroup && f.Type == flow.TypeAPI && hasCode { http.Redirect(w, r, f.ReturnTo, http.StatusSeeOther) return } diff --git a/selfservice/flow/registration/hook.go b/selfservice/flow/registration/hook.go index b3a23a066602..8a41b18ae08a 100644 --- a/selfservice/flow/registration/hook.go +++ b/selfservice/flow/registration/hook.go @@ -228,7 +228,7 @@ func (e *HookExecutor) PostRegistrationHook(w http.ResponseWriter, r *http.Reque Debug("Post registration execution hooks completed successfully.") if a.Type == flow.TypeAPI || x.IsJSONRequest(r) { - if handled, err := e.d.SessionManager().MaybeRedirectAPICodeFlow(w, r, a, s.ID); err != nil { + if handled, err := e.d.SessionManager().MaybeRedirectAPICodeFlow(w, r, a, s.ID, ct.ToUiNodeGroup()); err != nil { return errors.WithStack(err) } else if handled { return nil diff --git a/selfservice/hook/session_issuer.go b/selfservice/hook/session_issuer.go index e042a56e66b0..693c74bd07d8 100644 --- a/selfservice/hook/session_issuer.go +++ b/selfservice/hook/session_issuer.go @@ -10,6 +10,9 @@ import ( "go.opentelemetry.io/otel/trace" + "github.com/ory/kratos/identity" + "github.com/ory/kratos/ui/node" + "github.com/ory/kratos/x/events" "github.com/pkg/errors" @@ -62,10 +65,12 @@ func (e *SessionIssuer) executePostRegistrationPostPersistHook(w http.ResponseWr trace.SpanFromContext(r.Context()).AddEvent(events.NewSessionIssued(r.Context(), s.ID, s.IdentityID)) if a.Type == flow.TypeAPI { - if handled, err := e.r.SessionManager().MaybeRedirectAPICodeFlow(w, r, a, s.ID); err != nil { - return errors.WithStack(err) - } else if handled { - return nil + if s.AuthenticatedVia(identity.CredentialsTypeOIDC) { + if handled, err := e.r.SessionManager().MaybeRedirectAPICodeFlow(w, r, a, s.ID, node.OpenIDConnectGroup); err != nil { + return errors.WithStack(err) + } else if handled { + return nil + } } a.AddContinueWith(flow.NewContinueWithSetToken(s.Token)) diff --git a/session/manager.go b/session/manager.go index 1d906e883112..ad1055d3b408 100644 --- a/session/manager.go +++ b/session/manager.go @@ -10,6 +10,7 @@ import ( "github.com/ory/kratos/selfservice/flow" "github.com/ory/kratos/text" + "github.com/ory/kratos/ui/node" "github.com/ory/kratos/x/swagger" "github.com/gofrs/uuid" @@ -140,7 +141,7 @@ type Manager interface { // MaybeRedirectAPICodeFlow for API+Code flows redirects the user to the return_to URL and adds the code query parameter. // `handled` is true if the request a redirect was written, false otherwise. - MaybeRedirectAPICodeFlow(w http.ResponseWriter, r *http.Request, f flow.Flow, sessionID uuid.UUID) (handled bool, err error) + MaybeRedirectAPICodeFlow(w http.ResponseWriter, r *http.Request, f flow.Flow, sessionID uuid.UUID, uiNode node.UiNodeGroup) (handled bool, err error) } type ManagementProvider interface { diff --git a/session/manager_http.go b/session/manager_http.go index b808140f2a63..fdd42977cbd6 100644 --- a/session/manager_http.go +++ b/session/manager_http.go @@ -11,6 +11,7 @@ import ( "github.com/ory/kratos/selfservice/flow" "github.com/ory/kratos/selfservice/sessiontokenexchange" + "github.com/ory/kratos/ui/node" "github.com/ory/x/otelx" "github.com/ory/x/randx" @@ -324,10 +325,14 @@ func (s *ManagerHTTP) SessionAddAuthenticationMethods(ctx context.Context, sid u return s.r.SessionPersister().UpsertSession(ctx, sess) } -func (s *ManagerHTTP) MaybeRedirectAPICodeFlow(w http.ResponseWriter, r *http.Request, f flow.Flow, sessionID uuid.UUID) (handled bool, err error) { +func (s *ManagerHTTP) MaybeRedirectAPICodeFlow(w http.ResponseWriter, r *http.Request, f flow.Flow, sessionID uuid.UUID, uiNode node.UiNodeGroup) (handled bool, err error) { ctx, span := s.r.Tracer(r.Context()).Tracer().Start(r.Context(), "sessions.ManagerHTTP.MaybeRedirectAPICodeFlow") defer otelx.End(span, &err) + if uiNode != node.OpenIDConnectGroup { + return false, nil + } + code, ok, _ := s.r.SessionTokenExchangePersister().CodeForFlow(ctx, f.GetID()) if !ok { return false, nil diff --git a/session/session.go b/session/session.go index 9638f410ae1c..294f1bec6db3 100644 --- a/session/session.go +++ b/session/session.go @@ -158,6 +158,15 @@ func (s *Session) CompletedLoginFor(method identity.CredentialsType, aal identit s.AMR = append(s.AMR, AuthenticationMethod{Method: method, AAL: aal, CompletedAt: time.Now().UTC()}) } +func (s *Session) AuthenticatedVia(method identity.CredentialsType) bool { + for _, authMethod := range s.AMR { + if authMethod.Method == method { + return true + } + } + return false +} + func (s *Session) SetAuthenticatorAssuranceLevel() { if len(s.AMR) == 0 { // No AMR is set From 3662e7c71cbb9977f37c8f7b6f4e4cc3c6eca22d Mon Sep 17 00:00:00 2001 From: Henning Perl Date: Fri, 28 Apr 2023 20:00:43 +0200 Subject: [PATCH 5/7] Update persistence/sql/persister_cleanup_test.go Co-authored-by: Jonas Hungershausen --- persistence/sql/persister_cleanup_test.go | 2 +- test/e2e/profiles/kratos.base.yml | 2 ++ 2 files changed, 3 insertions(+), 1 deletion(-) diff --git a/persistence/sql/persister_cleanup_test.go b/persistence/sql/persister_cleanup_test.go index 31dc2d6a1f8f..65e95ea6ea00 100644 --- a/persistence/sql/persister_cleanup_test.go +++ b/persistence/sql/persister_cleanup_test.go @@ -150,7 +150,7 @@ func TestPersister_SessionTokenExchange_Cleanup(t *testing.T) { assert.Nil(t, p.DeleteExpiredExchangers(ctx, currentTime, reg.Config().DatabaseCleanupBatchSize(ctx))) }) - t.Run("case=should throw error on cleanup session token exchangers", func(t *testing.T) { + t.Run("case=should throw error on cleanup session token exchangers if DB is closed", func(t *testing.T) { p.GetConnection(ctx).Close() assert.Error(t, p.DeleteExpiredExchangers(ctx, currentTime, reg.Config().DatabaseCleanupBatchSize(ctx))) }) diff --git a/test/e2e/profiles/kratos.base.yml b/test/e2e/profiles/kratos.base.yml index 3b64785421f7..9df17bb07fd7 100644 --- a/test/e2e/profiles/kratos.base.yml +++ b/test/e2e/profiles/kratos.base.yml @@ -10,6 +10,8 @@ selfservice: default_browser_return_url: http://localhost:4455/ allowed_return_urls: - http://localhost:4455 + - http://localhost:4457/Callback + - exp://example.com/Callback - https://www.ory.sh/ - https://example.org/ - https://www.example.org/ From 0c503d0a9a0c946d14afacaa69eae5915704cc2f Mon Sep 17 00:00:00 2001 From: Henning Perl Date: Tue, 2 May 2023 10:12:14 +0200 Subject: [PATCH 6/7] fix: error handling with OIDC+Code --- selfservice/flow/login/error.go | 3 ++- selfservice/flow/login/handler.go | 2 -- 2 files changed, 2 insertions(+), 3 deletions(-) diff --git a/selfservice/flow/login/error.go b/selfservice/flow/login/error.go index 2ac919b49e96..ebbc7a468634 100644 --- a/selfservice/flow/login/error.go +++ b/selfservice/flow/login/error.go @@ -125,7 +125,8 @@ func (s *ErrorHandler) WriteFlowError(w http.ResponseWriter, r *http.Request, f return } - if _, hasCode, _ := s.d.SessionTokenExchangePersister().CodeForFlow(r.Context(), f.ID); f.Type == flow.TypeAPI && hasCode { + _, hasCode, _ := s.d.SessionTokenExchangePersister().CodeForFlow(r.Context(), f.ID) + if f.Type == flow.TypeAPI && hasCode && group == node.OpenIDConnectGroup { http.Redirect(w, r, f.ReturnTo, http.StatusSeeOther) return } diff --git a/selfservice/flow/login/handler.go b/selfservice/flow/login/handler.go index 1b37d126c619..3e4be7d2221c 100644 --- a/selfservice/flow/login/handler.go +++ b/selfservice/flow/login/handler.go @@ -42,8 +42,6 @@ const ( RouteGetFlow = "/self-service/login/flows" RouteSubmitFlow = "/self-service/login" - - RouteExchangeSessionToken = "/self-service/login/exchange-session-token" //nolint:gosec ) type ( From a258713d49f532a851af4256b3dd687a6ece2817 Mon Sep 17 00:00:00 2001 From: Henning Perl Date: Tue, 2 May 2023 10:52:42 +0200 Subject: [PATCH 7/7] fix: increase playwright timeout --- test/e2e/playwright.config.ts | 1 + 1 file changed, 1 insertion(+) diff --git a/test/e2e/playwright.config.ts b/test/e2e/playwright.config.ts index f5a8e00ccd25..663b48e04de3 100644 --- a/test/e2e/playwright.config.ts +++ b/test/e2e/playwright.config.ts @@ -44,6 +44,7 @@ export default defineConfig({ url: "http://localhost:4433/health/ready", reuseExistingServer: false, env: { DSN: dbToDsn() }, + timeout: 5 * 60 * 1000, // 5 minutes }, ], })