Conversation
Deploying adr with
|
| Latest commit: |
854b3a7
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://fa9037ec.adr-cvq.pages.dev |
| Branch Preview URL: | https://adr-288.adr-cvq.pages.dev |
LautaroPetaccio
left a comment
LautaroPetaccio
left a comment
Contributor
There was a problem hiding this comment.
Great job! 👍 I've left some comments to review
| participant Server as 🔒 Auth Server | ||
|
|
||
| User->>Client: Presses "Sign In" button | ||
| Client->>Client: Generates session & displays verification code (e.g., "67") |
Contributor
There was a problem hiding this comment.
The client generates the identity, sends it to the auth-server and the auth-server sends back the sessio token
Suggested change
| Client->>Client: Generates session & displays verification code (e.g., "67") | |
| Client->Server: Generates log-in request & displays verification code (e.g., "67") | |
| Server-->>Client: Responds with the request session token |
| - **Multiple context switches**: Users must switch between the Decentraland client and web browser multiple times | ||
| - **Manual verification step**: Users must manually read and confirm the verification code, adding cognitive load | ||
| - **Confusing for new users**: The verification process is not intuitive, especially for first-time users | ||
| - **High entry flow drop rate**: The complex multi-step process results in a high drop-off rate during onboarding |
Contributor
There was a problem hiding this comment.
As this seems to be part of the conclusion, caused by the issues, and it's explained down below, WDYT about removing it?
|
|
||
| ### Deep Link Token Verification Flow | ||
|
|
||
| The key words "MUST", "MUST NOT", "REQUIRED", "SHALL", "SHALL NOT", "SHOULD", "SHOULD NOT", "RECOMMENDED", "NOT RECOMMENDED", "MAY", and "OPTIONAL" in this document are to be interpreted as described in RFC 2119 and RFC 8174. |
Contributor
There was a problem hiding this comment.
This section is defined in ## RFC 2119 and RFC 8174 Would you mind removing it?
|
|
||
| ## Abstract | ||
|
|
||
| This ADR proposes adding a new opt-in alternative to the current verification code mechanism in Decentraland's sign-in flow: a more secure deep link token verification approach. The current system requires users to manually verify a code displayed in the client matches the one shown during web-based social login, which contributes to a high entry flow drop rate and is vulnerable to session hijacking attacks when authentication URLs are shared. The proposed deep link approach uses OS-level protocol handling to ensure tokens are delivered only to the local client, preventing impersonation attacks while significantly improving user experience by eliminating the manual verification step. This streamlined flow aims to reduce user abandonment during onboarding. The existing verification code mechanism will remain as the default authentication method, with the deep link flow available via an explicit `use_token=true` parameter. |
Contributor
There was a problem hiding this comment.
Suggested change
| This ADR proposes adding a new opt-in alternative to the current verification code mechanism in Decentraland's sign-in flow: a more secure deep link token verification approach. The current system requires users to manually verify a code displayed in the client matches the one shown during web-based social login, which contributes to a high entry flow drop rate and is vulnerable to session hijacking attacks when authentication URLs are shared. The proposed deep link approach uses OS-level protocol handling to ensure tokens are delivered only to the local client, preventing impersonation attacks while significantly improving user experience by eliminating the manual verification step. This streamlined flow aims to reduce user abandonment during onboarding. The existing verification code mechanism will remain as the default authentication method, with the deep link flow available via an explicit `use_token=true` parameter. | |
| This ADR proposes adding a new opt-in alternative to the current verification code mechanism in Decentraland's sign-in flow: a more secure deep link token verification approach. The current system requires users to manually verify if a code displayed in the client matches the one shown during web-based social login, which contributes to a high entry flow drop rate and is vulnerable to session hijacking attacks when authentication URLs are shared. The proposed deep link approach uses OS-level protocol handling to ensure tokens are delivered only to the local client, preventing impersonation attacks while significantly improving user experience by eliminating the manual verification step. This streamlined flow aims to reduce user abandonment during onboarding. The existing verification code mechanism will remain as the default authentication method, with the deep link flow available via an explicit `use_token=true` parameter. |
| 8. The AUTH CHAIN enables cryptographic signing capabilities within Decentraland | ||
|
|
||
| ```mermaid | ||
| sequenceDiagram |
Contributor
There was a problem hiding this comment.
I'm not sure if I would keep the mermaid and the bullet point lists, as both of them communicate the same process, whaty do you think about keeping one of them?
|
|
||
| **Status**: This option WILL remain as the default authentication method. The new deep link approach will be available as an opt-in alternative via the `use_token=true` parameter. | ||
|
|
||
| ### Option 2: Deep Link Token Verification (Proposed - To Be Implemented) |
Contributor
There was a problem hiding this comment.
There's also an alternative for this that does not require the server at all, which is to generate the identity on the site, and send it through a deep-link, base64 or URLEncoded. We've tried it before and if I'm not wrong, it will fit the length of deep-links.
| The web application MUST: | ||
| - Present social login options (Google, etc.) | ||
| - Complete the authentication flow with the chosen provider | ||
| - NOT send the AUTH CHAIN immediately after successful login |
Contributor
There was a problem hiding this comment.
Following this implementation, after logging in, the site should send the server the auth-chain to initiate the storage process which will result in the token being returned.
| - Generate a cryptographically secure random token (RECOMMENDED: at least 128 bits of entropy) | ||
| - Store the token temporarily with an association to: | ||
| - The authenticated user identity | ||
| - The original session identifier |
Contributor
There was a problem hiding this comment.
What's the original session identifier?
|
|
||
| For web-based explorers or environments where a native client is not available, alternative authentication mechanisms MUST be provided (outside the scope of this ADR). | ||
|
|
||
| ### Security Considerations |
Contributor
There was a problem hiding this comment.
This section seems to be providing information already explained before, WDYT about removing it?
| 9. Server validates the token and returns the AUTH CHAIN | ||
|
|
||
| ```mermaid | ||
| sequenceDiagram |
Contributor
There was a problem hiding this comment.
Sames as before, as we're already describing the process in the bullet points, what do you think about including only one of them?
Member
Author
|
Superseed by #312 |
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
2 participants
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.
No description provided.