Include Context information in config response #58
Conversation
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
There was a problem hiding this comment.
5 file(s) reviewed, 6 comment(s)
Edit PR Review Bot Settings | Greptile
| @@ -11,6 +12,7 @@ public class ConfigResponseModel : ResponseModel | |||
| public ServerConfigResponseModel Server { get; set; } | |||
| public EnvironmentConfigResponseModel Environment { get; set; } | |||
| public IDictionary<string, object> FeatureStates { get; set; } | |||
| public ContextResponseModel Context { get; set; } | |||
There was a problem hiding this comment.
logic: Including sensitive context information in the config response may expose user data unnecessarily. Consider the security implications of this change.
| public Guid? UserId { get; set; } | ||
| public Guid[] OrganizationIds { get; set; } |
There was a problem hiding this comment.
logic: Exposing UserId and OrganizationIds in the response could potentially be used for user enumeration attacks. Evaluate the necessity of including this information.
| public ContextResponseModel(Guid? userId, Guid[] organizationIds) | ||
| { | ||
| UserId = userId; | ||
| OrganizationIds = organizationIds; | ||
| } |
There was a problem hiding this comment.
style: Consider adding input validation to ensure userId and organizationIds are not null or empty before assigning.
| @@ -37,4 +43,5 @@ public interface IFeatureService | |||
| /// </summary> | |||
| /// <returns>A dictionary of feature keys and their values.</returns> | |||
| Dictionary<string, object> GetAll(); | |||
| FeatureFlagContext GetFlagContext(); | |||
There was a problem hiding this comment.
style: Consider adding XML documentation for the new GetFlagContext() method
| public FeatureFlagContext GetFlagContext() | ||
| { | ||
| return new FeatureFlagContext() | ||
| { | ||
| UserId = _currentContext.UserId, | ||
| OrganizationIds = _currentContext.Organizations?.Select(o => o.Id).ToArray() | ||
| }; | ||
| } |
There was a problem hiding this comment.
logic: Consider adding null checks for _currentContext and its properties to prevent potential null reference exceptions.
| return new FeatureFlagContext() | ||
| { | ||
| UserId = _currentContext.UserId, | ||
| OrganizationIds = _currentContext.Organizations?.Select(o => o.Id).ToArray() |
There was a problem hiding this comment.
style: This line may return null if _currentContext.Organizations is null. Consider using the null-coalescing operator to return an empty array instead.
Type of change
Objective
Adds feature flag context to config response. This is useful for debugging purposes.
Question: Is there a reason to consider this sensitive? It uses the same bearer token to, say, retrieve full sync data, so all information is retrievable through other endpoints.
Before you submit
dotnet format --verify-no-changes) (required)Greptile Summary
This pull request adds feature flag context to the configuration response, including user ID and organization IDs, to enhance debugging capabilities.
GetFlagContext()method toIFeatureServiceinterface and implemented inLaunchDarklyFeatureServiceConfigControllerto include feature flag context inConfigResponseModelConfigResponseModelto incorporate newFeatureFlagContextstructLaunchDarklyFeatureServiceTestsfor authenticated and unauthenticated user scenarios