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 app role assignments for users, groups and sp #39

Merged

Conversation

michaljirman
Copy link
Contributor

@michaljirman michaljirman commented May 3, 2021

  • Add support for app role assignments for users, groups and service principals
  • Refactor existing app role assignments code to be used with all resource types (users, groups and service principals) while keeping resource type differences hidden
  • Add app role assignments tests for users, groups and service principals

Supports app role assignments using the two available methods:

  1. appRoleAssignedTo relationship

    client example

    msgraph.NewServicePrincipalsClient(...)
    
  2. appRoleAssignments relationship

    client examples

    msgraph.NewGroupsAppRoleAssignmentsClient(...)
    msgraph.NewUsersAppRoleAssignmentsClient(...)
    msgraph.NewServicePrincipalsAppRoleAssignmentsClient(...) 
    

Related to this issue

@michaljirman michaljirman marked this pull request as ready for review May 3, 2021 11:32
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.

Thanks for looking at this @michaljirman.

I was thinking about app role assignments recently and wondered if it would make more sense to implement all of the functionality as part of the GroupsClient, ServicePrincipalsClient and UsersClient structs rather than having separate clients. WDYT?

@michaljirman
Copy link
Contributor Author

@manicminer implementing method 2 within the GroupsClient, ServicePrincipalsClient and UsersClient will result in 3 near-identical methods in each of them and a lot of code duplication (appRoleAssignments relationship). We could still consider this refactor but it would be good to keep abstraction of these methods somewhere else and just call it from all of these clients.

If we want to also keep method 1 we will additionally have extra 3 methods in the ServicePrincipalsClient (appRoleAssignedTo relationship)

@manicminer manicminer added this to the v0.14.0 milestone May 18, 2021
@manicminer manicminer added the enhancement New feature or request label May 18, 2021
@michaljirman
Copy link
Contributor Author

@manicminer we should probably get this reviewed/updated before I forget what it does :-)

@manicminer
Copy link
Owner

@michaljirman Sorry for the delay in reviewing this, I've been focused on fixes and updates for the AzureAD Terraform provider the last 2 weeks. I'm planning to get this in along with #42 in the next few days.

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.

Thanks again @michaljirman, this LGTM 🚀

@manicminer manicminer merged commit 88784ec into manicminer:main May 27, 2021
manicminer added a commit that referenced this pull request May 27, 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