Skip to content

Credman Helper#115

Merged
wickedcube merged 6 commits into
mainfrom
PGS-migration-helper-credman
Jul 15, 2026
Merged

Credman Helper#115
wickedcube merged 6 commits into
mainfrom
PGS-migration-helper-credman

Conversation

@wickedcube

Copy link
Copy Markdown
Contributor

Credman implementation in Java to support migration from PGS v1 to v2

@gemini-code-assist gemini-code-assist Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Code Review

This pull request replaces the legacy Google Sign-In V2 implementation with a new Credential Manager (CredMan) integration via a Java bridge (CredManBridge.java), updating AuthManager.cs to handle silent and interactive sign-in flows and manage session caching. The review feedback highlights several critical issues: a shadowed webClientId variable in AuthManager.cs that will fail authentication, an unhandled SilentFailed error that causes a soft-lock, invalid '#' comments and a non-existent hasResolution() method in the Java bridge that will cause compilation failures, and potential thread leaks from creating new executors on every sign-in call.

Important

The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.

Comment on lines +392 to +398
string webClientId = "";
AndroidJavaClass unityPlayer = new AndroidJavaClass("com.unity3d.player.UnityPlayer");
AndroidJavaObject currentActivity = unityPlayer.GetStatic<AndroidJavaObject>("currentActivity");
AndroidJavaClass bridge = new AndroidJavaClass("com.gamesamples.trivialkartunity.CredManBridge");

googleTaskComplete = true;
string methodName = interactive ? "signInInteractive" : "signInSilent";
bridge.CallStatic(methodName, currentActivity, webClientId);

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

critical

The local variable webClientId is declared as an empty string "", which shadows the class field webClientId (defined at line 61). This causes an empty client ID to be passed to the Java bridge, which will fail to authenticate or retrieve the server auth code. We should remove the local declaration and use the class field this.webClientId.

        AndroidJavaClass unityPlayer = new AndroidJavaClass("com.unity3d.player.UnityPlayer");
        AndroidJavaObject currentActivity = unityPlayer.GetStatic<AndroidJavaObject>("currentActivity");
        AndroidJavaClass bridge = new AndroidJavaClass("com.gamesamples.trivialkartunity.CredManBridge");
        
        string methodName = interactive ? "signInInteractive" : "signInSilent";
        bridge.CallStatic(methodName, currentActivity, this.webClientId);

Comment on lines +187 to 192
if (!string.IsNullOrEmpty(credManError))
{
// --- Success case ---
Debug.Log($"Google Sign-In successful for: {this.googleUser.Email}");
Debug.Log($"Retrieved Server Auth Code. Sending to backend...");
statusText.text = "Connecting to game server...";
StartCoroutine(ExchangeAuthcodeAndLink(authCodeToExchange));
Debug.LogError("CredMan Error: " + credManError);
statusText.text = "Sign-in Failed.";
ShowStartPanel();
}

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

critical

If silent sign-in fails, OnSignInError is called with "SilentFailed". Currently, this error is ignored and does not trigger the main thread dispatcher to show the start panel. As a result, the game will soft-lock on the loading screen. We should handle "SilentFailed" gracefully by showing the start panel and updating the status text to "Please sign in.".

            if (!string.IsNullOrEmpty(credManError))
            {
                Debug.LogError("CredMan Error: " + credManError);
                statusText.text = credManError == "SilentFailed" ? "Please sign in." : "Sign-in Failed.";
                ShowStartPanel();
            }

Comment on lines +402 to 406
public void OnSignInError(string error)
{
if (error == "SilentFailed") { Debug.Log("Silent failed. Idle."); return; }
credManError = error; googleTaskComplete = true;
}

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

critical

Returning early when error == "SilentFailed" prevents the main thread from being notified of the failure, leading to a soft-lock. We should set credManError and googleTaskComplete for all errors so the main thread can handle them and show the appropriate UI.

    public void OnSignInError(string error) 
    {
        credManError = error;
        googleTaskComplete = true;
    }

Comment thread trivialkart/trivialkart-unity/Assets/Scripts/Utils/CredManBridge.java Outdated
Comment thread trivialkart/trivialkart-unity/Assets/Scripts/Utils/CredManBridge.java Outdated
Comment on lines +34 to +41
public class CredManBridge {

// --- MODE 1: SILENT SIGN-IN (Called on Awake) ---
// Tries to auto-select an authorized account. If it fails, it does NOT show UI.
public static void signInSilent(Context context, String webClientId) {
CredentialManager credentialManager = CredentialManager.create(context);
CancellationSignal cancellationSignal = new CancellationSignal();
Executor executor = Executors.newSingleThreadExecutor();

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

medium

Creating a new single-thread executor on every call to signInSilent (and signInInteractive) is inefficient and can lead to thread leaks since these executors are never shut down. We should define a single static Executor at the class level and reuse it.

public class CredManBridge {

    private static final Executor executor = Executors.newSingleThreadExecutor();

    // --- MODE 1: SILENT SIGN-IN (Called on Awake) ---
    // Tries to auto-select an authorized account. If it fails, it does NOT show UI.
    public static void signInSilent(Context context, String webClientId) {
        CredentialManager credentialManager = CredentialManager.create(context);
        CancellationSignal cancellationSignal = new CancellationSignal();

public static void signInInteractive(Context context, String webClientId) {
CredentialManager credentialManager = CredentialManager.create(context);
CancellationSignal cancellationSignal = new CancellationSignal();
Executor executor = Executors.newSingleThreadExecutor();

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

medium

Since we introduced a shared static executor at the class level, we should remove the local executor instantiation here to avoid shadowing and reuse the shared one.

        // Using the shared static executor

import java.util.concurrent.Executors;


# [START google_signin_credman_example]

@borisf borisf Jun 8, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

We need to follow up with the DAC team, as this code will not compile

Can do a multiline comment (look below) , or a custom doclet (not 100% sure)

/*

[END google_signin_credman_example]

*/

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

This was requested by our tech writing team for linking to DAC

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

The code isn't compiling with the #'s , should it be // (for java comments instead of Python)


Account account = new Account(email, "com.google");
// Requesting GAMES_LITE scope to check for pre-existing V1 grants
List<Scope> requestedScopes = Collections.singletonList(new Scope("https://www.googleapis.com/auth/games_lite"));

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Let's make it a constant

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Same, let's make this a constant

Comment thread trivialkart/trivialkart-unity/Assets/Scripts/Utils/CredManBridge.java Outdated
private GoogleSignInUser googleUser;

// --- NEW VARIABLES for main-thread dispatching ---
// V2 (CredMan) Specific Variables

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

PGS v2 + link

#endif
}

private void Update()
{
#if PGS_V2
// V2 Main Thread Dispatcher

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

PGS v2

@@ -422,8 +432,6 @@ private IEnumerator ExchangeAuthcodeAndLink(string serverAuthCode)
Debug.LogError($"Backend Error: {request.error}");
Debug.LogError($"Response: {request.downloadHandler.text}");
statusText.text = "Failed to link account. Server error.";

GoogleSignIn.DefaultInstance.SignOut();
ShowStartPanel();
}

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Not sure about this, mismatched braces?

@wickedcube
wickedcube requested a review from patm1987 June 9, 2026 08:54
import android.util.Log;
import android.os.CancellationSignal;

import androidx.credentials.CredentialManager;

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Dependencies are missing for credential manager.

You can add a Dependencies.xml with the missing bits in an Editor folder or modify gradle directly. I think you need

implementation("androidx.credentials:credentials:1.6.0")
implementation("androidx.credentials:credentials-play-services-auth:1.6.0")
implementation("com.google.android.libraries.identity.googleid:googleid:1.2.0")

@patm1987 patm1987 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I wasn't able to build as is, I think it's some small things I called out

@wickedcube
wickedcube requested review from borisf and patm1987 June 30, 2026 17:38
interactiveRequest,
cancellationSignal,
executor,
new androidx.credentials.CredentialManagerCallback<GetCredentialResponse, GetCredentialException>() {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Lets' do an import for androidx.credentials to get the following

CredentialManagerCallback<GetCredentialResponse, GetCredentialException>() { ...


public class CredManBridge {

private static final Executor executor = Executors.newSingleThreadExecutor();

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Who is closing the executor

ExecutorService executor = Executors.newSingleThreadExecutor();
try {
executor.submit(() -> System.out.println("Processing task..."));
} finally {
executor.shutdown(); // Stops accepting new tasks, lets current ones finish
}

@borisf borisf left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Thanks

@wickedcube
wickedcube requested a review from borisf July 12, 2026 11:23

@borisf borisf left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Thanks

@wickedcube
wickedcube requested a review from borisf July 12, 2026 15:53

@borisf borisf left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Thanks

@patm1987

Copy link
Copy Markdown
Contributor

Running in Editor, I'm getting warnings for both FriendsPage and LeaderboardPage, that scripts are missing. Double check me that I don't just have a stale client, but I think this was left over from commit 209221d when FriendsPageController.cs was deleted and you can probably delete these nodes in the scene.

I tried doing that in a commit, but that added a lot of changes in TrivialKartScene.unity and I want to make sure I don't move that scene to a different Unity version than you're using.

@wickedcube

Copy link
Copy Markdown
Contributor Author

LGTM from Boris and Patrick

@wickedcube
wickedcube merged commit c06fcc3 into main Jul 15, 2026
7 checks passed
@wickedcube
wickedcube deleted the PGS-migration-helper-credman branch July 15, 2026 21:15
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.

3 participants