mirror of
https://github.com/navidrome/navidrome.git
synced 2026-10-21 08:47:16 +02:00
feat(subsonic): OpenSubsonic API key authentication (#6219)
* feat(persistence): store hashed API keys on players * feat(core): refresh key-bound players without renaming them Add Players.Touch, which records usage for a player already identified by an API key without guessing its identity or overwriting its name. Register also stops renaming players that have an API key. Register no longer returns player save errors (or a stale FindMatch ErrNotFound when the save is rate-limited); save failures are only logged, and only the transcoding lookup error is returned, same as Touch. * feat(subsonic): authenticate with OpenSubsonic API keys Co-authored-by: amCap1712 <amCap1712@users.noreply.github.com> * feat(subsonic): add tokenInfo and advertise apiKeyAuthentication * feat(server): add endpoints to generate and revoke player API keys * feat(ui): manage player API keys Co-authored-by: amCap1712 <amCap1712@users.noreply.github.com> * fix(subsonic): throttle API keys per key and IP A stale key on one device exhausted the shared per-IP bucket and locked out every valid key from the same IP. The limiter only stores a hash of the bucket string, so the key is not retained. Also adds e2e coverage of API key auth through the real repository, and clarifies the player resolution log message. * fix(ui): keep the new API key dialog open until closed The key is shown only once, so Escape and backdrop clicks no longer dismiss it. Also clarifies when the key can be used as a password. * refactor: simplify API key code paths Share the player refresh tail between Register and Touch, fold the ownership-filtered write tail into execOwned, parse the query once for apiKey conflicts, derive HasAPIKey in the player mock, share the player form inputs between create and edit, and pick the delete button by key state instead of spreading conditional props. * feat(players): set API keys through the player record The key is a write-only apiKey field applied on save: required and owner-only on create, optional on edit, empty to revoke. Replaces the generate/revoke endpoints. * fix(players): reject API keys already in use Creating or editing a player with a key another player already has now returns a validation error instead of a 500, and a create that loses the race no longer leaves a keyless player behind. Ownership is checked before the key on create. * feat(ui): edit player API keys as a form field Replaces the show-once dialog, whose icon-less Close button was invisible on mobile. The key is generated in the browser, required and pre-filled on create. * fix(ui): keep new player API keys out of the record cache The json-server create response echoes the request body, and undoable edits merge the payload into the cache, so the key could reappear on the edit page. Strip it from the create result and save player edits pessimistically. Also fall back to a prompt when the clipboard write fails. * fix(ui): polish player API key field Set userId on the created player record so owner actions show immediately, and show a neutral no-key message to non-owners. * refactor: simplify player API key create and field Write the key hash in the create INSERT so the unique index settles races, re-read the created player instead of hand-building the cached record, reuse isWritable for the revoke check, and collapse the key field's derived state and generate/regenerate buttons. * fix(ui): let the API key field size like other inputs fullWidth is now opt-in instead of forced. * fix(ui): align the API key field with other player inputs Apply react-admin's input className, move the actions (now including Copy) below the field, and use a monospace font so the whole key fits. * fix(ui): redirect to the player list after create Matches the other create pages. * refactor(persistence): name the write-access rule for owned rows Owned-row writes now say which row they target and who may write it: ownedRow(rowID, ownerOrAdmin|ownerOnly) builds the WHERE, updateOwnedRow applies it, and SetAPIKey uses ownerOnly instead of a hand-built user_id filter. updateOwned/deleteOwned keep their signatures. * fix(players): apply an edit's key change and fields atomically Update now runs SetAPIKey and the column update in one transaction. Also shares the key format check, drops FindByAPIKey's unneeded empty-key guard, and sets the context username only on the apiKey path. * fix(subsonic): treat any credential param sent with apiKey as a conflict The spec requires error 43 when u, p, t or s is present with apiKey, even with an empty value. * refactor(subsonic): leave the player cookie code unchanged for key-bound requests Return early instead of wrapping the cookie block, so the diff (and CodeQL's view of it) matches master. * fix(subsonic): don't count key lookup errors as failed logins A database error while checking a key sent as the password now surfaces as a server error instead of a bad password, so it no longer feeds the failed-login limiter. * feat(players): use nds_ as the API key prefix Part of a Navidrome secret prefix family (nd + a letter for the kind), alongside ndg_ for API v1 grants. * feat(ui): make player API keys easier to find Label the Settings menu entry "Players & API keys", add an API key filter to the player list, show the key icon in the mobile list, and add Brazilian Portuguese translations for the new player strings. Signed-off-by: Deluan <deluan@navidrome.org> * feat(ui): always show the player API key filter Signed-off-by: Deluan <deluan@navidrome.org> * fix(ui): hide the unset Last Seen date in the player list Players created by hand have no last_seen yet, which showed as 12/31/1. Signed-off-by: Deluan <deluan@navidrome.org> --------- Signed-off-by: Deluan <deluan@navidrome.org> Co-authored-by: amCap1712 <amCap1712@users.noreply.github.com>
This commit is contained in:
Deluan Quintão
GitHub
parent
ce484083bf
commit
4cdffd5633
42 changed files with 1582 additions and 100 deletions
|
|
@ -2,10 +2,16 @@ package persistence
|
|||
|
||||
import (
|
||||
"context"
|
||||
"crypto/sha256"
|
||||
"encoding/hex"
|
||||
"regexp"
|
||||
"strings"
|
||||
|
||||
. "github.com/Masterminds/squirrel"
|
||||
"github.com/deluan/rest"
|
||||
"github.com/navidrome/navidrome/consts"
|
||||
"github.com/navidrome/navidrome/model"
|
||||
"github.com/navidrome/navidrome/model/id"
|
||||
"github.com/pocketbase/dbx"
|
||||
)
|
||||
|
||||
|
|
@ -17,7 +23,8 @@ func NewPlayerRepository(db dbx.Builder) model.PlayerRepository {
|
|||
r := &playerRepository{}
|
||||
r.db = db
|
||||
r.registerModel(&model.Player{}, map[string]filterFunc{
|
||||
"name": containsFilter("player.name"),
|
||||
"name": containsFilter("player.name"),
|
||||
"hasapikey": hasAPIKeyFilter,
|
||||
})
|
||||
r.setSortMappings(map[string]string{
|
||||
"user_name": "username", //TODO rename all user_name and userName to username
|
||||
|
|
@ -25,6 +32,13 @@ func NewPlayerRepository(db dbx.Builder) model.PlayerRepository {
|
|||
return r
|
||||
}
|
||||
|
||||
func hasAPIKeyFilter(_ string, value any) Sqlizer {
|
||||
if v, _ := value.(string); strings.EqualFold(v, "true") {
|
||||
return NotEq{"player.api_key_hash": nil}
|
||||
}
|
||||
return Eq{"player.api_key_hash": nil}
|
||||
}
|
||||
|
||||
func (r *playerRepository) Put(ctx context.Context, p *model.Player) error {
|
||||
_, err := r.put(ctx, p.ID, p)
|
||||
return err
|
||||
|
|
@ -32,7 +46,7 @@ func (r *playerRepository) Put(ctx context.Context, p *model.Player) error {
|
|||
|
||||
func (r *playerRepository) selectPlayer(ctx context.Context, options ...model.QueryOptions) SelectBuilder {
|
||||
return r.newSelect(ctx, options...).
|
||||
Columns("player.*").
|
||||
Columns("player.*", "player.api_key_hash is not null as has_api_key").
|
||||
Join("user ON player.user_id = user.id").
|
||||
Columns("user.user_name username")
|
||||
}
|
||||
|
|
@ -103,32 +117,115 @@ func (r *playerRepository) ReadAll(ctx context.Context, options ...rest.QueryOpt
|
|||
return res, err
|
||||
}
|
||||
|
||||
// isPermitted authorizes creating a new record, based on the owner declared in the request body.
|
||||
// This is only safe for inserts: there is no stored row yet, and a non-admin may only create a
|
||||
// player they own. Updates must not use this (the body owner is attacker-controlled); they go
|
||||
// through updateOwned, which authorizes against the persisted user_id in the WHERE clause.
|
||||
func (r *playerRepository) isPermitted(ctx context.Context, p *model.Player) bool {
|
||||
u := loggedUser(ctx)
|
||||
return u.IsAdmin || p.UserId == u.ID
|
||||
var apiKeyFormat = regexp.MustCompile(`^` + consts.APIKeyPrefix + `[0-9A-Za-z]{22}$`)
|
||||
|
||||
func apiKeyValidationError(msg string) error {
|
||||
return &rest.ValidationError{Errors: map[string]string{"apiKey": msg}}
|
||||
}
|
||||
|
||||
func validateAPIKey(key string) error {
|
||||
if !apiKeyFormat.MatchString(key) {
|
||||
return apiKeyValidationError("resources.player.validation.apiKeyFormat")
|
||||
}
|
||||
return nil
|
||||
}
|
||||
|
||||
func (r *playerRepository) Save(ctx context.Context, t *model.Player) (string, error) {
|
||||
if !r.isPermitted(ctx, t) {
|
||||
u := loggedUser(ctx)
|
||||
if t.UserId == "" && u.ID != invalidUserId {
|
||||
t.UserId = u.ID
|
||||
}
|
||||
if t.UserId != u.ID {
|
||||
return "", rest.ErrPermissionDenied
|
||||
}
|
||||
return r.put(ctx, "", t) // Save only creates; edits go through the owner-scoped Update
|
||||
// Hand-made players are only reachable through a key, so one is required
|
||||
if t.APIKey == nil || *t.APIKey == "" {
|
||||
return "", apiKeyValidationError("ra.validation.required")
|
||||
}
|
||||
if err := validateAPIKey(*t.APIKey); err != nil {
|
||||
return "", err
|
||||
}
|
||||
values, err := toSQLArgs(t)
|
||||
if err != nil {
|
||||
return "", err
|
||||
}
|
||||
// Save only creates, so the key hash goes in the same INSERT and the unique index settles races
|
||||
values["id"] = id.NewRandom()
|
||||
values["api_key_hash"] = hashAPIKey(*t.APIKey)
|
||||
_, err = r.executeSQL(ctx, Insert(r.tableName).SetMap(values))
|
||||
if isUniqueViolation(err) {
|
||||
return "", apiKeyValidationError("ra.validation.unique")
|
||||
}
|
||||
if err != nil {
|
||||
return "", err
|
||||
}
|
||||
return values["id"].(string), nil
|
||||
}
|
||||
|
||||
func (r *playerRepository) Update(ctx context.Context, id string, entity model.Player, cols ...string) error {
|
||||
t := &entity
|
||||
t.ID = id
|
||||
return r.updateOwned(ctx, id, t, cols...)
|
||||
if t.APIKey == nil {
|
||||
return r.updateOwned(ctx, id, t, cols...)
|
||||
}
|
||||
// The key and the other columns are two writes; commit both or neither
|
||||
return r.inTx(func(tx *playerRepository) error {
|
||||
if err := tx.SetAPIKey(ctx, id, *t.APIKey); err != nil {
|
||||
return err
|
||||
}
|
||||
return tx.updateOwned(ctx, id, t, cols...)
|
||||
})
|
||||
}
|
||||
|
||||
func (r *playerRepository) inTx(block func(tx *playerRepository) error) error {
|
||||
conn, ok := r.db.(*dbx.DB)
|
||||
if !ok {
|
||||
return block(r) // already inside a transaction
|
||||
}
|
||||
return conn.Transactional(func(tx *dbx.Tx) error {
|
||||
return block(NewPlayerRepository(tx).(*playerRepository))
|
||||
})
|
||||
}
|
||||
|
||||
func (r *playerRepository) Delete(ctx context.Context, ids ...string) error {
|
||||
return r.deleteOwnedAll(ctx, ids...)
|
||||
}
|
||||
|
||||
// Keys are long random strings, not user-chosen passwords, so a fast unsalted hash is enough and keeps lookups indexed.
|
||||
func hashAPIKey(key string) string {
|
||||
sum := sha256.Sum256([]byte(key))
|
||||
return hex.EncodeToString(sum[:])
|
||||
}
|
||||
|
||||
func (r *playerRepository) FindByAPIKey(ctx context.Context, key string) (*model.Player, error) {
|
||||
sel := r.selectPlayer(ctx).Where(Eq{"player.api_key_hash": hashAPIKey(key)})
|
||||
var res model.Player
|
||||
if err := r.queryOne(ctx, sel, &res); err != nil {
|
||||
return nil, err
|
||||
}
|
||||
return &res, nil
|
||||
}
|
||||
|
||||
// SetAPIKey stores the key's hash, or revokes it when key is empty. Setting is owner-only, even for
|
||||
// admins, so nobody can mint a login for someone else.
|
||||
func (r *playerRepository) SetAPIKey(ctx context.Context, playerID, key string) error {
|
||||
if key == "" {
|
||||
return r.updateOwnedRow(ctx, playerID, ownerOrAdmin, map[string]any{"api_key_hash": nil})
|
||||
}
|
||||
if err := validateAPIKey(key); err != nil {
|
||||
return err
|
||||
}
|
||||
err := r.updateOwnedRow(ctx, playerID, ownerOnly, map[string]any{"api_key_hash": hashAPIKey(key)})
|
||||
if isUniqueViolation(err) {
|
||||
return apiKeyValidationError("ra.validation.unique")
|
||||
}
|
||||
return err
|
||||
}
|
||||
|
||||
func isUniqueViolation(err error) bool {
|
||||
return err != nil && strings.Contains(err.Error(), "UNIQUE constraint failed")
|
||||
}
|
||||
|
||||
var _ model.PlayerRepository = (*playerRepository)(nil)
|
||||
var _ rest.Repository[model.Player] = (*playerRepository)(nil)
|
||||
var _ rest.Persistable[model.Player] = (*playerRepository)(nil)
|
||||
|
|
|
|||
|
|
@ -2,6 +2,7 @@ package persistence
|
|||
|
||||
import (
|
||||
"context"
|
||||
"errors"
|
||||
|
||||
"github.com/deluan/rest"
|
||||
"github.com/navidrome/navidrome/log"
|
||||
|
|
@ -12,6 +13,14 @@ import (
|
|||
"github.com/pocketbase/dbx"
|
||||
)
|
||||
|
||||
const testAPIKey = "nds_0123456789abcdefghijkl"
|
||||
|
||||
func expectAPIKeyError(err error, msg string) {
|
||||
var verr *rest.ValidationError
|
||||
ExpectWithOffset(1, errors.As(err, &verr)).To(BeTrue())
|
||||
ExpectWithOffset(1, verr.Errors).To(HaveKeyWithValue("apiKey", msg))
|
||||
}
|
||||
|
||||
var _ = Describe("PlayerRepository", func() {
|
||||
var adminRepo *playerRepository
|
||||
var database *dbx.DB
|
||||
|
|
@ -178,11 +187,12 @@ var _ = Describe("PlayerRepository", func() {
|
|||
clone := player
|
||||
clone.ID = ""
|
||||
clone.IP = "192.168.1.1"
|
||||
clone.APIKey = new(testAPIKey)
|
||||
id, err := repo.Save(repoCtx, &clone)
|
||||
|
||||
if clone.UserId == "" {
|
||||
Expect(err).To(HaveOccurred())
|
||||
} else if !admin && player.Username == adminPlayer1.Username {
|
||||
} else if player.UserId != userPlayer.UserId {
|
||||
Expect(err).To(Equal(rest.ErrPermissionDenied))
|
||||
clone.UserId = ""
|
||||
} else {
|
||||
|
|
@ -202,12 +212,13 @@ var _ = Describe("PlayerRepository", func() {
|
|||
} else {
|
||||
Expect(count).To(Equal(baseCount + 1))
|
||||
Expect(err).To(BeNil())
|
||||
clone.APIKey = nil
|
||||
clone.HasAPIKey = true
|
||||
Expect(*newItem).To(Equal(clone))
|
||||
}
|
||||
},
|
||||
Entry("same user", userPlayer),
|
||||
Entry("other item", otherPlayer),
|
||||
Entry("fake item", model.Player{}),
|
||||
)
|
||||
})
|
||||
|
||||
|
|
@ -251,6 +262,259 @@ var _ = Describe("PlayerRepository", func() {
|
|||
Entry("regular context", false, model.Players{regularPlayer}, regularPlayer, adminPlayer1),
|
||||
)
|
||||
|
||||
Describe("API keys", func() {
|
||||
const key = testAPIKey
|
||||
const otherKey = "nds_ABCDEFGHIJKLMNOPQRSTUV"
|
||||
var ownerCtx, otherCtx context.Context
|
||||
|
||||
BeforeEach(func() {
|
||||
ownerCtx = request.WithUser(log.NewContext(GinkgoT().Context()), regularUser)
|
||||
otherCtx = request.WithUser(log.NewContext(GinkgoT().Context()), thirdUser)
|
||||
})
|
||||
|
||||
storedHash := func(id string) string {
|
||||
var row struct {
|
||||
Hash string `db:"api_key_hash"`
|
||||
}
|
||||
Expect(database.NewQuery("select coalesce(api_key_hash, '') as api_key_hash from player where id = {:id}").
|
||||
Bind(dbx.Params{"id": id}).One(&row)).To(Succeed())
|
||||
return row.Hash
|
||||
}
|
||||
|
||||
Describe("SetAPIKey", func() {
|
||||
It("stores only the hash and finds the player by the key", func() {
|
||||
Expect(adminRepo.SetAPIKey(ownerCtx, regularPlayer.ID, key)).To(Succeed())
|
||||
|
||||
Expect(storedHash(regularPlayer.ID)).To(Equal(hashAPIKey(key)))
|
||||
plr, err := adminRepo.FindByAPIKey(ctx, key)
|
||||
Expect(err).ToNot(HaveOccurred())
|
||||
Expect(plr.ID).To(Equal(regularPlayer.ID))
|
||||
Expect(plr.HasAPIKey).To(BeTrue())
|
||||
Expect(plr.APIKey).To(BeNil())
|
||||
})
|
||||
|
||||
It("replaces the previous key", func() {
|
||||
Expect(adminRepo.SetAPIKey(ownerCtx, regularPlayer.ID, key)).To(Succeed())
|
||||
Expect(adminRepo.SetAPIKey(ownerCtx, regularPlayer.ID, otherKey)).To(Succeed())
|
||||
|
||||
_, err := adminRepo.FindByAPIKey(ctx, key)
|
||||
Expect(err).To(MatchError(model.ErrNotFound))
|
||||
_, err = adminRepo.FindByAPIKey(ctx, otherKey)
|
||||
Expect(err).ToNot(HaveOccurred())
|
||||
})
|
||||
|
||||
DescribeTable("rejects malformed keys",
|
||||
func(bad string) {
|
||||
err := adminRepo.SetAPIKey(ownerCtx, regularPlayer.ID, bad)
|
||||
expectAPIKeyError(err, "resources.player.validation.apiKeyFormat")
|
||||
Expect(storedHash(regularPlayer.ID)).To(BeEmpty())
|
||||
},
|
||||
Entry("no prefix", "0123456789abcdefghijklmn"),
|
||||
Entry("too short", "nds_short"),
|
||||
Entry("too long", key+"x"),
|
||||
Entry("bad chars", "nds_0123456789abcdefghij-!"),
|
||||
)
|
||||
|
||||
It("revokes with an empty key, by the owner or an admin", func() {
|
||||
Expect(adminRepo.SetAPIKey(ownerCtx, regularPlayer.ID, key)).To(Succeed())
|
||||
Expect(adminRepo.SetAPIKey(ownerCtx, regularPlayer.ID, "")).To(Succeed())
|
||||
Expect(storedHash(regularPlayer.ID)).To(BeEmpty())
|
||||
|
||||
Expect(adminRepo.SetAPIKey(ownerCtx, regularPlayer.ID, key)).To(Succeed())
|
||||
Expect(adminRepo.SetAPIKey(ctx, regularPlayer.ID, "")).To(Succeed())
|
||||
Expect(storedHash(regularPlayer.ID)).To(BeEmpty())
|
||||
})
|
||||
|
||||
It("accepts revoking a player that has no key", func() {
|
||||
Expect(adminRepo.SetAPIKey(ownerCtx, regularPlayer.ID, "")).To(Succeed())
|
||||
})
|
||||
|
||||
It("does not let an admin set a key on another user's player", func() {
|
||||
Expect(adminRepo.SetAPIKey(ctx, regularPlayer.ID, key)).To(MatchError(rest.ErrPermissionDenied))
|
||||
Expect(storedHash(regularPlayer.ID)).To(BeEmpty())
|
||||
})
|
||||
|
||||
It("does not let another user set or revoke", func() {
|
||||
Expect(adminRepo.SetAPIKey(ownerCtx, regularPlayer.ID, key)).To(Succeed())
|
||||
Expect(adminRepo.SetAPIKey(otherCtx, regularPlayer.ID, otherKey)).To(MatchError(rest.ErrPermissionDenied))
|
||||
Expect(adminRepo.SetAPIKey(otherCtx, regularPlayer.ID, "")).To(MatchError(rest.ErrPermissionDenied))
|
||||
Expect(storedHash(regularPlayer.ID)).To(Equal(hashAPIKey(key)))
|
||||
})
|
||||
|
||||
It("returns not found for a missing player", func() {
|
||||
Expect(adminRepo.SetAPIKey(ownerCtx, "missing", key)).To(MatchError(rest.ErrNotFound))
|
||||
Expect(adminRepo.SetAPIKey(ownerCtx, "missing", "")).To(MatchError(rest.ErrNotFound))
|
||||
})
|
||||
|
||||
It("does not find unknown or empty keys", func() {
|
||||
_, err := adminRepo.FindByAPIKey(ctx, otherKey)
|
||||
Expect(err).To(MatchError(model.ErrNotFound))
|
||||
_, err = adminRepo.FindByAPIKey(ctx, "")
|
||||
Expect(err).To(MatchError(model.ErrNotFound))
|
||||
})
|
||||
|
||||
It("drops the key with the player", func() {
|
||||
Expect(adminRepo.SetAPIKey(ownerCtx, regularPlayer.ID, key)).To(Succeed())
|
||||
Expect(adminRepo.Delete(ownerCtx, regularPlayer.ID)).To(Succeed())
|
||||
_, err := adminRepo.FindByAPIKey(ctx, key)
|
||||
Expect(err).To(MatchError(model.ErrNotFound))
|
||||
})
|
||||
})
|
||||
|
||||
Describe("hasApiKey filter", func() {
|
||||
BeforeEach(func() {
|
||||
Expect(adminRepo.SetAPIKey(ownerCtx, regularPlayer.ID, key)).To(Succeed())
|
||||
})
|
||||
|
||||
filtered := func(value string) []string {
|
||||
res, err := adminRepo.ReadAll(ctx, rest.QueryOptions{Filters: map[string]any{"hasApiKey": value}})
|
||||
Expect(err).ToNot(HaveOccurred())
|
||||
var ids []string
|
||||
for _, p := range res {
|
||||
ids = append(ids, p.ID)
|
||||
}
|
||||
return ids
|
||||
}
|
||||
|
||||
It("lists only players with a key", func() {
|
||||
Expect(filtered("true")).To(ConsistOf(regularPlayer.ID))
|
||||
count, err := adminRepo.Count(ctx, rest.QueryOptions{Filters: map[string]any{"hasApiKey": "true"}})
|
||||
Expect(err).ToNot(HaveOccurred())
|
||||
Expect(count).To(Equal(int64(1)))
|
||||
})
|
||||
|
||||
It("lists only players without a key", func() {
|
||||
Expect(filtered("false")).To(ConsistOf(adminPlayer1.ID, adminPlayer2.ID))
|
||||
})
|
||||
})
|
||||
|
||||
Describe("Save (create)", func() {
|
||||
It("creates the player with the key, owned by the logged-in user", func() {
|
||||
id, err := adminRepo.Save(ownerCtx, &model.Player{Name: "Manual player", APIKey: new(key)})
|
||||
Expect(err).ToNot(HaveOccurred())
|
||||
|
||||
plr, err := adminRepo.FindByAPIKey(ctx, key)
|
||||
Expect(err).ToNot(HaveOccurred())
|
||||
Expect(plr.ID).To(Equal(id))
|
||||
Expect(plr.UserId).To(Equal(regularUser.ID))
|
||||
})
|
||||
|
||||
It("requires a key", func() {
|
||||
count, _ := adminRepo.CountAll(ctx)
|
||||
_, err := adminRepo.Save(ownerCtx, &model.Player{Name: "No key"})
|
||||
expectAPIKeyError(err, "ra.validation.required")
|
||||
|
||||
_, err = adminRepo.Save(ownerCtx, &model.Player{Name: "Empty key", APIKey: new("")})
|
||||
expectAPIKeyError(err, "ra.validation.required")
|
||||
Expect(adminRepo.CountAll(ctx)).To(Equal(count))
|
||||
})
|
||||
|
||||
It("rejects a malformed key without creating the player", func() {
|
||||
count, _ := adminRepo.CountAll(ctx)
|
||||
_, err := adminRepo.Save(ownerCtx, &model.Player{Name: "Bad", APIKey: new("nds_bad")})
|
||||
expectAPIKeyError(err, "resources.player.validation.apiKeyFormat")
|
||||
Expect(adminRepo.CountAll(ctx)).To(Equal(count))
|
||||
})
|
||||
|
||||
It("does not let an admin create a keyed player for another user", func() {
|
||||
count, _ := adminRepo.CountAll(ctx)
|
||||
_, err := adminRepo.Save(ctx, &model.Player{Name: "For someone", UserId: regularUser.ID, APIKey: new(key)})
|
||||
Expect(err).To(MatchError(rest.ErrPermissionDenied))
|
||||
_, err = adminRepo.Save(ctx, &model.Player{Name: "For someone", UserId: regularUser.ID})
|
||||
Expect(err).To(MatchError(rest.ErrPermissionDenied))
|
||||
Expect(adminRepo.CountAll(ctx)).To(Equal(count))
|
||||
})
|
||||
|
||||
It("rejects a key already used by another player without creating the player", func() {
|
||||
Expect(adminRepo.SetAPIKey(ctx, adminPlayer1.ID, key)).To(Succeed())
|
||||
count, _ := adminRepo.CountAll(ctx)
|
||||
_, err := adminRepo.Save(ownerCtx, &model.Player{Name: "Duplicate", APIKey: new(key)})
|
||||
expectAPIKeyError(err, "ra.validation.unique")
|
||||
Expect(adminRepo.CountAll(ctx)).To(Equal(count))
|
||||
})
|
||||
})
|
||||
|
||||
Describe("Update (edit)", func() {
|
||||
It("rolls back the key change when the rest of the edit fails", func() {
|
||||
_, err := database.NewQuery(`create trigger fail_player_rename before update of name on player
|
||||
when new.name = 'boom' begin select raise(abort, 'boom'); end`).Execute()
|
||||
Expect(err).ToNot(HaveOccurred())
|
||||
DeferCleanup(func() {
|
||||
_, _ = database.NewQuery("drop trigger if exists fail_player_rename").Execute()
|
||||
})
|
||||
|
||||
plr := regularPlayer
|
||||
plr.Name = "boom"
|
||||
plr.APIKey = new(key)
|
||||
Expect(adminRepo.Update(ownerCtx, plr.ID, plr, "name", "apiKey")).ToNot(Succeed())
|
||||
Expect(storedHash(regularPlayer.ID)).To(BeEmpty())
|
||||
})
|
||||
|
||||
It("keeps the key when apiKey is absent (a normal edit)", func() {
|
||||
Expect(adminRepo.SetAPIKey(ownerCtx, regularPlayer.ID, key)).To(Succeed())
|
||||
|
||||
plr := regularPlayer
|
||||
plr.Name = "Renamed"
|
||||
Expect(adminRepo.Update(ownerCtx, plr.ID, plr, "name", "hasApiKey")).To(Succeed())
|
||||
Expect(adminRepo.Update(ownerCtx, plr.ID, plr)).To(Succeed())
|
||||
|
||||
found, err := adminRepo.FindByAPIKey(ctx, key)
|
||||
Expect(err).ToNot(HaveOccurred())
|
||||
Expect(found.Name).To(Equal("Renamed"))
|
||||
})
|
||||
|
||||
It("sets a new key when apiKey has a value", func() {
|
||||
plr := regularPlayer
|
||||
plr.APIKey = new(key)
|
||||
Expect(adminRepo.Update(ownerCtx, plr.ID, plr, "name", "apiKey")).To(Succeed())
|
||||
Expect(storedHash(regularPlayer.ID)).To(Equal(hashAPIKey(key)))
|
||||
})
|
||||
|
||||
It("revokes the key when apiKey is empty", func() {
|
||||
Expect(adminRepo.SetAPIKey(ownerCtx, regularPlayer.ID, key)).To(Succeed())
|
||||
plr := regularPlayer
|
||||
plr.APIKey = new("")
|
||||
Expect(adminRepo.Update(ownerCtx, plr.ID, plr, "apiKey")).To(Succeed())
|
||||
Expect(storedHash(regularPlayer.ID)).To(BeEmpty())
|
||||
})
|
||||
|
||||
It("lets an admin edit another user's keyed player without touching the key", func() {
|
||||
Expect(adminRepo.SetAPIKey(ownerCtx, regularPlayer.ID, key)).To(Succeed())
|
||||
plr := regularPlayer
|
||||
plr.MaxBitRate = 192
|
||||
Expect(adminRepo.Update(ctx, plr.ID, plr, "maxBitRate", "hasApiKey")).To(Succeed())
|
||||
Expect(storedHash(regularPlayer.ID)).To(Equal(hashAPIKey(key)))
|
||||
})
|
||||
|
||||
It("refuses an admin setting a key on another user's player and leaves other columns alone", func() {
|
||||
plr := regularPlayer
|
||||
plr.Name = "Hijacked"
|
||||
plr.APIKey = new(key)
|
||||
Expect(adminRepo.Update(ctx, plr.ID, plr, "name", "apiKey")).To(MatchError(rest.ErrPermissionDenied))
|
||||
|
||||
got, err := adminRepo.Get(ctx, regularPlayer.ID)
|
||||
Expect(err).ToNot(HaveOccurred())
|
||||
Expect(got.Name).To(Equal(regularPlayer.Name))
|
||||
Expect(storedHash(regularPlayer.ID)).To(BeEmpty())
|
||||
})
|
||||
|
||||
It("refuses a key already used by another player and leaves other columns alone", func() {
|
||||
Expect(adminRepo.SetAPIKey(ctx, adminPlayer1.ID, key)).To(Succeed())
|
||||
plr := regularPlayer
|
||||
plr.Name = "Renamed"
|
||||
plr.APIKey = new(key)
|
||||
err := adminRepo.Update(ownerCtx, plr.ID, plr, "name", "apiKey")
|
||||
expectAPIKeyError(err, "ra.validation.unique")
|
||||
|
||||
got, err := adminRepo.Get(ctx, regularPlayer.ID)
|
||||
Expect(err).ToNot(HaveOccurred())
|
||||
Expect(got.Name).To(Equal(regularPlayer.Name))
|
||||
Expect(storedHash(regularPlayer.ID)).To(BeEmpty())
|
||||
Expect(storedHash(adminPlayer1.ID)).To(Equal(hashAPIKey(key)))
|
||||
})
|
||||
})
|
||||
})
|
||||
|
||||
Describe("Ownership enforcement (cross-tenant write protection)", func() {
|
||||
var regularRepo *playerRepository
|
||||
var regularCtx context.Context
|
||||
|
|
@ -287,6 +551,7 @@ var _ = Describe("PlayerRepository", func() {
|
|||
Name: "HIJACKED",
|
||||
UserId: regularUser.ID,
|
||||
ReportRealPath: true,
|
||||
APIKey: new(testAPIKey),
|
||||
}
|
||||
|
||||
id, err := regularRepo.Save(regularCtx, &spoofed)
|
||||
|
|
|
|||
|
|
@ -85,6 +85,22 @@ func (r sqlRepository) addRestriction(ctx context.Context, sql ...Sqlizer) Sqliz
|
|||
return s
|
||||
}
|
||||
|
||||
// writeAccess says who may change a row in a table with a user_id column.
|
||||
type writeAccess int
|
||||
|
||||
const (
|
||||
ownerOrAdmin writeAccess = iota // admins may write any row
|
||||
ownerOnly // even admins may only write their own rows
|
||||
)
|
||||
|
||||
// ownedRow matches the row rowID only if the logged-in user may write it under access.
|
||||
func (r sqlRepository) ownedRow(ctx context.Context, rowID string, access writeAccess) Sqlizer {
|
||||
if access == ownerOnly {
|
||||
return And{Eq{"id": rowID}, Eq{"user_id": loggedUser(ctx).ID}}
|
||||
}
|
||||
return r.addRestriction(ctx, Eq{"id": rowID})
|
||||
}
|
||||
|
||||
func (r *sqlRepository) registerModel(instance any, filters map[string]filterFunc) {
|
||||
if r.tableName == "" {
|
||||
r.tableName = strings.TrimPrefix(reflect.TypeOf(instance).String(), "*model.")
|
||||
|
|
@ -494,15 +510,12 @@ func (r sqlRepository) updateOwned(ctx context.Context, id string, m any, colsTo
|
|||
}
|
||||
updateValues := filterUpdateValues(values, id, colsToUpdate...)
|
||||
delete(updateValues, "user_id") // ownership is immutable on update
|
||||
update := Update(r.tableName).Where(r.addRestriction(ctx, Eq{"id": id})).SetMap(updateValues)
|
||||
count, err := r.executeSQL(ctx, update)
|
||||
if err != nil {
|
||||
return err
|
||||
}
|
||||
if count == 0 {
|
||||
return r.classifyOwnedWriteMiss(ctx, id)
|
||||
}
|
||||
return nil
|
||||
return r.updateOwnedRow(ctx, id, ownerOrAdmin, updateValues)
|
||||
}
|
||||
|
||||
// updateOwnedRow sets values on the row rowID if the logged-in user may write it under access.
|
||||
func (r sqlRepository) updateOwnedRow(ctx context.Context, rowID string, access writeAccess, values map[string]any) error {
|
||||
return r.runRowWrite(ctx, rowID, Update(r.tableName).SetMap(values).Where(r.ownedRow(ctx, rowID, access)))
|
||||
}
|
||||
|
||||
// deleteOwned performs an atomic, ownership-restricted delete of the row identified by id, for
|
||||
|
|
@ -511,12 +524,17 @@ func (r sqlRepository) updateOwned(ctx context.Context, id string, m any, colsTo
|
|||
// does not match and is left untouched. The failure path mirrors updateOwned (see
|
||||
// classifyOwnedWriteMiss), so there is no TOCTOU on the delete.
|
||||
func (r sqlRepository) deleteOwned(ctx context.Context, id string) error {
|
||||
count, err := r.executeSQL(ctx, Delete(r.tableName).Where(r.addRestriction(ctx, Eq{"id": id})))
|
||||
return r.runRowWrite(ctx, id, Delete(r.tableName).Where(r.ownedRow(ctx, id, ownerOrAdmin)))
|
||||
}
|
||||
|
||||
// runRowWrite executes q, a write already filtered by ownedRow(rowID, …), and classifies a miss.
|
||||
func (r sqlRepository) runRowWrite(ctx context.Context, rowID string, q Sqlizer) error {
|
||||
count, err := r.executeSQL(ctx, q)
|
||||
if err != nil {
|
||||
return err
|
||||
}
|
||||
if count == 0 {
|
||||
return r.classifyOwnedWriteMiss(ctx, id)
|
||||
return r.classifyOwnedWriteMiss(ctx, rowID)
|
||||
}
|
||||
return nil
|
||||
}
|
||||
|
|
|
|||
|
|
@ -19,6 +19,26 @@ var _ = Describe("sqlRepository", func() {
|
|||
r.tableName = "table"
|
||||
})
|
||||
|
||||
Describe("ownedRow", func() {
|
||||
DescribeTable("matches the row, limited to what the logged-in user may write",
|
||||
func(user model.User, access writeAccess, expectedSQL string, expectedArgs ...any) {
|
||||
userCtx := request.WithUser(GinkgoT().Context(), user)
|
||||
sql, args, err := r.ownedRow(userCtx, "row-1", access).ToSql()
|
||||
Expect(err).ToNot(HaveOccurred())
|
||||
Expect(sql).To(Equal(expectedSQL))
|
||||
Expect(args).To(Equal(expectedArgs))
|
||||
},
|
||||
Entry("admin, ownerOrAdmin: any row", model.User{ID: "admin", IsAdmin: true}, ownerOrAdmin,
|
||||
"(id = ?)", "row-1"),
|
||||
Entry("regular, ownerOrAdmin: own rows", model.User{ID: "user"}, ownerOrAdmin,
|
||||
"(id = ? AND user_id = ?)", "row-1", "user"),
|
||||
Entry("admin, ownerOnly: own rows", model.User{ID: "admin", IsAdmin: true}, ownerOnly,
|
||||
"(id = ? AND user_id = ?)", "row-1", "admin"),
|
||||
Entry("regular, ownerOnly: own rows", model.User{ID: "user"}, ownerOnly,
|
||||
"(id = ? AND user_id = ?)", "row-1", "user"),
|
||||
)
|
||||
})
|
||||
|
||||
Describe("applyOptions", func() {
|
||||
var sq squirrel.SelectBuilder
|
||||
BeforeEach(func() {
|
||||
|
|
|
|||
Loading…
Add table
Add a link
Reference in a new issue