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

Harden Attr/Relationship attributes against invalid input

Author: maurei

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());

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());

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());

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());

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);

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;
     }

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);
     }

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);
         }
     }

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;
 

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>();