Add wrapperIdentifier for callers to self-id in logs - #623
Closed
w-goog wants to merge 17 commits into
Closed
Conversation
…erences * Adds the `gidwrapper` logging-parameter key. * Adds a process-global, thread-safe, sanitized wrapper-identifier store with GIDWrapperIdentifier() / GIDSetWrapperIdentifier() accessors. * No emit sites wired yet; no behavior change.
…dentifier * Adds the public `GIDSignIn.wrapperIdentifier` property (backed by the GIDSignInPreferences global). * Emits `gidwrapper` on the authorization request, the token exchange/refresh, and the revoke URL (percent-encoded), guarded so nothing changes when unset. * Accessors are explicitly annotated nullable to match the header property inside the file NS_ASSUME_NONNULL region.
* The wrapper identifier now accompanies cold-start refreshes, matching the other logging-parameter emit sites.
* Adds nine tests covering round-trip, lowercasing, allowlist filtering, whitespace trimming, empty/invalid input, the 32-character cap, and last-write-wins. * Adds a tearDown that clears the process-global identifier between tests.
* testWrapperIdentifier_PresentInAuthorizationRequestWhenSet * testWrapperIdentifier_AbsentFromAuthorizationRequestWhenUnset * testWrapperIdentifier_PropertyReflectsSanitizedValue * testWrapperIdentifier_PercentEncodedOnRevokeURL — added, modelled on the existing testDisconnect_accessToken revoke flow and the isFetcherStarted/fetchedURL helpers. * Extends tearDown to clear the process-global identifier between tests.
* testWrapperIdentifier_PresentOnRefreshRequestWhenSet * testWrapperIdentifier_AbsentOnRefreshRequestWhenUnset * Captures the OIDTokenRequest in the existing performTokenRequest swizzle so the refresh requests additionalParameters can be asserted; no test file had inspected the request before. * Extends tearDown to clear the process-global identifier between tests.
* Rejects invalid identifiers rather than lowercasing, filtering and truncating them, so a malformed value cannot be silently misattributed to a registered wrapper. * Narrows the accepted charset to [a-z0-9-] to remove multiple spellings of the same name. * First valid write wins; a differing second write is ignored and asserted in debug builds, so multi-wrapper apps report deterministically. * Replaces the dispatch_once lock object with a static os_unfair_lock. * Converts the free C functions GIDVersion/GIDEnvironment/GIDWrapperIdentifier/ GIDSetWrapperIdentifier into GIDSignInPreferences class methods, matching the class idiom already used for the server accessors. * Adds +addLoggingParameters: ahead of consolidating the four emit sites.
… parameters * Replaces stringWithFormat query assembly in the revoke path with NSURLComponents/NSURLQueryItem, making query injection structurally impossible for every parameter rather than filtered for one. * Drops the manual percent-encoding, which was unreachable given the validator. * Routes all three emit sites, including revoke, through +[GIDSignInPreferences addLoggingParameters:]; query items are sorted by name so the emitted URL is deterministic. * Reports kGIDSignInErrorCodeUnknown if the revoke URL cannot be built, rather than signing out and reporting success while the token is still live server-side. * Updates the property accessors to call the GIDSignInPreferences class methods. * Rewrites the wrapperIdentifier doc comment to state naming policy instead of sanitizer mechanics.
* Replaces the fourth copy of the logging-parameter block with +[GIDSignInPreferences addLoggingParameters:]; behavior is unchanged.
* Replaces the "sanitized" wording with the accepted format and the reject-rather- than-reshape behavior.
Deletes nine obsolete tests that asserted the old normalizing behavior: Lowercases, StripsDisallowedCharacters, KeepsAllowedPunctuation, TrimsWhitespace, CapsAtThirtyTwoCharacters, LastWriteWins, EmptyOrWhitespaceBecomesNil, RoundTripsSimpleValue, UnsetReturnsNil. Adds fifteen tests for the new contract: four acceptance cases, six rejection cases, and five covering write-once, nil-reset and rejected-write-preserves-value. Rejection cases are included. The debug NSAssert raises NSInternalInconsistencyException, so each invalid write is wrapped in XCTAssertThrowsSpecificNamed and followed by an assertion that the store was left untouched. This couples the suite to a debug build, which is how the unit tests already run. Also updates testGIDVersion/testGIDEnvironment to the new +[GIDSignInPreferences sdkVersion] / +environment class methods.
* Replaces the vacuous testWrapperIdentifier_PercentEncodedOnRevokeURL, which substring-matched "gidwrapper=my-sdk" with an input percent-encoding never altered, so the assertion could not fail. The percent-encoding it was named for is gone. * Adds testWrapperIdentifier_PresentOnRevokeURL and testWrapperIdentifier_AbsentFromRevokeURLWhenUnset, which parse the captured URL with NSURLComponents and assert on queryItems. Both also assert gpsdk, gidenv and the token parameter survive, guarding the NSURLComponents rewrite against dropping them. * Replaces testWrapperIdentifier_PropertyReflectsSanitizedValue with testWrapperIdentifier_InvalidValueIsIgnored, asserting the rejected assignment throws and leaves the property nil. * Adds a local valueForQueryItemName:inArray: helper. * Updates the pre-existing gpsdk/gidenv assertions to the new +[GIDSignInPreferences sdkVersion] / +environment class methods. * No test skipped; the existing revoke capture harness was reused throughout.
* Adds testWrapperIdentifier_AbsentOnRefreshRequestWhenInvalid, using the invalid-value variant: the rejected write is asserted to throw NSInternalInconsistencyException and gidwrapper is then absent from the refresh request. * Also asserts gpsdk and gidenv ARE still present, which is the point of the test — all three parameters now come from +[GIDSignInPreferences addLoggingParameters:], so a mistake in that consolidation would drop them together. * The two existing wrapper refresh tests are unchanged; both values remain valid.
Header declarations use `///` with a summary line, a blank `///` line, detail, and `@param` tags, matching GIDSignIn_Private.h. Identifiers and literals are backticked.
w-goog
force-pushed
the
feature/add-sdkwrapper-identifier
branch
from
August 14, 2026 01:59
7715d2b to
b15b891
Compare
Contributor
Author
|
Oops, this should land later, and not be merged into the refactor. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
A small number of clients bundle GoogleSignIn-iOS with their own SDKs. To ensure that the way we understand adoption of the SDK is accurate, we'd like to allow those bundlers to say "Hi, this clientID may be App123, but I, Bundler0, am handling it on their behalf".
This is entirely optional and represents no functional change to the way authorization requests are handled on the backend.