Add Application Default Credentials & Service Account impersonation #1148
Conversation
|
Did you test this locally? |
|
Yep, we built the plugin locally, and published our Android app successfully. It used my personal Google account set up via the |
|
Just to add, we contemplated some more tests around this but the only "logic" added resulted in a different instantiation of a GoogleCredentials, so it meant several levels of mocking to assert on a type and the logic is fairly simple so it didn't seem like the cost of the test added much value. |
|
Good enough for me, thanks! I'll try to review this before the end of the weekend. |
|
Can you give me edit access to the PR? I'd like to tweak some stuff |
| credentialStream().use { | ||
| PlayPublisher(it, parameters.appId.get()) | ||
| val useAppDefaultCreds = parameters.useApplicationDefaultCredentials.getOrElse(false) | ||
| val useExplicitCreds = (parameters.credentials.asFile.orNull != null || |
There was a problem hiding this comment.
This should fail even if the file doesn't exist.
| val useExplicitCreds = (parameters.credentials.asFile.orNull != null || | |
| val useExplicitCreds = (parameters.credentials.present || |
There was a problem hiding this comment.
Do you mean isPresent??
There was a problem hiding this comment.
Yeah sorry I'm forgetting how Kotlin works 😅
play/plugin/src/main/kotlin/com/github/triplet/gradle/play/tasks/internal/PlayApiService.kt
Show resolved
Hide resolved
| .setSourceCredentials(appDefaultCreds) | ||
| .setTargetPrincipal(impersonateServiceAccount) | ||
| .setScopes(listOf(AndroidPublisherScopes.ANDROIDPUBLISHER)) | ||
| .setLifetime(300) |
There was a problem hiding this comment.
Max this out to 3600. We don't want folks with really long builds getting timed out.
There was a problem hiding this comment.
It seems the tokens are generated as the task is about to make a HTTP call (rather than at the start of a build) and also automatically refreshed if "stale" (i.e. expired or within 5mins of expiry), so the thinking was that this lower value was pretty safe..
But an hour isn't the end of the world so happy to just max it out if preferred...
There was a problem hiding this comment.
Yeah, I don't see how a token living for an hour is going to hurt much more than 5 mins.
|
@SUPERCILEX all done |
|
Thx. Can you give me PR access? https://docs.github.com/en/pull-requests/collaborating-with-pull-requests/working-with-forks/allowing-changes-to-a-pull-request-branch-created-from-a-fork I'd like to clean up some nits |
|
Don't think we can as this is forked into a non-personal repo... The link above seems to allude to it being in personal repos and I don't see the option on this PR either.. Might have to just suggest the clean ups and have us do them? |
|
Ah, sounds good. |
...ndroid-publisher/src/main/kotlin/com/github/triplet/gradle/androidpublisher/PlayPublisher.kt
Outdated
Show resolved
Hide resolved
Fixed those ones, let us know if there's anything else :) |
Context
Usage of Service Account keys is generally discouraged, with service account impersonation being a recommended alternative.
Changes
Allow Application Default Credentials to be used, and support Service Account impersonation with it. This could be used as an alternative to the existing Service Account key authentication. Since it is an alternative method, it should not affect backwards compatibility.
One point of discussion is the default values used to build
ImpersonatedCredentialsinAndroidPublisher.kt. We think these are suitable defaults - hardcoded values for the token lifetime was taken from GCP documentation and seems adequate given that tokens are autogenerated/refreshed by the underlying SDK as HTTP requests are constructed.