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`: https://github.com/guardian/play-googleauth/blob/d60875f5edf1ad8fa4323a659a35ba02317a4305/play-v27/src/main/scala/com/gu/googleauth/groups.scala#L61-L69

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 https://github.com/guardian/play-googleauth/pull/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:

https://github.com/guardian/play-googleauth/blob/d60875f5edf1ad8fa4323a659a35ba02317a4305/play-v27/src/main/scala/com/gu/googleauth/groups.scala#L22

...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:

https://github.com/guardian/play-googleauth/blob/d60875f5edf1ad8fa4323a659a35ba02317a4305/play-v27/src/main/scala/com/gu/googleauth/groups.scala#L24

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