Skip to content

Update permission references and add a note about additional required permissions #10026

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

Open
wants to merge 2 commits into
base: live
Choose a base branch
from

Conversation

rufer7
Copy link

@rufer7 rufer7 commented Feb 27, 2025

This pull request includes changes to the azure-sql/database/authentication-azure-ad-user-assigned-managed-identity.md file to update permission references and add a note about additional required permissions for creating a contained database user for a Microsoft Entra group.

Permission reference updates:

  • Updated the URLs for User.Read.All, GroupMember.Read.All, and Application.Read.All to correct paths.

Additional permissions note:

  • Added a note indicating that the Group.Read.All permission is also required for the creation of a contained database user for a Microsoft Entra group.

Copy link
Contributor

@rufer7 : Thanks for your contribution! The author(s) have been notified to review your proposed change.

@rufer7 rufer7 changed the title Update authentication-azure-ad-user-assigned-managed-identity.md Update permission references and add a note about additional required permissions Feb 27, 2025
Copy link
Contributor

Learn Build status updates of commit e1c2b07:

✅ Validation status: passed

File Status Preview URL Details
azure-sql/database/authentication-azure-ad-user-assigned-managed-identity.md ✅Succeeded

For more details, please refer to the build report.

For any questions, please:

@v-dirichards
Copy link
Contributor

@VanMSFT

Can you review the proposed changes?

Important: When the changes are ready for publication, adding a #sign-off comment is the best way to signal that the PR is ready for the review team to merge.

#label:"aq-pr-triaged"
@MicrosoftDocs/public-repo-pr-review-team

@prmerger-automator prmerger-automator bot added the aq-pr-triaged tracking label for the PR review team label Feb 27, 2025
Copy link
Contributor

Learn Build status updates of commit ee91508:

✅ Validation status: passed

File Status Preview URL Details
azure-sql/database/authentication-azure-ad-user-assigned-managed-identity.md ✅Succeeded

For more details, please refer to the build report.

For any questions, please:

@VanMSFT
Copy link
Member

VanMSFT commented Feb 27, 2025

Thanks, @rufer7 - I'm checking on this.

- [GroupMember.Read.All](/graph/permissions-reference#groupmemberreadall): Allows access to Microsoft Entra group information.
- [Application.Read.All](/graph/permissions-reference#applicationreadalls): Allows access to Microsoft Entra service principal (application) information.

To create a contained database user for a Microsoft Entra group, the `Group.Read.All` permission is required additionally to the ones listed above.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Remove this line.

- [GroupMember.Read.All](/graph/permissions-reference#group-permissions): Allows access to Microsoft Entra group information.
- [Application.Read.ALL](/graph/permissions-reference#application-resource-permissions): Allows access to Microsoft Entra service principal (application) information.
- [User.Read.All](/graph/permissions-reference#userreadall): Allows access to Microsoft Entra user information.
- [GroupMember.Read.All](/graph/permissions-reference#groupmemberreadall): Allows access to Microsoft Entra group information.
Copy link
Contributor

@PratimDasgupta PratimDasgupta Mar 1, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
- [GroupMember.Read.All](/graph/permissions-reference#groupmemberreadall): Allows access to Microsoft Entra group information.
- [Group.Read.All](/graph/permissions-reference#groupreadall): Allows access to contents of Microsoft Entra group.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@PratimDasgupta thanks for the review. Are you sure about the replacement? At least I didn't test without GroupMember.Read.All permission. In case the replacement is intended, I will also adjust the PoeerShell script in this file

Copy link
Contributor

@PratimDasgupta PratimDasgupta Mar 1, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am still working on this, but the idea is to use Group.read.all until GroupMember.Read.All or any other least privilaged api that can support all scenarios. Note: GroupMember.Read.All is a least privilaged api, & thus is a subset of Group.read.all .

Copy link
Author

@rufer7 rufer7 Mar 1, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@PratimDasgupta thx for the fast reply. Ah ok, I see. Will GroupMember.Read.All ever support the scenario outlined in the sentence I added in my PR? Because at least from the name itself it doesn't make sense to allow reading groups by GroupMember.Read.All

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@PratimDasgupta you can find more context on my use case here

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's difficult to say that. Overall, I was trying to consolidate the api(s).

Copy link
Author

@rufer7 rufer7 Mar 1, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@PratimDasgupta I would say least privilege depends on the use case. In the use case I described in the blog post linked in my last comment, assigning Group.Read.All only would be the least privilege way.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@rufer7 , just read the blog. Thanks for that, it seems you are using a SMI(Sytem managed identity) & not a UMI(User Managed Identity). Did you try using a UMI & GroupMember.Read.All?

Copy link
Author

@rufer7 rufer7 Mar 3, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@PratimDasgupta yes, exactly I'm using SMI. I prefer SMI over UMI but SMI case is not described in the docs. However the graph permission should not vary from SMI to UMI from my point of view. Otherwise it gets weird as both need to look up the Entra group when creating a user from external identity for a Entra group.

But no, I didn't test it with UMI

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants