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

Add device code, interactive, and user credentials #4585

Merged
merged 34 commits into from
Aug 2, 2019
Merged

Add device code, interactive, and user credentials #4585

merged 34 commits into from
Aug 2, 2019

Conversation

jianghaolu
Copy link
Contributor

No description provided.

JonathanGiles
JonathanGiles requested changes Jul 29, 2019
View reviewed changes
sdk/identity/azure-identity/src/main/java/com/azure/identity/DeviceCodeChallenge.java Show resolved Hide resolved
sdk/identity/azure-identity/src/main/java/com/azure/identity/DeviceCodeChallenge.java Show resolved Hide resolved
sdk/identity/azure-identity/src/main/java/com/azure/identity/IdentityClient.java Outdated Show resolved Hide resolved
sdk/identity/azure-identity/src/main/java/com/azure/identity/IdentityClient.java Outdated Show resolved Hide resolved
sdk/identity/azure-identity/src/main/java/com/azure/identity/IdentityClient.java Outdated Show resolved Hide resolved
...azure-identity/src/main/java/com/azure/identity/credential/InteractiveBrowserCredential.java Outdated Show resolved Hide resolved
...dentity/azure-identity/src/main/java/com/azure/identity/credential/DeviceCodeCredential.java Outdated Show resolved Hide resolved
sdk/identity/azure-identity/src/main/java/com/azure/identity/credential/UserCredential.java Outdated Show resolved Hide resolved
...zure-identity/src/main/java/com/azure/identity/implementation/AuthorizationCodeListener.java Outdated Show resolved Hide resolved
...zure-identity/src/main/java/com/azure/identity/implementation/AuthorizationCodeListener.java Outdated Show resolved Hide resolved
schaabs
schaabs reviewed Jul 29, 2019
View reviewed changes
sdk/identity/azure-identity/src/main/java/com/azure/identity/IdentityClient.java Outdated Show resolved Hide resolved
sdk/identity/azure-identity/src/main/java/com/azure/identity/IdentityClient.java Outdated Show resolved Hide resolved
sdk/identity/azure-identity/src/main/java/com/azure/identity/IdentityClient.java Outdated
try {
SilentParameters parameters;
if (currentApplication.get() != null) {
if (currentAccount.get() != null) {
Copy link
Member

Choose a reason for hiding this comment

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

If currentAccount is null how are you getting a token silently? What account are you getting a token for in this case?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

the IAccount argument is optional in MSAL - thus try calling that if the account is not available

sdk/identity/azure-identity/src/main/java/com/azure/identity/IdentityClient.java Outdated Show resolved Hide resolved
...azure-identity/src/main/java/com/azure/identity/credential/InteractiveBrowserCredential.java Outdated Show resolved Hide resolved
JonathanGiles
JonathanGiles requested changes Jul 31, 2019
View reviewed changes
Copy link
Member

@JonathanGiles JonathanGiles left a comment

Choose a reason for hiding this comment

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

Looks good.

sdk/identity/azure-identity/src/main/java/com/azure/identity/DeviceCodeChallenge.java Outdated Show resolved Hide resolved
sdk/identity/azure-identity/src/main/java/com/azure/identity/DeviceCodeChallenge.java Show resolved Hide resolved
...ity/azure-identity/src/main/java/com/azure/identity/credential/AadCredentialBuilderBase.java Outdated Show resolved Hide resolved
...zure-identity/src/main/java/com/azure/identity/credential/ChainedTokenCredentialBuilder.java Outdated Show resolved Hide resolved
.../azure-identity/src/main/java/com/azure/identity/credential/ClientCertificateCredential.java Outdated Show resolved Hide resolved
...identity/src/main/java/com/azure/identity/credential/ClientCertificateCredentialBuilder.java Outdated Show resolved Hide resolved
...identity/src/main/java/com/azure/identity/credential/ClientCertificateCredentialBuilder.java Outdated Show resolved Hide resolved
...entity/azure-identity/src/main/java/com/azure/identity/credential/CredentialBuilderBase.java Outdated Show resolved Hide resolved
...zure-identity/src/main/java/com/azure/identity/credential/ClientSecretCredentialBuilder.java Show resolved Hide resolved
sdk/identity/azure-identity/src/main/java/com/azure/identity/implementation/IdentityClient.java Outdated Show resolved Hide resolved
@jianghaolu jianghaolu marked this pull request as ready for review July 31, 2019 23:04
@jianghaolu jianghaolu requested a review from hemanttanwar as a code owner July 31, 2019 23:04
@jianghaolu jianghaolu requested a review from samvaity as a code owner July 31, 2019 23:54
schaabs
schaabs reviewed Aug 1, 2019
View reviewed changes
sdk/identity/azure-identity/src/main/java/com/azure/identity/implementation/IdentityClient.java
* The identity client that contains APIs to retrieve access tokens
* from various configurations.
*/
public class IdentityClient {
Copy link
Member

Choose a reason for hiding this comment

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

This class should not be public

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is in the implementation package - so credentials outside the package cannot see it unless it's public. But since it's in implementation package, no javadocs will be generated for it and Java devs know they shouldn't use it.

schaabs
schaabs reviewed Aug 1, 2019
View reviewed changes
...ty/azure-identity/src/main/java/com/azure/identity/implementation/IdentityClientBuilder.java
*
* @see IdentityClient
*/
public final class IdentityClientBuilder {
Copy link
Member

Choose a reason for hiding this comment

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

This class shouldn't be public

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Same as IdentityClient.

schaabs
schaabs approved these changes Aug 2, 2019
View reviewed changes
Copy link
Member

@schaabs schaabs left a comment

Choose a reason for hiding this comment

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

:shipit:

@jianghaolu
Copy link
Contributor Author

Jonathan approved over Teams.

@jianghaolu jianghaolu merged commit 5c4f419 into Azure:master Aug 2, 2019
srnagar pushed a commit to srnagar/azure-sdk-for-java that referenced this pull request Aug 5, 2019
@jianghaolu @srnagar
Add device code, interactive, and user credentials (Azure#4585)
0220ed0 
* latest network

* Replace adal with msal and support device code and interactive

* Add device code & interactive credentials

* Add user credential

* Up versions for azure-identity

* Wire up MSAL's token cache

* Add mock tests

* Device code and interactive unit tests

* Some cr feedback

* Add builders for credentials

* Temporarily fix dependency versions

* Add IdentityClientBuilder and some clean up

* Add missed files

* Add more identity client tests

* Use logAndThrow

* Update DefaultAzureCredential references in key vault

* More key vault references

* Code clean up

* Work around jdk 9 issue

* Work around JDK 9 time accuracy

* Add core to identity pom.service.xml

* Move tenantId and authorityHost builder methods

* Fix MSI test flakiness

* Add more docs in readme

* User cred improvements, docs, samples

* Add table of contents in readme
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@JonathanGiles JonathanGiles JonathanGiles requested changes

@schaabs schaabs schaabs approved these changes

@g2vinay g2vinay Awaiting requested review from g2vinay

@hemanttanwar hemanttanwar Awaiting requested review from hemanttanwar

@samvaity samvaity Awaiting requested review from samvaity

@danieljurek danieljurek Awaiting requested review from danieljurek

@mitchdenny mitchdenny Awaiting requested review from mitchdenny

@weshaggard weshaggard Awaiting requested review from weshaggard

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@jianghaolu @JonathanGiles @schaabs