Add security layer #162
Add security layer #162
Conversation
WalkthroughThis PR introduces resource URN tracking and organization-scoped data access with security policy integration. It adds a ResourceActionsAttribute for marking repository resources with actions, implements lazy-cached ResourceUrn properties, creates a new generic MongoOrganizationRepositoryBase with organization-scoped filtering and security policies, and defines INoSqlPolicyService for policy-based query filtering. Changes
Sequence DiagramsequenceDiagram
actor Client
participant Repo as MongoOrganizationRepositoryBase
participant Context as IRequestContext
participant PolicySvc as INoSqlPolicyService
participant Mongo as IMongoCollection
Client->>Repo: FindAsync(filter, skip, limit)
Repo->>Repo: Initialize context
Repo->>Context: Get current OrganizationId
Repo->>Repo: Build composite filter<br/>(id + org + isDeleted)
alt Has INoSqlPolicyService
Repo->>PolicySvc: GetQuery(action)
PolicySvc-->>Repo: Security filter (BsonDocument)
Repo->>Repo: Merge security filter
end
Repo->>Repo: Apply sorting & collation
Repo->>Mongo: Find & execute query
Mongo-->>Repo: Results
Repo-->>Client: IEnumerable<TDocument>
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
src/Simplic.OxS.Data.MongoDB/MongoReadOnlyRepositoryBase.cs(5 hunks)src/Simplic.OxS.Data.MongoDB/MongoRepositoryBase.cs(6 hunks)src/Simplic.OxS.Data.MongoDB/OrganizationRepository/MongoOrganizationRepositoryBase.cs(1 hunks)src/Simplic.OxS.Data/NoSql/IReadOnlyRepository.cs(1 hunks)src/Simplic.OxS.Data/Security/INoSqlPolicyService.cs(1 hunks)src/Simplic.OxS/Security/ResourceActionsAttribute.cs(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (3)
src/Simplic.OxS.Data.MongoDB/OrganizationRepository/MongoOrganizationRepositoryBase.cs (2)
src/Simplic.OxS.Data.MongoDB/MongoRepositoryBase.cs (12)
MongoRepositoryBase(14-168)MongoRepositoryBase(22-24)MongoRepositoryBase(26-28)Task(34-38)Task(44-48)Task(54-64)Task(71-78)Task(84-87)Task(109-120)Task(127-142)Task(149-167)FilterDefinition(95-101)src/Simplic.OxS.Data.MongoDB/MongoReadOnlyRepositoryBase.cs (8)
Task(36-44)Task(48-55)Task(57-63)Task(107-121)Task(129-139)IEnumerable(70-73)FilterDefinition(75-96)GetCollectionName(46-46)
src/Simplic.OxS.Data.MongoDB/MongoRepositoryBase.cs (2)
src/Simplic.OxS.Data.MongoDB/MongoReadOnlyRepositoryBase.cs (1)
FilterDefinition(75-96)src/Simplic.OxS.Data.MongoDB/OrganizationRepository/MongoOrganizationRepositoryBase.cs (1)
FilterDefinition(109-141)
src/Simplic.OxS.Data.MongoDB/MongoReadOnlyRepositoryBase.cs (4)
src/Simplic.OxS.Data.MongoDB/MongoRepositoryBase.cs (1)
FilterDefinition(95-101)src/Simplic.OxS.Data.MongoDB/OrganizationRepository/MongoOrganizationRepositoryBase.cs (1)
FilterDefinition(109-141)src/Simplic.OxS.Data/Security/INoSqlPolicyService.cs (1)
BsonDocument(15-15)src/Simplic.OxS/Security/ResourceActionsAttribute.cs (1)
ResourceActionsAttribute(9-13)
| if (_resourceUrn == "") | ||
| return null; | ||
|
|
||
| if (_resourceUrn == null) | ||
| { | ||
| var attr = (ResourceActionsAttribute?)Attribute.GetCustomAttributes( | ||
| GetType(), typeof(ResourceActionsAttribute))?.FirstOrDefault(); | ||
|
|
||
| if (attr == null) | ||
| { | ||
| // Set to an empty string, so taht the attribute is not checked for each request | ||
| // That means caching the not existing of the attribute | ||
| _resourceUrn = ""; | ||
| return null; | ||
| } | ||
|
|
||
| // Cache resource urn | ||
| _resourceUrn = attr.Name; | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Static cache breaks per-repository URN isolation; move to instance-level cache.
_resourceUrn is static, so every repository sharing the same generic arguments (e.g., two different repos over the same document/filter types) will reuse the first cached value. If one repo has a [ResourceActions] attribute and another does not—or they declare different resources—the latter will inherit the wrong URN and consequently apply the wrong security filter. That is a correctness/security issue: policy checks can be executed against an unrelated resource or be skipped entirely.
Please make the cache instance-specific (or keyed by Type) so each concrete repository resolves and stores its own URN.
Apply this diff to keep the cache per instance:
- private static string? _resourceUrn = null;
+ private string? _resourceUrn;
+ private bool _resourceUrnResolved;
@@
- if (_resourceUrn == "")
+ if (_resourceUrnResolved && string.IsNullOrWhiteSpace(_resourceUrn))
return null;
- if (_resourceUrn == null)
+ if (!_resourceUrnResolved)
{
var attr = (ResourceActionsAttribute?)Attribute.GetCustomAttributes(
GetType(), typeof(ResourceActionsAttribute))?.FirstOrDefault();
if (attr == null)
{
- // Set to an empty string, so taht the attribute is not checked for each request
- // That means caching the not existing of the attribute
- _resourceUrn = "";
+ _resourceUrnResolved = true;
return null;
}
// Cache resource urn
_resourceUrn = attr.Name;
+ _resourceUrnResolved = true;
}Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In src/Simplic.OxS.Data.MongoDB/MongoReadOnlyRepositoryBase.cs around lines 167
to 185, the code currently uses a static _resourceUrn which causes URN values to
be shared across repository instances and breaks per-repository isolation;
change the cache to be instance-scoped (remove static from the _resourceUrn
declaration) or implement a type-keyed cache (e.g., a
ConcurrentDictionary<Type,string> keyed by GetType()) and update the
lookup/assignment logic so each concrete repository resolves and stores its own
URN rather than sharing a single static value.
| return Collection.Find(BuildFilterQuery(predicate), findOptions).Sort(sort).Skip(skip).Limit(limit).ToList(); | ||
| } |
There was a problem hiding this comment.
Prevent null sort filter from throwing
FindAsync always calls .Sort(sort), but sort stays null whenever sortField is empty. The MongoDB driver then raises ArgumentNullException, breaking every unsorted query. Guard the call so we only sort when a definition exists.
- return Collection.Find(BuildFilterQuery(predicate), findOptions).Sort(sort).Skip(skip).Limit(limit).ToList();
+ var query = Collection.Find(BuildFilterQuery(predicate), findOptions);
+
+ if (sort != null)
+ query = query.Sort(sort);
+
+ return query.Skip(skip).Limit(limit).ToList();📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| return Collection.Find(BuildFilterQuery(predicate), findOptions).Sort(sort).Skip(skip).Limit(limit).ToList(); | |
| } | |
| var query = Collection.Find(BuildFilterQuery(predicate), findOptions); | |
| if (sort != null) | |
| query = query.Sort(sort); | |
| return query.Skip(skip).Limit(limit).ToList(); | |
| } |
🤖 Prompt for AI Agents
In
src/Simplic.OxS.Data.MongoDB/OrganizationRepository/MongoOrganizationRepositoryBase.cs
around lines 75-76, the code always calls .Sort(sort) even when sort is null
which causes ArgumentNullException; modify the method so you only call
.Sort(sort) when a sort definition exists (e.g., build the find cursor with var
cursor = Collection.Find(...); if (sort != null) cursor = cursor.Sort(sort);
then apply .Skip(skip).Limit(limit).ToList()), thereby skipping the Sort call
for unsorted queries.
| /// <summary> | ||
| /// Gets a urn for the resource represented by this repository. | ||
| /// E.g. oxs:logistics.orders | ||
| /// </summary> | ||
| public string? ResourceUrn { get; } |
There was a problem hiding this comment.
Remove invalid access modifier from interface property
Interfaces can’t declare members with the public modifier; this now fails to compile (CS0106). Drop the modifier so existing implementations can inherit the signature.
- public string? ResourceUrn { get; }
+ string? ResourceUrn { get; }📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| /// <summary> | |
| /// Gets a urn for the resource represented by this repository. | |
| /// E.g. oxs:logistics.orders | |
| /// </summary> | |
| public string? ResourceUrn { get; } | |
| /// <summary> | |
| /// Gets a urn for the resource represented by this repository. | |
| /// E.g. oxs:logistics.orders | |
| /// </summary> | |
| string? ResourceUrn { get; } |
🤖 Prompt for AI Agents
In src/Simplic.OxS.Data/NoSql/IReadOnlyRepository.cs around lines 48 to 52, the
interface property declaration includes the invalid 'public' access modifier
which causes a CS0106 compile error; remove the 'public' keyword so the property
is declared as just 'string? ResourceUrn { get; }' (leave nullable and getter
as-is) so existing implementations can inherit the signature.
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (2)
src/Simplic.OxS/Security/ResourceActionsAttribute.cs (1)
9-13: Consider adding parameter validation.The constructor doesn't validate that
nameandactionsare non-null or non-empty. This could lead to runtime issues if invalid values are passed.Consider adding validation:
public ResourceActionsAttribute(string name, string[] actions) { + if (string.IsNullOrWhiteSpace(name)) + throw new ArgumentException("Resource name cannot be null or empty.", nameof(name)); + if (actions == null || actions.Length == 0) + throw new ArgumentException("Actions cannot be null or empty.", nameof(actions)); + Name = name; Actions = actions; }src/Simplic.OxS.Data.MongoDB/OrganizationRepository/MongoOrganizationRepositoryBase.cs (1)
12-12: Remove redundant context field.The
contextfield duplicates theContextfield inherited fromMongoReadOnlyRepositoryBase. You can use the inheritedContextproperty directly.Apply this diff:
-private readonly IMongoContext context; private readonly IRequestContext requestContext; private readonly INoSqlPolicyService? noSqlPolicyService; protected MongoOrganizationRepositoryBase(IMongoContext context, IRequestContext requestContext, INoSqlPolicyService? noSqlPolicyService = null) : base(context) { - this.context = context; this.requestContext = requestContext; this.noSqlPolicyService = noSqlPolicyService; }Then update line 162 to use
Contextinstead ofcontext:-return context.GetCollection<TDocument>(GetCollectionName()).Find(filter).AsExecutable(); +return Context.GetCollection<TDocument>(GetCollectionName()).Find(filter).AsExecutable();
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
src/Simplic.OxS.Data.MongoDB/MongoReadOnlyRepositoryBase.cs(2 hunks)src/Simplic.OxS.Data.MongoDB/OrganizationRepository/MongoOrganizationRepositoryBase.cs(1 hunks)src/Simplic.OxS.Data/NoSql/IReadOnlyRepository.cs(1 hunks)src/Simplic.OxS.Data/Security/INoSqlPolicyService.cs(1 hunks)src/Simplic.OxS/Security/ResourceActionsAttribute.cs(1 hunks)
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-08-26T05:52:00.195Z
Learnt from: CR
Repo: simplic/simplic-oxs-storage-management PR: 0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-08-26T05:52:00.195Z
Learning: Applies to **/Repositories/**/*.cs : Implement repository pattern where beneficial
Applied to files:
src/Simplic.OxS.Data.MongoDB/OrganizationRepository/MongoOrganizationRepositoryBase.cs
🧬 Code graph analysis (2)
src/Simplic.OxS.Data.MongoDB/MongoReadOnlyRepositoryBase.cs (1)
src/Simplic.OxS/Security/ResourceActionsAttribute.cs (1)
ResourceActionsAttribute(9-13)
src/Simplic.OxS.Data.MongoDB/OrganizationRepository/MongoOrganizationRepositoryBase.cs (6)
src/Simplic.OxS.Data.MongoDB/MongoReadOnlyRepositoryBase.cs (8)
Task(31-39)Task(43-50)Task(52-58)Task(100-113)Task(121-131)IEnumerable(65-68)FilterDefinition(70-89)GetCollectionName(41-41)src/Simplic.OxS.MessageBroker/Filter/ConsumeContextFilter.cs (1)
Task(28-41)src/Simplic.OxS.Data/NoSql/IOrganizationRepository.cs (2)
Task(21-21)Task(27-27)src/Simplic.OxS.Data.MongoDB/MongoContext.cs (1)
Initialize(32-39)src/Simplic.OxS.Data/NoSql/OrganizationFilterBase.cs (1)
OrganizationFilterBase(6-25)src/Simplic.OxS.Data/NoSql/OrganizationDocumentBase.cs (1)
OrganizationDocumentBase(8-25)
🔇 Additional comments (4)
src/Simplic.OxS.Data/NoSql/IReadOnlyRepository.cs (1)
47-52: LGTM! Interface extension is well-documented.The new
ResourceUrnproperty is clearly documented with a practical example. Note that this is a breaking change for all implementations ofIReadOnlyRepository.src/Simplic.OxS.Data.MongoDB/MongoReadOnlyRepositoryBase.cs (1)
134-157: Lazy caching implementation is correct.The
ResourceUrnproperty correctly uses lazy initialization with the empty-string sentinel pattern to distinguish between "not checked" and "checked but not found". The use ofGetType()ensures derived classes retrieve their own attributes.Note: The static field lacks thread synchronization, but this is acceptable since concurrent initialization would only result in redundant attribute lookups with identical outcomes.
src/Simplic.OxS.Data.MongoDB/OrganizationRepository/MongoOrganizationRepositoryBase.cs (2)
128-135: Security filter integration looks good.The integration of
INoSqlPolicyServicecorrectly checks for null service and non-emptyResourceUrnbefore applying the security filter. The action string construction using$"{ResourceUrn}:read"is appropriate.
143-163: Verify organization ID handling in GetCollection.The
GetCollectionmethod usesrequestContext.OrganizationIddirectly without the same fallback logic present inFindAsyncandGetByFilterAsync(lines 56-62, 96-102). IfrequestContext.OrganizationIdis null, this could result in unexpected filtering behavior.Please verify the expected behavior when
requestContext.OrganizationIdis null. Consider applying the same fallback logic:public async Task<IExecutable<TDocument>> GetCollection() { var builder = Builders<TDocument>.Filter; var filterQueries = new List<FilterDefinition<TDocument>> { - builder.Eq(d => d.OrganizationId, requestContext.OrganizationId) + builder.Eq(d => d.OrganizationId, requestContext.OrganizationId ?? Guid.Empty) };Verify with this script to check if IRequestContext.OrganizationId is nullable:
| using System.Net.NetworkInformation; | ||
| using System.Runtime.CompilerServices; |
There was a problem hiding this comment.
Remove unused using directives.
The System.Net.NetworkInformation and System.Runtime.CompilerServices namespaces don't appear to be used in this file.
Apply this diff to remove unused imports:
using MongoDB.Driver;
using Simplic.OxS.Security;
-using System.Net.NetworkInformation;
-using System.Runtime.CompilerServices;📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| using System.Net.NetworkInformation; | |
| using System.Runtime.CompilerServices; | |
| using MongoDB.Driver; | |
| using Simplic.OxS.Security; |
🤖 Prompt for AI Agents
In src/Simplic.OxS.Data.MongoDB/MongoReadOnlyRepositoryBase.cs around lines 3 to
4, remove the unused using directives for System.Net.NetworkInformation and
System.Runtime.CompilerServices; open the file, delete those two using lines,
and run a build/IDE cleanup to confirm no references remain and no using
warnings persist.
| predicate.OrganizationId = predicate.OrganizationId.HasValue | ||
| ? predicate.OrganizationId | ||
| : predicate.QueryAllOrganizations | ||
| ? null | ||
| : requestContext.OrganizationId | ||
| // Using an empty guid to prevent reading data from all Organizations. | ||
| ?? Guid.Empty; |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Extract duplicate organization scoping logic.
The organization ID scoping logic is duplicated in FindAsync and GetByFilterAsync. Consider extracting this into a helper method to improve maintainability.
Add a helper method:
private Guid? GetScopedOrganizationId(TFilter filter)
{
return filter.OrganizationId.HasValue
? filter.OrganizationId
: filter.QueryAllOrganizations
? null
: requestContext.OrganizationId
// Using an empty guid to prevent reading data from all Organizations.
?? Guid.Empty;
}Then simplify both methods:
public async override Task<IEnumerable<TDocument>> FindAsync(TFilter predicate, int? skip, int? limit, string sortField = "", bool isAscending = true, Collation collation = null)
{
await Initialize();
- predicate.OrganizationId = predicate.OrganizationId.HasValue
- ? predicate.OrganizationId
- : predicate.QueryAllOrganizations
- ? null
- : requestContext.OrganizationId
- // Using an empty guid to prevent reading data from all Organizations.
- ?? Guid.Empty;
+ predicate.OrganizationId = GetScopedOrganizationId(predicate);
// ... rest of method
}Also applies to: 96-102
🤖 Prompt for AI Agents
In
src/Simplic.OxS.Data.MongoDB/OrganizationRepository/MongoOrganizationRepositoryBase.cs
around lines 56-62 (and similarly lines 96-102), the organization scoping logic
is duplicated; extract it into a private helper method GetScopedOrganizationId
that accepts the filter (TFilter) and returns the computed Guid? using the
existing precedence (filter.OrganizationId -> filter.QueryAllOrganizations ?
null : requestContext.OrganizationId ?? Guid.Empty), then replace the duplicated
expressions in FindAsync and GetByFilterAsync to call this helper to centralize
the logic and reduce duplication.
| using System; | ||
| using System.Collections.Generic; | ||
| using System.Linq; | ||
| using System.Text; | ||
| using System.Threading.Tasks; |
There was a problem hiding this comment.
Remove unused using directives.
The System, System.Collections.Generic, System.Linq, System.Text, and System.Threading.Tasks namespaces are not used in this file.
Apply this diff:
using MongoDB.Bson;
-using System;
-using System.Collections.Generic;
-using System.Linq;
-using System.Text;
-using System.Threading.Tasks;Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In src/Simplic.OxS.Data/Security/INoSqlPolicyService.cs around lines 2 to 6,
remove the unused using directives (using System; using
System.Collections.Generic; using System.Linq; using System.Text; using
System.Threading.Tasks;). Update the file to eliminate those five usings
(leaving only necessary namespaces, or none if none are required), save the
file, and rebuild to ensure there are no missing references or new compile
errors.
| public interface INoSqlPolicyService | ||
| { | ||
| BsonDocument GetQuery(string action); | ||
| } |
There was a problem hiding this comment.
Update return type to nullable and add documentation.
The GetQuery method's return type should be BsonDocument? to properly indicate it can return null (as confirmed by null checks in MongoOrganizationRepositoryBase.cs lines 133 and 156). Additionally, the interface and method lack XML documentation explaining their purpose.
Apply this diff:
+/// <summary>
+/// Service for retrieving NoSQL policy-based query filters.
+/// </summary>
public interface INoSqlPolicyService
{
+ /// <summary>
+ /// Gets a policy-based query filter for the specified action.
+ /// </summary>
+ /// <param name="action">The action identifier (e.g., "oxs:logistics.orders:read")</param>
+ /// <returns>A BsonDocument filter, or null if no policy applies</returns>
- BsonDocument GetQuery(string action);
+ BsonDocument? GetQuery(string action);
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| public interface INoSqlPolicyService | |
| { | |
| BsonDocument GetQuery(string action); | |
| } | |
| /// <summary> | |
| /// Service for retrieving NoSQL policy-based query filters. | |
| /// </summary> | |
| public interface INoSqlPolicyService | |
| { | |
| /// <summary> | |
| /// Gets a policy-based query filter for the specified action. | |
| /// </summary> | |
| /// <param name="action">The action identifier (e.g., "oxs:logistics.orders:read")</param> | |
| /// <returns>A BsonDocument filter, or null if no policy applies</returns> | |
| BsonDocument? GetQuery(string action); | |
| } |
🤖 Prompt for AI Agents
In src/Simplic.OxS.Data/Security/INoSqlPolicyService.cs around lines 10 to 13,
change the GetQuery return type to BsonDocument? to reflect it can return null
and add XML documentation comments for the interface and the GetQuery method;
update the interface summary to describe its purpose (policy-based NoSQL query
provider) and add a <param name="action"> description and a <returns> describing
that it may return null when no policy exists.
Summary by CodeRabbit
New Features
Infrastructure