Skip to content

[devices] public key operations - #258

Open
capcom6 wants to merge 1 commit into
masterfrom
devices/public-key-support
Open

[devices] public key operations#258
capcom6 wants to merge 1 commit into
masterfrom
devices/public-key-support

Conversation

@capcom6

@capcom6 capcom6 commented Jul 28, 2026

Copy link
Copy Markdown
Member

Greptile Summary

This PR adds end-to-end encryption key support to devices: two new nullable columns (public_key, key_version) are added to the devices table via migration, propagated through the GORM model, domain types, service, repository, handlers, and the 3rdparty API response. It also widens message_recipients.phone_number to 512 characters and removes the now-unused ErrInvalidUser sentinel.

  • The SQL migration correctly uses DEFAULT NULL for both columns, and the GORM model tags are consistent with the schema.
  • domain.go comments describe input validation (empty-string rejection and paired-field enforcement) using ErrInconsistentE2E, but that error is not defined in errors.go and no validation code implements the described contract — the comments document planned behaviour that does not yet exist.
  • client-go is upgraded to an untagged pseudo-version commit rather than a stable release, which is inadvisable for a production branch merge.

Confidence Score: 3/5

  • The migration and GORM model are consistent, but the domain layer advertises input validation (via doc-comments) that does not exist in code, leaving the E2E key fields unguarded against empty strings and mismatched pairs.
  • The doc-comments in domain.go explicitly promise that empty PublicKey strings and unpaired PublicKey/KeyVersion fields are rejected, yet ErrInconsistentE2E is not defined and no validation runs in service or repository. Any client sending a bad payload silently persists invalid state. Combined with the ongoing issues from prior review rounds (orphaned users on registration failure, domain errors surfacing as 500s in the mobile handler), the feature is functionally incomplete in a way that could corrupt device E2E state in production.
  • internal/sms-gateway/modules/devices/domain.go and internal/sms-gateway/modules/devices/errors.go need attention: the promised ErrInconsistentE2E sentinel and its enforcement logic are absent. internal/sms-gateway/handlers/mobile.go still needs error mapping for device domain errors.

Important Files Changed

Filename Overview
internal/sms-gateway/modules/devices/domain.go Adds PublicKey/KeyVersion fields to DeviceUpdate with comments that reference ErrInconsistentE2E — an error that is not defined in the package and whose validation logic is not implemented anywhere.
internal/sms-gateway/modules/devices/repository.go Moves error vars to errors.go and adds PublicKey/KeyVersion update paths. PublicKey is stored as *string pointer in the map (consistent with PushToken) while KeyVersion is dereferenced to int — functional but inconsistent.
internal/sms-gateway/modules/devices/errors.go Consolidates error vars (previously split between repository.go and the old errors.go) into a single file, removing ErrInvalidUser entirely. ErrInconsistentE2E is missing despite being referenced in domain comments.
internal/sms-gateway/models/migrations/mysql/20260728000000_add_device_e2e_keys.sql Adds public_key (text, nullable) and key_version (int, DEFAULT NULL) columns to devices, and widens phone_number in message_recipients to 512 chars. SQL uses DEFAULT NULL, consistent with GORM model. Missing trailing newline.
internal/sms-gateway/handlers/mobile.go Passes PublicKey/KeyVersion from request directly into the domain layer for both registration and PATCH. ErrInconsistentE2E (if ever added) would surface as 500 since errorsHandler has no mapping for device domain errors; prior review threads already flagged this.
go.mod Bumps client-go to a pseudo-version pre-release commit (v1.14.5-0.20260810024935-001d392724ab) rather than a stable tagged release, which is inadvisable for production dependencies.
internal/sms-gateway/openapi/docs.go Adds publicKey/keyVersion fields to the Device schema and widens phoneNumber maxLength to 512. The keyVersion has minimum: 1 in the spec but no corresponding server-side enforcement.

Sequence Diagram

sequenceDiagram
    participant App as Android App
    participant MH as Mobile Handler
    participant AS as Auth Service
    participant DS as Devices Service
    participant DR as Devices Repository
    participant DB as MySQL

    Note over App,DB: Device Registration (POST /mobile/v1/device)
    App->>MH: "{name, pushToken, publicKey, keyVersion}"
    MH->>AS: "RegisterDevice(userID, DeviceInfo{...})"
    AS->>DS: Insert(ctx, userID, DeviceInfo)
    DS->>DR: Insert(ctx, DeviceInput)
    DR->>DB: INSERT INTO devices (public_key, key_version, ...)
    DB-->>DR: OK
    DR-->>DS: Device
    DS-->>AS: Device
    AS-->>MH: Device
    MH-->>App: "201 {id, token, login, password}"

    Note over App,DB: Device Update (PATCH /mobile/v1/device)
    App->>MH: "{id, pushToken, publicKey, keyVersion}"
    MH->>DS: "Update(ctx, id, DeviceUpdate{...})"
    DS->>DR: Update(ctx, id, DeviceUpdate)
    DR->>DB: "UPDATE devices SET public_key=?, key_version=? WHERE id=?"
    DB-->>DR: OK
    DR-->>DS: nil
    DS-->>MH: nil
    MH-->>App: 204

    Note over App,DB: List Devices (GET /3rdparty/v1/devices)
    App->>MH: GET /3rdparty/v1/devices
    MH->>DS: Select(ctx, userID)
    DS->>DR: Select(ctx, filters...)
    DR->>DB: "SELECT * FROM devices WHERE user_id=?"
    DB-->>DR: []DeviceModel
    DR-->>DS: []Device (with PublicKey, KeyVersion)
    DS-->>MH: []Device
    MH-->>App: "200 [{..., publicKey, keyVersion}]"
Loading
Prompt To Fix All With AI
### Issue 1
internal/sms-gateway/modules/devices/domain.go:25-35
**`ErrInconsistentE2E` referenced in comments but never defined or enforced**

Both doc-comments say inputs are "rejected with `ErrInconsistentE2E`", but that sentinel is not declared anywhere in the `devices` package (it is absent from `errors.go`), and no validation code in `service.go` or `repository.go` actually rejects an empty `PublicKey` string or a mismatched `PublicKey`/`KeyVersion` pair. The comments therefore describe a contract the implementation does not honour: empty strings and half-populated pairs are silently persisted, leaving devices in an invalid E2E state. The comments should either reflect what the code currently does, or the missing error sentinel and validation logic should be added.

---

For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.

Reviews (20): Last reviewed commit: "[devices] public key operations" | Re-trigger Greptile

Greptile also left 1 inline comment on this PR.

@github-actions

github-actions Bot commented Jul 28, 2026

Copy link
Copy Markdown

🤖 Pull request artifacts

Platform File
🐳 Docker GitHub Container Registry
🍎 Darwin arm64 server_Darwin_arm64.tar.gz
🍎 Darwin x86_64 server_Darwin_x86_64.tar.gz
🐧 Linux arm64 server_Linux_arm64.tar.gz
🐧 Linux i386 server_Linux_i386.tar.gz
🐧 Linux x86_64 server_Linux_x86_64.tar.gz
🪟 Windows arm64 server_Windows_arm64.zip
🪟 Windows i386 server_Windows_i386.zip
🪟 Windows x86_64 server_Windows_x86_64.zip

@capcom6
capcom6 force-pushed the devices/public-key-support branch from 1c556ac to f8364a1 Compare July 29, 2026 07:50
@capcom6
capcom6 marked this pull request as ready for review July 29, 2026 07:50
Comment thread internal/sms-gateway/modules/devices/repository.go
Comment thread internal/sms-gateway/openapi/docs.go Outdated
Comment thread internal/sms-gateway/modules/devices/models.go Outdated
@capcom6
capcom6 force-pushed the devices/public-key-support branch from 7b74cd5 to 0bd4583 Compare July 30, 2026 00:29
@greptile-apps

greptile-apps Bot commented Jul 30, 2026

Copy link
Copy Markdown

Want your agent to iterate on Greptile's feedback? Try greploops.

@capcom6
capcom6 force-pushed the devices/public-key-support branch from 0bd4583 to 3d10ade Compare July 31, 2026 02:16
@capcom6
capcom6 force-pushed the devices/public-key-support branch from 4303779 to b167711 Compare August 1, 2026 01:21
Comment thread internal/sms-gateway/modules/devices/service.go Outdated
@capcom6

capcom6 commented Aug 2, 2026

Copy link
Copy Markdown
Member Author

@greptile review

@capcom6
capcom6 force-pushed the devices/public-key-support branch 3 times, most recently from 0883119 to 90b1780 Compare August 4, 2026 07:39
@capcom6 capcom6 added the ready label Aug 4, 2026
@capcom6
capcom6 force-pushed the devices/public-key-support branch from 90b1780 to 7b24780 Compare August 5, 2026 01:01
@github-actions github-actions Bot removed the ready label Aug 5, 2026
@capcom6 capcom6 added the ready label Aug 5, 2026
@github-actions github-actions Bot removed the ready label Aug 7, 2026
@capcom6
capcom6 force-pushed the devices/public-key-support branch 2 times, most recently from 5e03d03 to 04f32ec Compare August 10, 2026 02:55
Comment thread internal/sms-gateway/handlers/mobile.go
@capcom6
capcom6 force-pushed the devices/public-key-support branch from 5649879 to 8b37237 Compare August 11, 2026 02:13
Comment on lines +25 to +35
// PublicKey is a base64-encoded RSA public key (nil if no E2E).
// Setting a new key together with KeyVersion overwrites the previous key;
// clearing an existing key is intentionally unsupported. On insert, nil
// means the device is created without E2E; on update, a both-nil pair is
// a no-op that leaves the existing key unchanged. An empty string is
// rejected with ErrInconsistentE2E.
PublicKey *string
// KeyVersion is the key version used for rotation tracking (nil if no
// E2E). It must always be set together with PublicKey: providing exactly
// one of the two is rejected with ErrInconsistentE2E.
KeyVersion *int

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P1 ErrInconsistentE2E referenced in comments but never defined or enforced

Both doc-comments say inputs are "rejected with ErrInconsistentE2E", but that sentinel is not declared anywhere in the devices package (it is absent from errors.go), and no validation code in service.go or repository.go actually rejects an empty PublicKey string or a mismatched PublicKey/KeyVersion pair. The comments therefore describe a contract the implementation does not honour: empty strings and half-populated pairs are silently persisted, leaving devices in an invalid E2E state. The comments should either reflect what the code currently does, or the missing error sentinel and validation logic should be added.

Prompt To Fix With AI
This is a comment left during a code review.
Path: internal/sms-gateway/modules/devices/domain.go
Line: 25-35

Comment:
**`ErrInconsistentE2E` referenced in comments but never defined or enforced**

Both doc-comments say inputs are "rejected with `ErrInconsistentE2E`", but that sentinel is not declared anywhere in the `devices` package (it is absent from `errors.go`), and no validation code in `service.go` or `repository.go` actually rejects an empty `PublicKey` string or a mismatched `PublicKey`/`KeyVersion` pair. The comments therefore describe a contract the implementation does not honour: empty strings and half-populated pairs are silently persisted, leaving devices in an invalid E2E state. The comments should either reflect what the code currently does, or the missing error sentinel and validation logic should be added.

---

For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant