-
Notifications
You must be signed in to change notification settings - Fork 12
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
RBAC - REST APIs #2773
RBAC - REST APIs #2773
Conversation
src/IoTHub.Portal.Infrastructure/Services/RRAC/RoleManagementService.cs
Outdated
Show resolved
Hide resolved
src/IoTHub.Portal.Application/Services/RoleManagementService.cs
Outdated
Show resolved
Hide resolved
…User logic implemented in services
…e unused action...)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Additionally to requested changes on specific files, please also extend all your unit tests to cover situations where the tested feature doesn't work and returns an error, in order to make sure we handle errors properly, with the correct errors and error codes, etc
src/IoTHub.Portal.Server/Controllers/v1.0/DashboardController.cs
Outdated
Show resolved
Hide resolved
src/IoTHub.Portal.Server/Controllers/v1.0/DashboardController.cs
Outdated
Show resolved
Hide resolved
src/IoTHub.Portal.Server/Controllers/v1.0/SettingsController.cs
Outdated
Show resolved
Hide resolved
var existingName = await this.groupRepository.GetByNameAsync(group.Name); | ||
if (existingName is not null) | ||
{ | ||
throw new ResourceAlreadyExistsException($"The Group tis the name {group.Name} already exist !"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Error typo
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed
var existingName = await this.userRepository.GetByNameAsync(user.Name); | ||
if (existingName is not null) | ||
{ | ||
throw new ResourceAlreadyExistsException($"The User tis the name {user.Name} already exist !"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Error typo
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed
var user = await userRepository.GetByIdAsync(id); | ||
if (user is null) | ||
{ | ||
throw new ResourceNotFoundException($"The User with the id {id} that you want to delete does'nt exist !"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Error typo
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed
} | ||
|
||
/*[Test] | ||
public async Task DeleteAccessControlShouldReturnExpectedBehavior() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would be better if this test was uncommented and working :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I made the changes, its working now
return Ok(await groupManagementService.DeleteGroup(id)); | ||
try | ||
{ | ||
var result =await groupManagementService.DeleteGroup(id); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The code in the try is never reached
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I made the changes
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Use AutoFixtures in all Unit Tests, some are missing
Add tests of limits in methods
@@ -130,7 +130,7 @@ private static IServiceCollection ConfigureImageBlobStorage(this IServiceCollect | |||
var serviceClient = new BlobServiceClient(configuration.AzureStorageAccountConnectionString); | |||
var container = serviceClient.GetBlobContainerClient(opts.ImageContainerName); | |||
|
|||
_ = container.SetAccessPolicy(PublicAccessType.Blob); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This shouldn't be commented in production code since it only impacts us on our test infrastructure (and will be handled in #3076)
Description
What's new?
What kind of change does this PR introduce?