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.

Sign up for GitHub

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

Jump to bottom

Enhances spo folder roleassignment commands with Microsoft Entra groups, Closes #6196 #6500

Closed
wants to merge 1 commit into from
Closed

Enhances spo folder roleassignment commands with Microsoft Entra groups, Closes #6196 #6500

wants to merge 1 commit into from

Conversation

nicodecleyre
Copy link
Contributor

Closes #6196

@nicodecleyre nicodecleyre changed the title Enhances ´spo folder roleassignment´ commands with Microsoft Entra groups, Closes #6196 Enhances spo folder roleassignment commands with Microsoft Entra groups, Closes #6196 Nov 20, 2024
@milanholemans
Copy link
Contributor

Thanks Nico, We'll try to review it ASAP!

@Adam-it Adam-it self-assigned this Jan 5, 2025
Adam-it
Adam-it approved these changes Jan 5, 2025
View reviewed changes
Copy link
Member

@Adam-it Adam-it left a comment

Choose a reason for hiding this comment

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

@nicodecleyre awesome work 👏
I added a few comment I may resolve during merging or if you have time and want to help out then awesome.
I should merge this within few days
Checked locally and all fine.

docs/docs/cmd/spo/folder/folder-roleassignment-add.mdx

`--groupName [groupName]`
: The Microsoft Entra or SharePoint group name. Specify either `upn`, `groupName` or `principalId` but not multiple.
: The Microsoft Entra or SharePoint group name. Specify either `upn`, `groupName`, `principalId`, `entraGroupId` or `entraGroupName` but not multiple.
Copy link
Member

Choose a reason for hiding this comment

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

We should correct that now it relates to SharePoint Group only so we should dropt the first part and maybe leave it like

Suggested change
: The Microsoft Entra or SharePoint group name. Specify either `upn`, `groupName`, `principalId`, `entraGroupId` or `entraGroupName` but not multiple.
: The SharePoint group name to add. Specify either `upn`, `groupName`, `principalId`, `entraGroupId` or `entraGroupName` but not multiple.

docs/docs/cmd/spo/folder/folder-roleassignment-add.mdx
Add the role assignment to the specified folder based on the Entra Group Id and role definition id.

```sh
m365 spo folder roleassignment add --webUrl "https://contoso.sharepoint.com/sites/contoso-sales" --folderUrl "/Shared Documents/FolderPermission" --entraGroupId '27ae47f1-48f1-46f3-980b-d3c1470e398d' --roleDefinitionId 1073741827
Copy link
Member

Choose a reason for hiding this comment

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

lets use double quotes to be consistent

Suggested change
m365 spo folder roleassignment add --webUrl "https://contoso.sharepoint.com/sites/contoso-sales" --folderUrl "/Shared Documents/FolderPermission" --entraGroupId '27ae47f1-48f1-46f3-980b-d3c1470e398d' --roleDefinitionId 1073741827
m365 spo folder roleassignment add --webUrl "https://contoso.sharepoint.com/sites/contoso-sales" --folderUrl "/Shared Documents/FolderPermission" --entraGroupId "27ae47f1-48f1-46f3-980b-d3c1470e398d" --roleDefinitionId 1073741827

docs/docs/cmd/spo/folder/folder-roleassignment-remove.mdx

`--groupName [groupName]`
: The Microsoft Entra or SharePoint group name. Specify either `upn`, `groupName` or `principalId` but not multiple.
: The Microsoft Entra or SharePoint group name. Specify either `upn`, `groupName`, `principalId`, `entraGroupId` or `entraGroupName` but not multiple.
Copy link
Member

Choose a reason for hiding this comment

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

lets clarify that this should be used only for SPO groups

Suggested change
: The Microsoft Entra or SharePoint group name. Specify either `upn`, `groupName`, `principalId`, `entraGroupId` or `entraGroupName` but not multiple.
: The SharePoint group name to add. Specify either `upn`, `groupName`, `principalId`, `entraGroupId` or `entraGroupName` but not multiple.

docs/docs/cmd/spo/folder/folder-roleassignment-remove.mdx
Remove the role assignment from the specified folder based on the Entra group id.

```sh
m365 spo folder roleassignment remove --webUrl "https://contoso.sharepoint.com/sites/contoso-sales" --folderUrl "/Shared Documents/FolderPermission" --entraGroupId '27ae47f1-48f1-46f3-980b-d3c1470e398d'
Copy link
Member

Choose a reason for hiding this comment

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

lets use double quotes

Suggested change
m365 spo folder roleassignment remove --webUrl "https://contoso.sharepoint.com/sites/contoso-sales" --folderUrl "/Shared Documents/FolderPermission" --entraGroupId '27ae47f1-48f1-46f3-980b-d3c1470e398d'
m365 spo folder roleassignment remove --webUrl "https://contoso.sharepoint.com/sites/contoso-sales" --folderUrl "/Shared Documents/FolderPermission" --entraGroupId "27ae47f1-48f1-46f3-980b-d3c1470e398d"

src/m365/spo/commands/folder/folder-roleassignment-add.ts
Comment on lines +177 to +183
let group: Group;
if (args.options.entraGroupId) {
group = await entraGroup.getGroupById(args.options.entraGroupId);
}
else {
group = await entraGroup.getGroupByDisplayName(args.options.entraGroupName!);
}
Copy link
Member

Choose a reason for hiding this comment

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

lets simplify

Suggested change
let group: Group;
if (args.options.entraGroupId) {
group = await entraGroup.getGroupById(args.options.entraGroupId);
}
else {
group = await entraGroup.getGroupByDisplayName(args.options.entraGroupName!);
}
const group = args.options.entraGroupId
? await entraGroup.getGroupById(args.options.entraGroupId)
: await entraGroup.getGroupByDisplayName(args.options.entraGroupName!)

src/m365/spo/commands/folder/folder-roleassignment-add.ts
@@ -1,3 +1,4 @@
import { Group } from '@microsoft/microsoft-graph-types';
Copy link
Member

Choose a reason for hiding this comment

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

if we simplify the code as I suggested we may remove this one

Suggested change
import { Group } from '@microsoft/microsoft-graph-types';

src/m365/spo/commands/folder/folder-roleassignment-add.ts
}

await this.breakRoleAssignment(requestUrl);
Copy link
Member

Choose a reason for hiding this comment

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

woooot this command not only adds role assignment but breaks inheritance if it was not broken 😮
TBH I was not aware of it and the command description does not mention it I think.
@pnp/cli-for-microsoft-365-maintainers I think it was our mistake and it is a bug as we have separate command for breaking role inheritance and no other role assignment command for either file or list o list item will break the role inheritance.

src/m365/spo/commands/folder/folder-roleassignment-remove.ts
@@ -1,3 +1,4 @@
import { Group } from '@microsoft/microsoft-graph-types';
Copy link
Member

Choose a reason for hiding this comment

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

if we simplify the if we may remove this import

Suggested change
import { Group } from '@microsoft/microsoft-graph-types';

src/m365/spo/commands/folder/folder-roleassignment-remove.ts
Comment on lines +147 to +153
let group: Group;
if (args.options.entraGroupId) {
group = await entraGroup.getGroupById(args.options.entraGroupId);
}
else {
group = await entraGroup.getGroupByDisplayName(args.options.entraGroupName!);
}
Copy link
Member

Choose a reason for hiding this comment

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

lets simplify

Suggested change
let group: Group;
if (args.options.entraGroupId) {
group = await entraGroup.getGroupById(args.options.entraGroupId);
}
else {
group = await entraGroup.getGroupByDisplayName(args.options.entraGroupName!);
}
const group = args.options.entraGroupId
? await entraGroup.getGroupById(args.options.entraGroupId)
: await entraGroup.getGroupByDisplayName(args.options.entraGroupName!);

@Adam-it
Copy link
Member

Adam-it commented Jan 5, 2025

Ready to merge 🚀 I should perform small fixups when merging

@Adam-it
Copy link
Member

Adam-it commented Jan 8, 2025

Merged manually. Thank you for your awesome contribution. You Rock 🤩👏

@Adam-it Adam-it closed this Jan 8, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Adam-it Adam-it Adam-it approved these changes

Assignees

@Adam-it Adam-it

Labels
pr-merged
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Enhance spo folder roleassignment commands with Microsoft Entra groups
3 participants
@nicodecleyre @milanholemans @Adam-it