-
Notifications
You must be signed in to change notification settings - Fork 4
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
Automate authenticator oauth tests #432
Automate authenticator oauth tests #432
Conversation
…ond correctly to cancellation errors
…lkit into hud10837/automate-authenticator-oauth-tests
packagingOptions { | ||
exclude("META-INF/LICENSE-notice.md") | ||
exclude("META-INF/LICENSE.md") | ||
} |
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.
I couldn't compile after adding uiautomator and mockk without this, but I'm not sure if that's just a local issue
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.
I think that change is correct.
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.
Test Case 1
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.
Test Case 2
}.getOrElse { | ||
if (it is OperationCancelledException) { | ||
return ArcGISAuthenticationChallengeResponse.Cancel | ||
} else { | ||
return ArcGISAuthenticationChallengeResponse.ContinueAndFailWithError(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.
We should discuss this. Because the tests use handleArcGISAuthenticationChallenge()
directly, any exceptions that get thrown are thrown into the test. Normally the caller of handleArcGISAuthenticationChallenge
will handle the exceptions but it seems to me that we should be handling it here anyway.
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.
I don't think this change is needed. Any exception thrown, would be handled in the SDK's AuthenticationChallengeQueue
. In the test code, since we bypass that code, we should just catch the exception there.
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.
@hud10837 looks good, a few comments.
|
||
<data | ||
android:host="auth" | ||
android:scheme="kotlin-authentication-test-1" /> |
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.
👍
@Before | ||
fun signOut() { | ||
runBlocking { | ||
ArcGISEnvironment.authenticationManager.signOut() |
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.
Call sign out also in @After
?
@After | ||
fun closeBrowser() { | ||
try { | ||
UiDevice.getInstance(InstrumentationRegistry.getInstrumentation()).pressBack() |
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 this required? As long as the test uses a CCT, it should close automatically at the end of each test?
enterCredentialsOnBrowser("username", "password", composeTestRule.activity) | ||
clickByText("Sign In") | ||
}.await() | ||
assert(response is ArcGISAuthenticationChallengeResponse.ContinueWithCredential) |
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
val composeTestRule = createAndroidComposeRule<ComponentActivity>() | ||
|
||
private val arcGISOnline = "https://arcgis.com" | ||
private val authenticatorState = AuthenticatorState().apply { |
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.
I think this state object should be created locally within the scope of testOAuthChallengeWithStateRestoration()
, so that a new Authenticator()
call is passed a new instance of AuthenticatorState
.
@After | ||
fun closeBrowser() { | ||
try { | ||
UiDevice.getInstance(InstrumentationRegistry.getInstrumentation()).pressBack() |
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.
If we don't do this, does this cause any issues?
@Before | ||
fun signOut() { | ||
runBlocking { | ||
ArcGISEnvironment.authenticationManager.signOut() |
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
enterCredentialsOnBrowser("username", "password", composeTestRule.activity) | ||
clickByText("Sign In") | ||
}.await() | ||
assert(response is ArcGISAuthenticationChallengeResponse.ContinueWithCredential) |
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
@@ -18,5 +18,6 @@ | |||
--> | |||
|
|||
<manifest xmlns:android="http://schemas.android.com/apk/res/android"> | |||
<uses-permission android:name="android.permission.INTERNET" /> |
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 toolkit should not set any permissions in its manifest. This is the responsibility of the user.
}.getOrElse { | ||
if (it is OperationCancelledException) { | ||
return ArcGISAuthenticationChallengeResponse.Cancel | ||
} else { | ||
return ArcGISAuthenticationChallengeResponse.ContinueAndFailWithError(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.
I don't think this change is needed. Any exception thrown, would be handled in the SDK's AuthenticationChallengeQueue
. In the test code, since we bypass that code, we should just catch the exception there.
Thanks @gunt0001 I believe I have addressed all of your comments |
packagingOptions { | ||
exclude("META-INF/LICENSE-notice.md") | ||
exclude("META-INF/LICENSE.md") | ||
} |
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.
I think that change is correct.
@Test | ||
fun successfulSignIn() = runTest { | ||
val response = testOAuthChallengeWithStateRestoration { | ||
InstrumentationRegistry.getInstrumentation().context.startActivity(getSuccessfulRedirectIntent("kotlin-authentication-test-1://auth")) |
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.
Could you add a comment here, something along the lines of...
InstrumentationRegistry.getInstrumentation().context.startActivity(getSuccessfulRedirectIntent("kotlin-authentication-test-1://auth")) | |
// When the OAuth sign in dialog shows up, we simulate successful sign in by launching an intent with the redirect URl, which otherwise would be sent from the Portal server, but would require valid credentials | |
InstrumentationRegistry.getInstrumentation().context.startActivity(getSuccessfulRedirectIntent("kotlin-authentication-test-1://auth")) |
): Deferred<Result<ArcGISAuthenticationChallengeResponse>> { | ||
ArcGISEnvironment.configureArcGISHttpClient { | ||
interceptTokenRequests() | ||
} |
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.
I suggest to move this part of setting up an interceptor out of this function and call this individually as part of each test function. I don't think it belongs into this function here.
oAuthUserConfiguration = OAuthUserConfiguration( | ||
"https://arcgis.com", | ||
// This client ID is for demo purposes only. For use of the Authenticator in your own app, | ||
// // create your own client ID. For more info see: https://developers.arcgis.com/documentation/mapping-apis-and-services/security/tutorials/register-your-application/ |
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.
Comment needs formating
// // create your own client ID. For more info see: https://developers.arcgis.com/documentation/mapping-apis-and-services/security/tutorials/register-your-application/ | |
// create your own client ID. For more info see: https://developers.arcgis.com/documentation/mapping-apis-and-services/security/tutorials/register-your-application/ |
modifier = Modifier.testTag("Authenticator") | ||
) | ||
} | ||
// isse the OAuth challenge |
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.
// isse the OAuth challenge | |
// issue the OAuth challenge |
* | ||
* @since 200.5.0 | ||
*/ | ||
fun ArcGISHttpClient.Builder.interceptTokenRequests() { |
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.
nitpick naming suggestion:
fun ArcGISHttpClient.Builder.interceptTokenRequests() { | |
fun ArcGISHttpClient.Builder.setupTokenRequestInterceptor() { |
* | ||
* @since 200.5.0 | ||
*/ | ||
fun Request.getFakeTokenResponse(): Response = |
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.
nitpick naming suggestion:
fun Request.getFakeTokenResponse(): Response = | |
fun Request.createFakeTokenResponse(): Response = |
addHeader("transfer-encoding", "chunked") | ||
addHeader("vary", "X-Esri-Authorization") | ||
addHeader("x-content-type-options", "nosniff") | ||
}.build() |
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.
missing newline
@@ -0,0 +1,80 @@ | |||
package com.arcgismaps.toolkit.authentication |
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.
missing copyright info
* found. | ||
* | ||
* @param packageId the view's package Id to wait for. | ||
* @since 200.4.0 |
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.
Needs updating also in other places in this file...
* @since 200.4.0 | |
* @since 200.5.0 |
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.
Approving so this can move to second review once comments have been addressed.
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.
@hud10837 Looks good, just a few minor comments to consider
oAuthUserConfiguration = OAuthUserConfiguration( | ||
"https://arcgis.com", | ||
// This client ID is for demo purposes only. For use of the Authenticator in your own app, | ||
// create your own client ID. For more info see: https://developers.arcgis.com/documentation/mapping-apis-and-services/security/tutorials/register-your-application/ |
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.
Newline before the URL, to avoid line being unnecessarily long?
oAuthUserConfiguration = OAuthUserConfiguration( | ||
"https://arcgis.com/", | ||
// This client ID is for demo purposes only. For use of the Authenticator in your own app, | ||
// create your own client ID. For more info see: https://developers.arcgis.com/documentation/mapping-apis-and-services/security/tutorials/register-your-application/ |
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.
Newline before the URL?
|
||
|
||
/** | ||
* Waits for the View with the matching package to be visible. Throws an error if the view can't be |
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.
package -> [packageId]
and delete the @param
below?
* | ||
* @since 200.5.0 | ||
*/ | ||
fun UiDevice.enterCredentialsOnBrowser(username: String, password: String, activity: Activity) { |
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.
Seems this function, and the functions below that it calls, isn't used, but I guess you expect to use it later when we come up with a solution for test credentials in the toolkit?
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.
I will remove them for now, can always return to this PR to copy them.
Oh, one other thing - there are no tests for the following sub-cases, but I guess you have good reasons:
|
@alan-edi thanks for the review. Regarding those test cases:
|
@hud10837 I'm happy with what you say re test cases 1.2, 1.5 and 1.6, thanks |
Adds tests for OAuth sign in. This covers the manual test cases 1 and 2. Like the
ServerTrustTests
, the test logic is generally implemented in a single util function that is called by each test along with the corresponding test action (eg. enter credentials and sign in)I found that because we are calling
handleArcGISAuthenticationChallenge
directly, the tests were catching anyOperationCancelledException
s that were thrown by that method. It seems like this was an oversight so I have changed the implementation and it seems to be working (at least, the tests are passing)Currently the
signIn
tests will fail because they don't have working credentials until we come up with a solution for test credentials in the toolkit.This also uses the microapp's oauth configuration as well as the sample repo's oauth configuration in order to avoid a launcher disambiguation dialog in the tests. This is something we should confirm with @shubham7109 is okay or we should create new redirect urls and clientIds.The tests use two new OAuth clientId/redirectUrls.