fix(auth): fix remaining nullability in UserAuthorizer and Builder - #14158
Conversation
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces nullability annotations, adds non-null preconditions to several builder methods in UserAuthorizer, fixes a typo, and prevents a potential NullPointerException when storing credentials with a null expiration time. It also adds corresponding unit tests. The review feedback identifies a regression in setPKCEProvider where the newly added non-null precondition prevents unsetting or disabling PKCE, and suggests restoring the nullable behavior.
…th/oauth2/UserAuthorizer.java Co-authored-by: gemini-code-assist[bot] <176961590+gemini-code-assist[bot]@users.noreply.github.com>
| @@ -836,8 +837,7 @@ public URI getTokenServerUri() { | |||
| * | |||
| * @return The refresh token, or null if not granted. | |||
| */ | |||
| @Nullable | |||
| public String getRefreshToken() { | |||
| public @Nullable String getRefreshToken() { | |||
There was a problem hiding this comment.
Turns out Oauth2Utils#validateOptionalString can return null if it doesn't exist. So these two fields use it and it can be null
| this.clientId = Preconditions.checkNotNull(builder.clientId); | ||
| this.clientSecret = builder.clientSecret; | ||
| this.accessToken = builder.accessToken; | ||
| this.httpTransportFactory = builder.httpTransportFactory; | ||
| this.tokenServerUri = builder.tokenServerUri; | ||
| this.accessToken = Preconditions.checkNotNull(builder.accessToken); | ||
| this.httpTransportFactory = Preconditions.checkNotNull(builder.httpTransportFactory); | ||
| this.tokenServerUri = Preconditions.checkNotNull(builder.tokenServerUri); |
There was a problem hiding this comment.
these are passed in from UserAuthorizer which enforces them to be non-null
There was a problem hiding this comment.
TokenResponseWithConfig#Builder's setters are all package-private and internal usages of this ensures that clientId, accessToken, httpTransportFactory, and tokenServerUri are non-null (default value is created)
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request updates UserAuthorizer by adding @Nullable annotations, fixing typos, preventing integer overflow in expiration calculations, and introducing new unit tests. The reviewer identified a critical issue: omitting expiration_time_millis when storing credentials with a null access token will cause a NullPointerException or validation failure during loading, as the loading logic now unboxes this value into a primitive long. To resolve this, the reviewer suggests making expiration_time_millis optional during retrieval and expanding the test suite to verify that credentials with a null access token can be successfully loaded back.
…gleapis/google-cloud-java into fix_auth_user_authorizer_nullability
| Map<String, Object> detail = (Map<String, Object>) rawDetail; | ||
| String clientId = OAuth2Utils.validateString(detail, FIELD_CLIENT_ID, JSON_PARSE_ERROR); | ||
| if (clientId == null || clientId.length() == 0) { | ||
| if (clientId.isEmpty()) { |
There was a problem hiding this comment.
OAuth2Utils.validateString ensures that it's non-null
| if (expiresBy != null) { | ||
| tokenStateJson.put("expiration_time_millis", expiresBy.getTime()); | ||
| } |
There was a problem hiding this comment.
we can't ensure that expiresBy is non-null because accessToken may null here
2 participants
Fix remaining nullability annotations, builder defaults, and safe token storage in
UserAuthorizer.UserAuthorizer.Builderfields@Nullableso unconfigured fields default to null beforebuild()resolves defaults.UserAuthorizer.Buildersetters.UserAuthorizer.storeCredentials, safely guard expiration time checking to avoid NullPointerException whencredentials.getAccessToken()is null.TokenResponseWithConfigto markclientSecretas@Nullablefor public OAuth clients, and markrefreshTokenreturn type as@Nullable.UserAuthorizerTestcoveringtoBuilder(), builder defaults,TokenResponseWithConfig, andstoreCredentialswith null access token.