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

Device Code Flow Final PR on MSAL #1112

Merged
merged 29 commits into from
Aug 7, 2020
Merged

Device Code Flow Final PR on MSAL #1112

merged 29 commits into from
Aug 7, 2020

Conversation

t-fadura
Copy link
Contributor

@t-fadura t-fadura commented Jul 29, 2020 •
edited
Loading

Final major PR on MSAL as part of my internship project. End-to-end Device Code Flow is working alongside Common PR at: AzureAD/microsoft-authentication-library-common-for-android#983

Since LocalMsalController was moved to common, this PR is mainly just test cases now.

@t-fadura t-fadura mentioned this pull request Jul 29, 2020
Merged
@shahzaibj
Copy link
Contributor

@t-fadura Is this still WIP?

@t-fadura
Copy link
Contributor Author

All code included so far is completed. I put it as WIP because I have not done the test cases yet.

@t-fadura
Copy link
Contributor Author

What I'm saying is what's already in the PR is ready to be reviewed, there won't be any major changes on my end.

t-fadura
t-fadura commented Jul 30, 2020
View reviewed changes
Copy link
Contributor Author

@t-fadura t-fadura left a comment

Choose a reason for hiding this comment

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

Updated again

@t-fadura t-fadura changed the title [WIP] Device Code Flow Final PR on MSAL Device Code Flow Final PR on MSAL Jul 31, 2020
rpdome
rpdome reviewed Aug 5, 2020
View reviewed changes
Copy link
Member

@rpdome rpdome left a comment

Choose a reason for hiding this comment

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

Left a few comments.

msal/src/test/java/com/microsoft/identity/client/e2e/tests/mocked/DeviceCodeFlowApiTest.java Outdated Show resolved Hide resolved
msal/src/test/java/com/microsoft/identity/client/e2e/tests/mocked/DeviceCodeFlowApiTest.java Outdated Show resolved Hide resolved
msal/src/test/java/com/microsoft/identity/client/e2e/tests/mocked/DeviceCodeFlowApiTest.java Outdated Show resolved Hide resolved
shahzaibj
shahzaibj reviewed Aug 5, 2020
View reviewed changes
msal/src/main/java/com/microsoft/identity/client/internal/CommandParametersAdapter.java Outdated
Comment on lines 204 to 207

final String uri = ((AzureActiveDirectoryAuthority) configuration.getAuthorities().get(0)).getAudience().getCloudUrl();
final Authority authority = Authority.getAuthorityFromAuthorityUrl(uri);

Copy link
Contributor

Choose a reason for hiding this comment

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

Why can't we just use default authority?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Default authority has "common" as tenantID

Copy link
Contributor

Choose a reason for hiding this comment

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

Default authority would be whatever was declared default in the config....or if there is only one declared then it would be that one authority. It would have common if the dev put common in the config, otherwise it won't. If it does have common, why is that a problem?

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 my config:
image

Common as the tenantID is what's causing /common to be added to the end of the authority url later down the line. This issue doesn't occur in acquireToken() because the command parameters for that also use a custom authority.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

refer to getAuthorityUri() in AzureActiveDirectoryAuthroity, this is where the tenant is appended to the end of the authority.

Copy link
Contributor

Choose a reason for hiding this comment

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

Okay, I'm not completely clear on this. Let's sync up offline.

Copy link
Contributor

Choose a reason for hiding this comment

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

Synced offline with @t-fadura. There's a bug in MSAL and has been for quite some time and that's what's causing this issue. Created GitHub issue here: #1121

shahzaibj
shahzaibj reviewed Aug 5, 2020
View reviewed changes
msal/src/test/java/com/microsoft/identity/client/e2e/tests/mocked/DeviceCodeFlowApiTest.java Outdated
super.setup();

final PublicClientApplicationConfiguration config = mApplication.getConfiguration();
mUrlBody = ((AzureActiveDirectoryAuthority) config.getAuthorities().get(0)).getAudience().getCloudUrl();
Copy link
Contributor

@shahzaibj shahzaibj Aug 5, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

Why not config.getDefaultAuthority().getAuthorityUrl().toString()?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

.getAuthorityUrl() is not defined here, checked it in my code
Also tried type casting to AzureActiveDirectoryAuthority

Copy link
Contributor Author

Choose a reason for hiding this comment

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

sorry, it was .getAuthorityURL, which returns a url with /common at the end

Copy link
Contributor

Choose a reason for hiding this comment

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

It probably returns /common at the end because you probably have common in your config. Even if it does, I'm not clear on why that is a problem and why we can't use that?

Copy link
Contributor

Choose a reason for hiding this comment

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

So that's essentially a developer error right? (They need to make sure they declare the correct authority in the config). In this case, it looks @t-fadura did declare the correct authority. I see his config authority is of the form cloud/tenantId. So getDefaultAuthority() should be returning that right?

Copy link
Member

Choose a reason for hiding this comment

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

My bad. I removed my original comment because I thought I was talking about a different thing.

What Fadi was seeing is that somehow his authority has /common appended after tenant id, which is really weird. I'm not familiar with this layer, but most likely there's a bug here.

Copy link
Contributor

Choose a reason for hiding this comment

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

Yup, sycned offline with @t-fadura, there's a bug here in MSAL and has been like this forever.

Copy link
Contributor

Choose a reason for hiding this comment

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

Created GitHub issue: #1121

shahzaibj
shahzaibj approved these changes Aug 6, 2020
View reviewed changes
Copy link
Contributor

@shahzaibj shahzaibj left a comment

Choose a reason for hiding this comment

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

LGTM 🎉

@t-fadura t-fadura merged commit 50a9002 into dev Aug 7, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@rpdome rpdome rpdome left review comments

@iambmelt iambmelt iambmelt approved these changes

@Om83 Om83 Om83 approved these changes

@shahzaibj shahzaibj shahzaibj approved these changes

@shoatman shoatman Awaiting requested review from shoatman

@AdamBJohnsonx AdamBJohnsonx Awaiting requested review from AdamBJohnsonx

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

Successfully merging this pull request may close these issues.

None yet
5 participants
@t-fadura @shahzaibj @iambmelt @Om83 @rpdome