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

Fix project - integrate fixed MSID #1213

Merged
merged 7 commits into from
Jun 26, 2018
Merged

Fix project - integrate fixed MSID #1213

merged 7 commits into from
Jun 26, 2018

Conversation

unpluggedk
Copy link
Contributor

@unpluggedk unpluggedk commented Jun 19, 2018

Project file was broken, now it is fixed.
AzureAD/microsoft-authentication-library-common-for-objc#155

update after error code PR AzureAD/microsoft-authentication-library-common-for-objc#153

add extra config to disable system web view

@@ -1208,6 +1212,7 @@
isa = PBXFrameworksBuildPhase;
buildActionMask = 2147483647;
files = (
96285D7C20D87DF8004CA4BD /* SafariServices.framework in Frameworks */,
Copy link
Member

@oldalton oldalton Jun 22, 2018

Choose a reason for hiding this comment

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

it personally feels a bit weird to include SafariServices in ADAL if system webview won't be publicly available in ADAL. Can we do some conditional compilation so that we don't force SafariServices to be included without customers benefiting from it?

Copy link
Member

@oldalton oldalton left a comment

Choose a reason for hiding this comment

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

A couple of questions :)

96AC69391E70E184008183A7 /* ADAutoResultViewController.m in Sources */ = {isa = PBXBuildFile; fileRef = 96AC69381E70E184008183A7 /* ADAutoResultViewController.m */; };
96B9F03E20DDD411006C806C /* GSS.framework in Frameworks */ = {isa = PBXBuildFile; fileRef = 96B9F03D20DDD411006C806C /* GSS.framework */; };
Copy link
Member

Choose a reason for hiding this comment

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

just making sure that this was added only to macOS target :)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, confirmed :)

@@ -965,8 +967,12 @@
94DD18E11C5ACFBF00F80C62 /* ADAL-Unit-Tests.xctest */ = {isa = PBXFileReference; explicitFileType = wrapper.cfbundle; includeInIndex = 0; path = "ADAL-Unit-Tests.xctest"; sourceTree = BUILT_PRODUCTS_DIR; };
960E93721E296CC2008036C0 /* ADURLSessionDemux.h */ = {isa = PBXFileReference; fileEncoding = 4; lastKnownFileType = sourcecode.c.h; path = ADURLSessionDemux.h; sourceTree = "<group>"; };
960E93731E296CC2008036C0 /* ADURLSessionDemux.m */ = {isa = PBXFileReference; fileEncoding = 4; lastKnownFileType = sourcecode.c.objc; path = ADURLSessionDemux.m; sourceTree = "<group>"; };
96285D7020D87760004CA4BD /* SafariServices.framework */ = {isa = PBXFileReference; lastKnownFileType = wrapper.framework; name = SafariServices.framework; path = Platforms/iPhoneOS.platform/Developer/SDKs/iPhoneOS11.4.sdk/System/Library/Frameworks/SafariServices.framework; sourceTree = DEVELOPER_DIR; };
Copy link
Member

Choose a reason for hiding this comment

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

Do we still need it here with conditional inclusion?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It's just in the project file as a tombstone, but I will remove it manually :)

@@ -3562,13 +3576,15 @@
baseConfigurationReference = D6CF4EB01FC370BF00CD70C5 /* adal__framework__ios.xcconfig */;
buildSettings = {
GCC_OPTIMIZATION_LEVEL = 0;
GCC_PREPROCESSOR_DEFINITIONS = "$(inherited)";
Copy link
Member

Choose a reason for hiding this comment

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

should this go to xconfig instead?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It used to indicate DEBUG=1, I think it should be fine as is.

// Interactive flow
@(MSIDErrorAuthorizationFailed) : @(AD_ERROR_SERVER_AUTHORIZATION_CODE),
@(MSIDErrorUserCancel) : @(AD_ERROR_UI_USER_CANCEL),
@(MSIDErrorSessionCanceledProgramatically) : @(AD_ERROR_UNEXPECTED),
Copy link
Member

Choose a reason for hiding this comment

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

This currently produces AD_ERROR_UI_USER_CANCEL in ADAL (not very logical, but let's not break it), please change :)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

@(MSIDErrorServerOauth) : @(AD_ERROR_SERVER_OAUTH),
@(MSIDErrorServerInvalidResponse) : @(AD_ERROR_SERVER_INVALID_RESPONSE),
@(MSIDErrorServerRefreshTokenRejected) : @(AD_ERROR_SERVER_REFRESH_TOKEN_REJECTED),
@(MSIDErrorServerInvalidRequest) :@(AD_ERROR_DEVELOPER_INVALID_ARGUMENT),
Copy link
Member

Choose a reason for hiding this comment

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

Is there possibly a better corresponding ADAL error code?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

AD_ERROR_SERVER_OAUTH should be more accurate.

@unpluggedk unpluggedk merged commit 900d57f into dev Jun 26, 2018
@unpluggedk unpluggedk deleted the jak/msid-webview-pr-pull branch June 26, 2018 23:11
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.

2 participants