deprecated GoogleGroupChecker constructor seems broken

[pvighi]

I was encountering NullPointer exceptions when updating the play-googleauth version in our project.

I might be doing something wrong but it seems to me like the deprecated GoogleGroupChecker constructor that takes a GoogleServiceAccount doesn't build an equivalent ServiceAccountCredentials properly.

It sets privateKey and serviceAccountUser`:

play-googleauth/play-v27/src/main/scala/com/gu/googleauth/groups.scala

Lines 61 to 69 in d60875f

	def this(googleServiceAccount: GoogleServiceAccount) = {
	this(
	googleServiceAccount.impersonatedUser,
	ServiceAccountCredentials.newBuilder()
	.setPrivateKey(googleServiceAccount.privateKey)
	.setServiceAccountUser(googleServiceAccount.email)
	.build()
	)
	}

But the GoogleServiceAccount class requires clientEmail to be set (see code here )

It might be that we just should be setting clientEmail instead of, or in addition to serviceAccountUser I'm not sure what the difference is exactly.
I just refactored my code to not use this method anyway so It's not blocking anything on my side.

[rtyley]

But the GoogleServiceAccount class requires clientEmail to be set (see code here )

(slightly confusing typo here, I think you mean "ServiceAccountCredentials class requires clientEmail to be set")

That's interesting - it looks like that not-null requirement on clientEmail has been around for years, since 2015:

Good spot -ServiceAccountCredentials does require clientEmail as well as privateKey to be set, and the code in the deprecated GoogleGroupChecker constructor is only setting serviceAccountUser & privateKey - and having had a quick look at the way the builder works, the clientEmail definitely won't be set unless setClientEmail() is called on the builder. The broken code in the deprecated GoogleGroupChecker constructor was introduced by me in December 2020 with #81 . Apologies for not properly trying that out :(

The docs for ServiceAccountCredentials (the modern object we're trying to create) say:

clientEmail – Client email address of the service account from the console.

I guess this more closely matches the definition of our GoogleServiceAccount.email field:

play-googleauth/play-v27/src/main/scala/com/gu/googleauth/groups.scala

Line 22 in d60875f

* @param email email address of the Service Account

...but I had incorrectly been putting that into serviceAccountUser on ServiceAccountCredentials:

serviceAccountUser – Email of the user account to impersonate, if delegating domain-wide authority to the service account.

...that field serviceAccountUser sounds like it more closely matches the definition of our GoogleServiceAccount.impersonatedUser field:

play-googleauth/play-v27/src/main/scala/com/gu/googleauth/groups.scala

Line 24 in d60875f

* @param impersonatedUser the email address of the user the application will be impersonating

So the correct mapping would be:

• GoogleServiceAccount.impersonatedUser -> ServiceAccountCredentials.serviceAccountUser
• GoogleServiceAccount.email -> ServiceAccountCredentials.clientEmail

Although the GoogleServiceAccount constructor of GoogleGroupChecker is deprecated, it's obviously cost you some time and could potentially cost others time, so I guess there are two choices:

1. Fix it, using the updated mapping above
2. Delete it

I'm happy enough to do 1, though I don't think I have a good codebase to test it on - you could perhaps try it on yours, if I release it?

[pvighi]

I don't have anything in production that uses the deprecated method but I can refactor it back to test in CODE if that's good enough