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

Bug report: Impossible to create team when using application permissions #4916

Closed
MathijsVerbeeck opened this issue May 23, 2023 · 25 comments
Closed
Assignees
Milestone

Comments

@MathijsVerbeeck
Copy link
Contributor

MathijsVerbeeck commented May 23, 2023

Priority

(Low) Something is a little off

Description

It is currently impossible to create a team when using application permissions. This is due to the application permissions request expecting a members property in the request body, and we currently do not demand this.

More information can be found on this URL: https://learn.microsoft.com/en-us/graph/api/team-post?view=graph-rest-1.0&tabs=http#examples

Steps to reproduce

  1. Login using application permissions
  2. Execute m365 teams team add --name 'TEST' --description 'TEST'

Expected results

Team being created or a prompt for members

Actual results

Error: Request failed with status code 400

Diagnostics

Request error:
{
  "url": "https://graph.microsoft.com/v1.0/teams",
  "status": 400,
  "statusText": "Bad Request",
  "headers": {
    "transfer-encoding": "chunked",
    "content-type": "application/json",
    "vary": "Accept-Encoding",
    "strict-transport-security": "max-age=31536000",
    "request-id": "51e35905-5bb0-4c3b-8616-df164c8099e3",
    "client-request-id": "51e35905-5bb0-4c3b-8616-df164c8099e3",
    "x-ms-ags-diagnostic": "{\"ServerInfo\":{\"DataCenter\":\"North Europe\",\"Slice\":\"E\",\"Ring\":\"4\",\"ScaleUnit\":\"003\",\"RoleInstance\":\"DB1PEPF0002D0AB\"}}",
    "date": "Tue, 23 May 2023 09:52:51 GMT",
    "connection": "close"
  }
}
@Adam-it
Copy link
Contributor

Adam-it commented May 23, 2023

Thanks, @MathijsVerbeeck for rising this 👍. We will try to recheck this one ASAP

@milanholemans
Copy link
Contributor

Makes sense I guess. I think the only fix here would be to introduce 2 new options

  • --ownerUserId [ownerUserId
  • --ownerUserName [ownerUserName]

Both can be used with delegated or app only auth, but they will be required when using app only auth.

@MathijsVerbeeck
Copy link
Contributor Author

Makes sense I guess. I think the only fix here would be to introduce 2 new options

  • --ownerUserId [ownerUserId
  • --ownerUserName [ownerUserName]

Both can be used with delegated or app only auth, but they will be required when using app only auth.

I also thought something like that. Makes sense too.

@martinlingstuyl
Copy link
Contributor

martinlingstuyl commented May 27, 2023

Yeah, a team needs at least an owner indeed. We could also add a --memberUserNames option and allow a comma separated string of UPNs... (just like the chat message send command)

@MathijsVerbeeck
Copy link
Contributor Author

Yeah, a team needs at least an owner indeed. We could also add a --memberUserNames option and allow a comma separated string of UPNs... (just like the chat message send command)

I'd say we start with just specifying a single owner, to at least get this command to work, and we can extend this command in general in the future I believe.

@waldekmastykarz
Copy link
Member

Yeah, a team needs at least an owner indeed. We could also add a --memberUserNames option and allow a comma separated string of UPNs... (just like the chat message send command)

I'd say we start with just specifying a single owner, to at least get this command to work, and we can extend this command in general in the future I believe.

I suggest that we decide now if we'd like to support a single- or multiple users, so that we can avoid breaking changes later on. If I for example look at spo site add and aad o365group add, we support --owners [owners]. I suggest we use the same convention here for consistency.

@martinlingstuyl
Copy link
Contributor

So

  • in delegated mode, when you don't specify --owners, it will add the signed in user.
  • in delegated mode, when you specify --owners, it will add the owners, but not the signed in user if he isn't part of the owners string.
  • in app only mode, --owners is required.

?

@milanholemans
Copy link
Contributor

What are owners? UPNs? IDs? Usually when we refer to a user we provide an option that accepts UPNs and an option that accepts IDs.

@martinlingstuyl
Copy link
Contributor

Yes, thinking about that though: why do we even do that? If it's a GUID or an UPN, why don't we verify what kind of value is being used in the CLI? Should be easy to do, and it would clean up a lot of commands with these superfluous options.

@milanholemans
Copy link
Contributor

I don't know. 😊
Consistency could be an argument. For some commands this won't be that simple, e.g. for aad user get, how do you call an option that accepts both the ID and UPN? Also, some commands accept ID, UPN, and Email, in that case, we cannot tell whether an Email is UPN or a mail.

@MathijsVerbeeck
Copy link
Contributor Author

Any updates regarding this issue?

@milanholemans
Copy link
Contributor

Should we add 2 options? ownerUserNames and ownerIds? Which can be used in both delegated and application auth?
If we add this, I feel like we should introduce the same options to add members.

@martinlingstuyl
Copy link
Contributor

Let's do that. And ownerEmails?

@milanholemans
Copy link
Contributor

Let's do that. And ownerEmails?

Could include that one as well, so do we agree to add the options below?

  • ownerUserNames
  • ownerIds
  • ownerEmails
  • memberUserNames
  • memberIds
  • memberEmails

@Adam-it
Copy link
Contributor

Adam-it commented Sep 23, 2023

Perfect. Thank you @martinlingstuyl, @milanholemans for the awesome brainstorm.
@MathijsVerbeeck I think we area ready to open this up. Would you like to take the lead?

@MathijsVerbeeck
Copy link
Contributor Author

Perfect. Thank you @martinlingstuyl, @milanholemans for the awesome brainstorm.
@MathijsVerbeeck I think we area ready to open this up. Would you like to take the lead?

Sure thing.!

@Adam-it Adam-it self-assigned this Sep 23, 2023
@Adam-it
Copy link
Contributor

Adam-it commented Sep 23, 2023

Awesome 🤩.
Please do remember Hacktoberfest starts on 26.09 so if you participate it's best to open the PR after that date 👍

@Adam-it Adam-it assigned MathijsVerbeeck and unassigned Adam-it Sep 24, 2023
@MathijsVerbeeck
Copy link
Contributor Author

Hi guys

Just some feedback - I came across the following issue:
image

Using just a single user works just fine. So it seems that either we have to change the new specs a little bit to allow only one user, or we have to update the team immediately, but then we have to enforce the --wait parameter.

@milanholemans
Copy link
Contributor

That's a bummer! I think we should stick to the spec and update the team members afterward.

@MathijsVerbeeck
Copy link
Contributor Author

MathijsVerbeeck commented Sep 25, 2023

That's a bummer! I think we should stick to the spec and update the team members afterward.

Sure thing. So I create the team using the first user passed in either one of the owner options and then afterwards patch the team?

@MathijsVerbeeck
Copy link
Contributor Author

One (hopefully final) further remark I have is that we will have to only include this when using --wait, otherwise the team will most likely not yet be provisioned and it will throw an error.

@Jwaegebaert
Copy link
Contributor

good point, I don't think there is any better approach at the moment than enforcing --wait when you want to add multiple owners. Let's make sure that we have good error handling for this scenario and that we add a remark for it as well.

@Jwaegebaert
Copy link
Contributor

Hey @MathijsVerbeeck, just checking in. What is the latest regarding this issue?

@waldekmastykarz
Copy link
Member

Resetting due to lack of response

@MathijsVerbeeck
Copy link
Contributor Author

Can I restart working on this one?

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

Successfully merging a pull request may close this issue.

6 participants