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

Msal - Broker changes #433

Merged
merged 33 commits into from
Mar 26, 2019
Merged

Msal - Broker changes #433

merged 33 commits into from
Mar 26, 2019

Conversation

kreedula
Copy link
Contributor

@kreedula kreedula commented Mar 26, 2019

  • Renamed/Moved IMicrosoftAuthService.aidl
  • Added API's to IMicrosoftAuthService
  • Deleted BrokerRequest.aidl and BrokerResult.aidl
  • Renamed/ added constants as necessary.
  • Changed BrokerRequest and BrokerResult to Serializable POJO's
  • Moved BrokerAcquireTokenOperationParameters to common.
  • Added Result and Request adapters to transform input/output fields between broker and msal

ad-accounts PR : https://github.com/AzureAD/ad-accounts-for-android/pull/911
msal PR : AzureAD/microsoft-authentication-library-for-android#550

kreedula added 26 commits March 17, 2019 19:32
@kreedula kreedula self-assigned this Mar 26, 2019
@@ -0,0 +1,231 @@
package com.microsoft.identity.common.internal.request;
Copy link
Member

Choose a reason for hiding this comment

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

License needed

Copy link
Member

@iambmelt iambmelt left a comment

Choose a reason for hiding this comment

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

Couple of minor things but overall LGTM

@@ -41,6 +41,9 @@
@SerializedName("correlation_id")
private String mCorrelationId;

@SerializedName("oAuth_metadata")
private String mOAuthErrorMetadata;
Copy link
Member

Choose a reason for hiding this comment

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

Curious. Any example of error metadata?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think currently we don't get any, but eventually we would I guess, just moved this from BrokerErrorResponse

}

@Override
public BrokerAcquireTokenOperationParameters brokerInteractiveParametersFromActivity(@NonNull final Activity callingActivity) {
Copy link
Member

Choose a reason for hiding this comment

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

Are these all moved from ad-accounts' BrokerUtil? Is there any extra change you made in this PR?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Moved and refactored with some changes as deemed appropriate. Anything suspicious?

Copy link
Member

@heidijinxujia heidijinxujia left a comment

Choose a reason for hiding this comment

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

Thank you, Krishna.

@kreedula
Copy link
Contributor Author

Thanks @iambmelt @heidijinxujia for the review

@kreedula kreedula merged commit 5d41b39 into dev Mar 26, 2019
@kreedula kreedula deleted the kreedula/msal-broker-changes branch March 26, 2019 22:48
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants