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

az ad sp create-for-rbac results don't use standard terminology #1483

Closed
xtophs opened this issue Nov 30, 2016 · 10 comments
Closed

az ad sp create-for-rbac results don't use standard terminology #1483

xtophs opened this issue Nov 30, 2016 · 10 comments

Comments

@xtophs
Copy link

xtophs commented Nov 30, 2016

The output from creating a service principal from az ad sp create-for-rbac is:

{
"appId": "some GUID",
"name": "http://azure-cli-2016-more characters",
"password": "password GUID",
"tenant": "tenant GUID"
}

It would be easier to follow if the property naming followed the standard terminology used all over Azure AD Service Principal documentation, i.e.: appId -> client_id, password -> client_secret

@xtophs
Copy link
Author

xtophs commented Nov 30, 2016

adding @yugangw-msft

@yugangw-msft
Copy link
Contributor

yugangw-msft commented Dec 1, 2016

@xtophs, I used the client_id before, but changed through #1178
There is no good answer. Among AAD, graph, portal (old and new), there have been inconsistency terminologies across. Because this command is working around directory graph, I decided to choose the naming from that domain. appId is indeed a property of of service principal, the password is also correct because it comes from the password credentials of associated the graph application.

@colemickens
Copy link
Contributor

I agree with @xtophs here, especially since most open source software built for OAuth interactions use the terms client_id and client_secret.

In ACS-Engine, @xtophs was trying to write better documentation about the Service Principal process... and we realize that it's much wordy than it needs to be because we have to specifically explain what each field is. appId and password aren't going to look familiar to anyone who isn't pretty intimately familiar with the workings of AAD.

In my opinion, this would be an improvement.

{
"clientId: "00000000-0000-0000-0000-000000000000",
"name": "http://some-name-that-is-also-an-app-uri-identifier",
"clientSecret": "00000000-0000-0000-0000-000000000000",
"tenantId": "00000000-0000-0000-0000-000000000000"
}

cc: @ahmetalpbalkan

@yugangw-msft
Copy link
Contributor

Mark it as discussion.
//cc @sajayantony @djyou

@sajayantony
Copy link
Contributor

This was our initial proposal as well and as Cole points out https://tools.ietf.org/html/rfc6749#section-2.3.1

Client id and secret definitely sounds right. Unless your are living and breathing AAD, app just sounds odd to any new user.

@djyou
Copy link
Member

djyou commented Dec 1, 2016

Client id and secret would be the right terminology. We don't have a preference for our commands, but the inconsistency is from the fact that client_id is not a property of service principal or application object as returned by the server and written in the spec. create-for-rbac is a special command that does application creation, service principal creation, and role assignment all together. The user, however, will not be able to see the terminology client_id or client_secret from any other commands in az ad app and az ad sp. Our commands could live either way, the burden is on the user to figure out which one of appId or objectId or some other property is in fact client_id.

@yugangw-msft
Copy link
Contributor

The discussions made in this issue has really nothing new from the original #1178. I made the same argument there, but later decided to take @djyou's suggestion and choose not to expose the concepts of client_id, and client_secret. Also, the login command doesn't expose the client_id either, rather still username, and password. Cli users should not need to learn this oauth2 terms, which are just the implementation details for login.

@colemickens
Copy link
Contributor

I'd keep fighting hard for client_id/client_secret, but I concede it's a good point that it would be inconsistent with output of other az commands and inconsistent with the Graph API itself.

I definitely think that at the very least, create-for-rbac and login should have consistent names for these two fields.

Other questions about the create-for-rbac output:

  • appId vs applicationId (I prefer applicationId)
  • tenant vs tenantId (I prefer tenantId)

Should we stick with applicationId and password? If so, I can make an Issue for login...

@squillace
Copy link

FWIW, this is an artifact, probably, of the fact that an app, in the AAD sense, does not need to be a client. But in the case of all non-AAD uses, it likely is, so.... most people would always use clientID (because, their app is a client) but the original implementation did not make that assumption.

Unfortunate. I would like it to be "thing", but that's not going to fly.

@yugangw-msft
Copy link
Contributor

Let us keep what we have :)?

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

No branches or pull requests

7 participants