Merge pull request #1268 from json-api-dotnet/harden-attributes

maurei · web-flow · commit 627bae99c71d · 2023-05-11T14:07:57.000+02:00
Harden Attr/Relationship attributes against invalid input
diff --git a/src/JsonApiDotNetCore.Annotations/Resources/Annotations/AttrAttribute.cs b/src/JsonApiDotNetCore.Annotations/Resources/Annotations/AttrAttribute.cs
@@ -31,6 +31,7 @@ public AttrCapabilities Capabilities
         set => _capabilities = value;
     }
 
+    /// <inheritdoc />
     public override bool Equals(object? obj)
     {
         if (ReferenceEquals(this, obj))
@@ -48,6 +49,7 @@ public override bool Equals(object? obj)
         return Capabilities == other.Capabilities && base.Equals(other);
     }
 
+    /// <inheritdoc />
     public override int GetHashCode()
     {
         return HashCode.Combine(Capabilities, base.GetHashCode());
diff --git a/src/JsonApiDotNetCore.Annotations/Resources/Annotations/HasManyAttribute.cs b/src/JsonApiDotNetCore.Annotations/Resources/Annotations/HasManyAttribute.cs
@@ -1,3 +1,4 @@
+using System.Collections;
 using JetBrains.Annotations;
 
 // ReSharper disable NonReadonlyMemberInGetHashCode
@@ -65,6 +66,34 @@ private bool EvaluateIsManyToMany()
         return false;
     }
 
+    /// <inheritdoc />
+    public override void SetValue(object resource, object? newValue)
+    {
+        ArgumentGuard.NotNull(newValue);
+        AssertIsIdentifiableCollection(newValue);
+
+        base.SetValue(resource, newValue);
+    }
+
+    private void AssertIsIdentifiableCollection(object newValue)
+    {
+        if (newValue is not IEnumerable enumerable)
+        {
+            throw new InvalidOperationException($"Resource of type '{newValue.GetType()}' must be a collection.");
+        }
+
+        foreach (object? element in enumerable)
+        {
+            if (element == null)
+            {
+                throw new InvalidOperationException("Resource collection must not contain null values.");
+            }
+
+            AssertIsIdentifiable(element);
+        }
+    }
+
+    /// <inheritdoc />
     public override bool Equals(object? obj)
     {
         if (ReferenceEquals(this, obj))
@@ -82,6 +111,7 @@ public override bool Equals(object? obj)
         return _capabilities == other._capabilities && base.Equals(other);
     }
 
+    /// <inheritdoc />
     public override int GetHashCode()
     {
         return HashCode.Combine(_capabilities, base.GetHashCode());
diff --git a/src/JsonApiDotNetCore.Annotations/Resources/Annotations/HasOneAttribute.cs b/src/JsonApiDotNetCore.Annotations/Resources/Annotations/HasOneAttribute.cs
@@ -64,6 +64,14 @@ private bool EvaluateIsOneToOne()
         return false;
     }
 
+    /// <inheritdoc />
+    public override void SetValue(object resource, object? newValue)
+    {
+        AssertIsIdentifiable(newValue);
+        base.SetValue(resource, newValue);
+    }
+
+    /// <inheritdoc />
     public override bool Equals(object? obj)
     {
         if (ReferenceEquals(this, obj))
@@ -81,6 +89,7 @@ public override bool Equals(object? obj)
         return _capabilities == other._capabilities && base.Equals(other);
     }
 
+    /// <inheritdoc />
     public override int GetHashCode()
     {
         return HashCode.Combine(_capabilities, base.GetHashCode());
diff --git a/src/JsonApiDotNetCore.Annotations/Resources/Annotations/RelationshipAttribute.cs b/src/JsonApiDotNetCore.Annotations/Resources/Annotations/RelationshipAttribute.cs
@@ -86,6 +86,7 @@ public bool CanInclude
         set => _canInclude = value;
     }
 
+    /// <inheritdoc />
     public override bool Equals(object? obj)
     {
         if (ReferenceEquals(this, obj))
@@ -103,6 +104,7 @@ public override bool Equals(object? obj)
         return _rightType?.ClrType == other._rightType?.ClrType && Links == other.Links && base.Equals(other);
     }
 
+    /// <inheritdoc />
     public override int GetHashCode()
     {
         return HashCode.Combine(_rightType?.ClrType, Links, base.GetHashCode());
diff --git a/src/JsonApiDotNetCore.Annotations/Resources/Annotations/ResourceFieldAttribute.cs b/src/JsonApiDotNetCore.Annotations/Resources/Annotations/ResourceFieldAttribute.cs
@@ -68,6 +68,7 @@ internal set
     public object? GetValue(object resource)
     {
         ArgumentGuard.NotNull(resource);
+        AssertIsIdentifiable(resource);
 
         if (Property.GetMethod == null)
         {
@@ -82,17 +83,18 @@ internal set
         {
             throw new InvalidOperationException(
                 $"Unable to get property value of '{Property.DeclaringType!.Name}.{Property.Name}' on instance of type '{resource.GetType().Name}'.",
-                exception);
+                exception.InnerException ?? exception);
         }
     }
 
     /// <summary>
     /// Sets the value of this field on the specified resource instance. Throws if the property is read-only or if the field does not belong to the specified
     /// resource instance.
     /// </summary>
-    public void SetValue(object resource, object? newValue)
+    public virtual void SetValue(object resource, object? newValue)
     {
         ArgumentGuard.NotNull(resource);
+        AssertIsIdentifiable(resource);
 
         if (Property.SetMethod == null)
         {
@@ -107,15 +109,25 @@ public void SetValue(object resource, object? newValue)
         {
             throw new InvalidOperationException(
                 $"Unable to set property value of '{Property.DeclaringType!.Name}.{Property.Name}' on instance of type '{resource.GetType().Name}'.",
-                exception);
+                exception.InnerException ?? exception);
         }
     }
 
+    protected void AssertIsIdentifiable(object? resource)
+    {
+        if (resource != null && resource is not IIdentifiable)
+        {
+            throw new InvalidOperationException($"Resource of type '{resource.GetType()}' does not implement {nameof(IIdentifiable)}.");
+        }
+    }
+
+    /// <inheritdoc />
     public override string? ToString()
     {
         return _publicName ?? (_property != null ? _property.Name : base.ToString());
     }
 
+    /// <inheritdoc />
     public override bool Equals(object? obj)
     {
         if (ReferenceEquals(this, obj))
@@ -133,6 +145,7 @@ public override bool Equals(object? obj)
         return _publicName == other._publicName && _property == other._property;
     }
 
+    /// <inheritdoc />
     public override int GetHashCode()
     {
         return HashCode.Combine(_publicName, _property);
diff --git a/src/JsonApiDotNetCore/Queries/FieldSelectors.cs b/src/JsonApiDotNetCore/Queries/FieldSelectors.cs
@@ -52,9 +52,10 @@ public void IncludeAttributes(IEnumerable<AttrAttribute> attributes)
         }
     }
 
-    public void IncludeRelationship(RelationshipAttribute relationship, QueryLayer? queryLayer)
+    public void IncludeRelationship(RelationshipAttribute relationship, QueryLayer queryLayer)
     {
         ArgumentGuard.NotNull(relationship);
+        ArgumentGuard.NotNull(queryLayer);
 
         this[relationship] = queryLayer;
     }
diff --git a/src/JsonApiDotNetCore/Queries/Internal/Parsing/FilterParser.cs b/src/JsonApiDotNetCore/Queries/Internal/Parsing/FilterParser.cs
@@ -420,7 +420,7 @@ private object ConvertStringToType(string value, Type type)
 
     private Converter<string, object> GetConstantValueConverterForAttribute(AttrAttribute attribute)
     {
-        return stringValue => attribute.Property.Name == nameof(IIdentifiable<object>.Id)
+        return stringValue => attribute.Property.Name == nameof(Identifiable<object>.Id)
             ? DeObfuscateStringId(attribute.Type.ClrType, stringValue)
             : ConvertStringToType(stringValue, attribute.Property.PropertyType);
     }
diff --git a/src/JsonApiDotNetCore/Queries/Internal/QueryableBuilding/SelectClauseBuilder.cs b/src/JsonApiDotNetCore/Queries/Internal/QueryableBuilding/SelectClauseBuilder.cs
@@ -162,9 +162,9 @@ private void IncludeAllScalarProperties(Type elementType, Dictionary<PropertyInf
 
     private static void IncludeFields(FieldSelectors fieldSelectors, Dictionary<PropertyInfo, PropertySelector> propertySelectors)
     {
-        foreach ((ResourceFieldAttribute resourceField, QueryLayer? queryLayer) in fieldSelectors)
+        foreach ((ResourceFieldAttribute resourceField, QueryLayer? nextLayer) in fieldSelectors)
         {
-            var propertySelector = new PropertySelector(resourceField.Property, queryLayer);
+            var propertySelector = new PropertySelector(resourceField.Property, nextLayer);
             IncludeWritableProperty(propertySelector, propertySelectors);
         }
     }
diff --git a/src/JsonApiDotNetCore/Repositories/EntityFrameworkCoreRepository.cs b/src/JsonApiDotNetCore/Repositories/EntityFrameworkCoreRepository.cs
@@ -163,6 +163,8 @@ public virtual Task<TResource> GetForCreateAsync(Type resourceClrType, TId id, C
             id
         });
 
+        ArgumentGuard.NotNull(resourceClrType);
+
         var resource = (TResource)_resourceFactory.CreateInstance(resourceClrType);
         resource.Id = id;
 
diff --git a/test/JsonApiDotNetCoreTests/IntegrationTests/QueryStrings/SparseFieldSets/SparseFieldSetTests.cs b/test/JsonApiDotNetCoreTests/IntegrationTests/QueryStrings/SparseFieldSets/SparseFieldSetTests.cs
@@ -791,7 +791,7 @@ public async Task Cannot_select_ToMany_relationship_with_blocked_capability()
     }
 
     [Fact]
-    public async Task Retrieves_all_properties_when_fieldset_contains_readonly_attribute()
+    public async Task Fetches_all_scalar_properties_when_fieldset_contains_readonly_attribute()
     {
         // Arrange
         var store = _testContext.Factory.Services.GetRequiredService<ResourceCaptureStore>();
diff --git a/test/JsonApiDotNetCoreTests/UnitTests/ResourceGraph/HasManyAttributeTests.cs b/test/JsonApiDotNetCoreTests/UnitTests/ResourceGraph/HasManyAttributeTests.cs
@@ -0,0 +1,141 @@
+using FluentAssertions;
+using JsonApiDotNetCore.Resources;
+using JsonApiDotNetCore.Resources.Annotations;
+using TestBuildingBlocks;
+using Xunit;
+
+namespace JsonApiDotNetCoreTests.UnitTests.ResourceGraph;
+
+public sealed class HasManyAttributeTests
+{
+    [Fact]
+    public void Cannot_set_value_to_null()
+    {
+        // Arrange
+        var attribute = new HasManyAttribute
+        {
+            Property = typeof(TestResource).GetProperty(nameof(TestResource.Children))!
+        };
+
+        var resource = new TestResource();
+
+        // Act
+        Action action = () => attribute.SetValue(resource, null);
+
+        // Assert
+        action.Should().ThrowExactly<ArgumentNullException>();
+    }
+
+    [Fact]
+    public void Cannot_set_value_to_primitive_type()
+    {
+        // Arrange
+        var attribute = new HasManyAttribute
+        {
+            Property = typeof(TestResource).GetProperty(nameof(TestResource.Children))!
+        };
+
+        var resource = new TestResource();
+
+        // Act
+        Action action = () => attribute.SetValue(resource, 1);
+
+        // Assert
+        action.Should().ThrowExactly<InvalidOperationException>().WithMessage("Resource of type 'System.Int32' must be a collection.");
+    }
+
+    [Fact]
+    public void Cannot_set_value_to_single_resource()
+    {
+        // Arrange
+        var attribute = new HasManyAttribute
+        {
+            Property = typeof(TestResource).GetProperty(nameof(TestResource.Children))!
+        };
+
+        var resource = new TestResource();
+
+        // Act
+        Action action = () => attribute.SetValue(resource, resource);
+
+        // Assert
+        action.Should().ThrowExactly<InvalidOperationException>().WithMessage($"Resource of type '{typeof(TestResource).FullName}' must be a collection.");
+    }
+
+    [Fact]
+    public void Can_set_value_to_collection_with_single_resource()
+    {
+        // Arrange
+        var attribute = new HasManyAttribute
+        {
+            Property = typeof(TestResource).GetProperty(nameof(TestResource.Children))!
+        };
+
+        var resource = new TestResource();
+
+        var children = new List<TestResource>
+        {
+            resource
+        };
+
+        // Act
+        attribute.SetValue(resource, children);
+
+        // Assert
+        attribute.GetValue(resource).Should().BeOfType<List<TestResource>>().Subject.ShouldHaveCount(1);
+    }
+
+    [Fact]
+    public void Cannot_set_value_to_collection_with_null_element()
+    {
+        // Arrange
+        var attribute = new HasManyAttribute
+        {
+            Property = typeof(TestResource).GetProperty(nameof(TestResource.Children))!
+        };
+
+        var resource = new TestResource();
+
+        var children = new List<TestResource>
+        {
+            resource,
+            null!
+        };
+
+        // Act
+        Action action = () => attribute.SetValue(resource, children);
+
+        // Assert
+        action.Should().ThrowExactly<InvalidOperationException>().WithMessage("Resource collection must not contain null values.");
+    }
+
+    [Fact]
+    public void Cannot_set_value_to_collection_with_primitive_element()
+    {
+        // Arrange
+        var attribute = new HasManyAttribute
+        {
+            Property = typeof(TestResource).GetProperty(nameof(TestResource.Children))!
+        };
+
+        var resource = new TestResource();
+
+        var children = new List<object>
+        {
+            resource,
+            1
+        };
+
+        // Act
+        Action action = () => attribute.SetValue(resource, children);
+
+        // Assert
+        action.Should().ThrowExactly<InvalidOperationException>().WithMessage("Resource of type 'System.Int32' does not implement IIdentifiable.");
+    }
+
+    private sealed class TestResource : Identifiable<long>
+    {
+        [HasMany]
+        public IEnumerable<TestResource> Children { get; set; } = new HashSet<TestResource>();
+    }
+}
diff --git a/test/JsonApiDotNetCoreTests/UnitTests/ResourceGraph/HasOneAttributeTests.cs b/test/JsonApiDotNetCoreTests/UnitTests/ResourceGraph/HasOneAttributeTests.cs
diff --git a/test/JsonApiDotNetCoreTests/UnitTests/ResourceGraph/ResourceFieldAttributeTests.cs b/test/JsonApiDotNetCoreTests/UnitTests/ResourceGraph/ResourceFieldAttributeTests.cs
diff --git a/test/TestBuildingBlocks/DbContextExtensions.cs b/test/TestBuildingBlocks/DbContextExtensions.cs

Original file line number	Diff line number	Diff line change
`@@ -31,6 +31,7 @@ public AttrCapabilities Capabilities`
`31`	`31`	`set => _capabilities = value;`
`32`	`32`	`}`
`33`	`33`
	`34`	`+ /// <inheritdoc />`
`34`	`35`	`public override bool Equals(object? obj)`
`35`	`36`	`{`
`36`	`37`	`if (ReferenceEquals(this, obj))`
`@@ -48,6 +49,7 @@ public override bool Equals(object? obj)`
`48`	`49`	`return Capabilities == other.Capabilities && base.Equals(other);`
`49`	`50`	`}`
`50`	`51`
	`52`	`+ /// <inheritdoc />`
`51`	`53`	`public override int GetHashCode()`
`52`	`54`	`{`
`53`	`55`	`return HashCode.Combine(Capabilities, base.GetHashCode());`
Original file line number	Diff line number	Diff line change
`@@ -64,6 +64,14 @@ private bool EvaluateIsOneToOne()`
`64`	`64`	`return false;`
`65`	`65`	`}`
`66`	`66`
	`67`	`+ /// <inheritdoc />`
	`68`	`+ public override void SetValue(object resource, object? newValue)`
	`69`	`+ {`
	`70`	`+ AssertIsIdentifiable(newValue);`
	`71`	`+ base.SetValue(resource, newValue);`
	`72`	`+ }`
	`73`	`+`
	`74`	`+ /// <inheritdoc />`
`67`	`75`	`public override bool Equals(object? obj)`
`68`	`76`	`{`
`69`	`77`	`if (ReferenceEquals(this, obj))`
`@@ -81,6 +89,7 @@ public override bool Equals(object? obj)`
`81`	`89`	`return _capabilities == other._capabilities && base.Equals(other);`
`82`	`90`	`}`
`83`	`91`
	`92`	`+ /// <inheritdoc />`
`84`	`93`	`public override int GetHashCode()`
`85`	`94`	`{`
`86`	`95`	`return HashCode.Combine(_capabilities, base.GetHashCode());`
Original file line number	Diff line number	Diff line change
`@@ -86,6 +86,7 @@ public bool CanInclude`
`86`	`86`	`set => _canInclude = value;`
`87`	`87`	`}`
`88`	`88`
	`89`	`+ /// <inheritdoc />`
`89`	`90`	`public override bool Equals(object? obj)`
`90`	`91`	`{`
`91`	`92`	`if (ReferenceEquals(this, obj))`
`@@ -103,6 +104,7 @@ public override bool Equals(object? obj)`
`103`	`104`	`return _rightType?.ClrType == other._rightType?.ClrType && Links == other.Links && base.Equals(other);`
`104`	`105`	`}`
`105`	`106`
	`107`	`+ /// <inheritdoc />`
`106`	`108`	`public override int GetHashCode()`
`107`	`109`	`{`
`108`	`110`	`return HashCode.Combine(_rightType?.ClrType, Links, base.GetHashCode());`
Original file line number	Diff line number	Diff line change
`@@ -68,6 +68,7 @@ internal set`
`68`	`68`	`public object? GetValue(object resource)`
`69`	`69`	`{`
`70`	`70`	`ArgumentGuard.NotNull(resource);`
	`71`	`+ AssertIsIdentifiable(resource);`
`71`	`72`
`72`	`73`	`if (Property.GetMethod == null)`
`73`	`74`	`{`
`@@ -82,17 +83,18 @@ internal set`
`82`	`83`	`{`
`83`	`84`	`throw new InvalidOperationException(`
`84`	`85`	`$"Unable to get property value of '{Property.DeclaringType!.Name}.{Property.Name}' on instance of type '{resource.GetType().Name}'.",`
`85`		`- exception);`
	`86`	`+ exception.InnerException ?? exception);`
`86`	`87`	`}`
`87`	`88`	`}`
`88`	`89`
`89`	`90`	`/// <summary>`
`90`	`91`	`/// Sets the value of this field on the specified resource instance. Throws if the property is read-only or if the field does not belong to the specified`
`91`	`92`	`/// resource instance.`
`92`	`93`	`/// </summary>`
`93`		`- public void SetValue(object resource, object? newValue)`
	`94`	`+ public virtual void SetValue(object resource, object? newValue)`
`94`	`95`	`{`
`95`	`96`	`ArgumentGuard.NotNull(resource);`
	`97`	`+ AssertIsIdentifiable(resource);`
`96`	`98`
`97`	`99`	`if (Property.SetMethod == null)`
`98`	`100`	`{`
`@@ -107,15 +109,25 @@ public void SetValue(object resource, object? newValue)`
`107`	`109`	`{`
`108`	`110`	`throw new InvalidOperationException(`
`109`	`111`	`$"Unable to set property value of '{Property.DeclaringType!.Name}.{Property.Name}' on instance of type '{resource.GetType().Name}'.",`
`110`		`- exception);`
	`112`	`+ exception.InnerException ?? exception);`
`111`	`113`	`}`
`112`	`114`	`}`
`113`	`115`
	`116`	`+ protected void AssertIsIdentifiable(object? resource)`
	`117`	`+ {`
	`118`	`+ if (resource != null && resource is not IIdentifiable)`
	`119`	`+ {`
	`120`	`+ throw new InvalidOperationException($"Resource of type '{resource.GetType()}' does not implement {nameof(IIdentifiable)}.");`
	`121`	`+ }`
	`122`	`+ }`
	`123`	`+`
	`124`	`+ /// <inheritdoc />`
`114`	`125`	`public override string? ToString()`
`115`	`126`	`{`
`116`	`127`	`return _publicName ?? (_property != null ? _property.Name : base.ToString());`
`117`	`128`	`}`
`118`	`129`
	`130`	`+ /// <inheritdoc />`
`119`	`131`	`public override bool Equals(object? obj)`
`120`	`132`	`{`
`121`	`133`	`if (ReferenceEquals(this, obj))`
`@@ -133,6 +145,7 @@ public override bool Equals(object? obj)`
`133`	`145`	`return _publicName == other._publicName && _property == other._property;`
`134`	`146`	`}`
`135`	`147`
	`148`	`+ /// <inheritdoc />`
`136`	`149`	`public override int GetHashCode()`
`137`	`150`	`{`
`138`	`151`	`return HashCode.Combine(_publicName, _property);`
Original file line number	Diff line number	Diff line change
`@@ -52,9 +52,10 @@ public void IncludeAttributes(IEnumerable<AttrAttribute> attributes)`
`52`	`52`	`}`
`53`	`53`	`}`
`54`	`54`
`55`		`- public void IncludeRelationship(RelationshipAttribute relationship, QueryLayer? queryLayer)`
	`55`	`+ public void IncludeRelationship(RelationshipAttribute relationship, QueryLayer queryLayer)`
`56`	`56`	`{`
`57`	`57`	`ArgumentGuard.NotNull(relationship);`
	`58`	`+ ArgumentGuard.NotNull(queryLayer);`
`58`	`59`
`59`	`60`	`this[relationship] = queryLayer;`
`60`	`61`	`}`
Original file line number	Diff line number	Diff line change
`@@ -420,7 +420,7 @@ private object ConvertStringToType(string value, Type type)`
`420`	`420`
`421`	`421`	`private Converter<string, object> GetConstantValueConverterForAttribute(AttrAttribute attribute)`
`422`	`422`	`{`
`423`		`- return stringValue => attribute.Property.Name == nameof(IIdentifiable<object>.Id)`
	`423`	`+ return stringValue => attribute.Property.Name == nameof(Identifiable<object>.Id)`
`424`	`424`	`? DeObfuscateStringId(attribute.Type.ClrType, stringValue)`
`425`	`425`	`: ConvertStringToType(stringValue, attribute.Property.PropertyType);`
`426`	`426`	`}`
Original file line number	Diff line number	Diff line change
`@@ -162,9 +162,9 @@ private void IncludeAllScalarProperties(Type elementType, Dictionary<PropertyInf`
`162`	`162`
`163`	`163`	`private static void IncludeFields(FieldSelectors fieldSelectors, Dictionary<PropertyInfo, PropertySelector> propertySelectors)`
`164`	`164`	`{`
`165`		`- foreach ((ResourceFieldAttribute resourceField, QueryLayer? queryLayer) in fieldSelectors)`
	`165`	`+ foreach ((ResourceFieldAttribute resourceField, QueryLayer? nextLayer) in fieldSelectors)`
`166`	`166`	`{`
`167`		`- var propertySelector = new PropertySelector(resourceField.Property, queryLayer);`
	`167`	`+ var propertySelector = new PropertySelector(resourceField.Property, nextLayer);`
`168`	`168`	`IncludeWritableProperty(propertySelector, propertySelectors);`
`169`	`169`	`}`
`170`	`170`	`}`
Original file line number	Diff line number	Diff line change
`@@ -791,7 +791,7 @@ public async Task Cannot_select_ToMany_relationship_with_blocked_capability()`
`791`	`791`	`}`
`792`	`792`
`793`	`793`	`[Fact]`
`794`		`- public async Task Retrieves_all_properties_when_fieldset_contains_readonly_attribute()`
	`794`	`+ public async Task Fetches_all_scalar_properties_when_fieldset_contains_readonly_attribute()`
`795`	`795`	`{`
`796`	`796`	`// Arrange`
`797`	`797`	`var store = _testContext.Factory.Services.GetRequiredService<ResourceCaptureStore>();`