I have found that while IMustHaveTenant ensures that newly inserted entities are stored against the active tenant, the same behavior is not applied to modified entities.
While I accept that it's possible to have a scenario where by one would want to move an entity from one tenant to another, I don't believe it's wise for this to be the default behavior.
Currently, if you instantiate an entity (IMustHaveTenant), and store it - it will receive the active tenant ID. If you then load that entity, change its tenant id (to that of a valid tenant), it will be moved to that tenant.
I feel there should be a filter in place, to mark the TenantId property as "unmodified" such that the EF context never updates it. It should then be the developer's responsibility to disable this filter if they want to move an entity between tenants.
This is the safer default behavior.
I discovered this, because I had a bad AutoMapper configuration, which reset the TenantId of an entity to 0 during an update, and it moved the entity to the host tenant.
The customer though he had lost data, fortunately it was possible to just move it back to the correct tenant, however it highlighted this security concern.
In the interim, my guard is implemented on my AbpDbContext as follows:
protected override void ApplyAbpConceptsForModifiedEntity(EntityEntry entry, long? userId, EntityChangeReport changeReport)
{
// prevent changes to TenantId
if (entry.Entity is IMustHaveTenant || entry.Entity is IMayHaveTenant)
{
var tenantProperty = entry.Property(nameof(IMustHaveTenant.TenantId));
if (tenantProperty.IsModified)
{
tenantProperty.IsModified = false;
object[] keyParts = entry.Metadata.FindPrimaryKey()
.Properties
.Select(p => entry.Property(p.Name).CurrentValue)
.ToArray();
var keyAsString = String.Join(", ", Array.ConvertAll(keyParts, x => $"'{x}'"));
this.Logger.Warn($"Blocked attempted change to {entry.Metadata.ClrType.Name}.{tenantProperty.Metadata.Name} for Key {keyAsString}, from {tenantProperty.OriginalValue} to {tenantProperty.CurrentValue}");
}
}
base.ApplyAbpConceptsForModifiedEntity(entry, userId, changeReport);
}
3 Answer(s)
-
0
Hi @hra
Thank you for sharing your finding with us. I tihnk your implementation is the easiest way for solving your problem. But, we will not include it into AspNet Zero since it will bring other problems like developer setting the TenantId field to a value and it will not be updated. Which is also a strange behaviour.
Thanks again,
-
0
Hi @ismcagdas,
Thank you for your response.
I hear your point, however I would like to respectfully appeal that you reassess your position for the sake of the AspNetZero/Boilerplate platform. All things being equal, if both are strange behaviors, it would be far safer to err on the side of not leaking potentially sensitive data to unintended users/organisations.
At best, data could be moved from tenant to host, causing the effect I had, a customer noticing missing data. At worse, data could be moved to another tenant, causing leakage of potentially sensitive data outside of the organisation, exposing your customers to potential legal ramifications.
Should the user have intended the data to be moved to another tenant, this specific failure would be observed in feature testing - as it's an explicit feature rather than an edge case. We could mitigate this being overlooked by throwing an exception should the tenant change without the "block tenant changes guard/filter" being disabled - this would clearly highlight the violation in testing.
There is no impact on my organisation given I have caught this flaw and mitigated it, so I promote discussion on this topic for the benefit of my peers in this community. I feel this also reflects on AspNetZero's internal security strategy whether or not to err on the side of caution.
Thank you and best Regards,
-
0
Hi @hra
I totally understand your ponit but we believe that this should be developer's responsibility to handle such cases.
