From 1d72d9138a2d6671216d08637be694189136fb7d Mon Sep 17 00:00:00 2001 From: reshetnik-alexey Date: Wed, 16 Feb 2022 20:10:24 +0530 Subject: [PATCH] fix: mr comment fix --- courier/sms.go | 3 +- courier/sms_test.go | 14 ++++- courier/smtp_test.go | 9 +-- courier/template/sms/otp.go | 5 +- courier/template/sms/stub.go | 5 +- embedx/config.schema.json | 40 +++++++------- request/builder.go | 55 +++++++++---------- request/builder_test.go | 2 +- selfservice/hook/web_hook.go | 2 +- .../root.courierSMS.yaml | 23 ++++++++ x/require.go | 5 -- 11 files changed, 95 insertions(+), 68 deletions(-) create mode 100644 test/schema/fixtures/config.schema.test.success/root.courierSMS.yaml diff --git a/courier/sms.go b/courier/sms.go index 059fa638b539..7a26ad7e0136 100644 --- a/courier/sms.go +++ b/courier/sms.go @@ -19,7 +19,6 @@ type sendSMSRequestBody struct { type smsClient struct { *http.Client - Host string RequestConfig json.RawMessage GetTemplateType func(t SMSTemplate) (TemplateType, error) @@ -81,7 +80,7 @@ func (c *courier) dispatchSMS(ctx context.Context, msg Message) error { return err } - builder, err := request.NewBuilder(c.smsClient.RequestConfig, c.deps.Logger()) + builder, err := request.NewBuilder(c.smsClient.RequestConfig, c.deps.HTTPClient(ctx), c.deps.Logger()) if err != nil { return err } diff --git a/courier/sms_test.go b/courier/sms_test.go index 0178e2fee4fd..4f1796157114 100644 --- a/courier/sms_test.go +++ b/courier/sms_test.go @@ -10,6 +10,8 @@ import ( "testing" "time" + "github.com/gofrs/uuid" + "github.com/pkg/errors" "github.com/sirupsen/logrus" "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" @@ -17,7 +19,7 @@ import ( "github.com/ory/kratos/courier/template/sms" "github.com/ory/kratos/driver/config" "github.com/ory/kratos/internal" - "github.com/ory/kratos/x" + "github.com/ory/x/resilience" ) func TestQueueSMS(t *testing.T) { @@ -89,14 +91,20 @@ func TestQueueSMS(t *testing.T) { for _, message := range expectedSMS { id, err := c.QueueSMS(ctx, sms.NewTestStub(reg, message)) require.NoError(t, err) - x.RequireNotNilUUID(t, id) + require.NotEqual(t, uuid.Nil, id) } go func() { require.NoError(t, c.Work(ctx)) }() - time.Sleep(time.Second) + require.NoError(t, resilience.Retry(reg.Logger(), time.Millisecond*250, time.Second*10, func() error { + if len(actual) == len(expectedSMS) { + return nil + } + return errors.New("capacity not reached") + })) + for i, message := range actual { expected := expectedSMS[i] diff --git a/courier/smtp_test.go b/courier/smtp_test.go index 58df4f9fc9b6..626d2c45d175 100644 --- a/courier/smtp_test.go +++ b/courier/smtp_test.go @@ -8,6 +8,7 @@ import ( "testing" "time" + "github.com/gofrs/uuid" "github.com/pkg/errors" "github.com/sirupsen/logrus" "github.com/stretchr/testify/assert" @@ -70,7 +71,7 @@ func TestQueueEmail(t *testing.T) { conf.MustSet(config.ViperKeyCourierSMTPFrom, "test-stub@ory.sh") reg.Logger().Level = logrus.TraceLevel - c := reg.Courier(ctx) //??? + c := reg.Courier(ctx) ctx, cancel := context.WithCancel(ctx) defer cancel() @@ -81,7 +82,7 @@ func TestQueueEmail(t *testing.T) { Body: "test-body-1", })) require.NoError(t, err) - x.RequireNotNilUUID(t, id) + require.NotEqual(t, uuid.Nil, id) id, err = c.QueueEmail(ctx, templates.NewTestStub(reg, &templates.TestStubModel{ To: "test-recipient-2@example.org", @@ -89,7 +90,7 @@ func TestQueueEmail(t *testing.T) { Body: "test-body-2", })) require.NoError(t, err) - x.RequireNotNilUUID(t, id) + require.NotEqual(t, uuid.Nil, id) // The third email contains a sender name and custom headers conf.MustSet(config.ViperKeyCourierSMTPFromName, "Bob") @@ -103,7 +104,7 @@ func TestQueueEmail(t *testing.T) { Body: "test-body-3", })) require.NoError(t, err) - x.RequireNotNilUUID(t, id) + require.NotEqual(t, uuid.Nil, id) go func() { require.NoError(t, c.Work(ctx)) diff --git a/courier/template/sms/otp.go b/courier/template/sms/otp.go index 38e31446c1aa..ef003f63b0ea 100644 --- a/courier/template/sms/otp.go +++ b/courier/template/sms/otp.go @@ -15,8 +15,9 @@ type ( } OTPMessageModel struct { - To string - Code string + To string + Code string + Identity map[string]interface{} } ) diff --git a/courier/template/sms/stub.go b/courier/template/sms/stub.go index 841400156350..fa2fb19e3b5f 100644 --- a/courier/template/sms/stub.go +++ b/courier/template/sms/stub.go @@ -15,8 +15,9 @@ type ( } TestStubModel struct { - To string - Body string + To string + Body string + Identity map[string]interface{} } ) diff --git a/embedx/config.schema.json b/embedx/config.schema.json index 362a4902b51f..0f3674dd358a 100644 --- a/embedx/config.schema.json +++ b/embedx/config.schema.json @@ -1478,26 +1478,23 @@ "type": "string", "description": "The HTTP method to use (GET, POST, etc)." }, + "header": { + "type": "object", + "description": "The HTTP headers that must be applied to request", + "additionalProperties": { + "type": "string" + } + }, "body": { "type": "string", - "oneOf": [ - { - "format": "uri", - "pattern": "^(http|https|file|base64)://", - "description": "URI pointing to the jsonnet template used for payload generation. Only used for those HTTP methods, which support HTTP body payloads", - "examples": [ - "file:///path/to/body.jsonnet", - "file://./body.jsonnet", - "base64://ZnVuY3Rpb24oY3R4KSB7CiAgaWRlbnRpdHlfaWQ6IGlmIGN0eFsiaWRlbnRpdHkiXSAhPSBudWxsIHRoZW4gY3R4LmlkZW50aXR5LmlkLAp9=", - "https://oryapis.com/default_body.jsonnet" - ] - }, - { - "description": "DEPRECATED: please use a URI instead (i.e. prefix your filepath with 'file://')", - "not": { - "pattern": "^(http|https|file|base64)://" - } - } + "format": "uri", + "pattern": "^(http|https|file|base64)://", + "description": "URI pointing to the jsonnet template used for payload generation. Only used for those HTTP methods, which support HTTP body payloads", + "examples": [ + "file:///path/to/body.jsonnet", + "file://./body.jsonnet", + "base64://ZnVuY3Rpb24oY3R4KSB7CiAgaWRlbnRpdHlfaWQ6IGlmIGN0eFsiaWRlbnRpdHkiXSAhPSBudWxsIHRoZW4gY3R4LmlkZW50aXR5LmlkLAp9=", + "https://oryapis.com/default_body.jsonnet" ] }, "auth": { @@ -1514,7 +1511,12 @@ ] }, "additionalProperties": false - } + }, + "required": [ + "url", + "method" + ], + "additionalProperties": false } }, "additionalProperties": false diff --git a/request/builder.go b/request/builder.go index 21783c73c109..69d39bf9d43e 100644 --- a/request/builder.go +++ b/request/builder.go @@ -11,6 +11,7 @@ import ( "strings" "github.com/google/go-jsonnet" + "github.com/hashicorp/go-retryablehttp" "github.com/ory/x/fetcher" "github.com/ory/x/logrusx" @@ -22,12 +23,13 @@ const ( ) type Builder struct { - r *http.Request - log *logrusx.Logger - conf *Config + r *http.Request + log *logrusx.Logger + conf *Config + fetchClient *retryablehttp.Client } -func NewBuilder(config json.RawMessage, l *logrusx.Logger) (*Builder, error) { +func NewBuilder(config json.RawMessage, client *retryablehttp.Client, l *logrusx.Logger) (*Builder, error) { c, err := parseConfig(config) if err != nil { return nil, err @@ -39,9 +41,10 @@ func NewBuilder(config json.RawMessage, l *logrusx.Logger) (*Builder, error) { } return &Builder{ - r: r, - log: l, - conf: c, + r: r, + log: l, + conf: c, + fetchClient: client, }, nil } @@ -69,13 +72,18 @@ func (b *Builder) addBody(body interface{}) error { return errors.New("got empty template path for request with body") } + tpl, err := b.readTemplate() + if err != nil { + return err + } + switch contentType { case ContentTypeForm: - if err := b.addURLEncodedBody(body); err != nil { + if err := b.addURLEncodedBody(tpl, body); err != nil { return err } case ContentTypeJSON: - if err := b.addJSONBody(body); err != nil { + if err := b.addJSONBody(tpl, body); err != nil { return err } default: @@ -85,14 +93,7 @@ func (b *Builder) addBody(body interface{}) error { return nil } -func (b *Builder) addJSONBody(body interface{}) error { - tURL := b.conf.TemplateURI - - tpl, err := readTemplate(tURL, b.log) - if err != nil { - return err - } - +func (b *Builder) addJSONBody(template *bytes.Buffer, body interface{}) error { buf := new(bytes.Buffer) enc := json.NewEncoder(buf) enc.SetEscapeHTML(false) @@ -105,7 +106,7 @@ func (b *Builder) addJSONBody(body interface{}) error { vm := jsonnet.MakeVM() vm.TLACode("ctx", buf.String()) - res, err := vm.EvaluateAnonymousSnippet(tURL, tpl.String()) + res, err := vm.EvaluateAnonymousSnippet(b.conf.TemplateURI, template.String()) if err != nil { return err } @@ -117,13 +118,7 @@ func (b *Builder) addJSONBody(body interface{}) error { return nil } -func (b *Builder) addURLEncodedBody(body interface{}) error { - tURL := b.conf.TemplateURI - tpl, err := readTemplate(tURL, b.log) - if err != nil { - return err - } - +func (b *Builder) addURLEncodedBody(template *bytes.Buffer, body interface{}) error { buf := new(bytes.Buffer) enc := json.NewEncoder(buf) enc.SetEscapeHTML(false) @@ -136,7 +131,7 @@ func (b *Builder) addURLEncodedBody(body interface{}) error { vm := jsonnet.MakeVM() vm.TLACode("ctx", buf.String()) - res, err := vm.EvaluateAnonymousSnippet(tURL, tpl.String()) + res, err := vm.EvaluateAnonymousSnippet(b.conf.TemplateURI, template.String()) if err != nil { return err } @@ -175,18 +170,20 @@ func (b *Builder) BuildRequest(body interface{}) (*http.Request, error) { return b.r, nil } -func readTemplate(templateURI string, l *logrusx.Logger) (*bytes.Buffer, error) { +func (b *Builder) readTemplate() (*bytes.Buffer, error) { + templateURI := b.conf.TemplateURI + if templateURI == "" { return nil, nil } - f := fetcher.NewFetcher() + f := fetcher.NewFetcher(fetcher.WithClient(b.fetchClient)) tpl, err := f.Fetch(templateURI) if errors.Is(err, fetcher.ErrUnknownScheme) { // legacy filepath templateURI = "file://" + templateURI - l.WithError(err).Warnf("support for filepaths without a 'file://' scheme will be dropped in the next release, please use %s instead in your config", templateURI) + b.log.WithError(err).Warnf("support for filepaths without a 'file://' scheme will be dropped in the next release, please use %s instead in your config", templateURI) tpl, err = f.Fetch(templateURI) } diff --git a/request/builder_test.go b/request/builder_test.go index aa8fd9a90782..1deb04dee07f 100644 --- a/request/builder_test.go +++ b/request/builder_test.go @@ -238,7 +238,7 @@ func TestBuildRequest(t *testing.T) { t.Run("request-type="+tc.name, func(t *testing.T) { l := logrusx.New("kratos", "test") - rb, err := NewBuilder(json.RawMessage(tc.rawConfig), l) + rb, err := NewBuilder(json.RawMessage(tc.rawConfig), nil, l) require.NoError(t, err) assert.Equal(t, tc.bodyTemplateURI, rb.conf.TemplateURI) diff --git a/selfservice/hook/web_hook.go b/selfservice/hook/web_hook.go index 1ae2f5fe6a4d..53f0466c4424 100644 --- a/selfservice/hook/web_hook.go +++ b/selfservice/hook/web_hook.go @@ -117,7 +117,7 @@ func (e *WebHook) ExecuteSettingsPostPersistHook(_ http.ResponseWriter, req *htt } func (e *WebHook) execute(ctx context.Context, data *templateContext) error { - builder, err := request.NewBuilder(e.conf, e.deps.Logger()) + builder, err := request.NewBuilder(e.conf, e.deps.HTTPClient(ctx), e.deps.Logger()) if err != nil { return err } diff --git a/test/schema/fixtures/config.schema.test.success/root.courierSMS.yaml b/test/schema/fixtures/config.schema.test.success/root.courierSMS.yaml new file mode 100644 index 000000000000..d8c9707ed891 --- /dev/null +++ b/test/schema/fixtures/config.schema.test.success/root.courierSMS.yaml @@ -0,0 +1,23 @@ +selfservice: + default_browser_return_url: "#/definitions/defaultReturnTo" + +dsn: foo + +identity: + schemas: + - id: default + url: https://example.com + +courier: + smtp: + connection_uri: smtps://foo:bar@my-mailserver:1234/ + from_address: no-reply@ory.kratos.sh + sms: + enabled: true + from: "+19592155527" + request_config: + url: https://sms.example.com + method: POST + body: file://request.config.twilio.jsonnet + header: + 'Content-Type': "application/x-www-form-urlencoded" diff --git a/x/require.go b/x/require.go index 689dc6a1aab5..17154b1593f6 100644 --- a/x/require.go +++ b/x/require.go @@ -5,7 +5,6 @@ import ( "encoding/json" "testing" - "github.com/gofrs/uuid" "github.com/stretchr/testify/require" ) @@ -14,7 +13,3 @@ func RequireJSONMarshal(t *testing.T, in interface{}) []byte { require.NoError(t, json.NewEncoder(&b).Encode(in)) return b.Bytes() } - -func RequireNotNilUUID(t *testing.T, id uuid.UUID) { - require.NotEqual(t, uuid.Nil, id) -}