From 9e0015acec7f8d927498e48366b377e22ec768b7 Mon Sep 17 00:00:00 2001 From: aeneasr Date: Sat, 23 Nov 2019 13:48:32 +0100 Subject: [PATCH] ss/profile: Improve success and error flows This patch completes the profile management flow by implementing proper error and success states and adding several data integrity tests. Closes #112 --- docs/api.swagger.json | 24 ++-- identity/identity.go | 12 ++ identity/identity_test.go | 27 ++++ sdk/go/kratos/models/error_parser.go | 10 -- sdk/go/kratos/models/field_setter.go | 10 -- sdk/go/kratos/models/login_request.go | 2 +- sdk/go/kratos/models/login_request_method.go | 22 ++++ .../models/profile_management_request.go | 16 ++- sdk/go/kratos/models/r_w_mutex.go | 21 ++++ .../models/registration_request_method.go | 22 ++++ sdk/go/kratos/models/resetter.go | 10 -- sdk/go/kratos/models/value_setter.go | 10 -- selfservice/flow/login/request.go | 2 +- selfservice/flow/profile/handler.go | 55 ++++++-- selfservice/flow/profile/handler_test.go | 119 +++++++++++++++--- selfservice/flow/profile/request.go | 34 +++-- .../flow/profile/stub/identity.schema.json | 14 +++ selfservice/form/html_form.go | 6 +- selfservice/persistence/ephermal.go | 8 +- selfservice/strategy/oidc/form.go | 5 +- selfservice/strategy/password/registration.go | 1 + session/handler_mock.go | 8 +- 22 files changed, 338 insertions(+), 100 deletions(-) delete mode 100644 sdk/go/kratos/models/error_parser.go delete mode 100644 sdk/go/kratos/models/field_setter.go create mode 100644 sdk/go/kratos/models/r_w_mutex.go delete mode 100644 sdk/go/kratos/models/resetter.go delete mode 100644 sdk/go/kratos/models/value_setter.go diff --git a/docs/api.swagger.json b/docs/api.swagger.json index e62ef930a3b..4d36bb7ce44 100755 --- a/docs/api.swagger.json +++ b/docs/api.swagger.json @@ -335,18 +335,10 @@ } } }, - "ErrorParser": { + "RWMutex": { + "description": "A RWMutex must not be copied after first use.\n\nIf a goroutine holds a RWMutex for reading and another goroutine might\ncall Lock, no goroutine should expect to be able to acquire a read lock\nuntil the initial read lock is released. In particular, this prohibits\nrecursive read locking. This is to ensure that the lock eventually becomes\navailable; a blocked Lock call excludes new readers from acquiring the\nlock.", "type": "object", - "title": "ErrorParser is capable of parsing and processing errors." - }, - "FieldSetter": { - "type": "object" - }, - "Resetter": { - "type": "object" - }, - "ValueSetter": { - "type": "object" + "title": "A RWMutex is a reader/writer mutual exclusion lock.\nThe lock can be held by an arbitrary number of readers or a single writer.\nThe zero value for a RWMutex is an unlocked mutex." }, "form": { "description": "HTMLForm represents a HTML Form. The container can work with both HTTP Form and JSON requests", @@ -523,7 +515,7 @@ "format": "date-time" }, "id": { - "description": "ID represents the request's unique ID. When performing the login flow, this\nrepresents the id in the login ui's query parameter: http://login-ui/?request=\u003cid\u003e", + "description": "ID represents the request's unique ID. When performing the login flow, this\nrepresents the id in the login ui's query parameter: http://\u003curls.login_ui\u003e/?request=\u003cid\u003e", "type": "string" }, "issued_at": { @@ -618,6 +610,7 @@ "title": "Request presents a profile management request", "properties": { "expires_at": { + "description": "ExpiresAt is the time (UTC) when the request expires. If the user still wishes to update the profile,\na new request has to be initiated.", "type": "string", "format": "date-time" }, @@ -625,17 +618,24 @@ "$ref": "#/definitions/form" }, "id": { + "description": "ID represents the request's unique ID. When performing the profile management flow, this\nrepresents the id in the profile ui's query parameter: http://\u003curls.profile_ui\u003e?request=\u003cid\u003e", "type": "string" }, "identity": { "$ref": "#/definitions/identity" }, "issued_at": { + "description": "IssuedAt is the time (UTC) when the request occurred.", "type": "string", "format": "date-time" }, "request_url": { + "description": "RequestURL is the initial URL that was requested from ORY Kratos. It can be used\nto forward information contained in the URL's path or query for example.", "type": "string" + }, + "update_successful": { + "description": "UpdateSuccessful, if true, indicates that the profile has been updated successfully with the provided data.\nDone will stay true when repeatedly checking. If set to true, done will revert back to false only\nwhen a request with invalid (e.g. \"please use a valid phone number\") data was sent.", + "type": "boolean" } } }, diff --git a/identity/identity.go b/identity/identity.go index 06b13ba6d29..1ca8fd57366 100644 --- a/identity/identity.go +++ b/identity/identity.go @@ -58,6 +58,18 @@ func (i *Identity) SetCredentials(t CredentialsType, c Credentials) { i.Credentials[t] = c } +func (i *Identity) CopyCredentials() map[CredentialsType]Credentials { + result := make(map[CredentialsType]Credentials) + for id, credential := range i.Credentials { + result[id] = Credentials{ + ID: credential.ID, + Identifiers: append([]string{}, credential.Identifiers...), + Config: append([]byte{}, credential.Config...), + } + } + return result +} + func (i *Identity) GetCredentials(t CredentialsType) (*Credentials, bool) { i.lock().RLock() defer i.lock().RUnlock() diff --git a/identity/identity_test.go b/identity/identity_test.go index 7ca156c915e..3d0d73e50c4 100644 --- a/identity/identity_test.go +++ b/identity/identity_test.go @@ -1,9 +1,11 @@ package identity import ( + "encoding/json" "testing" "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" ) func TestNewIdentity(t *testing.T) { @@ -13,3 +15,28 @@ func TestNewIdentity(t *testing.T) { assert.NotEmpty(t, i.Traits) assert.NotNil(t, i.Credentials) } + +func TestCopyCredentials(t *testing.T) { + i := NewIdentity("") + i.Credentials = map[CredentialsType]Credentials{ + "foo": { + ID: "foo", + Identifiers: []string{"bar"}, + Config: json.RawMessage(`{"foo":"bar"}`), + }, + } + + creds := i.CopyCredentials() + creds["bar"] = Credentials{ + ID: "bar", + Identifiers: []string{"bar"}, + Config: json.RawMessage(`{"foo":"bar"}`), + } + + conf := creds["foo"].Config + require.NoError(t, json.Unmarshal([]byte(`{"bar":"bar"}`), &conf)) + + assert.Empty(t, i.Credentials["bar"]) + assert.Equal(t, `{"foo":"bar"}`, string(i.Credentials["foo"].Config)) + assert.Equal(t, `{"bar":"bar"}`, string(creds["foo"].Config)) +} diff --git a/sdk/go/kratos/models/error_parser.go b/sdk/go/kratos/models/error_parser.go deleted file mode 100644 index 5244b8c57ce..00000000000 --- a/sdk/go/kratos/models/error_parser.go +++ /dev/null @@ -1,10 +0,0 @@ -// Code generated by go-swagger; DO NOT EDIT. - -package models - -// This file was generated by the swagger tool. -// Editing this file might prove futile when you re-run the swagger generate command - -// ErrorParser ErrorParser is capable of parsing and processing errors. -// swagger:model ErrorParser -type ErrorParser interface{} diff --git a/sdk/go/kratos/models/field_setter.go b/sdk/go/kratos/models/field_setter.go deleted file mode 100644 index 7c7e28f938b..00000000000 --- a/sdk/go/kratos/models/field_setter.go +++ /dev/null @@ -1,10 +0,0 @@ -// Code generated by go-swagger; DO NOT EDIT. - -package models - -// This file was generated by the swagger tool. -// Editing this file might prove futile when you re-run the swagger generate command - -// FieldSetter field setter -// swagger:model FieldSetter -type FieldSetter interface{} diff --git a/sdk/go/kratos/models/login_request.go b/sdk/go/kratos/models/login_request.go index 80866a511aa..d974a365292 100644 --- a/sdk/go/kratos/models/login_request.go +++ b/sdk/go/kratos/models/login_request.go @@ -26,7 +26,7 @@ type LoginRequest struct { ExpiresAt strfmt.DateTime `json:"expires_at,omitempty"` // ID represents the request's unique ID. When performing the login flow, this - // represents the id in the login ui's query parameter: http://login-ui/?request= + // represents the id in the login ui's query parameter: http:///?request= ID string `json:"id,omitempty"` // IssuedAt is the time (UTC) when the request occurred. diff --git a/sdk/go/kratos/models/login_request_method.go b/sdk/go/kratos/models/login_request_method.go index ff13f554ee1..c9938f84d55 100644 --- a/sdk/go/kratos/models/login_request_method.go +++ b/sdk/go/kratos/models/login_request_method.go @@ -27,6 +27,10 @@ type LoginRequestMethod struct { func (m *LoginRequestMethod) Validate(formats strfmt.Registry) error { var res []error + if err := m.validateConfig(formats); err != nil { + res = append(res, err) + } + if err := m.validateMethod(formats); err != nil { res = append(res, err) } @@ -37,6 +41,24 @@ func (m *LoginRequestMethod) Validate(formats strfmt.Registry) error { return nil } +func (m *LoginRequestMethod) validateConfig(formats strfmt.Registry) error { + + if swag.IsZero(m.Config) { // not required + return nil + } + + if m.Config != nil { + if err := m.Config.Validate(formats); err != nil { + if ve, ok := err.(*errors.Validation); ok { + return ve.ValidateName("config") + } + return err + } + } + + return nil +} + func (m *LoginRequestMethod) validateMethod(formats strfmt.Registry) error { if swag.IsZero(m.Method) { // not required diff --git a/sdk/go/kratos/models/profile_management_request.go b/sdk/go/kratos/models/profile_management_request.go index 9f08b4b5abb..518b58df860 100644 --- a/sdk/go/kratos/models/profile_management_request.go +++ b/sdk/go/kratos/models/profile_management_request.go @@ -22,25 +22,33 @@ import ( // swagger:model profileManagementRequest type ProfileManagementRequest struct { - // expires at + // ExpiresAt is the time (UTC) when the request expires. If the user still wishes to update the profile, + // a new request has to be initiated. // Format: date-time ExpiresAt strfmt.DateTime `json:"expires_at,omitempty"` // form Form *Form `json:"form,omitempty"` - // id + // ID represents the request's unique ID. When performing the profile management flow, this + // represents the id in the profile ui's query parameter: http://?request= ID string `json:"id,omitempty"` // identity Identity *Identity `json:"identity,omitempty"` - // issued at + // IssuedAt is the time (UTC) when the request occurred. // Format: date-time IssuedAt strfmt.DateTime `json:"issued_at,omitempty"` - // request url + // RequestURL is the initial URL that was requested from ORY Kratos. It can be used + // to forward information contained in the URL's path or query for example. RequestURL string `json:"request_url,omitempty"` + + // UpdateSuccessful, if true, indicates that the profile has been updated successfully with the provided data. + // Done will stay true when repeatedly checking. If set to true, done will revert back to false only + // when a request with invalid (e.g. "please use a valid phone number") data was sent. + UpdateSuccessful bool `json:"update_successful,omitempty"` } // Validate validates this profile management request diff --git a/sdk/go/kratos/models/r_w_mutex.go b/sdk/go/kratos/models/r_w_mutex.go new file mode 100644 index 00000000000..5d20e8f1fc3 --- /dev/null +++ b/sdk/go/kratos/models/r_w_mutex.go @@ -0,0 +1,21 @@ +// Code generated by go-swagger; DO NOT EDIT. + +package models + +// This file was generated by the swagger tool. +// Editing this file might prove futile when you re-run the swagger generate command + +// RWMutex A RWMutex is a reader/writer mutual exclusion lock. +// The lock can be held by an arbitrary number of readers or a single writer. +// The zero value for a RWMutex is an unlocked mutex. +// +// A RWMutex must not be copied after first use. +// +// If a goroutine holds a RWMutex for reading and another goroutine might +// call Lock, no goroutine should expect to be able to acquire a read lock +// until the initial read lock is released. In particular, this prohibits +// recursive read locking. This is to ensure that the lock eventually becomes +// available; a blocked Lock call excludes new readers from acquiring the +// lock. +// swagger:model RWMutex +type RWMutex interface{} diff --git a/sdk/go/kratos/models/registration_request_method.go b/sdk/go/kratos/models/registration_request_method.go index 9a563398e79..f4c89c6ff76 100644 --- a/sdk/go/kratos/models/registration_request_method.go +++ b/sdk/go/kratos/models/registration_request_method.go @@ -27,6 +27,10 @@ type RegistrationRequestMethod struct { func (m *RegistrationRequestMethod) Validate(formats strfmt.Registry) error { var res []error + if err := m.validateConfig(formats); err != nil { + res = append(res, err) + } + if err := m.validateMethod(formats); err != nil { res = append(res, err) } @@ -37,6 +41,24 @@ func (m *RegistrationRequestMethod) Validate(formats strfmt.Registry) error { return nil } +func (m *RegistrationRequestMethod) validateConfig(formats strfmt.Registry) error { + + if swag.IsZero(m.Config) { // not required + return nil + } + + if m.Config != nil { + if err := m.Config.Validate(formats); err != nil { + if ve, ok := err.(*errors.Validation); ok { + return ve.ValidateName("config") + } + return err + } + } + + return nil +} + func (m *RegistrationRequestMethod) validateMethod(formats strfmt.Registry) error { if swag.IsZero(m.Method) { // not required diff --git a/sdk/go/kratos/models/resetter.go b/sdk/go/kratos/models/resetter.go deleted file mode 100644 index bd4483258db..00000000000 --- a/sdk/go/kratos/models/resetter.go +++ /dev/null @@ -1,10 +0,0 @@ -// Code generated by go-swagger; DO NOT EDIT. - -package models - -// This file was generated by the swagger tool. -// Editing this file might prove futile when you re-run the swagger generate command - -// Resetter resetter -// swagger:model Resetter -type Resetter interface{} diff --git a/sdk/go/kratos/models/value_setter.go b/sdk/go/kratos/models/value_setter.go deleted file mode 100644 index 27a9d04e40c..00000000000 --- a/sdk/go/kratos/models/value_setter.go +++ /dev/null @@ -1,10 +0,0 @@ -// Code generated by go-swagger; DO NOT EDIT. - -package models - -// This file was generated by the swagger tool. -// Editing this file might prove futile when you re-run the swagger generate command - -// ValueSetter value setter -// swagger:model ValueSetter -type ValueSetter interface{} diff --git a/selfservice/flow/login/request.go b/selfservice/flow/login/request.go index 04b680ece88..692e57b20e2 100644 --- a/selfservice/flow/login/request.go +++ b/selfservice/flow/login/request.go @@ -34,7 +34,7 @@ type RequestMethodConfig interface { // swagger:model loginRequest type Request struct { // ID represents the request's unique ID. When performing the login flow, this - // represents the id in the login ui's query parameter: http://login-ui/?request= + // represents the id in the login ui's query parameter: http:///?request= ID string `json:"id"` // ExpiresAt is the time (UTC) when the request expires. If the user still wishes to log in, diff --git a/selfservice/flow/profile/handler.go b/selfservice/flow/profile/handler.go index 00eb71989f5..baea0047388 100644 --- a/selfservice/flow/profile/handler.go +++ b/selfservice/flow/profile/handler.go @@ -4,6 +4,7 @@ import ( "encoding/json" "net/http" "net/url" + "reflect" "github.com/julienschmidt/httprouter" "github.com/justinas/nosurf" @@ -262,6 +263,7 @@ func (h *Handler) completeProfileManagementFlow(w http.ResponseWriter, r *http.R decoderx.HTTPFormDecoder(), option, decoderx.HTTPDecoderSetValidatePayloads(false), + decoderx.HTTPDecoderSetIgnoreParseErrorsStrategy(decoderx.ParseErrorIgnore), ); err != nil { h.handleProfileManagementError(w, r, nil, s.Identity.Traits, err) return @@ -288,23 +290,31 @@ func (h *Handler) completeProfileManagementFlow(w http.ResponseWriter, r *http.R return } - i := s.Identity - i.Traits = p.Traits // identity.TraitsSchemaURL + creds, err := h.d.IdentityPool().GetClassified(r.Context(), s.Identity.ID) + if err != nil { + h.handleProfileManagementError(w, r, ar, p.Traits, err) + return + } + + i := *s.Identity + i.Traits = p.Traits + i.Credentials = creds.CopyCredentials() + // If credential identifiers have changed we need to block this action UNLESS // the identity has been authenticated in that request: // // - https://security.stackexchange.com/questions/24291/why-do-we-ask-for-a-users-existing-password-when-changing-their-password // We need to make sure that the identity has a valid schema before passing it down to the identity pool. - if err := h.d.IdentityValidator().Validate(i); err != nil { + if err := h.d.IdentityValidator().Validate(&i); err != nil { h.handleProfileManagementError(w, r, ar, i.Traits, err) return } // Check if any credentials-related field changed. - if len(i.Credentials) > 0 { + if !reflect.DeepEqual(creds.Credentials, i.Credentials) { h.handleProfileManagementError(w, r, ar, i.Traits, errors.WithStack( herodot.ErrInternalServerError. @@ -313,25 +323,46 @@ func (h *Handler) completeProfileManagementFlow(w http.ResponseWriter, r *http.R return } - if _, err := h.d.IdentityPool().Update(r.Context(), i); err != nil { + if _, err := h.d.IdentityPool().Update(r.Context(), &i); err != nil { + h.handleProfileManagementError(w, r, ar, i.Traits, err) + return + } + + ar.Form.Reset() + ar.UpdateSuccessful = true + for name, field := range form.NewHTMLFormFromJSON("", i.Traits, "traits").Fields { + ar.Form.SetField(name, field) + } + ar.Form.SetValue("request", r.Form.Get("request")) + ar.Form.SetCSRF(nosurf.Token(r)) + + if err := h.d.ProfileRequestPersister().UpdateProfileRequest(r.Context(), ar.ID, ar); err != nil { h.handleProfileManagementError(w, r, ar, i.Traits, err) return } - http.Redirect(w, r, urlx.AppendPaths(h.c.SelfPublicURL(), BrowserProfilePath).String(), http.StatusFound) + http.Redirect(w, r, + urlx.CopyWithQuery(h.c.ProfileURL(), url.Values{"request": {ar.ID}}).String(), + http.StatusFound, + ) } // handleProfileManagementError is a convenience function for handling all types of errors that may occur (e.g. validation error) // during a profile management request. func (h *Handler) handleProfileManagementError(w http.ResponseWriter, r *http.Request, rr *Request, traits json.RawMessage, err error) { - rr.Form.Reset() - if traits != nil { - for name, field := range form.NewHTMLFormFromJSON("", traits, "traits").Fields { - rr.Form.SetField(name, field) + if rr != nil { + rr.Form.Reset() + rr.UpdateSuccessful = false + + if traits != nil { + for name, field := range form.NewHTMLFormFromJSON("", traits, "traits").Fields { + rr.Form.SetField(name, field) + } } + rr.Form.SetValue("request", r.Form.Get("request")) + rr.Form.SetCSRF(nosurf.Token(r)) } - rr.Form.SetValue("request", r.Form.Get("request")) - rr.Form.SetCSRF(nosurf.Token(r)) + h.d.ProfileRequestRequestErrorHandler().HandleProfileManagementError(w, r, identity.CredentialsTypePassword, rr, err) } diff --git a/selfservice/flow/profile/handler_test.go b/selfservice/flow/profile/handler_test.go index 0dbf89b3d9e..5a4dd66eced 100644 --- a/selfservice/flow/profile/handler_test.go +++ b/selfservice/flow/profile/handler_test.go @@ -3,6 +3,7 @@ package profile_test import ( "encoding/json" "fmt" + "io/ioutil" "net/http" "net/http/httptest" "net/url" @@ -75,10 +76,12 @@ func TestUpdateProfile(t *testing.T) { viper.Set(configuration.ViperKeyURLsLogin, ui.URL+"/login") primaryIdentity := &identity.Identity{ - ID: uuid.New().String(), - Credentials: nil, + ID: uuid.New().String(), + Credentials: map[identity.CredentialsType]identity.Credentials{ + "password": {ID: "password", Identifiers: []string{"john@doe.com"}, Config: json.RawMessage(`{"hashed_password":"foo"}`)}, + }, TraitsSchemaURL: "file://./stub/identity.schema.json", - Traits: json.RawMessage(`{"stringy":"foobar","booly":false,"numby":2.5}`), + Traits: json.RawMessage(`{"email":"john@doe.com","stringy":"foobar","booly":false,"numby":2.5}`), } kratos := func() *httptest.Server { @@ -212,33 +215,121 @@ func TestUpdateProfile(t *testing.T) { "traits.stringy": models.FormField{Name: "traits.stringy", Required: false, Type: "text", Value: "foobar"}, "traits.numby": models.FormField{Name: "traits.numby", Required: false, Type: "number", Value: json.Number("2.5")}, "traits.booly": models.FormField{Name: "traits.booly", Required: false, Type: "checkbox", Value: false}, + "traits.email": models.FormField{Name: "traits.email", Required: false, Type: "text", Value: "john@doe.com"}, }, }, pr.Payload.Form) }) - t.Run("description=should come back with form errors if some profile data is invalid", func(t *testing.T) { - rs := makeRequest(t) - f := rs.Payload.Form - values := fieldsToURLValues(f.Fields) - values.Set("traits.should_long_string", "too-short") - t.Logf("%+v", values) - res, err := primaryUser.PostForm(f.Action, values) + submitForm := func(t *testing.T, req *public.GetProfileManagementRequestOK, values url.Values) (string, *public.GetProfileManagementRequestOK) { + res, err := primaryUser.PostForm(req.Payload.Form.Action, values) require.NoError(t, err) assert.EqualValues(t, http.StatusNoContent, res.StatusCode) assert.Equal(t, ui.URL, res.Request.URL.Scheme+"://"+res.Request.URL.Host) assert.Equal(t, "/profile", res.Request.URL.Path, "should end up at the profile URL") - rs, err = kratosClient.Public.GetProfileManagementRequest( + rs, err := kratosClient.Public.GetProfileManagementRequest( public.NewGetProfileManagementRequestParams().WithHTTPClient(primaryUser). WithRequest(res.Request.URL.Query().Get("request")), ) require.NoError(t, err) + body, err := json.Marshal(rs.Payload) + require.NoError(t, err) + return string(body), rs + } + + t.Run("description=should come back with form errors if some profile data is invalid", func(t *testing.T) { + rs := makeRequest(t) + values := fieldsToURLValues(rs.Payload.Form.Fields) + values.Set("traits.should_long_string", "too-short") + values.Set("traits.stringy", "bazbar") // it should still override new values! + actual, _ := submitForm(t, rs, values) + + assert.NotEmpty(t, "too-short", gjson.Get(actual, "form.fields.csrf_token.value").String(), "%s", actual) + assert.Equal(t, "too-short", gjson.Get(actual, "form.fields.traits\\.should_long_string.value").String(), "%s", actual) + assert.Equal(t, "bazbar", gjson.Get(actual, "form.fields.traits\\.stringy.value").String(), "%s", actual) + assert.Equal(t, "2.5", gjson.Get(actual, "form.fields.traits\\.numby.value").String(), "%s", actual) + assert.Equal(t, "traits.should_long_string: String length must be greater than or equal to 25", gjson.Get(actual, "form.fields.traits\\.should_long_string.errors.0.message").String(), "%s", actual) + }) + + t.Run("description=should come back with form errors if trying to update email", func(t *testing.T) { + rs := makeRequest(t) + values := fieldsToURLValues(rs.Payload.Form.Fields) + values.Set("traits.email", "not-john-doe") + res, err := primaryUser.PostForm(rs.Payload.Form.Action, values) + require.NoError(t, err) + defer res.Body.Close() - actual, err := json.Marshal(rs.Payload) + assert.Contains(t, res.Request.URL.String(), errTs.URL) + assert.EqualValues(t, http.StatusOK, res.StatusCode) + + body, err := ioutil.ReadAll(res.Body) require.NoError(t, err) + assert.Contains(t, gjson.Get(string(body), "0.reason").String(), "A field was modified that updates one or more credentials-related settings", "%s", body) + }) + + t.Run("description=should retry with invalid payloads multiple times before succeeding", func(t *testing.T) { + t.Run("flow=fail first update", func(t *testing.T) { + rs := makeRequest(t) + values := fieldsToURLValues(rs.Payload.Form.Fields) + values.Set("traits.should_big_number", "1") + actual, response := submitForm(t, rs, values) + assert.False(t, response.Payload.UpdateSuccessful, "%s", actual) + + assert.Equal(t, "1", gjson.Get(actual, "form.fields.traits\\.should_big_number.value").String(), "%s", actual) + assert.Equal(t, "traits.should_big_number: Must be greater than or equal to 1200", gjson.Get(actual, "form.fields.traits\\.should_big_number.errors.0.message").String(), "%s", actual) + + assert.Equal(t, "foobar", gjson.Get(actual, "form.fields.traits\\.stringy.value").String(), "%s", actual) // sanity check if original payload is still here + }) + + t.Run("flow=fail second update", func(t *testing.T) { + rs := makeRequest(t) + values := fieldsToURLValues(rs.Payload.Form.Fields) + values.Del("traits.should_big_number") + values.Set("traits.should_long_string", "short") + values.Set("traits.numby", "this-is-not-a-number") + actual, response := submitForm(t, rs, values) + assert.False(t, response.Payload.UpdateSuccessful, "%s", actual) + + assert.Empty(t, gjson.Get(actual, "form.fields.traits\\.should_big_number.errors.0.message").String(), "%s", actual) + assert.Empty(t, gjson.Get(actual, "form.fields.traits\\.should_big_number.value").String(), "%s", actual) - assert.Equal(t, "too-short", gjson.Get(string(actual), "form.fields.traits\\.should_long_string.value").String(), "%s", actual) - assert.Equal(t, "traits.should_long_string: String length must be greater than or equal to 25", gjson.Get(string(actual), "form.fields.traits\\.should_long_string.errors.0.message").String(), "%s", actual) + assert.Equal(t, "short", gjson.Get(actual, "form.fields.traits\\.should_long_string.value").String(), "%s", actual) + assert.Equal(t, "traits.should_long_string: String length must be greater than or equal to 25", gjson.Get(actual, "form.fields.traits\\.should_long_string.errors.0.message").String(), "%s", actual) + + assert.Equal(t, "this-is-not-a-number", gjson.Get(actual, "form.fields.traits\\.numby.value").String(), "%s", actual) + assert.Equal(t, "traits.numby: Invalid type. Expected: number, given: string", gjson.Get(actual, "form.fields.traits\\.numby.errors.0.message").String(), "%s", actual) + + assert.Equal(t, "foobar", gjson.Get(actual, "form.fields.traits\\.stringy.value").String(), "%s", actual) // sanity check if original payload is still here + }) + + t.Run("flow=succeed with final request", func(t *testing.T) { + rs := makeRequest(t) + values := fieldsToURLValues(rs.Payload.Form.Fields) + values.Set("traits.email", "john@doe.com") + values.Set("traits.numby", "15") + values.Set("traits.should_big_number", "9001") + values.Set("traits.should_long_string", "this is such a long string, amazing stuff!") + actual, response := submitForm(t, rs, values) + assert.True(t, response.Payload.UpdateSuccessful, "%s", actual) + + assert.Empty(t, gjson.Get(actual, "form.fields.traits\\.numby.errors").Value(), "%s", actual) + assert.Empty(t, gjson.Get(actual, "form.fields.traits\\.should_big_number.errors").Value(), "%s", actual) + assert.Empty(t, gjson.Get(actual, "form.fields.traits\\.should_long_string.errors").Value(), "%s", actual) + + assert.Equal(t, 15.0, gjson.Get(actual, "form.fields.traits\\.numby.value").Value(), "%s", actual) + assert.Equal(t, 9001.0, gjson.Get(actual, "form.fields.traits\\.should_big_number.value").Value(), "%s", actual) + assert.Equal(t, "this is such a long string, amazing stuff!", gjson.Get(actual, "form.fields.traits\\.should_long_string.value").Value(), "%s", actual) + + assert.Equal(t, "foobar", gjson.Get(actual, "form.fields.traits\\.stringy.value").String(), "%s", actual) // sanity check if original payload is still here + }) + + t.Run("flow=try another update with invalid data", func(t *testing.T) { + rs := makeRequest(t) + values := fieldsToURLValues(rs.Payload.Form.Fields) + values.Set("traits.should_long_string", "short") + actual, response := submitForm(t, rs, values) + assert.False(t, response.Payload.UpdateSuccessful, "%s", actual) + }) }) } diff --git a/selfservice/flow/profile/request.go b/selfservice/flow/profile/request.go index 53e36f2f973..ebac5ea5255 100644 --- a/selfservice/flow/profile/request.go +++ b/selfservice/flow/profile/request.go @@ -24,13 +24,33 @@ import ( // // swagger:model profileManagementRequest type Request struct { - ID string `json:"id"` - IssuedAt time.Time `json:"issued_at"` - ExpiresAt time.Time `json:"expires_at"` - RequestURL string `json:"request_url"` - identityID string `json:"-"` - Form *form.HTMLForm `json:"form"` - Identity *identity.Identity `json:"identity"` + // ID represents the request's unique ID. When performing the profile management flow, this + // represents the id in the profile ui's query parameter: http://?request= + ID string `json:"id"` + + // ExpiresAt is the time (UTC) when the request expires. If the user still wishes to update the profile, + // a new request has to be initiated. + ExpiresAt time.Time `json:"expires_at"` + + // IssuedAt is the time (UTC) when the request occurred. + IssuedAt time.Time `json:"issued_at"` + + // RequestURL is the initial URL that was requested from ORY Kratos. It can be used + // to forward information contained in the URL's path or query for example. + RequestURL string `json:"request_url"` + + // Form contains form fields, errors, and so on. + Form *form.HTMLForm `json:"form"` + + // Identity contains all of the identity's data in raw form. + Identity *identity.Identity `json:"identity"` + + // UpdateSuccessful, if true, indicates that the profile has been updated successfully with the provided data. + // Done will stay true when repeatedly checking. If set to true, done will revert back to false only + // when a request with invalid (e.g. "please use a valid phone number") data was sent. + UpdateSuccessful bool `json:"update_successful,omitempty"` + + identityID string `json:"-"` } func NewRequest(exp time.Duration, r *http.Request, s *session.Session) *Request { diff --git a/selfservice/flow/profile/stub/identity.schema.json b/selfservice/flow/profile/stub/identity.schema.json index 396e0ca13ba..6c9a78ada03 100644 --- a/selfservice/flow/profile/stub/identity.schema.json +++ b/selfservice/flow/profile/stub/identity.schema.json @@ -4,6 +4,16 @@ "title": "Person", "type": "object", "properties": { + "email": { + "type": "string", + "ory.sh/kratos": { + "credentials": { + "password": { + "identifier": true + } + } + } + }, "stringy": { "type": "string" }, @@ -13,6 +23,10 @@ "booly": { "type": "boolean" }, + "should_big_number": { + "type": "number", + "minimum": 1200 + }, "should_long_string": { "type": "string", "minLength": 25 diff --git a/selfservice/form/html_form.go b/selfservice/form/html_form.go index 96a2551a854..6bb6e5405ac 100644 --- a/selfservice/form/html_form.go +++ b/selfservice/form/html_form.go @@ -40,7 +40,7 @@ type HTMLForm struct { Fields Fields `json:"fields"` // Errors contains all form errors. These will be duplicates of the individual field errors. - Errors []Error `json:"errors"` + Errors []Error `json:"errors,omitempty"` } // NewHTMLForm returns an empty container. @@ -57,7 +57,9 @@ func NewHTMLForm(action string) *HTMLForm { func NewHTMLFormFromRequestBody(r *http.Request, action string, compiler decoderx.HTTPDecoderOption) (*HTMLForm, error) { c := NewHTMLForm(action) raw := json.RawMessage(`{}`) - if err := decoder.Decode(r, &raw, compiler); err != nil { + if err := decoder.Decode(r, &raw, compiler, + decoderx.HTTPDecoderSetIgnoreParseErrorsStrategy(decoderx.ParseErrorIgnore), + ); err != nil { if err := c.ParseError(err); err != nil { return nil, err } diff --git a/selfservice/persistence/ephermal.go b/selfservice/persistence/ephermal.go index 681bfcb7682..a66d27c20f9 100644 --- a/selfservice/persistence/ephermal.go +++ b/selfservice/persistence/ephermal.go @@ -138,12 +138,12 @@ func (m *RequestManagerMemory) GetProfileRequest(ctx context.Context, id string) func (m *RequestManagerMemory) UpdateProfileRequest(ctx context.Context, id string, request *profile.Request) error { m.Lock() defer m.Unlock() - r, ok := m.pr[id] - if !ok { + + if _, ok := m.pr[id]; !ok { return errors.WithStack(herodot.ErrNotFound.WithReasonf("Unable to find request: %s", id)) } - *r.Form = *request.Form - m.pr[id] = r + request.ID = id + m.pr[id] = *request return nil } diff --git a/selfservice/strategy/oidc/form.go b/selfservice/strategy/oidc/form.go index 1bea77289f2..c638b0c1362 100644 --- a/selfservice/strategy/oidc/form.go +++ b/selfservice/strategy/oidc/form.go @@ -45,7 +45,10 @@ func merge(userFormValues string, openIDProviderValues json.RawMessage, option d if err := decoderx.NewHTTP().Decode( req, &decodedForm, - decoderx.HTTPFormDecoder(), option, decoderx.HTTPDecoderSetValidatePayloads(false), + decoderx.HTTPFormDecoder(), + option, + decoderx.HTTPDecoderSetIgnoreParseErrorsStrategy(decoderx.ParseErrorIgnore), + decoderx.HTTPDecoderSetValidatePayloads(false), ); err != nil { return nil, err } diff --git a/selfservice/strategy/password/registration.go b/selfservice/strategy/password/registration.go index 3a7202c7754..54d51b3cb4b 100644 --- a/selfservice/strategy/password/registration.go +++ b/selfservice/strategy/password/registration.go @@ -121,6 +121,7 @@ func (s *Strategy) handleRegistration(w http.ResponseWriter, r *http.Request, _ if err := decoderx.NewHTTP().Decode(r, &p, decoderx.HTTPFormDecoder(), option, + decoderx.HTTPDecoderSetIgnoreParseErrorsStrategy(decoderx.ParseErrorIgnore), decoderx.HTTPDecoderSetValidatePayloads(false), ); err != nil { s.handleRegistrationError(w, r, ar, &p, err) diff --git a/session/handler_mock.go b/session/handler_mock.go index cbbacdc4e55..0f06f3ab32b 100644 --- a/session/handler_mock.go +++ b/session/handler_mock.go @@ -88,16 +88,20 @@ func MockHydrateCookieClient(t *testing.T, c *http.Client, u string) { func MockSessionCreateHandlerWithIdentity(t *testing.T, reg Registry, i *identity.Identity) (httprouter.Handle, *Session) { var sess Session require.NoError(t, faker.FakeData(&sess)) - sess.Identity = i if viper.GetString(configuration.ViperKeyDefaultIdentityTraitsSchemaURL) == "" { viper.Set(configuration.ViperKeyDefaultIdentityTraitsSchemaURL, "file://./stub/fake-session.schema.json") } - _, err := reg.IdentityPool().Create(context.Background(), sess.Identity) + _, err := reg.IdentityPool().Create(context.Background(), i) require.NoError(t, err) + inserted, err := reg.IdentityPool().GetClassified(context.Background(), i.ID) + require.NoError(t, err) + sess.Identity = inserted + require.NoError(t, reg.SessionManager().Create(context.Background(), &sess)) + require.EqualValues(t, inserted.Credentials, i.Credentials) return func(w http.ResponseWriter, r *http.Request, _ httprouter.Params) { require.NoError(t, reg.SessionManager().Create(context.Background(), &sess))