Conversation
|
@shridarpatil this would work only with live apps, as what i tested. p.s: tests are really hard, i tried to mock FB Servers alot. :( .. |
|
Plans for future:
|
|
Can you add a video/gif of this? |
Register button for what? |
| webhook_verify_token = "random-string-change-this" | ||
| api_version = "v24.0" | ||
| base_url = "https://graph.facebook.com" | ||
| app_id = "" # Meta App ID for Embedded Signup |
There was a problem hiding this comment.
shouldn't this moved to the database?
Otherwise a single embbed phone number could be registered per whatomate instance
There was a problem hiding this comment.
Nope, this is the tech provider or solution provider app
Which users signup and give him access through the embedded signup
There was a problem hiding this comment.
This is wrong. You shouldn't be using it for a tech provider. This was meant for the WhatsApp account. You should give an option on the UI to add these details without saving them in the database.
Justification for Embedded Signup ImplementationClick to view more thinkingExecutive SummaryThe feedback stating that using "Embedded Signup" is "wrong" or suggesting that configuration details should be input via the UI is inaccurate in the context of our operation as a Tech Provider (Business Solution Provider Model). Our implementation adheres to the official standard recommended by Meta for any platform (SaaS / CRM / Tech Provider) seeking to enable clients to connect their numbers and create WhatsApp Business API accounts directly from within the system. 1. Addressing the Reviewer's FeedbackThe reviewer mentioned:
This feedback likely stems from a misunderstanding of two distinct integration models. The reviewer might be suggesting that App ID and App Secret should be input by the user in the UI instead of being static in the configuration. This is fundamentally incorrect for a Tech Provider. Why User-Input for App Config is Wrong:
Refutation: If we do not store this data, we cannot send messages asynchronously (automation/campaigns). If we ask the user to input App details, we are no longer a Tech Provider but merely a basic API client wrapper, forcing them to do all the technical setup themselves. 2. Code Validation & ComplianceI have reviewed our Backend Code (internal/handlers/accounts.go), and it strictly follows the confirmed standards:
3. Proposed Response to ReviewerReport Title: Justification for Embedded Signup & Configuration Strategy Context: Response regarding "UI Input for Config":
Conclusion: @shridarpatil , you can read more about in meta docs, to use embedded signup is meant to be platform credentials and other people give you access to manage their whatsapps, and to be easy click to give you that access. here is a video too ( phone is pending registration because it's waiting for our biz to be verified as tech provider, but in testing environments it would say success ) Screen.Recording.2026-02-06.at.2.33.15.PM-2.mp4P.S: tests are failing now after merging with master, will fix it on sunday. |
shridarpatil
left a comment
There was a problem hiding this comment.
Review: PR #127 — Feature: Embedded Signup
Overview
This PR adds WhatsApp Embedded Signup flow, allowing users to connect accounts via Facebook OAuth instead of manual credential entry. It spans 12 files (+1859/-13 lines) across backend, frontend, and tests.
Security Issues (Critical)
1. /api/config exposes config publicly (internal/handlers/app.go, cmd/whatomate/main.go)
The /api/config endpoint is added to the public routes bypass list (no auth required). While app_id and config_id aren't strictly secret, exposing a blanket /api/config endpoint publicly sets a bad precedent — any future config additions could accidentally leak sensitive values. Consider:
- Naming it more specifically (e.g.,
/api/embedded-signup/config) - Only returning values when they're non-empty
2. PIN stored in plaintext in DB (internal/models/models.go)
Pin string `gorm:"size:6" json:"pin"`The 2FA PIN is stored as plaintext and returned in API responses. The PIN is also exposed via json:"pin" on the model — meaning any endpoint that serializes a WhatsAppAccount could leak it.
3. PIN returned in API response (internal/handlers/accounts.go)
response["pin"] = account.PinAnd in RegisterPhone:
"pin": pin,The PIN is sent back to the frontend where it's shown in a toast notification for 10 seconds. Consider whether this is necessary after initial setup.
Architecture Issues
4. Config approach (flagged by reviewers)
As noted in the existing review comments — the app_id, app_secret, and config_id are currently in the TOML config file, making this a single-tenant embedded signup setup. For a multi-tenant platform, each organization might want to bring their own Meta App credentials. This is the most significant architectural concern.
5. ExchangeToken does too much (300+ lines)
This single handler does: token exchange, WABA discovery, phone discovery, phone info lookup, account creation/update, auto-registration, webhook subscription, and response building. Consider breaking into smaller functions for readability and testability:
discoverWABAAndPhone()createOrUpdateAccount()attemptAutoRegistration()
6. generateVerifyToken() called but not defined in the diff
Line 490 calls generateVerifyToken() but this function isn't in the diff. If it's defined elsewhere, fine — but it should be verified it exists.
Code Quality Issues
7. Excessive debug logging in production code
Both frontend and backend have heavy console.log/a.Log.Info with [FB_SIGNUP] prefix. These are useful during development but should be cleaned up:
- Frontend: 15+
console.log/console.error/console.warncalls inAccountsView.vue - Backend: 20+ log statements with full request/response dumps including token lengths
8. Hardcoded strings in frontend (AccountsView.vue)
<p class="text-lg font-medium ...">No WhatsApp accounts connected</p>
<p class="text-sm ...">Connect your WhatsApp Business account via Facebook...</p>And:
Connect with FacebookThese should use $t() i18n translations like the rest of the codebase.
9. TestApp_ExchangeToken_MissingFields test has incorrect expectations
The test expects StatusBadRequest for missing phone_id and waba_id, but the handler actually supports discovery mode when these are missing (it proceeds to debug_token flow). Only missing code should return 400. The missing_phone_id and missing_waba_id test cases will hit the mock server which returns 400 for everything, masking the real behavior.
10. RegisterPhone returns 200 with success: false on failure
return r.SendEnvelope(map[string]interface{}{
"success": false,
"error": err.Error(),
})This returns HTTP 200 with an error body, which is inconsistent with the rest of the codebase that uses SendErrorEnvelope with proper HTTP status codes.
11. E2E test change may be premature (AccountsPage.ts)
-this.addButton = page.getByRole('button', { name: /Add Account/i }).first()
+this.addButton = page.getByRole('button', { name: /Manual Entry/i }).first()The "Add Account" button text hasn't actually changed in the template — it still uses {{ $t('accounts.addAccount') }}. The e2e locator change seems based on an unreleased UI change.
12. Modulo bias in PIN generation
b[i] = (b[i] % 10) + '0'Using % 10 on a random byte (0-255) introduces slight bias: digits 0-5 are ~2% more likely than 6-9. Minor for a 6-digit PIN but could use crypto/rand integer generation for correctness.
13. Fallback PIN is deterministic
if _, err := rand.Read(b); err != nil {
return "123456" // Fallback
}If the CSPRNG fails, all accounts get PIN "123456". Better to return an error instead.
Summary
The feature is well-structured with good test coverage (comprehensive mock servers, cross-org isolation tests, error cases). Main concerns:
- Must fix: Hardcoded English strings should use i18n
- Must fix:
RegisterPhoneshould use proper HTTP error codes - Should fix: Clean up debug logging before merge
- Should fix: PIN storage/exposure needs review
- Architectural: The TOML config approach limits multi-tenancy (per existing review comments)
- Consider: Break up the 300-line
ExchangeTokenhandler
|
When can we expect it to be merged into main branch? |
Original PR: Saifallak#6
Feature: WhatsApp Embedded Signup Integration
This PR implements the full Embedded Signup flow for WhatsApp Business Platform, allowing users to connect their accounts directly through the application using their Facebook credentials.
Frontend Changes
authResponse.code), and send it to the backend.Backend Changes
New API Endpoint (
POST /accounts/exchange-token):New API Endpoint (
POST /accounts/{id}/register):Infrastructure (pkg/whatsapp/client.go):
Testing