Skip to content
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

Conversation

michaljirman
Copy link
Contributor

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

  • which is already activated in the tenant returns HTTP 400 and error message
  • on a directory role that is not activated in the tenant returns HTTP 200 and properties of the activated/created 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 the Activate 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 the Activate 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

@manicminer manicminer added the enhancement New feature or request label Apr 14, 2021
@manicminer
Copy link
Owner

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.

@michaljirman
Copy link
Contributor Author

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"
    }
  }
}

@michaljirman michaljirman force-pushed the feature/support-directory-role-activation branch from ff74cad to a57339d Compare April 14, 2021 15:37
@michaljirman michaljirman marked this pull request as ready for review April 14, 2021 15:37
@manicminer manicminer added this to the v0.12.0 milestone Apr 21, 2021
Copy link
Owner

@manicminer manicminer left a 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.

@manicminer manicminer merged commit 364e746 into manicminer:main Apr 21, 2021
manicminer added a commit that referenced this pull request Apr 21, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants