Enhances spo web roleassignment commands with Microsoft Entra groups, Closes #6193 #6463
Conversation
|
Thank you! We'll have a look at it soon. |
There was a problem hiding this comment.
@nicodecleyre Awesome work 👏
I added some small comments connected to refactoring that if you want and have time you could resolve and if not I will do it when merging 👍 no problem
Checked and works
| add role assignment using a Entra Group ID and a role definition | ||
|
|
||
| ```sh | ||
| m365 spo web roleassignment add --webUrl "https://contoso.sharepoint.com/sites/project-x" --entraGroupId '27ae47f1-48f1-46f3-980b-d3c1470e398d' --roleDefinitionId 1073741829 |
There was a problem hiding this comment.
we could use double quotes just to align with what we already have
| m365 spo web roleassignment add --webUrl "https://contoso.sharepoint.com/sites/project-x" --entraGroupId '27ae47f1-48f1-46f3-980b-d3c1470e398d' --roleDefinitionId 1073741829 | |
| m365 spo web roleassignment add --webUrl "https://contoso.sharepoint.com/sites/project-x" --entraGroupId "27ae47f1-48f1-46f3-980b-d3c1470e398d" --roleDefinitionId 1073741829 |
| Remove roleassignment from web based on Entra Group Id | ||
|
|
||
| ```sh | ||
| m365 spo web roleassignment remove --webUrl "https://contoso.sharepoint.com/sites/contoso-sales" --entraGroupId '27ae47f1-48f1-46f3-980b-d3c1470e398d' --force |
There was a problem hiding this comment.
same here
| m365 spo web roleassignment remove --webUrl "https://contoso.sharepoint.com/sites/contoso-sales" --entraGroupId '27ae47f1-48f1-46f3-980b-d3c1470e398d' --force | |
| m365 spo web roleassignment remove --webUrl "https://contoso.sharepoint.com/sites/contoso-sales" --entraGroupId "27ae47f1-48f1-46f3-980b-d3c1470e398d" --force |
| @@ -57,6 +63,12 @@ Remove roleassignment from web based on principal Id without prompting for confi | |||
| m365 spo web roleassignment remove --webUrl "https://contoso.sharepoint.com/sites/contoso-sales" --principalId 2 --force | |||
| ``` | |||
|
|
|||
| Remove roleassignment from web based on Entra Group Id | |||
There was a problem hiding this comment.
lets add a remark that this is force delete as it someone just do a copy/past and execute than this may have some not pleasent implications
| Remove roleassignment from web based on Entra Group Id | |
| Remove roleassignment from web based on Entra Group Id and don't prompt for confirmation |
| }); | ||
|
|
||
|
|
||
| it('fails validation if the entaGroupId is not a valid guid', async () => { |
There was a problem hiding this comment.
Lets keep no more than single blank line in code
| }); | |
| it('fails validation if the entaGroupId is not a valid guid', async () => { | |
| }); | |
| it('fails validation if the entaGroupId is not a valid guid', async () => { |
| let group: Group; | ||
| if (args.options.entraGroupId) { | ||
| group = await entraGroup.getGroupById(args.options.entraGroupId); | ||
| } | ||
| else { | ||
| group = await entraGroup.getGroupByDisplayName(args.options.entraGroupName!); | ||
| } |
There was a problem hiding this comment.
similar like in the listItem PR we may just simplify this and remove the type. The type is returned by the method so it is applied allready no need to mention it I guess.
| 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!); |
| @@ -1,3 +1,4 @@ | |||
| import { Group } from '@microsoft/microsoft-graph-types'; | |||
There was a problem hiding this comment.
When we apply my previous comment we may get rid of this line
| import { Group } from '@microsoft/microsoft-graph-types'; |
| sinon.stub(spo, 'ensureEntraGroup').withArgs('https://contoso.sharepoint.com', graphGroup).resolves(entraGroupResponse); | ||
|
|
||
| sinon.stub(request, 'post').callsFake(async (opts) => { | ||
| if ((opts.url as string).indexOf('_api/web/roleassignments/removeroleassignment(principalid=\'11\')') > -1) { |
There was a problem hiding this comment.
in general we should avoid using indexOf and instead use exact equal to url using ===
| @@ -1,3 +1,4 @@ | |||
| import { Group } from '@microsoft/microsoft-graph-types'; | |||
There was a problem hiding this comment.
when we refactor the if condition we may remove this line
| import { Group } from '@microsoft/microsoft-graph-types'; |
| let group: Group; | ||
| if (options.entraGroupId) { | ||
| group = await entraGroup.getGroupById(options.entraGroupId); | ||
| } | ||
| else { | ||
| group = await entraGroup.getGroupByDisplayName(options.entraGroupName!); | ||
| } |
There was a problem hiding this comment.
this may be simplified to
| let group: Group; | |
| if (options.entraGroupId) { | |
| group = await entraGroup.getGroupById(options.entraGroupId); | |
| } | |
| else { | |
| group = await entraGroup.getGroupByDisplayName(options.entraGroupName!); | |
| } | |
| const group = options.entraGroupId | |
| ? await entraGroup.getGroupById(options.entraGroupId) | |
| : await entraGroup.getGroupByDisplayName(options.entraGroupName!); |
|
Ready to merge 🚀 I should perform small fixup before merge |
|
Merged manually. Thank you for your awesome contribution. You Rock 🤩👏 |
Closes #6193