-
Notifications
You must be signed in to change notification settings - Fork 94
Conversation
This fixes #86 for the broken links
README.md
Outdated
@@ -106,7 +106,11 @@ return context.acquire_token_with_authorization_code( | |||
``` | |||
|
|||
## Samples and Documentation | |||
[We provide a full suite of sample applications and documentation on GitHub](https://github.com/AzureADSamples) to help you get started with learning the Azure Identity system. This includes tutorials for native clients such as Windows, Windows Phone, iOS, OSX, Android, and Linux. We also provide full walkthroughs for authentication flows such as OAuth2, OpenID Connect, Graph API, and other awesome features. | |||
We provide a full suite of [sample applications on GitHub](https://github.com/azure-samples?q=adal) and an [Azure AD developer landing page](https://docs.microsoft.com/en-us/azure/active-directory/develop/active-directory-developers-guide) to help you get started with learning the Azure Identity system. This includes tutorials for native clients such as Windows, Windows Phone, iOS, OSX, Android, and Linux. We also provide full walkthroughs for authentication flows such as OAuth2, OpenID Connect, Graph API, and other awesome features. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Use this link for samples: https://github.com/azure-samples?utf8=%E2%9C%93&q=active-directory&type=&language=
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Don't say "Azure Identity System". "Get started with Azure AD".
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For platforms just say Windows, iOS, Android, and other popular platforms.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Graph API and "other awesome features" are not auth flows. "We also provide full code samples for authentication with Azure AD, calling the Microsoft Graph, and several other core identity and access management scenarios". Feel free to play with it tho!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Made a few comments. Otherwise, it mostly looks good.
README.md
Outdated
@@ -106,7 +106,11 @@ return context.acquire_token_with_authorization_code( | |||
``` | |||
|
|||
## Samples and Documentation | |||
[We provide a full suite of sample applications and documentation on GitHub](https://github.com/AzureADSamples) to help you get started with learning the Azure Identity system. This includes tutorials for native clients such as Windows, Windows Phone, iOS, OSX, Android, and Linux. We also provide full walkthroughs for authentication flows such as OAuth2, OpenID Connect, Graph API, and other awesome features. | |||
We provide a full suite of [sample applications on GitHub](https://github.com/azure-samples?q=adal) and an [Azure AD developer landing page](https://docs.microsoft.com/en-us/azure/active-directory/develop/active-directory-developers-guide) to help you get started with learning the Azure Identity system. This includes tutorials for native clients such as Windows, Windows Phone, iOS, OSX, Android, and Linux. We also provide full walkthroughs for authentication flows such as OAuth2, OpenID Connect, Graph API, and other awesome features. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is there a reason for mentioning the native clients explicitly? If not, I would omit the line or just mention native clients and web applications.
Secondly, you can say "walkthroughs for authentication flows such as OAuth2, OpenID Connect and for calling APIs such as the Graph API." This is because the Graph API is not exactly an authentication protocol.
README.md
Outdated
@@ -31,7 +31,7 @@ The convinient methods in 0.1.0 have been removed, and now your application shou | |||
|
|||
2 Reasons: | |||
|
|||
* Each adal client should have a unique id representing an valid application registered in a tenant. The old methods borrowed the client-id of [azure-cli](https://github.com/Azure/azure-xplat-cli), which is never right. It is simple to register your application and get a client id. Many walkthroughs exist. You can follow [one of those](http://www.bradygaster.com/post/using-windows-azure-active-directory-to-authenticate-the-management-libraries). Though that involves C# client, but the flow, and particularly the wizard snapshots are the same with adal-python. Do check out if you are new to AAD. | |||
* Each adal client should have a unique id representing an valid application registered in a tenant. The old methods borrowed the client-id of [azure-cli](https://github.com/Azure/azure-xplat-cli), which is never right. It is simple to register your application and get a client id. Many walkthroughs exist. You can follow [one of those](https://docs.microsoft.com/en-us/azure/app-service-mobile/app-service-mobile-how-to-configure-active-directory-authentication). Do check out if you are new to AAD. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
rather than say "unique id" I would say "have an Application ID" and give steps on how to register an app (there's a doc on it).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The app doesn't need to be registered in your tenant, consider removing that line.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That new link is to Azure App Services, this is a different service and will not be super compatible with ADAL.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good catch for my copy-and-paste error! I mean to use this App Registration link from the dev landing page instead.
As a side note, this entire paragraph was added 1 year ago, and its original purpose was presumably an explanation of a change after version 0.1.0. Now, it feels somewhat weird to have this section show up early in this README. Shall we demote this paragraph to the very end of README, in favor of drawing user's attention to the "Samples and Documentation" section instead?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@rayluo sounds like a good idea!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A few changes as per my comments before I can approve.
The "Changes on 'client_id' and 'resource' arguments after 0.1.0" section was added 1 year ago as an explanation of a change after version 0.1.0. Now, it feels somewhat weird to have this section show up early in this README. We decide to demote this paragraph to the very end of README, in favor of drawing user's attention to the "Samples and Documentation" section instead.
@navyasric 's feedback #97 (comment)
This fixes #86 for the broken links