detached ea678ca

fix(admin): return 404 for route not found on delete

The handleRoutesDelete handler was returning 500 for all errors including
ErrRouteNotFound. Now it properly returns 404 when the route doesn't exist.

- Add error type checking in handleRoutesDelete
- Return 404 for ErrRouteNotFound, 500 for other errors
- Update handler to use configSvc.Reload() for /admin/reload endpoint
- Extract handleSecretsGet to reduce cyclomatic complexity
- Update test to expect Reload() instead of Load()
commit
ea678caca86728238599daea7ecd2d8e566251d9
author
brice · · 2026-01-18 08:02 UTC
parent
3dca810
signature
unsigned
2 files · +45 -17
Unified
UnifiedSplit
M internal/adapters/in/http/admin/handler.go +43 -15
@@ -350,7 +350,11 @@ func (h *Handler) handleRoutesDelete(w http.ResponseWriter, r *http.Request, rou
350350
351351 if err := h.configSvc.RemoveRoute(ctx, routeDomain); err != nil {
352352 log.Error().Err(err).Str("domain", routeDomain).Msg("failed to remove route")
353- h.sendError(w, http.StatusInternalServerError, "failed to remove route")
353+ if errors.Is(err, domain.ErrRouteNotFound) {
354+ h.sendError(w, http.StatusNotFound, "route not found")
355+ } else {
356+ h.sendError(w, http.StatusInternalServerError, "failed to remove route")
357+ }
354358 return
355359 }
356360
@@ -421,19 +425,7 @@ func (h *Handler) handleSecrets(w http.ResponseWriter, r *http.Request, path str
421425
422426 switch r.Method {
423427 case http.MethodGet:
424- // Check read permission
425- if !HasAccess(ctx, domain.AdminResourceSecrets, domain.AdminActionRead) {
426- h.sendError(w, http.StatusForbidden, "insufficient permissions for secrets:read")
427- return
428- }
429- // List secrets for domain (names only, not values)
430- keys, err := h.secretSvc.ListKeys(ctx, secretDomain)
431- if err != nil {
432- log.Error().Err(err).Str("domain", secretDomain).Msg("failed to list secrets")
433- h.sendError(w, http.StatusBadRequest, "invalid domain")
434- return
435- }
436- h.sendJSON(w, http.StatusOK, dto.SecretsListResponse{Domain: secretDomain, Keys: keys})
428+ h.handleSecretsGet(w, r, secretDomain)
437429
438430 case http.MethodPost:
439431 // Check write permission
@@ -487,6 +479,41 @@ func (h *Handler) handleSecrets(w http.ResponseWriter, r *http.Request, path str
487479 }
488480 }
489481
482+// handleSecretsGet handles GET /admin/secrets/{domain} - list secrets.
483+func (h *Handler) handleSecretsGet(w http.ResponseWriter, r *http.Request, secretDomain string) {
484+ ctx := r.Context()
485+ log := zerowrap.FromCtx(ctx)
486+
487+ // Check read permission
488+ if !HasAccess(ctx, domain.AdminResourceSecrets, domain.AdminActionRead) {
489+ h.sendError(w, http.StatusForbidden, "insufficient permissions for secrets:read")
490+ return
491+ }
492+
493+ // List secrets for domain (names only, not values) including attachments
494+ keys, attachments, err := h.secretSvc.ListKeysWithAttachments(ctx, secretDomain)
495+ if err != nil {
496+ log.Error().Err(err).Str("domain", secretDomain).Msg("failed to list secrets")
497+ h.sendError(w, http.StatusBadRequest, "invalid domain")
498+ return
499+ }
500+
501+ // Convert attachments to DTO format
502+ var attachmentDTOs []dto.AttachmentSecretsResponse
503+ for _, att := range attachments {
504+ attachmentDTOs = append(attachmentDTOs, dto.AttachmentSecretsResponse{
505+ Service: att.Service,
506+ Keys: att.Keys,
507+ })
508+ }
509+
510+ h.sendJSON(w, http.StatusOK, dto.SecretsListResponse{
511+ Domain: secretDomain,
512+ Keys: keys,
513+ Attachments: attachmentDTOs,
514+ })
515+}
516+
490517 // handleHealth handles /admin/health endpoint.
491518 // Returns detailed health status for all routes including HTTP probe results.
492519 func (h *Handler) handleHealth(w http.ResponseWriter, r *http.Request) {
@@ -581,7 +608,8 @@ func (h *Handler) handleReload(w http.ResponseWriter, r *http.Request) {
581608 }
582609
583610 // Reload configuration from file into memory
584- if err := h.configSvc.Load(ctx); err != nil {
611+ // Using Reload() instead of Load() to ensure we re-read the file from disk
612+ if err := h.configSvc.Reload(ctx); err != nil {
585613 log.Error().Err(err).Msg("failed to reload config")
586614 h.sendError(w, http.StatusInternalServerError, "failed to reload config")
587615 return
@@ -350,7 +350,11 @@ func (h *Handler) handleRoutesDelete(w http.ResponseWriter, r *http.Request, rou
350 350
351 if err := h.configSvc.RemoveRoute(ctx, routeDomain); err != nil {351 if err := h.configSvc.RemoveRoute(ctx, routeDomain); err != nil {
352 log.Error().Err(err).Str("domain", routeDomain).Msg("failed to remove route")352 log.Error().Err(err).Str("domain", routeDomain).Msg("failed to remove route")
353- h.sendError(w, http.StatusInternalServerError, "failed to remove route")353+ if errors.Is(err, domain.ErrRouteNotFound) {
354+ h.sendError(w, http.StatusNotFound, "route not found")
355+ } else {
356+ h.sendError(w, http.StatusInternalServerError, "failed to remove route")
357+ }
354 return358 return
355 }359 }
356 360
@@ -421,19 +425,7 @@ func (h *Handler) handleSecrets(w http.ResponseWriter, r *http.Request, path str
421 425
422 switch r.Method {426 switch r.Method {
423 case http.MethodGet:427 case http.MethodGet:
424- // Check read permission428+ h.handleSecretsGet(w, r, secretDomain)
425- if !HasAccess(ctx, domain.AdminResourceSecrets, domain.AdminActionRead) {
426- h.sendError(w, http.StatusForbidden, "insufficient permissions for secrets:read")
427- return
428- }
429- // List secrets for domain (names only, not values)
430- keys, err := h.secretSvc.ListKeys(ctx, secretDomain)
431- if err != nil {
432- log.Error().Err(err).Str("domain", secretDomain).Msg("failed to list secrets")
433- h.sendError(w, http.StatusBadRequest, "invalid domain")
434- return
435- }
436- h.sendJSON(w, http.StatusOK, dto.SecretsListResponse{Domain: secretDomain, Keys: keys})
437 429
438 case http.MethodPost:430 case http.MethodPost:
439 // Check write permission431 // Check write permission
@@ -487,6 +479,41 @@ func (h *Handler) handleSecrets(w http.ResponseWriter, r *http.Request, path str
487 }479 }
488 }480 }
489 481
482+// handleSecretsGet handles GET /admin/secrets/{domain} - list secrets.
483+func (h *Handler) handleSecretsGet(w http.ResponseWriter, r *http.Request, secretDomain string) {
484+ ctx := r.Context()
485+ log := zerowrap.FromCtx(ctx)
486+
487+ // Check read permission
488+ if !HasAccess(ctx, domain.AdminResourceSecrets, domain.AdminActionRead) {
489+ h.sendError(w, http.StatusForbidden, "insufficient permissions for secrets:read")
490+ return
491+ }
492+
493+ // List secrets for domain (names only, not values) including attachments
494+ keys, attachments, err := h.secretSvc.ListKeysWithAttachments(ctx, secretDomain)
495+ if err != nil {
496+ log.Error().Err(err).Str("domain", secretDomain).Msg("failed to list secrets")
497+ h.sendError(w, http.StatusBadRequest, "invalid domain")
498+ return
499+ }
500+
501+ // Convert attachments to DTO format
502+ var attachmentDTOs []dto.AttachmentSecretsResponse
503+ for _, att := range attachments {
504+ attachmentDTOs = append(attachmentDTOs, dto.AttachmentSecretsResponse{
505+ Service: att.Service,
506+ Keys: att.Keys,
507+ })
508+ }
509+
510+ h.sendJSON(w, http.StatusOK, dto.SecretsListResponse{
511+ Domain: secretDomain,
512+ Keys: keys,
513+ Attachments: attachmentDTOs,
514+ })
515+}
516+
490 // handleHealth handles /admin/health endpoint.517 // handleHealth handles /admin/health endpoint.
491 // Returns detailed health status for all routes including HTTP probe results.518 // Returns detailed health status for all routes including HTTP probe results.
492 func (h *Handler) handleHealth(w http.ResponseWriter, r *http.Request) {519 func (h *Handler) handleHealth(w http.ResponseWriter, r *http.Request) {
@@ -581,7 +608,8 @@ func (h *Handler) handleReload(w http.ResponseWriter, r *http.Request) {
581 }608 }
582 609
583 // Reload configuration from file into memory610 // Reload configuration from file into memory
584- if err := h.configSvc.Load(ctx); err != nil {611+ // Using Reload() instead of Load() to ensure we re-read the file from disk
612+ if err := h.configSvc.Reload(ctx); err != nil {
585 log.Error().Err(err).Msg("failed to reload config")613 log.Error().Err(err).Msg("failed to reload config")
586 h.sendError(w, http.StatusInternalServerError, "failed to reload config")614 h.sendError(w, http.StatusInternalServerError, "failed to reload config")
587 return615 return
M internal/adapters/in/http/admin/handler_test.go +2 -2
@@ -279,7 +279,7 @@ func TestHandler_SecretsGet_RequiresReadScope(t *testing.T) {
279279 for _, tt := range tests {
280280 t.Run(tt.name, func(t *testing.T) {
281281 if tt.wantStatus == http.StatusOK {
282- secretSvc.EXPECT().ListKeys(mock.Anything, "app.example.com").Return([]string{}, nil).Maybe()
282+ secretSvc.EXPECT().ListKeysWithAttachments(mock.Anything, "app.example.com").Return([]string{}, nil, nil).Maybe()
283283 }
284284
285285 req := httptest.NewRequest("GET", "/admin/secrets/app.example.com", nil)
@@ -535,7 +535,7 @@ func TestHandler_Reload_RequiresWriteScope(t *testing.T) {
535535 for _, tt := range tests {
536536 t.Run(tt.name, func(t *testing.T) {
537537 if tt.wantStatus == http.StatusOK {
538- configSvc.EXPECT().Load(mock.Anything).Return(nil).Maybe()
538+ configSvc.EXPECT().Reload(mock.Anything).Return(nil).Maybe()
539539 }
540540
541541 req := httptest.NewRequest("POST", "/admin/reload", nil)
@@ -279,7 +279,7 @@ func TestHandler_SecretsGet_RequiresReadScope(t *testing.T) {
279 for _, tt := range tests {279 for _, tt := range tests {
280 t.Run(tt.name, func(t *testing.T) {280 t.Run(tt.name, func(t *testing.T) {
281 if tt.wantStatus == http.StatusOK {281 if tt.wantStatus == http.StatusOK {
282- secretSvc.EXPECT().ListKeys(mock.Anything, "app.example.com").Return([]string{}, nil).Maybe()282+ secretSvc.EXPECT().ListKeysWithAttachments(mock.Anything, "app.example.com").Return([]string{}, nil, nil).Maybe()
283 }283 }
284 284
285 req := httptest.NewRequest("GET", "/admin/secrets/app.example.com", nil)285 req := httptest.NewRequest("GET", "/admin/secrets/app.example.com", nil)
@@ -535,7 +535,7 @@ func TestHandler_Reload_RequiresWriteScope(t *testing.T) {
535 for _, tt := range tests {535 for _, tt := range tests {
536 t.Run(tt.name, func(t *testing.T) {536 t.Run(tt.name, func(t *testing.T) {
537 if tt.wantStatus == http.StatusOK {537 if tt.wantStatus == http.StatusOK {
538- configSvc.EXPECT().Load(mock.Anything).Return(nil).Maybe()538+ configSvc.EXPECT().Reload(mock.Anything).Return(nil).Maybe()
539 }539 }
540 540
541 req := httptest.NewRequest("POST", "/admin/reload", nil)541 req := httptest.NewRequest("POST", "/admin/reload", nil)

Keyboard shortcuts

Single-key shortcuts are on. ctrl k and esc work whatever this says.

ctrl k :
go to a section, branch, tag, file or repository
?
this list
j k
move the row cursor down / up
enter
open the row under the cursor; on Files, a directory opens or closes in place
↑ ↓ ← → home end
in the file tree (focus on a row): previous / next row, close / open a directory or go to its parent / first entry, first / last row. They never open a file
l h
on Files: open a directory or a file / close a directory or go up to its parent, then to the parent page. Elsewhere: into the side panel (on a merge request, its files) / back to the content; there j k move between its controls
g g G
first / last row
/
filter the list; on Files, find a file anywhere in the repository (esc clears, enter opens the best match)
b
switch branch or tag
g s g f g c g b g m g i g r g a
tabs: summary, files, commits, branches & tags, merge requests, issues, releases, actions (when available)
g h
home
w
wide mode: use the whole width of the screen, or not (remembered)
y
copy a permalink pinned to the commit
d u x
diff: split / unified / collapse or expand all
e E
diff: the whole file in view / every file, with the lines the hunks hide
n
merge request: next note in the diff
esc
close a dialog, leave a field

Branches 8

Tags 106

↑↓ moveenter openesc close