Skip to content

Commit 684737f

Browse files
aranaran
committed
fix: tailor validation errors and requiredness to schema
The implementation up to now unblocks using SMS verification, but it had the downside of always adding phone fields to verification unconditionally. (The previous implementation added the email field unconditionally.) This commit makes the verification fields conditional on the schema.
1 parent a021590 commit 684737f

9 files changed

+402
-25
lines changed

‎selfservice/flow/verification/error_test.go

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -41,6 +41,7 @@ func TestHandleError(t *testing.T) {
4141
ctx := context.Background()
4242
conf, reg := internal.NewFastRegistryWithMocks(t)
4343
conf.MustSet(ctx, config.ViperKeySelfServiceVerificationEnabled, true)
44+
testhelpers.SetDefaultIdentitySchema(conf, "file://./stub/identity.schema.json")
4445

4546
public, _ := testhelpers.NewKratosServer(t, reg)
4647

‎selfservice/flow/verification/hook_test.go

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -28,6 +28,7 @@ import (
2828
func TestVerificationExecutor(t *testing.T) {
2929
ctx := context.Background()
3030
conf, reg := internal.NewFastRegistryWithMocks(t)
31+
testhelpers.SetDefaultIdentitySchema(conf, "file://./stub/identity.schema.json")
3132

3233
newServer := func(t *testing.T, i *identity.Identity, ft flow.Type) *httptest.Server {
3334
router := httprouter.New()

‎selfservice/strategy/code/.snapshots/TestVerification-description=should_respect_schema_when_showing_verification_payloads-case=shows_fields_based_on_email-phone_schema.json

Lines changed: 70 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,70 @@
1+
[
2+
{
3+
"attributes": {
4+
"disabled": false,
5+
"name": "email",
6+
"node_type": "input",
7+
"type": "email"
8+
},
9+
"group": "code",
10+
"messages": [],
11+
"meta": {
12+
"label": {
13+
"id": 1070007,
14+
"text": "Email",
15+
"type": "info"
16+
}
17+
},
18+
"type": "input"
19+
},
20+
{
21+
"attributes": {
22+
"disabled": false,
23+
"name": "phone",
24+
"node_type": "input",
25+
"type": "tel"
26+
},
27+
"group": "code",
28+
"messages": [],
29+
"meta": {
30+
"label": {
31+
"id": 1070016,
32+
"text": "Phone",
33+
"type": "info"
34+
}
35+
},
36+
"type": "input"
37+
},
38+
{
39+
"attributes": {
40+
"disabled": false,
41+
"name": "method",
42+
"node_type": "input",
43+
"type": "submit",
44+
"value": "code"
45+
},
46+
"group": "code",
47+
"messages": [],
48+
"meta": {
49+
"label": {
50+
"id": 1070009,
51+
"text": "Continue",
52+
"type": "info"
53+
}
54+
},
55+
"type": "input"
56+
},
57+
{
58+
"attributes": {
59+
"disabled": false,
60+
"name": "csrf_token",
61+
"node_type": "input",
62+
"required": true,
63+
"type": "hidden"
64+
},
65+
"group": "default",
66+
"messages": [],
67+
"meta": {},
68+
"type": "input"
69+
}
70+
]

‎selfservice/strategy/code/.snapshots/TestVerification-description=should_respect_schema_when_showing_verification_payloads-case=shows_fields_based_on_phone-only_schema.json

Lines changed: 53 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,53 @@
1+
[
2+
{
3+
"attributes": {
4+
"disabled": false,
5+
"name": "phone",
6+
"node_type": "input",
7+
"required": true,
8+
"type": "tel"
9+
},
10+
"group": "code",
11+
"messages": [],
12+
"meta": {
13+
"label": {
14+
"id": 1070016,
15+
"text": "Phone",
16+
"type": "info"
17+
}
18+
},
19+
"type": "input"
20+
},
21+
{
22+
"attributes": {
23+
"disabled": false,
24+
"name": "method",
25+
"node_type": "input",
26+
"type": "submit",
27+
"value": "code"
28+
},
29+
"group": "code",
30+
"messages": [],
31+
"meta": {
32+
"label": {
33+
"id": 1070009,
34+
"text": "Continue",
35+
"type": "info"
36+
}
37+
},
38+
"type": "input"
39+
},
40+
{
41+
"attributes": {
42+
"disabled": false,
43+
"name": "csrf_token",
44+
"node_type": "input",
45+
"required": true,
46+
"type": "hidden"
47+
},
48+
"group": "default",
49+
"messages": [],
50+
"meta": {},
51+
"type": "input"
52+
}
53+
]

‎selfservice/strategy/code/strategy.go

Lines changed: 128 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -6,6 +6,7 @@ package code
66
import (
77
"context"
88
"encoding/json"
9+
"io"
910
"net/http"
1011
"sort"
1112
"strings"
@@ -16,6 +17,7 @@ import (
1617
"github.com/tidwall/gjson"
1718

1819
"github.com/ory/herodot"
20+
"github.com/ory/jsonschema/v3"
1921
"github.com/ory/kratos/continuity"
2022
"github.com/ory/kratos/courier"
2123
"github.com/ory/kratos/driver/config"
@@ -247,6 +249,128 @@ func (s *Strategy) PopulateMethod(r *http.Request, f flow.Flow) error {
247249
return nil
248250
}
249251

252+
func (s *Strategy) GetSupportedVerificationChannels(ctx context.Context) (map[identity.VerifiableAddressType]bool, error) {
253+
channels := make(map[identity.VerifiableAddressType]bool)
254+
255+
schemaList, err := s.deps.IdentityTraitsSchemas(ctx)
256+
if err != nil {
257+
return nil, err
258+
}
259+
260+
schemaID := s.deps.Config().DefaultIdentityTraitsSchemaID(ctx)
261+
identitySchema, err := schemaList.GetByID(schemaID)
262+
if err != nil {
263+
return nil, err
264+
}
265+
266+
rawURL := identitySchema.RawURL
267+
if rawURL == "" && identitySchema.URL != nil {
268+
rawURL = identitySchema.URL.String()
269+
}
270+
271+
schemaFile, err := jsonschema.LoadURL(ctx, rawURL)
272+
if err != nil {
273+
return nil, err
274+
}
275+
276+
schemaData, err := io.ReadAll(io.LimitReader(schemaFile, 1024*1024))
277+
if err != nil {
278+
return nil, err
279+
}
280+
281+
var schemaJSON map[string]interface{}
282+
if err := json.Unmarshal(schemaData, &schemaJSON); err != nil {
283+
return nil, err
284+
}
285+
286+
// properties.traits.properties.<trait>."ory.sh/kratos".verification.via
287+
if props, ok := schemaJSON["properties"].(map[string]interface{}); ok {
288+
if traits, ok := props["traits"].(map[string]interface{}); ok {
289+
if traitProps, ok := traits["properties"].(map[string]interface{}); ok {
290+
for _, propValue := range traitProps {
291+
if prop, ok := propValue.(map[string]interface{}); ok {
292+
if ext, ok := prop[schema.ExtensionName].(map[string]interface{}); ok {
293+
if verification, ok := ext["verification"].(map[string]interface{}); ok {
294+
// Set the appropriate channel based on verification via value
295+
switch verification["via"].(string) {
296+
case identity.ChannelTypeEmail:
297+
channels[identity.VerifiableAddressTypeEmail] = true
298+
case identity.ChannelTypeSMS:
299+
channels[identity.VerifiableAddressTypePhone] = true
300+
}
301+
}
302+
}
303+
}
304+
}
305+
}
306+
}
307+
}
308+
309+
return channels, nil
310+
}
311+
312+
func (s *Strategy) GetVerificationRequiredField(ctx context.Context) (pointer string, property string, err error) {
313+
channels, err := s.GetSupportedVerificationChannels(ctx)
314+
if err != nil {
315+
return "#/identifier", "identifier", err
316+
}
317+
318+
activeChannelCount := 0
319+
for _, active := range channels {
320+
if active {
321+
activeChannelCount++
322+
}
323+
}
324+
325+
// Return field-specific errors if only one channel is supported
326+
if activeChannelCount == 1 {
327+
if channels[identity.VerifiableAddressTypeEmail] {
328+
return "#/email", "email", nil
329+
}
330+
if channels[identity.VerifiableAddressTypePhone] {
331+
return "#/phone", "phone", nil
332+
}
333+
}
334+
335+
// Default to generic identifier error
336+
return "#/identifier", "identifier", nil
337+
}
338+
339+
func (s *Strategy) addVerificationNodes(ctx context.Context, nodes *node.Nodes, email interface{}, phone interface{}) error {
340+
channels, err := s.GetSupportedVerificationChannels(ctx)
341+
if err != nil {
342+
return err
343+
}
344+
345+
activeChannelCount := 0
346+
for _, active := range channels {
347+
if active {
348+
activeChannelCount++
349+
}
350+
}
351+
352+
opts := []node.InputAttributesModifier{}
353+
if activeChannelCount == 1 {
354+
opts = append(opts, node.WithRequiredInputAttribute)
355+
}
356+
357+
if channels[identity.VerifiableAddressTypeEmail] {
358+
nodes.Append(
359+
node.NewInputField("email", email, node.CodeGroup, node.InputAttributeTypeEmail, opts...).
360+
WithMetaLabel(text.NewInfoNodeInputEmail()),
361+
)
362+
}
363+
364+
if channels[identity.VerifiableAddressTypePhone] {
365+
nodes.Append(
366+
node.NewInputField("phone", phone, node.CodeGroup, node.InputAttributeTypeTel, opts...).
367+
WithMetaLabel(text.NewInfoNodeInputPhone()),
368+
)
369+
}
370+
371+
return nil
372+
}
373+
250374
func (s *Strategy) populateChooseMethodFlow(r *http.Request, f flow.Flow) error {
251375
ctx := r.Context()
252376
switch f := f.(type) {
@@ -260,15 +384,10 @@ func (s *Strategy) populateChooseMethodFlow(r *http.Request, f flow.Flow) error
260384
WithMetaLabel(text.NewInfoNodeLabelContinue()),
261385
)
262386
case *verification.Flow:
263-
// Add both email and phone fields for verification
264-
f.GetUI().Nodes.Append(
265-
node.NewInputField("email", nil, node.CodeGroup, node.InputAttributeTypeEmail).
266-
WithMetaLabel(text.NewInfoNodeInputEmail()),
267-
)
268-
f.GetUI().Nodes.Append(
269-
node.NewInputField("phone", nil, node.CodeGroup, node.InputAttributeTypeTel).
270-
WithMetaLabel(text.NewInfoNodeInputPhone()),
271-
)
387+
if err := s.addVerificationNodes(ctx, &f.GetUI().Nodes, nil, nil); err != nil {
388+
return err
389+
}
390+
272391
f.GetUI().Nodes.Append(
273392
node.NewInputField("method", s.ID(), node.CodeGroup, node.InputAttributeTypeSubmit).
274393
WithMetaLabel(text.NewInfoNodeLabelContinue()),

‎selfservice/strategy/code/strategy_verification.go

Lines changed: 28 additions & 16 deletions
Original file line numberDiff line numberDiff line change
@@ -65,7 +65,6 @@ func (s *Strategy) decodeVerification(r *http.Request) (*updateVerificationFlowW
6565
return &body, nil
6666
}
6767

68-
// handleVerificationError is a convenience function for handling all types of errors that may occur (e.g. validation error).
6968
func (s *Strategy) handleVerificationError(r *http.Request, f *verification.Flow, body *updateVerificationFlowWithCodeMethod, err error) error {
7069
if f != nil {
7170
f.UI.SetCSRF(s.deps.GenerateCSRFToken(r))
@@ -76,12 +75,11 @@ func (s *Strategy) handleVerificationError(r *http.Request, f *verification.Flow
7675
phone = body.Phone
7776
}
7877

79-
f.UI.GetNodes().Upsert(
80-
node.NewInputField("email", email, node.CodeGroup, node.InputAttributeTypeEmail).WithMetaLabel(text.NewInfoNodeInputEmail()),
81-
)
82-
f.UI.GetNodes().Upsert(
83-
node.NewInputField("phone", phone, node.CodeGroup, node.InputAttributeTypeTel).WithMetaLabel(text.NewInfoNodeInputPhone()),
84-
)
78+
f.UI.Nodes = node.Nodes{}
79+
80+
if updateErr := s.addVerificationNodes(r.Context(), &f.UI.Nodes, email, phone); updateErr != nil {
81+
return errors.Wrap(err, updateErr.Error())
82+
}
8583
}
8684

8785
return err
@@ -217,9 +215,25 @@ func (s *Strategy) verificationHandleFormSubmission(ctx context.Context, w http.
217215

218216
// If not GET: try to use the submitted code
219217
return s.verificationUseCode(ctx, w, r, body.Code, f)
220-
} else if len(body.Email) == 0 && len(body.Phone) == 0 {
221-
// If no code, email, or phone was provided, fail with a validation error
222-
return s.handleVerificationError(r, f, body, schema.NewRequiredError("#/identifier", "identifier"))
218+
}
219+
220+
channels, err := s.GetSupportedVerificationChannels(ctx)
221+
if err != nil {
222+
return s.handleVerificationError(r, f, body, err)
223+
}
224+
225+
hasValidIdentifier := false
226+
if channels[identity.VerifiableAddressTypeEmail] && len(body.Email) > 0 {
227+
hasValidIdentifier = true
228+
}
229+
if channels[identity.VerifiableAddressTypePhone] && len(body.Phone) > 0 {
230+
hasValidIdentifier = true
231+
}
232+
233+
if !hasValidIdentifier {
234+
// If no code and no valid identifier was provided, fail with a validation error
235+
pointer, property, _ := s.GetVerificationRequiredField(ctx)
236+
return s.handleVerificationError(r, f, body, schema.NewRequiredError(pointer, property))
223237
}
224238

225239
if err := flow.EnsureCSRF(s.deps, r, f.Type, s.deps.Config().DisableAPIFlowEnforcement(ctx), s.deps.GenerateCSRFToken, body.CSRFToken); err != nil {
@@ -230,16 +244,15 @@ func (s *Strategy) verificationHandleFormSubmission(ctx context.Context, w http.
230244
return s.handleVerificationError(r, f, body, err)
231245
}
232246

233-
// Handle verification via email or SMS based on provided identifier
234-
if len(body.Email) > 0 {
247+
if channels[identity.VerifiableAddressTypeEmail] && len(body.Email) > 0 {
235248
if err := s.deps.CodeSender().SendVerificationCode(ctx, f, identity.VerifiableAddressTypeEmail, body.Email); err != nil {
236249
if !errors.Is(err, ErrUnknownAddress) {
237250
return s.handleVerificationError(r, f, body, err)
238251
}
239252
// Continue execution
240253
}
241254
f.State = flow.StateEmailSent
242-
} else if len(body.Phone) > 0 {
255+
} else if channels[identity.VerifiableAddressTypePhone] && len(body.Phone) > 0 {
243256
if err := s.deps.CodeSender().SendVerificationCode(ctx, f, identity.VerifiableAddressTypePhone, body.Phone); err != nil {
244257
if !errors.Is(err, ErrUnknownAddress) {
245258
return s.handleVerificationError(r, f, body, err)
@@ -253,15 +266,14 @@ func (s *Strategy) verificationHandleFormSubmission(ctx context.Context, w http.
253266
return s.handleVerificationError(r, f, body, err)
254267
}
255268

256-
// Add appropriate resend buttons based on the identifier used
257-
if body.Email != "" {
269+
if channels[identity.VerifiableAddressTypeEmail] && body.Email != "" {
258270
f.UI.Nodes.Append(
259271
node.NewInputField("email", body.Email, node.CodeGroup, node.InputAttributeTypeSubmit).
260272
WithMetaLabel(text.NewInfoNodeResendOTP()),
261273
)
262274
}
263275

264-
if body.Phone != "" {
276+
if channels[identity.VerifiableAddressTypePhone] && body.Phone != "" {
265277
f.UI.Nodes.Append(
266278
node.NewInputField("phone", body.Phone, node.CodeGroup, node.InputAttributeTypeSubmit).
267279
WithMetaLabel(text.NewInfoNodeResendOTP()),

0 commit comments

Comments
 (0)