Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Introduce PropertyFormat to avoid validation errors that can lead to internal exceptions #248

Merged
merged 12 commits into from
Aug 26, 2024
Merged
Original file line number Diff line number Diff line change
Expand Up @@ -39,6 +39,8 @@ public sealed record IntegrationProperty(string Name, PropertyType Type)

public string? Pattern { get; init; }

public PropertyFormat Format { get; init; } = PropertyFormat.None;
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Validation is missing in the TryGetString method.


public bool IsValid(string? input, [MaybeNullWhen(true)] out string error)
{
switch (Type)
Expand Down Expand Up @@ -170,6 +172,32 @@ private bool TryGetString(string? input, [MaybeNullWhen(true)] out string error,
}
}

if (!string.IsNullOrWhiteSpace(input) && Format != PropertyFormat.None)
{
switch (Format)
{
case PropertyFormat.Email:
if (!Regex.IsMatch(input, ValidationPatterns.Email))
{
error = Texts.IntegrationPropertyFormatEmail;
return false;
}

break;
case PropertyFormat.HttpUrl:
// We only allow "http" and "https" schemas to enable the usage of URL field for HttpClient requests.
if (!Uri.TryCreate(input, UriKind.Absolute, out var uri)
|| (!string.Equals(uri.Scheme, "http", StringComparison.OrdinalIgnoreCase) && !string.Equals(uri.Scheme, "https", StringComparison.OrdinalIgnoreCase))
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You can just create a static readonly field with HttpSchemes, would have been shorter ;)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should I do it in another pull request? I see you already merged this one :)

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, plz :)

)
{
error = Texts.IntegrationPropertyFormatHttpUrl;
return false;
}

break;
}
}

return true;
}

Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,15 @@
// ==========================================================================
// Notifo.io
// ==========================================================================
// Copyright (c) Sebastian Stehle
// All rights reserved. Licensed under the MIT license.
// ==========================================================================

namespace Notifo.Domain.Integrations;

public enum PropertyFormat
{
None,
Email,
HttpUrl
}

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

Original file line number Diff line number Diff line change
Expand Up @@ -120,6 +120,12 @@
<data name="IntegrationPropertyAllowedValue" xml:space="preserve">
<value>Field is not an allowed value.</value>
</data>
<data name="IntegrationPropertyFormatEmail" xml:space="preserve">
<value>Field is not a valid e-mail.</value>
</data>
<data name="IntegrationPropertyFormatHttpUrl" xml:space="preserve">
<value>Field is not a valid URL (remember about the protocol - http or https).</value>
</data>
<data name="IntegrationPropertyInvalidBoolean" xml:space="preserve">
<value>Not a valid boolean value.</value>
</data>
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,14 @@
// ==========================================================================
// Notifo.io
// ==========================================================================
// Copyright (c) Sebastian Stehle
// All rights reserved. Licensed under the MIT license.
// ==========================================================================

namespace Notifo.Domain.Integrations;

public static class ValidationPatterns
{
// Taken from Yup's source code (validation library used on the front-end side).
public const string Email = @"^[a-zA-Z0-9.!#$%&'*+\/=?^_`{|}~-]+@[a-zA-Z0-9](?:[a-zA-Z0-9-]{0,61}[a-zA-Z0-9])?(?:\.[a-zA-Z0-9](?:[a-zA-Z0-9-]{0,61}[a-zA-Z0-9])?)*$";
}
Original file line number Diff line number Diff line change
Expand Up @@ -29,11 +29,11 @@ public sealed class IntegratedAmazonSESIntegration : IIntegration, IInitializabl

public static readonly IntegrationProperty FromEmailProperty = new IntegrationProperty("fromEmail", PropertyType.Text)
{
Pattern = Patterns.Email,
EditorLabel = Texts.Email_FromEmailLabel,
EditorDescription = Texts.Email_FromEmailDescription,
IsRequired = true,
Summary = true
Summary = true,
Format = PropertyFormat.Email
};

public static readonly IntegrationProperty FromNameProperty = new IntegrationProperty("fromName", PropertyType.Text)
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -18,7 +18,8 @@ public sealed partial class HttpIntegration : IIntegration
EditorLabel = Texts.Webhook_URLLabel,
EditorDescription = Texts.Webhook_URLHints,
IsRequired = true,
Summary = true
Summary = true,
Format = PropertyFormat.HttpUrl,
};

private static readonly IntegrationProperty HttpMethodProperty = new IntegrationProperty("Method", PropertyType.Text)
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -22,11 +22,11 @@ public sealed partial class MailchimpIntegration : IIntegration

public static readonly IntegrationProperty FromEmailProperty = new IntegrationProperty("fromEmail", PropertyType.Text)
{
Pattern = Patterns.Email,
EditorLabel = Texts.Email_FromEmailLabel,
EditorDescription = Texts.Email_FromEmailDescription,
IsRequired = true,
Summary = true
Summary = true,
Format = PropertyFormat.Email
};

public static readonly IntegrationProperty FromNameProperty = new IntegrationProperty("fromName", PropertyType.Text)
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -29,11 +29,11 @@ public sealed partial class MailjetIntegration : IIntegration

public static readonly IntegrationProperty FromEmailProperty = new IntegrationProperty("fromEmail", PropertyType.Text)
{
Pattern = Patterns.Email,
EditorLabel = Texts.Email_FromEmailLabel,
EditorDescription = Texts.Email_FromEmailDescription,
IsRequired = true,
Summary = true
Summary = true,
Format = PropertyFormat.Email
};

public static readonly IntegrationProperty FromNameProperty = new IntegrationProperty("fromName", PropertyType.Text)
Expand Down
13 changes: 0 additions & 13 deletions backend/src/Notifo.Domain.Integrations/Patterns.cs

This file was deleted.

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

Original file line number Diff line number Diff line change
Expand Up @@ -438,7 +438,7 @@ When you have done this send me an /update command.</value>
<value>Send Always</value>
</data>
<data name="Webhook_URLHints" xml:space="preserve">
<value>The URL to your server endpoint.</value>
<value>The URL to your server endpoint. Remember about the protocol (http, https).</value>
</data>
<data name="Webhook_URLLabel" xml:space="preserve">
<value>URL</value>
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -42,11 +42,11 @@ public sealed partial class SmtpIntegration : IIntegration

public static readonly IntegrationProperty FromEmailProperty = new IntegrationProperty("fromEmail", PropertyType.Text)
{
Pattern = Patterns.Email,
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you put this property at the end, just like in the other property?. My brain is annoying sometimes ;)

EditorLabel = Texts.Email_FromEmailLabel,
EditorDescription = Texts.Email_FromEmailDescription,
IsRequired = true,
Summary = true
Summary = true,
Format = PropertyFormat.Email,
};

public static readonly IntegrationProperty FromNameProperty = new IntegrationProperty("fromName", PropertyType.Text)
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -72,6 +72,11 @@ public sealed class IntegrationPropertyDto
/// </summary>
public string? Pattern { get; set; }

/// <summary>
/// Format of the field, used to both validate the input and to provide hints to the user.
/// </summary>
public PropertyFormat Format { get; set; }

/// <summary>
/// The default value.
/// </summary>
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -158,6 +158,77 @@ public void Should_not_fail_if_undefined_value_is_not_an_allowed_value(string? i
Assert.Equal("allowed", property.GetString(source));
}

[Theory]
[InlineData("localhost.com/test")]
[InlineData("192.168.0.101")]
[InlineData("randomString")]
public void Should_fail_if_url_is_invalid(string? input)
{
var source = new Dictionary<string, string>
{
["key"] = input!
};

var property = new IntegrationProperty("key", PropertyType.Text)
{
Format = PropertyFormat.HttpUrl
};

Assert.Throws<ValidationException>(() => property.GetString(source));
}

[Theory]
[InlineData("http://192.168.0.101/")]
[InlineData("http://localhost/test")]
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A test with http://<IP> might be useful. Perhaps you also have it above

[InlineData("https://example.com/test?query=example")]
[InlineData("http://login:[email protected]/random")]
public void Should_get_url_if_value_is_valid(string? input)
{
var source = new Dictionary<string, string>
{
["key"] = input!
};

var property = new IntegrationProperty("key", PropertyType.Text)
{
Format = PropertyFormat.HttpUrl
};

Assert.Equal(input, property.GetString(source));
}

[Fact]
public void Should_fail_if_email_is_invalid()
{
var source = new Dictionary<string, string>
{
["key"] = "invalidEmail"
};

var property = new IntegrationProperty("key", PropertyType.Text)
{
Format = PropertyFormat.Email
};

Assert.Throws<ValidationException>(() => property.GetString(source));
}

[Fact]
public void Should_get_email_if_value_is_valid()
{
var source = new Dictionary<string, string>
{
["key"] = "[email protected]"
};

var property = new IntegrationProperty("key", PropertyType.Text)
{
Format = PropertyFormat.Email
};

Assert.Equal(source["key"], property.GetString(source));
}

[Fact]
public void Should_get_value_if_all_requirements_are_met()
{
Expand Down
2 changes: 1 addition & 1 deletion frontend/src/app/pages/app/AppAuth.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -86,7 +86,7 @@ const FormSchema = Yup.object().shape({

// Valid URL.
signoutRedirectUrl: Yup.string()
.label(texts.auth.signoutRedirectUrl).urlI18n(),
.label(texts.auth.signoutRedirectUrl).httpUrlI18n(),
});

const AuthForm = ({ appDetails, scheme }: { scheme?: AuthSchemeDto } & AppAuthProps) => {
Expand Down
2 changes: 1 addition & 1 deletion frontend/src/app/pages/app/AppSettings.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -28,7 +28,7 @@ const FormSchema = Yup.object().shape({

// Valid URL
confirmUrl: Yup.string().nullable()
.label(texts.app.confirmUrl).urlI18n(),
.label(texts.app.confirmUrl).httpUrlI18n(),
});

export interface AppSettingsProps {
Expand Down
25 changes: 25 additions & 0 deletions frontend/src/app/pages/integrations/IntegrationDialog.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -124,6 +124,17 @@ export const IntegrationDialog = (props: IntegrationDialogProps) => {
propertyType = propertyType.max(property.maxLength, texts.validation.maxLengthFn);
}

if (property.format && property.format !== "None") {
switch (property.format) {
case "Email":
propertyType = propertyType.emailI18n();
break;
case "HttpUrl":
propertyType = propertyType.httpUrlI18n();
break;
}
}

if (property.pattern) {
propertyType = propertyType.matches(new RegExp(property.pattern), texts.validation.patternFn);
}
Expand Down Expand Up @@ -271,6 +282,20 @@ export const FormField = ({ property }: { property: IntegrationPropertyDto }) =>
label={label} hints={property.editorDescription} />
);
} else {
if (property.format && property.format !== 'None') {
switch (property.format) {
case 'Email':
return (
<Forms.Email name={name}
label={label} hints={property.editorDescription} />
);
case 'HttpUrl':
return (
<Forms.Url name={name}
label={label} hints={property.editorDescription} />
);
}
}
return (
<Forms.Text name={name}
label={label} hints={property.editorDescription} />
Expand Down
2 changes: 1 addition & 1 deletion frontend/src/app/pages/users/UserDialog.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -119,7 +119,7 @@ export const UserDialog = (props: UserDialogProps) => {
<Forms.Text name='emailAddress'
label={texts.common.emailAddress} />

<Forms.Text name='phoneNumber'
<Forms.Phone name='phoneNumber'
label={texts.common.phoneNumber} />

<Forms.Select name='preferredLanguage' options={coreLanguages}
Expand Down
3 changes: 3 additions & 0 deletions frontend/src/app/service/service.ts
Original file line number Diff line number Diff line change
Expand Up @@ -7430,11 +7430,14 @@ export interface IntegrationPropertyDto {
maxLength?: number | undefined;
/** The pattern (for strings). */
pattern?: string | undefined;
/** Format of the field, used to both validate the input and to provide hints to the user. */
format: PropertyFormat;
/** The default value. */
defaultValue?: any | undefined;
}

export type PropertyType = "Text" | "Number" | "MultilineText" | "Password" | "Boolean";
export type PropertyFormat = "None" | "Email" | "HttpUrl";

export interface IntegrationCreatedDto {
/** The ID of the integration. */
Expand Down
8 changes: 8 additions & 0 deletions frontend/src/app/shared/components/Forms.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -177,6 +177,14 @@ export module Forms {
);
};

export const Phone = ({ placeholder, ...other }: FormEditorProps) => {
return (
<Forms.Row {...other}>
<InputSpecial type='tel' name={other.name} placeholder={placeholder} />
</Forms.Row>
);
}

export const Url = ({ placeholder, ...other }: FormEditorProps) => {
return (
<Forms.Row {...other}>
Expand Down
Loading