From b3f36f0a1cb01250d80a6c7d296609cd5489604c Mon Sep 17 00:00:00 2001 From: aeneasr Date: Mon, 17 Feb 2020 18:12:24 +0100 Subject: [PATCH 1/2] fix: Resolve several verification problems --- .docker/mailslurper/Dockerfile | 18 -------- .docker/mailslurper/config.json | 20 --------- .docker/mailslurper/mailslurper-cert.pem | 15 ------- .docker/mailslurper/mailslurper-key.pem | 15 ------- .docker/mailslurper/mailslurper.csr | 12 ----- Makefile | 2 +- cmd/daemon/serve.go | 4 +- cmd/serve.go | 1 + contrib/quickstart/kratos/sqlite/.gitignore | 1 + courier/courier.go | 16 ++++++- courier/courier_test.go | 4 +- identity/manager_test.go | 4 ++ identity/pool.go | 11 ++++- persistence/sql/persister.go | 4 ++ persistence/sql/persister_identity.go | 22 ++++++--- persistence/sql/persister_session.go | 6 ++- quickstart.yml | 17 +++---- selfservice/flow/verify/handler.go | 50 ++++++++++++++++++--- selfservice/flow/verify/handler_test.go | 49 ++++++++++++++++---- selfservice/flow/verify/sender.go | 28 +++++++----- selfservice/flow/verify/sender_test.go | 14 +++++- selfservice/hook/verify.go | 6 ++- 22 files changed, 186 insertions(+), 133 deletions(-) delete mode 100644 .docker/mailslurper/Dockerfile delete mode 100644 .docker/mailslurper/config.json delete mode 100644 .docker/mailslurper/mailslurper-cert.pem delete mode 100644 .docker/mailslurper/mailslurper-key.pem delete mode 100644 .docker/mailslurper/mailslurper.csr create mode 100644 contrib/quickstart/kratos/sqlite/.gitignore diff --git a/.docker/mailslurper/Dockerfile b/.docker/mailslurper/Dockerfile deleted file mode 100644 index bec4344d8915..000000000000 --- a/.docker/mailslurper/Dockerfile +++ /dev/null @@ -1,18 +0,0 @@ -FROM golang:1.13-alpine as builder - -RUN apk --no-cache add git libc-dev gcc -RUN go get github.com/mjibson/esc - -WORKDIR /go/src/github.com/mailslurper/mailslurper -RUN git clone https://github.com/mailslurper/mailslurper.git . - -ADD . cmd/mailslurper - -RUN cd cmd/mailslurper && \ - go get -v && \ - go generate && \ - go install -v - -ADD . . - -ENTRYPOINT mailslurper diff --git a/.docker/mailslurper/config.json b/.docker/mailslurper/config.json deleted file mode 100644 index b8b0b0e7935e..000000000000 --- a/.docker/mailslurper/config.json +++ /dev/null @@ -1,20 +0,0 @@ -{ - "wwwAddress": "0.0.0.0", - "wwwPort": 4436, - "serviceAddress": "0.0.0.0", - "servicePort": 4437, - "smtpAddress": "0.0.0.0", - "smtpPort": 1025, - "dbEngine": "SQLite", - "dbHost": "", - "dbPort": 0, - "dbDatabase": "mailslurper.db", - "dbUserName": "", - "dbPassword": "", - "maxWorkers": 1000, - "autoStartBrowser": false, - "keyFile": "mailslurper-key.pem", - "certFile": "mailslurper-cert.pem", - "adminKeyFile": "", - "adminCertFile": "" -} diff --git a/.docker/mailslurper/mailslurper-cert.pem b/.docker/mailslurper/mailslurper-cert.pem deleted file mode 100644 index 232bf0921dc1..000000000000 --- a/.docker/mailslurper/mailslurper-cert.pem +++ /dev/null @@ -1,15 +0,0 @@ ------BEGIN CERTIFICATE----- -MIICYzCCAcwCCQCsySmDdu0W0DANBgkqhkiG9w0BAQUFADB2MQswCQYDVQQGEwJB -QjEMMAoGA1UECAwDb3J5MQwwCgYDVQQHDANvcnkxETAPBgNVBAoMCG9yeSBjb3Jw -MQwwCgYDVQQLDANvcnkxDDAKBgNVBAMMA29yeTEcMBoGCSqGSIb3DQEJARYNb2Zm -aWNlQG9yeS5zaDAeFw0yMDAyMTYxNzE1NTJaFw0yMDAzMTcxNzE1NTJaMHYxCzAJ -BgNVBAYTAkFCMQwwCgYDVQQIDANvcnkxDDAKBgNVBAcMA29yeTERMA8GA1UECgwI -b3J5IGNvcnAxDDAKBgNVBAsMA29yeTEMMAoGA1UEAwwDb3J5MRwwGgYJKoZIhvcN -AQkBFg1vZmZpY2VAb3J5LnNoMIGfMA0GCSqGSIb3DQEBAQUAA4GNADCBiQKBgQCn -gzB9hUAjSlqB+IgXwzGdHcTrR2dQtVqq7d407RK9WWRoDN9UpCzW+sW+BUtbdN5g -zpqsffc0rE8ic4ADQR4ZU9pIljoWHLjPEKEamC/EUIuFGj7ivipA2R9LUx+SuElE -9YNw0KQLi12xobGZBmaP/6yb0yU0J4RwY0L7r/DY+wIDAQABMA0GCSqGSIb3DQEB -BQUAA4GBAA6sGmFjHxFQSo0yph7/DHuDJjsd5dYQSr6MH3eEedag1LTrXV7aLKfS -h9oR8BioJL8YpmE259avefTo/gKh29yEkL+RS6I+/S401nqOnvD/fidf8vy0NWWP -pK6UBcLU6q4EbwRUbSO719joFL4gpWQpBM3/oi4NJPGiry+XXEdU ------END CERTIFICATE----- diff --git a/.docker/mailslurper/mailslurper-key.pem b/.docker/mailslurper/mailslurper-key.pem deleted file mode 100644 index 9ed13e5e2145..000000000000 --- a/.docker/mailslurper/mailslurper-key.pem +++ /dev/null @@ -1,15 +0,0 @@ ------BEGIN RSA PRIVATE KEY----- -MIICXAIBAAKBgQCngzB9hUAjSlqB+IgXwzGdHcTrR2dQtVqq7d407RK9WWRoDN9U -pCzW+sW+BUtbdN5gzpqsffc0rE8ic4ADQR4ZU9pIljoWHLjPEKEamC/EUIuFGj7i -vipA2R9LUx+SuElE9YNw0KQLi12xobGZBmaP/6yb0yU0J4RwY0L7r/DY+wIDAQAB -AoGAS8m0H6Yw/YZ/anxafn3GrbIxTM2ydbaHffw+4607JEYUgmsIhA//ZYhx/OYC -US+QCTaQjmgzdzZaW1jsWwyUI9/JFMql4HnQRtbb9+iDt+9bKPsl8j2C0r7DLfem -A8ahO4l+Ya+wEiy9Wkz3TB6hA97g6LT4RYnjqWt0rtxGjrECQQDZsIi9wu+Kirh6 -isZZmOj8ME4O8nYLMM3E7jtKLf1zo9K8dPxLmpwNsyGrB1zvdWM4vkB2xPGNgkRP -GnksrYXJAkEAxP4MhAwZdYAyyqjw8Xv51zNH6XAAQ3xnhZaZH5uEmXJ2Wii3sJip -kDLLWAgQ2jGWUTYkQtrSIMeqxQvMvwxaowJBALHTo+Bf/Y4PA+QWuTE32Bsq2pkb -N5Ksq2rTsVtHdmOgz+VjKzYXdqM6UaaEvUZffk48HCzpdOlEIMj9tz7oAQECQBP3 -uGcOHuqFIyDdvQaNTYbdwNVNsAknLAsjd8P3bJptOsfqxqvU3aMrMudqceLcEeOL -fGN2cMQ32Px+NLPM2ccCQFJRyXn6Qkcmk+1x413G++KsUueSjKmsB5SrOLd1Khqr -yCb+GyePuATqIhHyrZd9cqL812GEe61z7798wro+Rm8= ------END RSA PRIVATE KEY----- diff --git a/.docker/mailslurper/mailslurper.csr b/.docker/mailslurper/mailslurper.csr deleted file mode 100644 index 3d49608ae7e3..000000000000 --- a/.docker/mailslurper/mailslurper.csr +++ /dev/null @@ -1,12 +0,0 @@ ------BEGIN CERTIFICATE REQUEST----- -MIIBtjCCAR8CAQAwdjELMAkGA1UEBhMCQUIxDDAKBgNVBAgMA29yeTEMMAoGA1UE -BwwDb3J5MREwDwYDVQQKDAhvcnkgY29ycDEMMAoGA1UECwwDb3J5MQwwCgYDVQQD -DANvcnkxHDAaBgkqhkiG9w0BCQEWDW9mZmljZUBvcnkuc2gwgZ8wDQYJKoZIhvcN -AQEBBQADgY0AMIGJAoGBAKeDMH2FQCNKWoH4iBfDMZ0dxOtHZ1C1Wqrt3jTtEr1Z -ZGgM31SkLNb6xb4FS1t03mDOmqx99zSsTyJzgANBHhlT2kiWOhYcuM8QoRqYL8RQ -i4UaPuK+KkDZH0tTH5K4SUT1g3DQpAuLXbGhsZkGZo//rJvTJTQnhHBjQvuv8Nj7 -AgMBAAGgADANBgkqhkiG9w0BAQsFAAOBgQBSTle705N73CG+xn5VHVfOfGyZE1Lr -EDEjZa4okk5QmgFOPdDQ7yMJbJROW0ysSnIyVcmQLqFvYgFDhh/CpMLGVmZDI3IU -UvGAt6PNRpNr0ldVK8enkqgkhGDhpybOQMCImRMdxcXYEtstIIU/r33usCjgSryl -zYgE1ZQfXu2xLw== ------END CERTIFICATE REQUEST----- diff --git a/Makefile b/Makefile index 372418507809..9bf3bb84f270 100644 --- a/Makefile +++ b/Makefile @@ -84,7 +84,7 @@ quickstart: quickstart-dev: docker build -f .docker/Dockerfile-build -t oryd/kratos:latest-sqlite . - docker pull oryd/kratos-selfservice-ui-node:latest + # docker pull oryd/kratos-selfservice-ui-node:latest docker-compose -f quickstart.yml up --build --force-recreate # Formats the code diff --git a/cmd/daemon/serve.go b/cmd/daemon/serve.go index c4d88a9fec27..2745884677f3 100644 --- a/cmd/daemon/serve.go +++ b/cmd/daemon/serve.go @@ -150,8 +150,8 @@ func sqa(cmd *cobra.Command, d driver.Driver) *metricsx.Service { profile.PublicProfileManagementPath, profile.PublicProfileManagementUpdatePath, verify.PublicVerificationCompletePath, - strings.ReplaceAll(verify.PublicVerificationConfirmPath, ":code", ""), - strings.ReplaceAll(verify.PublicVerificationInitPath, ":via", ""), + strings.ReplaceAll(strings.ReplaceAll(verify.PublicVerificationConfirmPath, ":via", "email"), ":code", ""), + strings.ReplaceAll(verify.PublicVerificationInitPath, ":via", "email"), verify.PublicVerificationRequestPath, errorx.ErrorsPath, }, diff --git a/cmd/serve.go b/cmd/serve.go index b350339da0a3..78d85c9a3c43 100644 --- a/cmd/serve.go +++ b/cmd/serve.go @@ -21,6 +21,7 @@ import ( "github.com/fsnotify/fsnotify" "github.com/gobuffalo/packr/v2" + "github.com/ory/viper" "github.com/ory/x/flagx" "github.com/ory/x/viperx" diff --git a/contrib/quickstart/kratos/sqlite/.gitignore b/contrib/quickstart/kratos/sqlite/.gitignore new file mode 100644 index 000000000000..9b1dffd90fdc --- /dev/null +++ b/contrib/quickstart/kratos/sqlite/.gitignore @@ -0,0 +1 @@ +*.sqlite diff --git a/courier/courier.go b/courier/courier.go index 8fbbc2aa26de..d8c5b6e501a6 100644 --- a/courier/courier.go +++ b/courier/courier.go @@ -12,6 +12,8 @@ import ( "github.com/pkg/errors" "gopkg.in/gomail.v2" + "github.com/ory/x/errorsx" + "github.com/ory/kratos/driver/configuration" "github.com/ory/kratos/x" ) @@ -99,11 +101,14 @@ func (m *Courier) watchMessages(ctx context.Context, errChan chan error) { if err := backoff.Retry(func() error { messages, err := m.d.CourierPersister().NextMessages(ctx, 10) if err != nil { + if errorsx.Cause(err) == ErrQueueEmpty { + return nil + } return err } for k := range messages { - var msg Message = messages[k] + var msg = messages[k] switch msg.Type { case MessageTypeEmail: @@ -120,7 +125,8 @@ func (m *Courier) watchMessages(ctx context.Context, errChan chan error) { WithError(err). WithField("smtp_server", fmt.Sprintf("%s:%d", m.dialer.Host, m.dialer.Port)). WithField("smtp_ssl_enabled", m.dialer.SSL). - WithField("email_to", msg.Recipient).WithField("email_from", from). + // WithField("email_to", msg.Recipient). + WithField("message_from", from). Error("Unable to send email using SMTP connection.") continue } @@ -132,6 +138,12 @@ func (m *Courier) watchMessages(ctx context.Context, errChan chan error) { Error(`Unable to set the message status to "sent".`) return err } + + m.d.Logger(). + WithField("message_id", msg.ID). + WithField("message_type", msg.Type). + WithField("message_subject", msg.Subject). + Debug("Courier sent out message.") default: return errors.Errorf("received unexpected message type: %d", msg.Type) } diff --git a/courier/courier_test.go b/courier/courier_test.go index 586e9877240c..9de5c02f2555 100644 --- a/courier/courier_test.go +++ b/courier/courier_test.go @@ -95,8 +95,8 @@ func TestSMTP(t *testing.T) { } smtp, api := runTestSMTP(t) - t.Logf("SMTP URL: %s",smtp) - t.Logf("API URL: %s",api) + t.Logf("SMTP URL: %s", smtp) + t.Logf("API URL: %s", api) conf, reg := internal.NewRegistryDefault(t) viper.Set(configuration.ViperKeyCourierSMTPURL, smtp) diff --git a/identity/manager_test.go b/identity/manager_test.go index 8da495520e95..8288de26e5d4 100644 --- a/identity/manager_test.go +++ b/identity/manager_test.go @@ -91,5 +91,9 @@ func TestManager(t *testing.T) { require.NoError(t, reg.IdentityManager().RefreshVerifyAddress(context.Background(), address)) assert.NotEqual(t, pc, address.Code) assert.NotEqual(t, ea, address.ExpiresAt) + + fromStore, err := reg.IdentityPool().GetIdentity(context.Background(), original.ID) + require.NoError(t, err) + assert.NotEqual(t, pc, fromStore.Addresses[0].Code) }) } diff --git a/identity/pool.go b/identity/pool.go index 00383b07518a..8502c195effe 100644 --- a/identity/pool.go +++ b/identity/pool.go @@ -433,8 +433,17 @@ func TestPool(p PrivilegedPool) func(t *testing.T) { } }) + t.Run("case=verify expired should not work", func(t *testing.T) { + address := createIdentityWithAddresses(t, -time.Minute, "verify.TestPersister.VerifyAddress.expired@ory.sh") + require.EqualError(t, errorsx.Cause(p.VerifyAddress(context.Background(), address.Code)), sqlcon.ErrNoRows.Error()) + }) + + t.Run("case=create and verify", func(t *testing.T) { + require.EqualError(t, errorsx.Cause(p.VerifyAddress(context.Background(), "i-do-not-exist")), sqlcon.ErrNoRows.Error()) + }) + t.Run("case=create and verify", func(t *testing.T) { - address := createIdentityWithAddresses(t, time.Minute, "verify.TestPersister.VerifyAddress@ory.sh") + address := createIdentityWithAddresses(t, time.Minute, "verify.TestPersister.VerifyAddress.valid@ory.sh") require.NoError(t, p.VerifyAddress(context.Background(), address.Code)) actual, err := p.FindAddressByValue(context.Background(), address.Via, address.Value) diff --git a/persistence/sql/persister.go b/persistence/sql/persister.go index 2db32251297d..706f5ee03dad 100644 --- a/persistence/sql/persister.go +++ b/persistence/sql/persister.go @@ -41,6 +41,10 @@ func NewPersister(r persisterDependencies, conf configuration.Provider, c *pop.C return &Persister{c: c, mb: m, cf: conf, r: r}, nil } +func (p *Persister) BeginTX() (context.Context, error) { + panic("Ni") +} + func (p *Persister) MigrationStatus(c context.Context, w io.Writer) error { return errors.WithStack(p.mb.Status(w)) } diff --git a/persistence/sql/persister_identity.go b/persistence/sql/persister_identity.go index 0d009512daf7..89203e2e27fe 100644 --- a/persistence/sql/persister_identity.go +++ b/persistence/sql/persister_identity.go @@ -165,7 +165,9 @@ func (p *Persister) ListIdentities(ctx context.Context, limit, offset int) ([]id is := make([]identity.Identity, 0) /* #nosec G201 TableName is static */ - if err := sqlcon.HandleError(p.c.RawQuery(fmt.Sprintf("SELECT * FROM %s LIMIT ? OFFSET ?", new(identity.Identity).TableName()), limit, offset).All(&is)); err != nil { + if err := sqlcon.HandleError(p.c. + RawQuery(fmt.Sprintf("SELECT * FROM %s LIMIT ? OFFSET ?", new(identity.Identity).TableName()), limit, offset). + Eager("Addresses").All(&is)); err != nil { return nil, err } @@ -226,7 +228,7 @@ func (p *Persister) DeleteIdentity(ctx context.Context, id uuid.UUID) error { func (p *Persister) GetIdentity(ctx context.Context, id uuid.UUID) (*identity.Identity, error) { var i identity.Identity - if err := p.c.Find(&i, id); err != nil { + if err := p.c.Eager("Addresses").Find(&i, id); err != nil { return nil, sqlcon.HandleError(err) } i.Credentials = nil @@ -299,17 +301,27 @@ func (p *Persister) VerifyAddress(ctx context.Context, code string) error { return err } - return sqlcon.HandleError(p.c.RawQuery( + count, err := p.c.RawQuery( /* #nosec G201 TableName is static */ fmt.Sprintf( - "UPDATE %s SET status = ?, verified = true, verified_at = ?, code = ? WHERE code = ?", + "UPDATE %s SET status = ?, verified = true, verified_at = ?, code = ? WHERE code = ? AND expires_at > ?", new(identity.VerifiableAddress).TableName(), ), identity.VerifiableAddressStatusCompleted, time.Now().UTC().Round(time.Second), newCode, code, - ).Exec()) + time.Now().UTC(), + ).ExecWithCount() + if err != nil { + return sqlcon.HandleError(err) + } + + if count == 0 { + return sqlcon.HandleError(sqlcon.ErrNoRows) + } + + return nil } func (p *Persister) UpdateVerifiableAddress(ctx context.Context, address *identity.VerifiableAddress) error { diff --git a/persistence/sql/persister_session.go b/persistence/sql/persister_session.go index 308e63759351..7878614dcfb0 100644 --- a/persistence/sql/persister_session.go +++ b/persistence/sql/persister_session.go @@ -14,12 +14,14 @@ var _ session.Persister = new(Persister) func (p *Persister) GetSession(ctx context.Context, sid uuid.UUID) (*session.Session, error) { var s session.Session - if err := p.c.Eager().Find(&s, sid); err != nil { + if err := p.c.Find(&s, sid); err != nil { return nil, sqlcon.HandleError(err) } - if err := p.injectTraitsSchemaURL(s.Identity); err != nil { + i, err := p.GetIdentity(ctx, s.IdentityID) + if err != nil { return nil, err } + s.Identity = i return &s, nil } diff --git a/quickstart.yml b/quickstart.yml index cf14c80f1293..8ab3343a6c6b 100644 --- a/quickstart.yml +++ b/quickstart.yml @@ -12,10 +12,9 @@ services: - DSN=sqlite:///home/ory/sqlite/db.sqlite?_fk=true volumes: - - type: volume - source: kratos-sqlite + type: bind + source: ./contrib/quickstart/kratos/sqlite target: /home/ory/sqlite - read_only: false - type: bind source: ./contrib/quickstart/kratos/email-password @@ -75,10 +74,9 @@ services: serve -c /etc/config/kratos/.kratos.yml --dev volumes: - - type: volume - source: kratos-sqlite + type: bind + source: ./contrib/quickstart/kratos/sqlite target: /home/ory/sqlite - read_only: false - type: bind source: ./contrib/quickstart/kratos/email-password @@ -87,8 +85,7 @@ services: - intranet mailslurper: - build: - context: .docker/mailslurper + image: oryd/mailslurper:latest-smtps ports: - "4436:4436" - "4437:4437" @@ -98,5 +95,5 @@ services: networks: intranet: -volumes: - kratos-sqlite: +#volumes: +# kratos-sqlite: diff --git a/selfservice/flow/verify/handler.go b/selfservice/flow/verify/handler.go index 9902ea41ee4f..765580dcf484 100644 --- a/selfservice/flow/verify/handler.go +++ b/selfservice/flow/verify/handler.go @@ -8,6 +8,9 @@ import ( "github.com/justinas/nosurf" "github.com/pkg/errors" + "github.com/ory/x/errorsx" + "github.com/ory/x/sqlcon" + "github.com/ory/herodot" "github.com/ory/jsonschema/v3" "github.com/ory/x/urlx" @@ -16,14 +19,15 @@ import ( "github.com/ory/kratos/identity" "github.com/ory/kratos/schema" "github.com/ory/kratos/selfservice/errorx" + "github.com/ory/kratos/selfservice/form" "github.com/ory/kratos/x" ) const ( - PublicVerificationInitPath = "/self-service/browser/flows/verification/init/:via" + PublicVerificationInitPath = "/self-service/browser/flows/verification/:via" PublicVerificationCompletePath = "/self-service/browser/flows/verification/complete" PublicVerificationRequestPath = "/self-service/browser/flows/requests/verification" - PublicVerificationConfirmPath = "/self-service/browser/flows/verification/confirm/:code" + PublicVerificationConfirmPath = "/self-service/browser/flows/verification/:via/confirm/:code" ) type ( @@ -258,9 +262,11 @@ func (h *Handler) completeViaEmail(w http.ResponseWriter, r *http.Request, vr *R return } - if err := h.d.VerificationSender().SendCode(r.Context(), identity.VerifiableAddressTypeEmail, to); err != nil { - h.handleError(w, r, vr, err) - return + if _, err := h.d.VerificationSender().SendCode(r.Context(), identity.VerifiableAddressTypeEmail, to); err != nil { + if errorsx.Cause(err) != ErrUnknownAddress { + h.handleError(w, r, vr, err) + return + } } vr.Form = nil @@ -279,9 +285,17 @@ type selfServiceBrowserVerifyParameters struct { // required: true // in: path Code string `json:"code"` + + // What to verify + // + // Currently only "email" is supported. + // + // required: true + // in: path + Via string `json:"via"` } -// swagger:route GET /self-service/browser/flows/verification/confirm/{code} public selfServiceBrowserVerify +// swagger:route GET /self-service/browser/flows/verification/:via/confirm/{code} public selfServiceBrowserVerify // // Complete the browser-based verification flows // @@ -301,7 +315,31 @@ type selfServiceBrowserVerifyParameters struct { // 302: emptyResponse // 500: genericError func (h *Handler) verify(w http.ResponseWriter, r *http.Request, ps httprouter.Params) { + via, err := h.toVia(ps) + if err != nil { + h.handleError(w, r, nil, err) + return + } + if err := h.d.PrivilegedIdentityPool().VerifyAddress(r.Context(), ps.ByName("code")); err != nil { + if errorsx.Cause(err) == sqlcon.ErrNoRows { + a := NewRequest( + h.c.SelfServiceProfileRequestLifespan(), r, via, + urlx.AppendPaths(h.c.SelfPublicURL(), PublicVerificationCompletePath), h.d.GenerateCSRFToken, + ) + a.Form.AddError(&form.Error{Message: "The verification code has expired or was otherwise invalid. Please request another code."}) + + if err := h.d.VerificationPersister().CreateVerifyRequest(r.Context(), a); err != nil { + h.handleError(w, r, nil, err) + return + } + + http.Redirect(w, r, + urlx.CopyWithQuery(h.c.VerificationURL(), url.Values{"request": {a.ID.String()}}).String(), + http.StatusFound, + ) + } + h.d.SelfServiceErrorManager().Forward(r.Context(), w, r, err) return } diff --git a/selfservice/flow/verify/handler_test.go b/selfservice/flow/verify/handler_test.go index 6f8ce5a1a805..87c37901678f 100644 --- a/selfservice/flow/verify/handler_test.go +++ b/selfservice/flow/verify/handler_test.go @@ -6,6 +6,7 @@ import ( "net/http" "net/http/httptest" "net/url" + "regexp" "strings" "testing" "time" @@ -153,14 +154,8 @@ func TestHandler(t *testing.T) { to string subject string }{ - "tracked": { - to: "does-not-exist@ory.sh", - subject: "tried to verify", - }, - "untracked": { - to: "exists@ory.sh", - subject: "Please verify", - }, + "untracked": {to: "does-not-exist@ory.sh", subject: "tried to verify"}, + "tracked": {to: "exists@ory.sh", subject: "Please verify"}, } { t.Run("case=request verify of "+name+" address", func(t *testing.T) { hc := &http.Client{Jar: x.EasyCookieJar(t, nil)} @@ -194,6 +189,44 @@ func TestHandler(t *testing.T) { }) } + t.Run("case=verify address", func(t *testing.T) { + hc := &http.Client{Jar: x.EasyCookieJar(t, nil)} + svr, err := publicClient.Common.GetSelfServiceVerificationRequest(common. + NewGetSelfServiceVerificationRequestParams().WithHTTPClient(hc). + WithRequest(string(x.EasyGetBody(t, hc, initURL)))) + require.NoError(t, err) + + _, err = hc.PostForm(genForm(t, svr, "exists@ory.sh")) + require.NoError(t, err) + m, err := reg.CourierPersister().LatestQueuedMessage(context.Background()) + require.NoError(t, err) + + match := regexp.MustCompile(``).FindStringSubmatch(m.Body) + require.Len(t, match, 2) + + res, err := hc.Get(match[1]) + require.NoError(t, err) + + assert.Equal(t, redirTS.URL, res.Request.URL.String()) + assert.Equal(t, http.StatusNoContent, res.StatusCode) + }) + + t.Run("case=verify unknown code", func(t *testing.T) { + hc := &http.Client{Jar: x.EasyCookieJar(t, nil)} + res, _ := x.EasyGet(t, hc, + publicTS.URL+strings.ReplaceAll( + strings.ReplaceAll(verify.PublicVerificationConfirmPath, ":code", "unknown-code"), + ":via", "email")) + assert.Contains(t, res.Request.URL.String(), verifyTS.URL) + + rid := res.Request.URL.Query().Get("request") + require.NotEmpty(t, rid) + + svr, err := adminClient.Common.GetSelfServiceVerificationRequest(common.NewGetSelfServiceVerificationRequestParams().WithRequest(rid)) + require.NoError(t, err) + assert.Equal(t, "The verification code has expired or was otherwise invalid. Please request another code.", svr.Payload.Form.Errors[0].Message) + }) + t.Run("case=complete expired", func(t *testing.T) { hc := &http.Client{Jar: x.EasyCookieJar(t, nil)} rid := string(x.EasyGetBody(t, hc, initURL)) diff --git a/selfservice/flow/verify/sender.go b/selfservice/flow/verify/sender.go index 319eb637dea3..27863fd422a5 100644 --- a/selfservice/flow/verify/sender.go +++ b/selfservice/flow/verify/sender.go @@ -17,6 +17,8 @@ import ( "github.com/ory/kratos/x" ) +var ErrUnknownAddress = errors.New("verification requested for unknown address") + type ( senderDependencies interface { courier.Provider @@ -37,25 +39,31 @@ func NewSender(r senderDependencies, c configuration.Provider) *Sender { return &Sender{r: r, c: c} } -func (m *Sender) SendCode(ctx context.Context, via identity.VerifiableAddressType, value string) error { +// SendCode sends a code to the specified address. If the address does not exist in the store, an email is +// still being sent to prevent account enumeration attacks. In that case, this function returns the ErrUnknownAddress +// error. +func (m *Sender) SendCode(ctx context.Context, via identity.VerifiableAddressType, value string) (*identity.VerifiableAddress, error) { m.r.Logger().WithField("via", via).Debug("Sending out verification code.") address, err := m.r.IdentityPool().FindAddressByValue(ctx, via, value) if err != nil { if errorsx.Cause(err) == sqlcon.ErrNoRows { if err := m.sendToUnknownAddress(ctx, identity.VerifiableAddressTypeEmail, value); err != nil { - return err + return nil, err } - return nil + return nil, errors.Cause(ErrUnknownAddress) } - return err + return nil, err } if err := m.r.IdentityManager().RefreshVerifyAddress(ctx, address); err != nil { - return err + return nil, err } - return m.sendCodeToKnownAddress(ctx, address) + if err := m.sendCodeToKnownAddress(ctx, address); err != nil { + return nil, err + } + return address, nil } func (m *Sender) sendToUnknownAddress(ctx context.Context, via identity.VerifiableAddressType, address string) error { @@ -75,10 +83,10 @@ func (m *Sender) sendCodeToKnownAddress(ctx context.Context, address *identity.V To: address.Value, VerifyURL: urlx.AppendPaths( m.c.SelfPublicURL(), - strings.Replace( - PublicVerificationConfirmPath, ":code", - address.Code, 1, - )).String(), + strings.ReplaceAll( + strings.ReplaceAll(PublicVerificationConfirmPath, ":via", string(address.Via)), + ":code", address.Code)). + String(), }, )) return err diff --git a/selfservice/flow/verify/sender_test.go b/selfservice/flow/verify/sender_test.go index 25c1c2c1ae8d..a8000d1e9480 100644 --- a/selfservice/flow/verify/sender_test.go +++ b/selfservice/flow/verify/sender_test.go @@ -13,6 +13,7 @@ import ( "github.com/ory/kratos/driver/configuration" "github.com/ory/kratos/identity" "github.com/ory/kratos/internal" + "github.com/ory/kratos/selfservice/flow/verify" ) func TestManager(t *testing.T) { @@ -31,8 +32,11 @@ func TestManager(t *testing.T) { i.Traits = identity.Traits("{}") require.NoError(t, reg.PrivilegedIdentityPool().CreateIdentity(context.Background(), i)) - require.NoError(t, reg.VerificationSender().SendCode(context.Background(), address.Via, address.Value)) - require.NoError(t, reg.VerificationSender().SendCode(context.Background(), address.Via, "not-tracked@ory.sh")) + address, err = reg.VerificationSender().SendCode(context.Background(), address.Via, address.Value) + require.NoError(t, err) + + _, err = reg.VerificationSender().SendCode(context.Background(), address.Via, "not-tracked@ory.sh") + require.EqualError(t, err, verify.ErrUnknownAddress.Error()) messages, err := reg.CourierPersister().NextMessages(context.Background(), 12) require.NoError(t, err) @@ -40,6 +44,12 @@ func TestManager(t *testing.T) { assert.EqualValues(t, address.Value, messages[0].Recipient) assert.Contains(t, messages[0].Subject, "Please verify") + + assert.Contains(t, messages[0].Body, address.Code) + fromStore, err := reg.Persister().GetIdentity(context.Background(), i.ID) + require.NoError(t, err) + assert.Contains(t, messages[0].Body, fromStore.Addresses[0].Code) + assert.EqualValues(t, "not-tracked@ory.sh", messages[1].Recipient) assert.Contains(t, messages[1].Subject, "tried to verify") }) diff --git a/selfservice/hook/verify.go b/selfservice/hook/verify.go index ae427938b238..b648b9df16d2 100644 --- a/selfservice/hook/verify.go +++ b/selfservice/hook/verify.go @@ -27,10 +27,12 @@ func (e *Verifier) ExecuteRegistrationPostHook(w http.ResponseWriter, r *http.Re // Ths is called after the identity has been created so we can safely assume that all addresses are available // already. - for _, address := range s.Identity.Addresses { - if err := e.r.VerificationSender().SendCode(r.Context(), address.Via, address.Value); err != nil { + for k, address := range s.Identity.Addresses { + sent, err := e.r.VerificationSender().SendCode(r.Context(), address.Via, address.Value) + if err != nil { return err } + s.Identity.Addresses[k] = *sent } return nil From ec67e0f6e5e6cc0a084e3419095e61453dfd47e2 Mon Sep 17 00:00:00 2001 From: aeneasr Date: Mon, 17 Feb 2020 18:57:53 +0100 Subject: [PATCH 2/2] u --- docs/api.swagger.json | 33 +++++++++++-------- .../httpclient/client/public/public_client.go | 2 +- .../self_service_browser_verify_parameters.go | 23 +++++++++++++ .../self_service_browser_verify_responses.go | 4 +-- selfservice/flow/verify/handler.go | 2 +- 5 files changed, 47 insertions(+), 17 deletions(-) diff --git a/docs/api.swagger.json b/docs/api.swagger.json index a433a7ca9669..be033441b373 100755 --- a/docs/api.swagger.json +++ b/docs/api.swagger.json @@ -705,13 +705,9 @@ } } }, - "/self-service/browser/flows/verification/confirm/{code}": { + "/self-service/browser/flows/verification/init/{via}": { "get": { - "description": "This endpoint completes a browser-based verification flow.\n\n\u003e This endpoint is NOT INTENDED for API clients and only works with browsers (Chrome, Firefox, ...) and HTML Forms.\n\nMore information can be found at [ORY Kratos Email and Phone Verification Documentation](https://www.ory.sh/docs/kratos/selfservice/flows/verify-email-account-activation).", - "consumes": [ - "application/json", - "application/x-www-form-urlencoded" - ], + "description": "This endpoint initializes a browser-based profile management flow. Once initialized, the browser will be redirected to\n`urls.profile_ui` with the request ID set as a query parameter. If no valid user session exists, a login\nflow will be initialized.\n\n\u003e This endpoint is NOT INTENDED for API clients and only works\nwith browsers (Chrome, Firefox, ...).\n\nMore information can be found at [ORY Kratos Email and Phone Verification Documentation](https://www.ory.sh/docs/kratos/selfservice/flows/verify-email-account-activation).", "schemes": [ "http", "https" @@ -719,12 +715,13 @@ "tags": [ "public" ], - "summary": "Complete the browser-based verification flows", - "operationId": "selfServiceBrowserVerify", + "summary": "Initialize browser-based verification flow", + "operationId": "initializeSelfServiceBrowserVerificationFlow", "parameters": [ { "type": "string", - "name": "code", + "description": "What to verify\n\nCurrently only \"email\" is supported.", + "name": "via", "in": "path", "required": true } @@ -742,9 +739,13 @@ } } }, - "/self-service/browser/flows/verification/init/{via}": { + "/self-service/browser/flows/verification/{via}/confirm/{code}": { "get": { - "description": "This endpoint initializes a browser-based profile management flow. Once initialized, the browser will be redirected to\n`urls.profile_ui` with the request ID set as a query parameter. If no valid user session exists, a login\nflow will be initialized.\n\n\u003e This endpoint is NOT INTENDED for API clients and only works\nwith browsers (Chrome, Firefox, ...).\n\nMore information can be found at [ORY Kratos Email and Phone Verification Documentation](https://www.ory.sh/docs/kratos/selfservice/flows/verify-email-account-activation).", + "description": "This endpoint completes a browser-based verification flow.\n\n\u003e This endpoint is NOT INTENDED for API clients and only works with browsers (Chrome, Firefox, ...) and HTML Forms.\n\nMore information can be found at [ORY Kratos Email and Phone Verification Documentation](https://www.ory.sh/docs/kratos/selfservice/flows/verify-email-account-activation).", + "consumes": [ + "application/json", + "application/x-www-form-urlencoded" + ], "schemes": [ "http", "https" @@ -752,9 +753,15 @@ "tags": [ "public" ], - "summary": "Initialize browser-based verification flow", - "operationId": "initializeSelfServiceBrowserVerificationFlow", + "summary": "Complete the browser-based verification flows", + "operationId": "selfServiceBrowserVerify", "parameters": [ + { + "type": "string", + "name": "code", + "in": "path", + "required": true + }, { "type": "string", "description": "What to verify\n\nCurrently only \"email\" is supported.", diff --git a/internal/httpclient/client/public/public_client.go b/internal/httpclient/client/public/public_client.go index 2d5a21eb9c58..f2382ed65f3b 100644 --- a/internal/httpclient/client/public/public_client.go +++ b/internal/httpclient/client/public/public_client.go @@ -320,7 +320,7 @@ func (a *Client) SelfServiceBrowserVerify(params *SelfServiceBrowserVerifyParams _, err := a.transport.Submit(&runtime.ClientOperation{ ID: "selfServiceBrowserVerify", Method: "GET", - PathPattern: "/self-service/browser/flows/verification/confirm/{code}", + PathPattern: "/self-service/browser/flows/verification/{via}/confirm/{code}", ProducesMediaTypes: []string{"application/json"}, ConsumesMediaTypes: []string{"application/json", "application/x-www-form-urlencoded"}, Schemes: []string{"http", "https"}, diff --git a/internal/httpclient/client/public/self_service_browser_verify_parameters.go b/internal/httpclient/client/public/self_service_browser_verify_parameters.go index ca03dfaaabbf..751741d7ccf4 100644 --- a/internal/httpclient/client/public/self_service_browser_verify_parameters.go +++ b/internal/httpclient/client/public/self_service_browser_verify_parameters.go @@ -63,6 +63,13 @@ type SelfServiceBrowserVerifyParams struct { /*Code*/ Code string + /*Via + What to verify + + Currently only "email" is supported. + + */ + Via string timeout time.Duration Context context.Context @@ -113,6 +120,17 @@ func (o *SelfServiceBrowserVerifyParams) SetCode(code string) { o.Code = code } +// WithVia adds the via to the self service browser verify params +func (o *SelfServiceBrowserVerifyParams) WithVia(via string) *SelfServiceBrowserVerifyParams { + o.SetVia(via) + return o +} + +// SetVia adds the via to the self service browser verify params +func (o *SelfServiceBrowserVerifyParams) SetVia(via string) { + o.Via = via +} + // WriteToRequest writes these params to a swagger request func (o *SelfServiceBrowserVerifyParams) WriteToRequest(r runtime.ClientRequest, reg strfmt.Registry) error { @@ -126,6 +144,11 @@ func (o *SelfServiceBrowserVerifyParams) WriteToRequest(r runtime.ClientRequest, return err } + // path param via + if err := r.SetPathParam("via", o.Via); err != nil { + return err + } + if len(res) > 0 { return errors.CompositeValidationError(res...) } diff --git a/internal/httpclient/client/public/self_service_browser_verify_responses.go b/internal/httpclient/client/public/self_service_browser_verify_responses.go index a70d8de4eeba..3b14ee9e2454 100644 --- a/internal/httpclient/client/public/self_service_browser_verify_responses.go +++ b/internal/httpclient/client/public/self_service_browser_verify_responses.go @@ -56,7 +56,7 @@ type SelfServiceBrowserVerifyFound struct { } func (o *SelfServiceBrowserVerifyFound) Error() string { - return fmt.Sprintf("[GET /self-service/browser/flows/verification/confirm/{code}][%d] selfServiceBrowserVerifyFound ", 302) + return fmt.Sprintf("[GET /self-service/browser/flows/verification/{via}/confirm/{code}][%d] selfServiceBrowserVerifyFound ", 302) } func (o *SelfServiceBrowserVerifyFound) readResponse(response runtime.ClientResponse, consumer runtime.Consumer, formats strfmt.Registry) error { @@ -78,7 +78,7 @@ type SelfServiceBrowserVerifyInternalServerError struct { } func (o *SelfServiceBrowserVerifyInternalServerError) Error() string { - return fmt.Sprintf("[GET /self-service/browser/flows/verification/confirm/{code}][%d] selfServiceBrowserVerifyInternalServerError %+v", 500, o.Payload) + return fmt.Sprintf("[GET /self-service/browser/flows/verification/{via}/confirm/{code}][%d] selfServiceBrowserVerifyInternalServerError %+v", 500, o.Payload) } func (o *SelfServiceBrowserVerifyInternalServerError) GetPayload() *models.GenericError { diff --git a/selfservice/flow/verify/handler.go b/selfservice/flow/verify/handler.go index 765580dcf484..5e7816b93259 100644 --- a/selfservice/flow/verify/handler.go +++ b/selfservice/flow/verify/handler.go @@ -295,7 +295,7 @@ type selfServiceBrowserVerifyParameters struct { Via string `json:"via"` } -// swagger:route GET /self-service/browser/flows/verification/:via/confirm/{code} public selfServiceBrowserVerify +// swagger:route GET /self-service/browser/flows/verification/{via}/confirm/{code} public selfServiceBrowserVerify // // Complete the browser-based verification flows //