Changes er code review

src/Microsoft.Health.ControlPlane.Core.UnitTests/Features/Rbac/RbacServiceTests.cs

@@ -31,7 +31,6 @@ public RbacServiceTests()
             _controlPlaneDataStore.UpsertIdentityProviderAsync(_identityProvider, Arg.Any<string>(), Arg.Any<CancellationToken>()).Returns(new UpsertResponse<IdentityProvider>(_identityProvider, UpsertOutcome.Updated, "testEtag"));
 
             IList<ResourcePermission> resourcePermissions = new List<ResourcePermission>();
-            resourcePermissions.Add(new ResourcePermission(new List<ResourceAction> { ResourceAction.Read }));
 
             _role = GetRole("clinician", resourcePermissions);
             _controlPlaneDataStore.GetRoleAsync(_role.Name, Arg.Any<CancellationToken>()).Returns(_role);

src/Microsoft.Health.ControlPlane.Core/Features/Persistence/IControlPlaneDataStore.cs

@@ -24,7 +24,7 @@ public interface IControlPlaneDataStore
 
         Task<UpsertResponse<Role>> UpsertRoleAsync(Role role, string eTag, CancellationToken cancellationToken);
 
-        Task<IEnumerable<Role>> GetAllRoleAsync(CancellationToken cancellationToken);
+        Task<IEnumerable<Role>> GetRoleForAllAsync(CancellationToken cancellationToken);
 
         Task DeleteRoleAsync(string name, string eTag, CancellationToken cancellationToken);
     }

src/Microsoft.Health.ControlPlane.Core/Features/Rbac/RbacService.cs

@@ -65,7 +65,7 @@ public async Task<UpsertResponse<IdentityProvider>> UpsertIdentityProviderAsync(
 
         public async Task<IEnumerable<Role>> GetRoleForAllAsync(CancellationToken cancellationToken)
         {
-            return await _controlPlaneDataStore.GetAllRoleAsync(cancellationToken);
+            return await _controlPlaneDataStore.GetRoleForAllAsync(cancellationToken);
         }
 
         public async Task<UpsertResponse<Role>> UpsertRoleAsync(Role role, string eTag, CancellationToken cancellationToken)

src/Microsoft.Health.ControlPlane.Core/Features/Rbac/Role.cs

@@ -36,13 +36,13 @@ internal Role(string name, IList<ResourcePermission> resourcePermissions, string
         }
 
         [JsonProperty("name")]
-        public virtual string Name { get;  set; }
+        public virtual string Name { get; protected set; }
 
         [JsonProperty("etag")]
-        public virtual string VersionTag { get;  set; }
+        public virtual string VersionTag { get; protected set; }
 
         [JsonProperty("resourcePermissions")]
-        public virtual IList<ResourcePermission> ResourcePermissions { get;  set; } = new List<ResourcePermission>();
+        public virtual IList<ResourcePermission> ResourcePermissions { get; protected set; } = new List<ResourcePermission>();
 
         public virtual IEnumerable<ValidationResult> Validate(ValidationContext validationContext)
         {
@@ -53,15 +53,15 @@ public virtual IEnumerable<ValidationResult> Validate(ValidationContext validati
 
             if (ResourcePermissions.Count == 0)
             {
-                yield return new ValidationResult(string.Format(CultureInfo.InvariantCulture, Core.Resources.ResourcePermissionEmpty));
+                yield return new ValidationResult(Resources.ResourcePermissionEmpty);
             }
             else
             {
                 foreach (ResourcePermission permission in ResourcePermissions)
                 {
                     if (permission.Actions.Count == 0)
                     {
-                        yield return new ValidationResult(string.Format(CultureInfo.InvariantCulture, Core.Resources.RoleResourcePermissionWithNoAction, Name));
+                        yield return new ValidationResult(Resources.RoleResourcePermissionWithNoAction);
                     }
                 }
             }

src/Microsoft.Health.ControlPlane.Core/Resources.resx

@@ -124,7 +124,7 @@
     <value>Identity provider authority is null or empty.</value>
   </data>
   <data name="IdentityProviderDefinitionIsInvalid" xml:space="preserve">
-    <value>Identity provider definition Is invalid.</value>
+    <value>Identity provider definition is invalid.</value>
   </data>
   <data name="IdentityProviderInvalidAudience" xml:space="preserve">
     <value>One or more of audience values is null or empty.</value>
@@ -142,21 +142,21 @@
     <value>Identity provider '{0}' was not found.</value>
   </data>
   <data name="ResourcePermissionEmpty" xml:space="preserve">
-    <value>Role must have one or more ResourcePermissions.</value>
+    <value>Role must have one or more resource permissions.</value>
   </data>
   <data name="RoleNameEmpty" xml:space="preserve">
-    <value>Role Name is null or empty</value>
+    <value>Role name is null or empty.</value>
   </data>
   <data name="RoleNotFound" xml:space="preserve">
-    <value>Role '{0}' was not found.</value>
+    <value>Role was not found.</value>
   </data>
   <data name="RoleResourcePermissionWithNoAction" xml:space="preserve">
-    <value>Role '{0}' contains a ResourcePermission with no Actions.</value>
+    <value>Role contains a resource permissions with no actions.</value>
   </data>
   <data name="InvalidDocumentTypeForDelete" xml:space="preserve">
     <value>Document type '{0}' for delete is invalid.</value>
   </data>
   <data name="RoleDefinitionIsInvalid" xml:space="preserve">
-    <value>Role definition Is invalid.</value>
+    <value>Role definition is invalid.</value>
   </data>
 </root>

src/Microsoft.Health.ControlPlane.CosmosDb/Features/Storage/ControlPlaneDataStore.cs

@@ -37,8 +37,6 @@ public class ControlPlaneDataStore : IControlPlaneDataStore
         private readonly Uri _collectionUri;
         private readonly HardDeleteIdentityProvider _hardDeleteIdentityProvider;
         private readonly RetryExceptionPolicyFactory _retryExceptionPolicyFactory;
-        private readonly string _collectionId;
-        private readonly string _databaseId;
         private readonly HardDeleteRole _hardDeleteRole;
 
         public ControlPlaneDataStore(
@@ -54,16 +52,13 @@ public ControlPlaneDataStore(
             EnsureArg.IsNotNull(cosmosDocumentQueryFactory, nameof(cosmosDocumentQueryFactory));
             EnsureArg.IsNotNull(retryExceptionPolicyFactory, nameof(retryExceptionPolicyFactory));
             EnsureArg.IsNotNull(namedCosmosCollectionConfigurationAccessor, nameof(namedCosmosCollectionConfigurationAccessor));
-            EnsureArg.IsNotNull(retryExceptionPolicyFactory, nameof(retryExceptionPolicyFactory));
             EnsureArg.IsNotNull(logger, nameof(logger));
 
             var collectionConfig = namedCosmosCollectionConfigurationAccessor.Get(Constants.CollectionConfigurationName);
 
             _documentClient = documentClient;
             _collectionUri = cosmosDataStoreConfiguration.GetRelativeCollectionUri(collectionConfig.CollectionId);
             _retryExceptionPolicyFactory = retryExceptionPolicyFactory;
-            _collectionId = collectionConfig.CollectionId;
-            _databaseId = cosmosDataStoreConfiguration.DatabaseId;
             _cosmosDocumentQueryFactory = cosmosDocumentQueryFactory;
             _retryExceptionPolicyFactory = retryExceptionPolicyFactory;
             _logger = logger;
@@ -110,12 +105,7 @@ public async Task DeleteRoleAsync(string name, string eTag, CancellationToken ca
             await DeleteSystemDocumentsByIdAsync<Role>(name, CosmosRole.RolePartition, eTag, cancellationToken);
         }
 
-        private static string GetValue(HttpStatusCode type)
-        {
-            return ((int)type).ToString();
-        }
-
-        public async Task<IEnumerable<Role>> GetAllRoleAsync(CancellationToken cancellationToken)
+        public async Task<IEnumerable<Role>> GetRoleForAllAsync(CancellationToken cancellationToken)
         {
             var role = await GetSystemDocumentsAsync<CosmosRole>(null, CosmosRole.RolePartition, cancellationToken);
             return role.Select(cr => cr.ToRole());

src/Microsoft.Health.Fhir.Core.UnitTests/Features/Security/AuthPermissionConfigTests.cs

@@ -32,7 +32,7 @@ public void GivenAnInvalidAuthorizationConfigurationForRoleWithNoPermissions_Whe
 
             InvalidDefinitionException validationException = Assert.Throws<InvalidDefinitionException>(() => invalidAuthorizationConfiguration.ValidateRoles());
 
-            Assert.NotNull(validationException.Issues.SingleOrDefault(issueComp => issueComp.Diagnostics.Equals("Role must have one or more ResourcePermissions.")));
+            Assert.NotNull(validationException.Issues.SingleOrDefault(issueComp => issueComp.Diagnostics.Equals("Role must have one or more resource permissions.")));
         }
 
         [Fact]
@@ -41,7 +41,7 @@ public void GivenAnInvalidAuthorizationConfigurationForRoleWithNoActions_WhenVal
             var invalidAuthorizationConfiguration = Samples.GetJsonSample<AuthorizationConfiguration>("AuthConfigWithInvalidEntries");
             InvalidDefinitionException validationException = Assert.Throws<InvalidDefinitionException>(() => invalidAuthorizationConfiguration.ValidateRoles());
 
-            Assert.NotNull(validationException.Issues.SingleOrDefault(issueComp => issueComp.Diagnostics.Equals("Role 'Nurse' contains a ResourcePermission with no Actions.")));
+            Assert.NotNull(validationException.Issues.SingleOrDefault(issueComp => issueComp.Diagnostics.Equals("Role contains a resource permissions with no actions.")));
         }
     }
 }