-
Notifications
You must be signed in to change notification settings - Fork 62
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
Add support for directory role activation #31
Add support for directory role activation #31
Conversation
Hi @michaljirman, thanks for the PR. It's unfortunate that the API doesn't support getting single roles, or filtering the list of roles. Do you have some examples of the error message returned for duplicate role activations? We might be able to pattern match on that as a workaround, but we can't really gloss over any 400 error as this will mask other problems. I think evaluating whether a role should be activated or not is something best done by the consuming app rather than the SDK, as it would generate additional API calls and lead to unexpected results. It's perfectly OK to implement this in the tests, this can also serve as an example. |
Yes, it makes sense and I agree. I will leave the method as it is for now. Leaving the error handling to a consuming app but keeping the extra logic in the test as an example and also allowing for the test to pass even on multiple runs. Here is an example of the response for the future reference {
"error": {
"code": "Request_BadRequest",
"message": "A conflicting object with one or more of the specified property values is present in the directory.",
"innerError": {
"date": "2021-04-14T15:27:20",
"request-id": "ddfae071-1b11-4c99-a967-3bbd16a11b11",
"client-request-id": "ddfae071-59a5-4c99-1b11-3bbd16a11b11"
}
}
} |
ff74cad
to
a57339d
Compare
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.
@michaljirman Using the latest changes on main, I've added error handling to the DirectoryRolesClient{}.Activate() method so it will try to handle the API duplicate error. In this condition, the method wiil not return an error but the original status code will still be returned so the error can be detected downstream if needed. This is pretty much the same approach as with ApplicationsClient, GroupsClient and ServicePrincipalsClient, though I may seek to improve the returned values in future since it's not super intuitive at present.
Thanks again, will merge this shortly.
Draft PR to add support for directory role activation. Some conversations probably needed before we can open a proper PR.
Activate method on a directory role
API does not support retrieving a single directory role by role template id; it does not support the OData Query Parameters either.
We could consider implementing the
ValidStatusFunc
on the client for theActivate
method but we wouldn't be able to return the object of the directory role.We could consider listing all directory roles within the
Activate
operation firstly and then check if the directory role exists. Activating it only if it does not exist. This is what a test function does at the current implementation. It could become a problem if implemented in theActivate
method in the case of List method returns a huge amount of directory roles.@manicminer Please let me know your thoughts.
Some reference links:
https://docs.microsoft.com/en-us/graph/api/resources/directoryroletemplate?view=graph-rest-beta#properties
https://docs.microsoft.com/en-us/azure/active-directory/roles/permissions-reference