chore: authentication deep link login#9100
Conversation
|
New build in progress, come back later! |
…ogin' into chore/authentication/deep-link-login
…ogin' into chore/authentication/deep-link-login
popuz
added
force-build
|
Warnings count reduced: 19087 => 14668 Warnings/errors in files changed by this PR (418)
…and 368 more (see the |
popuz
changed the title
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
Ludmilafantaniella
left a comment
Ludmilafantaniella
left a comment
There was a problem hiding this comment.
Tested on Windows and Mac. ✅
- Deep-link login flow works end-to-end: browser redirects back to the running Explorer without opening a second Launcher window ✅
- Login with Metamask and Google both complete correctly ✅
- Login cancellation returns to the login selection screen and retry works ✅
- Second sign-in after cancellation still receives the deep link ✅
- Web3 operations (Donations, Gifts) still work correctly after refactored socket signature path ✅
- Multi-instance flow (
--multi-instance) working on both platforms ✅ - Gifts working on both platforms ✅
- Found a bug during testing: a second Launcher instance was opening when switching accounts instead of reusing the existing one — fixed by Nico and verified on both platforms ✅
LGTM 🚀
Windows Evidence:
9100-evi.mp4
Mac Evidence:
9100-evi.mac.mp4
DONATIONS EVIDENCE:
donations.mp4
GIFT EVIDENCE:
gift.mp4
Co-authored-by: claude[bot] <209825114+claude[bot]@users.noreply.github.com> Signed-off-by: Nicolas Lorusso <56365551+lorux0@users.noreply.github.com>
This comment has been minimized.
This comment has been minimized.
Co-authored-by: claude[bot] <209825114+claude[bot]@users.noreply.github.com> Signed-off-by: Nicolas Lorusso <56365551+lorux0@users.noreply.github.com>
|
Claude finished @lorux0's task in 2m 6s —— View job PR Review: Authentication Deep Link Login (re-review after latest commits)
New commit since last review
Step 2 — Root-cause check ✅
Step 3 — Design & integration ✅Architecture is unchanged. Step 4 — Member audit ✅No new public members introduced by Step 5 — Line-level review ✅All prior blocking issues confirmed resolved:
No new blocking issues found. REVIEW_RESULT: PASS ✅ 💰 Review cost: $0.56 · |
decentraland-bot
left a comment
decentraland-bot
left a comment
There was a problem hiding this comment.
Automated review
STEP 1 — Context loaded: CLAUDE.md, docs/README.md, docs/web3-authentication.md, docs/app-arguments.md, docs/async-programming.md, docs/mvc.md, the PR diff and changed files.
STEP 2 — Root-cause check: FAIL. The PR addresses the browser → Launcher → Explorer handoff, but introduces shared mutable signin state and logging of raw auth deeplinks. Those are root-cause issues in the new auth lifecycle, not just local style concerns.
STEP 3 — Design & integration: FAIL. New long-lived auth state is owned through BootstrapContainer.DeeplinkSigninIdentityId / DeeplinkLoginAwaitingSigninRequestId; the existing owners are the Web3 auth provider (BootstrapContainer.CreateWeb3Dependencies, CompositeWeb3Provider, DappDeepLinkAuthenticator) and runtime deep-link routing (DeepLinkHandle / DeepLinkSentinel from DynamicWorldContainer). The new flow should be serialized and auth-owned rather than writable shared bootstrap state.
STEP 4 — Member audit: LoginAsync and Dispose implement IWeb3Authenticator; DTO fields are consumed only inside FetchIdentityByIdAsync; DeepLinkHandle.HandleDeepLink is consumed by the sentinel. No public API consumer break found beyond the intentional authenticator replacement.
STEP 5 — Findings:
- P1:
DappDeepLinkAuthenticator.LoginAsyncis not serialized; concurrent login attempts share and cleardeeplinkSigninIdentityId/loginAwaitingSigninRequestId. - P1: Runtime deeplink logs print raw
signin/authRequestIdvalues. Application argument logging also prints parsed app args, so a process launched withsigninwould expose the same auth material. - P2:
identityIdfrom the deeplink is interpolated into the auth API path without format validation or path-segment encoding.
STEP 6 — Complexity assessment: COMPLEX.
STEP 7 — QA assessment: YES.
STEP 8 — Non-blocking warnings: none.
Security review: security issues found (raw signin/authRequestId logging and unvalidated auth path input). CI status: all reported checks are passing.
REVIEW_RESULT: FAIL ❌
COMPLEXITY: COMPLEX
COMPLEXITY_REASON: Changes runtime authentication, deep-link routing, browser opening, app arguments, and Web3 identity retrieval.
QA_REQUIRED: YES
Reviewed by Jarvis 🤖 · Requested by Ignacio Mazzara (<@U9ETM8CJH>) via Slack
# Conflicts: # Explorer/Assets/DCL/NetworkDefinitions/Browser/IWebBrowser.cs
decentraland-bot
left a comment
decentraland-bot
left a comment
There was a problem hiding this comment.
PR Review — chore: authentication deep link login
STEP 2 — Root-cause check
This PR replaces the legacy socket-based Dapp wallet verification-code sign-in with a browser deep-link flow (AUTH_DEEPLINK_FLOW). The change addresses the root cause directly — the old socket + verification-code mechanism is being replaced by a fundamentally different (and simpler) protocol where the browser fires an OS deep link (decentraland://?signin={identityId}) that the client picks up. PASS.
STEP 3 — Design & integration
Responsibility split: The diff cleanly separates login (now DappDeepLinkAuthenticator) from Ethereum API operations (now DappWeb3EthereumApi, renamed from DappWeb3Authenticator). CompositeWeb3Provider delegates login to DappDeepLinkAuthenticator and eth operations to DappWeb3EthereumApi. The legacy socket login survives only inside the Default nested class for Archipelago dev playgrounds. This is the right decomposition.
Owner search — DappDeepLinkAuthenticator:
- Created in
BootstrapContainer.CreateWeb3Dependencies(composition root). - Owned by
CompositeWeb3Providerwhich callsDispose()in its ownDispose(). - No existing owner can host this logic — it is a new authentication mechanism with its own lifecycle.
- PASS — correctly placed.
Owner search — ReactiveProperty<string?> coordination (DeeplinkSigninIdentityId, DeeplinkLoginAwaitingSigninRequestId):
- Live on
BootstrapContainer, shared betweenDappDeepLinkAuthenticator(consumer/writer of the request ID, reader of the identity ID) andDeepLinkHandle(reader of the request ID, writer of the identity ID). - These cross-cut the deep-link subsystem (polling layer) and the web3 auth subsystem.
BootstrapContaineris the common ancestor that wires both — correct placement. - PASS.
IWebBrowser interface removal: The IWebBrowser interface had a single implementation (UnityAppWebBrowser) and no test stubs. Per CLAUDE.md §11 ("Interfaces with one implementation and no test coverage — delete the interface"), removing it is correct. The rename OpenUrl → OpenUrlMainThreadOnly clarifies the threading contract. PASS.
IDappVerificationHandler removal: The entire code-verification flow is deleted. The interface, the VerificationRequired event, the ICodeVerificationFeatureFlag, and the WaitForCodeVerificationAsync method are all removed. The IdentityVerificationDappAuthState is renamed to IdentityVerificationDappDeepLinkAuthState and no longer drives a verification screen — it sets AuthStatus.VerificationRequested immediately for analytics continuity. PASS.
Teardown / consumption trace:
DappDeepLinkAuthenticator.WaitForSigninAsync:deeplinkSigninIdentityIdsubscription viaUseCurrentValueAndSubscribeToUpdate→ properly disposed viausing var subscription. ✓loginAwaitingSigninRequestId→ cleared infinallyblock ofWaitForSigninAsync. ✓deeplinkSigninIdentityId→ cleared infinallyblock ofWaitForSigninAsyncAND at the start ofLoginAsync. ✓DappDeepLinkAuthenticator.loginMutex(DCLSemaphoreSlim) → NOT disposed inDispose(). See P2 finding below.DeepLinkSentinel.TryDeleteBridgeFile→ wrapped in try/catch, logs error on failure. ✓SatelliteController.OnClickedGenesisCityLinksubscription (view.OnClickedGenesisCityLink += ...) → no unsubscription, but this is pre-existing and unchanged by this PR.
STEP 4 — Member audit
| New/changed public member | Consumers | Verdict |
|---|---|---|
DappDeepLinkAuthenticator.LoginAsync |
CompositeWeb3Provider.currentAuthenticator (1) |
Interface implementation — OK |
DappDeepLinkAuthenticator.Dispose() |
CompositeWeb3Provider.Dispose() (1) |
Empty body — see P2 finding |
DappWeb3EthereumApi.DisconnectFromAuthApiAsync (was private) |
CompositeWeb3Provider.LogoutAsync (1) |
Correctly promoted to public for logout path |
ICompositeWeb3Provider.LogoutAsync |
Auth flow (1) | Moved from IWeb3Authenticator — only CompositeWeb3Provider has meaningful logout |
BootstrapContainer.DeeplinkSigninIdentityId |
CreateWeb3Dependencies, DynamicWorldContainer (2) |
Coordination property — OK |
BootstrapContainer.DeeplinkLoginAwaitingSigninRequestId |
CreateWeb3Dependencies, DynamicWorldContainer (2) |
Coordination property — OK |
DeepLinkHandleResult enum |
DeepLinkSentinel, DeepLinkHandle, DeepLinkHandleImplementation (3) |
Replaces Result — cleaner semantics |
STEP 5 — Line-level findings
See inline comments below.
Security review
- Secrets & credentials: No hardcoded secrets. Auth API URLs from
IDecentralandUrlsSource. ✓ - Input validation: Deep link parameters parsed via
DeepLink.FromJsonwith error handling. New regex inProcessDeepLinkParametersstrips host segments safely — tested with 3 unit tests. ✓ - Auth flow: Client-generated GUID request IDs for correlation (not security — server validates identity).
authRequestIdmatching prevents stale/foreign deep links from completing a login. ✓ - IP validation: Server-side 403 for IP mismatch — client handles with clear
DeeplinkSigninRetrievalException. ✓ - Ephemeral key handling: Private key fetched from auth server over HTTPS, used only in memory for
EthECKeyconstruction. Ephemeral key validated against auth chain before use. ✓ - Bridge file: Pre-existing disk-based mechanism. File is read/deleted with proper error handling.
DEFERRED_SIGNIN_LIFETIME(300s) prevents indefinite retention. ✓ #if UNITY_EDITORguard:&bridgeOnlyflag prevents standalone build from stealing signin during editor testing. ✓
No security issues found.
STEP 6 — Complexity
COMPLEX — modifies auth/web3 signing, dependency injection wiring (BootstrapContainer, CompositeWeb3Provider), deep link handling, authentication state machine, and removes/renames interfaces used across assemblies.
STEP 7 — QA assessment
QA_REQUIRED: YES — changes the user-facing wallet sign-in flow, deep link handling, and web3 operation paths (donations, gifts).
STEP 8 — Non-blocking warnings
None. Main scene not modified.
STEP 9 — Verdict
REVIEW_RESULT: PASS ✅
COMPLEXITY: COMPLEX
COMPLEXITY_REASON: Modifies auth/web3 signing flow, dependency injection wiring (BootstrapContainer, CompositeWeb3Provider), deep link handling architecture, and authentication state machine
QA_REQUIRED: YES
Reviewed by Jarvis 🤖 · Requested by mikhail-dcl via GitHub
|
🔍 Jarvis reviewed this PR and found no blocking issues, but assessed it as complex — human DEV review is still required before merging. |
…appDeepLinkAuthenticator.cs Co-authored-by: Muna <44584806+decentraland-bot@users.noreply.github.com> Signed-off-by: Nicolas Lorusso <56365551+lorux0@users.noreply.github.com>
# Conflicts: # Explorer/Assets/DCL/Passport/Modules/Creations/CreationsDetailsPassportModuleController.cs # Explorer/Assets/DCL/Passport/Modules/EquippedItemsPassportModuleController.cs
lorux0
added
no-warning-ratchet
6 participants
Pull Request Description
What does this PR change?
Replaces the legacy socket-based Dapp wallet sign-in with a new deep-link sign-in flow (
AUTH_DEEPLINK_FLOW).Main files
DappDeepLinkAuthenticator(new) — encapsulated deep-link login: creates a sign-in request on the auth server, opens the browser for the user to sign with their wallet, awaits the OS deep link (decentraland://?signin={identityId}) and resolves the resulting identity from the server.IdentityVerificationDappDeepLinkAuthState(new) — auth screen state driving this flow (reportsVerificationRequestedto keep the analytics funnel intact).DeepLinkSentinel/DeepLinkHandle— architectural change in deep-link consumption: the launcher bridge file is no longer deleted unconditionally.HandleDeepLinknow returns whether the deep link was actually consumed, and asignindeep link is only consumed when a login flow is actively subscribed (DeeplinkSigninDispatcher.HasSubscriber). If nobody is awaiting the sign-in yet, the sentinel leaves the bridge file on disk and retries on the next poll — so a sign-in arriving before the login flow starts is not lost.Supporting changes
DeeplinkSigninDispatcher(new) — single-slot signin bus betweenDeepLinkHandle(producer) and the authenticator (consumer).CompositeWeb3Provider/BootstrapContainerwiring: login is routed through the deep-link authenticator; web3 operations keep using the socket implementation.Removed / renamed
IDappVerificationHandler,IdentityVerificationDappAuthState, code-verification feature-flag machinery and dead DTOs.DappWeb3Authenticator→DappWeb3EthereumApi(files renamed accordingly): it no longer implementsIWeb3Authenticator— in production it only serves signature/confirmation for web3 operations (IEthereumApi). The socket login survives solely for the Archipelago dev playgrounds via the nestedDefault.Follow-up (separate task)
A deeper clean-up of the legacy flow is still pending: besides the playground-only socket login code, unused assets (most likely the old verification screen prefab) need to be removed as well.
Test Instructions
Prerequisites
latest, and place it into%LocalAppData%\DecentralandLauncherLight— the deep-link flow only works when the client is launched from there (OS deep-link routing goes through the light launcher and its bridge file).Test Steps
latestfolder)decentraland://deep link; the client picks it up and completes the login (no verification-code screen anymore).Additional Testing Notes
Quality Checklist
Code Review Reference
Please review our Branch & PR Standards before submitting. It explains the automated review flow, QA/DEV approval requirements, and what each label does — especially useful for first-time contributors.