mirror of https://github.com/coder/coder.git
chore: correct 500 -> 404 on workspace agent mw (#11129)
* chore: correct 500 -> 404
This commit is contained in:
parent
0181e036f6
commit
dba0dfa859
|
@ -7,6 +7,7 @@ import (
|
||||||
"runtime"
|
"runtime"
|
||||||
"strings"
|
"strings"
|
||||||
"sync"
|
"sync"
|
||||||
|
"sync/atomic"
|
||||||
"testing"
|
"testing"
|
||||||
|
|
||||||
"github.com/google/uuid"
|
"github.com/google/uuid"
|
||||||
|
@ -16,6 +17,7 @@ import (
|
||||||
"golang.org/x/xerrors"
|
"golang.org/x/xerrors"
|
||||||
|
|
||||||
"github.com/coder/coder/v2/coderd"
|
"github.com/coder/coder/v2/coderd"
|
||||||
|
"github.com/coder/coder/v2/coderd/database"
|
||||||
"github.com/coder/coder/v2/coderd/database/dbauthz"
|
"github.com/coder/coder/v2/coderd/database/dbauthz"
|
||||||
"github.com/coder/coder/v2/coderd/rbac"
|
"github.com/coder/coder/v2/coderd/rbac"
|
||||||
"github.com/coder/coder/v2/coderd/rbac/regosql"
|
"github.com/coder/coder/v2/coderd/rbac/regosql"
|
||||||
|
@ -451,3 +453,22 @@ func must[T any](value T, err error) T {
|
||||||
}
|
}
|
||||||
return value
|
return value
|
||||||
}
|
}
|
||||||
|
|
||||||
|
type FakeAccessControlStore struct{}
|
||||||
|
|
||||||
|
func (FakeAccessControlStore) GetTemplateAccessControl(t database.Template) dbauthz.TemplateAccessControl {
|
||||||
|
return dbauthz.TemplateAccessControl{
|
||||||
|
RequireActiveVersion: t.RequireActiveVersion,
|
||||||
|
}
|
||||||
|
}
|
||||||
|
|
||||||
|
func (FakeAccessControlStore) SetTemplateAccessControl(context.Context, database.Store, uuid.UUID, dbauthz.TemplateAccessControl) error {
|
||||||
|
panic("not implemented")
|
||||||
|
}
|
||||||
|
|
||||||
|
func AccessControlStorePointer() *atomic.Pointer[dbauthz.AccessControlStore] {
|
||||||
|
acs := &atomic.Pointer[dbauthz.AccessControlStore]{}
|
||||||
|
var tacs dbauthz.AccessControlStore = FakeAccessControlStore{}
|
||||||
|
acs.Store(&tacs)
|
||||||
|
return acs
|
||||||
|
}
|
||||||
|
|
|
@ -13,6 +13,7 @@ import (
|
||||||
"golang.org/x/xerrors"
|
"golang.org/x/xerrors"
|
||||||
|
|
||||||
"cdr.dev/slog"
|
"cdr.dev/slog"
|
||||||
|
|
||||||
"github.com/coder/coder/v2/coderd/coderdtest"
|
"github.com/coder/coder/v2/coderd/coderdtest"
|
||||||
"github.com/coder/coder/v2/coderd/database"
|
"github.com/coder/coder/v2/coderd/database"
|
||||||
"github.com/coder/coder/v2/coderd/database/dbauthz"
|
"github.com/coder/coder/v2/coderd/database/dbauthz"
|
||||||
|
@ -62,7 +63,7 @@ func TestAsNoActor(t *testing.T) {
|
||||||
func TestPing(t *testing.T) {
|
func TestPing(t *testing.T) {
|
||||||
t.Parallel()
|
t.Parallel()
|
||||||
|
|
||||||
q := dbauthz.New(dbmem.New(), &coderdtest.RecordingAuthorizer{}, slog.Make(), accessControlStorePointer())
|
q := dbauthz.New(dbmem.New(), &coderdtest.RecordingAuthorizer{}, slog.Make(), coderdtest.AccessControlStorePointer())
|
||||||
_, err := q.Ping(context.Background())
|
_, err := q.Ping(context.Background())
|
||||||
require.NoError(t, err, "must not error")
|
require.NoError(t, err, "must not error")
|
||||||
}
|
}
|
||||||
|
@ -74,7 +75,7 @@ func TestInTX(t *testing.T) {
|
||||||
db := dbmem.New()
|
db := dbmem.New()
|
||||||
q := dbauthz.New(db, &coderdtest.RecordingAuthorizer{
|
q := dbauthz.New(db, &coderdtest.RecordingAuthorizer{
|
||||||
Wrapped: &coderdtest.FakeAuthorizer{AlwaysReturn: xerrors.New("custom error")},
|
Wrapped: &coderdtest.FakeAuthorizer{AlwaysReturn: xerrors.New("custom error")},
|
||||||
}, slog.Make(), accessControlStorePointer())
|
}, slog.Make(), coderdtest.AccessControlStorePointer())
|
||||||
actor := rbac.Subject{
|
actor := rbac.Subject{
|
||||||
ID: uuid.NewString(),
|
ID: uuid.NewString(),
|
||||||
Roles: rbac.RoleNames{rbac.RoleOwner()},
|
Roles: rbac.RoleNames{rbac.RoleOwner()},
|
||||||
|
@ -110,8 +111,8 @@ func TestNew(t *testing.T) {
|
||||||
|
|
||||||
// Double wrap should not cause an actual double wrap. So only 1 rbac call
|
// Double wrap should not cause an actual double wrap. So only 1 rbac call
|
||||||
// should be made.
|
// should be made.
|
||||||
az := dbauthz.New(db, rec, slog.Make(), accessControlStorePointer())
|
az := dbauthz.New(db, rec, slog.Make(), coderdtest.AccessControlStorePointer())
|
||||||
az = dbauthz.New(az, rec, slog.Make(), accessControlStorePointer())
|
az = dbauthz.New(az, rec, slog.Make(), coderdtest.AccessControlStorePointer())
|
||||||
|
|
||||||
w, err := az.GetWorkspaceByID(ctx, exp.ID)
|
w, err := az.GetWorkspaceByID(ctx, exp.ID)
|
||||||
require.NoError(t, err, "must not error")
|
require.NoError(t, err, "must not error")
|
||||||
|
@ -128,7 +129,7 @@ func TestDBAuthzRecursive(t *testing.T) {
|
||||||
t.Parallel()
|
t.Parallel()
|
||||||
q := dbauthz.New(dbmem.New(), &coderdtest.RecordingAuthorizer{
|
q := dbauthz.New(dbmem.New(), &coderdtest.RecordingAuthorizer{
|
||||||
Wrapped: &coderdtest.FakeAuthorizer{AlwaysReturn: nil},
|
Wrapped: &coderdtest.FakeAuthorizer{AlwaysReturn: nil},
|
||||||
}, slog.Make(), accessControlStorePointer())
|
}, slog.Make(), coderdtest.AccessControlStorePointer())
|
||||||
actor := rbac.Subject{
|
actor := rbac.Subject{
|
||||||
ID: uuid.NewString(),
|
ID: uuid.NewString(),
|
||||||
Roles: rbac.RoleNames{rbac.RoleOwner()},
|
Roles: rbac.RoleNames{rbac.RoleOwner()},
|
||||||
|
|
|
@ -6,7 +6,6 @@ import (
|
||||||
"reflect"
|
"reflect"
|
||||||
"sort"
|
"sort"
|
||||||
"strings"
|
"strings"
|
||||||
"sync/atomic"
|
|
||||||
"testing"
|
"testing"
|
||||||
|
|
||||||
"github.com/golang/mock/gomock"
|
"github.com/golang/mock/gomock"
|
||||||
|
@ -17,6 +16,7 @@ import (
|
||||||
"golang.org/x/xerrors"
|
"golang.org/x/xerrors"
|
||||||
|
|
||||||
"cdr.dev/slog"
|
"cdr.dev/slog"
|
||||||
|
|
||||||
"github.com/coder/coder/v2/coderd/coderdtest"
|
"github.com/coder/coder/v2/coderd/coderdtest"
|
||||||
"github.com/coder/coder/v2/coderd/database"
|
"github.com/coder/coder/v2/coderd/database"
|
||||||
"github.com/coder/coder/v2/coderd/database/dbauthz"
|
"github.com/coder/coder/v2/coderd/database/dbauthz"
|
||||||
|
@ -60,7 +60,7 @@ func (s *MethodTestSuite) SetupSuite() {
|
||||||
mockStore := dbmock.NewMockStore(ctrl)
|
mockStore := dbmock.NewMockStore(ctrl)
|
||||||
// We intentionally set no expectations apart from this.
|
// We intentionally set no expectations apart from this.
|
||||||
mockStore.EXPECT().Wrappers().Return([]string{}).AnyTimes()
|
mockStore.EXPECT().Wrappers().Return([]string{}).AnyTimes()
|
||||||
az := dbauthz.New(mockStore, nil, slog.Make(), accessControlStorePointer())
|
az := dbauthz.New(mockStore, nil, slog.Make(), coderdtest.AccessControlStorePointer())
|
||||||
// Take the underlying type of the interface.
|
// Take the underlying type of the interface.
|
||||||
azt := reflect.TypeOf(az).Elem()
|
azt := reflect.TypeOf(az).Elem()
|
||||||
s.methodAccounting = make(map[string]int)
|
s.methodAccounting = make(map[string]int)
|
||||||
|
@ -111,7 +111,7 @@ func (s *MethodTestSuite) Subtest(testCaseF func(db database.Store, check *expec
|
||||||
rec := &coderdtest.RecordingAuthorizer{
|
rec := &coderdtest.RecordingAuthorizer{
|
||||||
Wrapped: fakeAuthorizer,
|
Wrapped: fakeAuthorizer,
|
||||||
}
|
}
|
||||||
az := dbauthz.New(db, rec, slog.Make(), accessControlStorePointer())
|
az := dbauthz.New(db, rec, slog.Make(), coderdtest.AccessControlStorePointer())
|
||||||
actor := rbac.Subject{
|
actor := rbac.Subject{
|
||||||
ID: uuid.NewString(),
|
ID: uuid.NewString(),
|
||||||
Roles: rbac.RoleNames{rbac.RoleOwner()},
|
Roles: rbac.RoleNames{rbac.RoleOwner()},
|
||||||
|
@ -399,22 +399,3 @@ func (emptyPreparedAuthorized) Authorize(_ context.Context, _ rbac.Object) error
|
||||||
func (emptyPreparedAuthorized) CompileToSQL(_ context.Context, _ regosql.ConvertConfig) (string, error) {
|
func (emptyPreparedAuthorized) CompileToSQL(_ context.Context, _ regosql.ConvertConfig) (string, error) {
|
||||||
return "", nil
|
return "", nil
|
||||||
}
|
}
|
||||||
|
|
||||||
func accessControlStorePointer() *atomic.Pointer[dbauthz.AccessControlStore] {
|
|
||||||
acs := &atomic.Pointer[dbauthz.AccessControlStore]{}
|
|
||||||
var tacs dbauthz.AccessControlStore = fakeAccessControlStore{}
|
|
||||||
acs.Store(&tacs)
|
|
||||||
return acs
|
|
||||||
}
|
|
||||||
|
|
||||||
type fakeAccessControlStore struct{}
|
|
||||||
|
|
||||||
func (fakeAccessControlStore) GetTemplateAccessControl(t database.Template) dbauthz.TemplateAccessControl {
|
|
||||||
return dbauthz.TemplateAccessControl{
|
|
||||||
RequireActiveVersion: t.RequireActiveVersion,
|
|
||||||
}
|
|
||||||
}
|
|
||||||
|
|
||||||
func (fakeAccessControlStore) SetTemplateAccessControl(context.Context, database.Store, uuid.UUID, dbauthz.TemplateAccessControl) error {
|
|
||||||
panic("not implemented")
|
|
||||||
}
|
|
||||||
|
|
|
@ -2,8 +2,6 @@ package httpmw
|
||||||
|
|
||||||
import (
|
import (
|
||||||
"context"
|
"context"
|
||||||
"database/sql"
|
|
||||||
"errors"
|
|
||||||
"net/http"
|
"net/http"
|
||||||
|
|
||||||
"github.com/go-chi/chi/v5"
|
"github.com/go-chi/chi/v5"
|
||||||
|
@ -35,9 +33,9 @@ func ExtractWorkspaceAgentParam(db database.Store) func(http.Handler) http.Handl
|
||||||
}
|
}
|
||||||
|
|
||||||
agent, err := db.GetWorkspaceAgentByID(ctx, agentUUID)
|
agent, err := db.GetWorkspaceAgentByID(ctx, agentUUID)
|
||||||
if errors.Is(err, sql.ErrNoRows) {
|
if httpapi.Is404Error(err) {
|
||||||
httpapi.Write(ctx, rw, http.StatusNotFound, codersdk.Response{
|
httpapi.Write(ctx, rw, http.StatusNotFound, codersdk.Response{
|
||||||
Message: "Agent doesn't exist with that id.",
|
Message: "Agent doesn't exist with that id, or you do not have access to it.",
|
||||||
})
|
})
|
||||||
return
|
return
|
||||||
}
|
}
|
||||||
|
|
|
@ -6,11 +6,15 @@ import (
|
||||||
"net/http/httptest"
|
"net/http/httptest"
|
||||||
"testing"
|
"testing"
|
||||||
|
|
||||||
|
"cdr.dev/slog"
|
||||||
"github.com/go-chi/chi/v5"
|
"github.com/go-chi/chi/v5"
|
||||||
"github.com/google/uuid"
|
"github.com/google/uuid"
|
||||||
"github.com/stretchr/testify/require"
|
"github.com/stretchr/testify/require"
|
||||||
|
"golang.org/x/xerrors"
|
||||||
|
|
||||||
|
"github.com/coder/coder/v2/coderd/coderdtest"
|
||||||
"github.com/coder/coder/v2/coderd/database"
|
"github.com/coder/coder/v2/coderd/database"
|
||||||
|
"github.com/coder/coder/v2/coderd/database/dbauthz"
|
||||||
"github.com/coder/coder/v2/coderd/database/dbgen"
|
"github.com/coder/coder/v2/coderd/database/dbgen"
|
||||||
"github.com/coder/coder/v2/coderd/database/dbmem"
|
"github.com/coder/coder/v2/coderd/database/dbmem"
|
||||||
"github.com/coder/coder/v2/coderd/httpmw"
|
"github.com/coder/coder/v2/coderd/httpmw"
|
||||||
|
@ -26,8 +30,10 @@ func TestWorkspaceAgentParam(t *testing.T) {
|
||||||
_, token = dbgen.APIKey(t, db, database.APIKey{
|
_, token = dbgen.APIKey(t, db, database.APIKey{
|
||||||
UserID: user.ID,
|
UserID: user.ID,
|
||||||
})
|
})
|
||||||
|
tpl = dbgen.Template(t, db, database.Template{})
|
||||||
workspace = dbgen.Workspace(t, db, database.Workspace{
|
workspace = dbgen.Workspace(t, db, database.Workspace{
|
||||||
OwnerID: user.ID,
|
OwnerID: user.ID,
|
||||||
|
TemplateID: tpl.ID,
|
||||||
})
|
})
|
||||||
build = dbgen.WorkspaceBuild(t, db, database.WorkspaceBuild{
|
build = dbgen.WorkspaceBuild(t, db, database.WorkspaceBuild{
|
||||||
WorkspaceID: workspace.ID,
|
WorkspaceID: workspace.ID,
|
||||||
|
@ -91,6 +97,36 @@ func TestWorkspaceAgentParam(t *testing.T) {
|
||||||
require.Equal(t, http.StatusNotFound, res.StatusCode)
|
require.Equal(t, http.StatusNotFound, res.StatusCode)
|
||||||
})
|
})
|
||||||
|
|
||||||
|
t.Run("NotAuthorized", func(t *testing.T) {
|
||||||
|
t.Parallel()
|
||||||
|
db := dbmem.New()
|
||||||
|
fakeAuthz := &coderdtest.FakeAuthorizer{AlwaysReturn: xerrors.Errorf("constant failure")}
|
||||||
|
dbFail := dbauthz.New(db, fakeAuthz, slog.Make(), coderdtest.AccessControlStorePointer())
|
||||||
|
|
||||||
|
rtr := chi.NewRouter()
|
||||||
|
rtr.Use(
|
||||||
|
httpmw.ExtractAPIKeyMW(httpmw.ExtractAPIKeyConfig{
|
||||||
|
DB: db,
|
||||||
|
RedirectToLogin: false,
|
||||||
|
}),
|
||||||
|
// Only fail authz in this middleware
|
||||||
|
httpmw.ExtractWorkspaceAgentParam(dbFail),
|
||||||
|
)
|
||||||
|
rtr.Get("/", func(rw http.ResponseWriter, r *http.Request) {
|
||||||
|
_ = httpmw.WorkspaceAgentParam(r)
|
||||||
|
rw.WriteHeader(http.StatusOK)
|
||||||
|
})
|
||||||
|
|
||||||
|
r, _ := setupAuthentication(db)
|
||||||
|
|
||||||
|
rw := httptest.NewRecorder()
|
||||||
|
rtr.ServeHTTP(rw, r)
|
||||||
|
|
||||||
|
res := rw.Result()
|
||||||
|
defer res.Body.Close()
|
||||||
|
require.Equal(t, http.StatusNotFound, res.StatusCode)
|
||||||
|
})
|
||||||
|
|
||||||
t.Run("WorkspaceAgent", func(t *testing.T) {
|
t.Run("WorkspaceAgent", func(t *testing.T) {
|
||||||
t.Parallel()
|
t.Parallel()
|
||||||
db := dbmem.New()
|
db := dbmem.New()
|
||||||
|
|
|
@ -78,6 +78,10 @@ func (api *API) workspaceAgent(rw http.ResponseWriter, r *http.Request) {
|
||||||
return err
|
return err
|
||||||
})
|
})
|
||||||
err := eg.Wait()
|
err := eg.Wait()
|
||||||
|
if httpapi.Is404Error(err) {
|
||||||
|
httpapi.ResourceNotFound(rw)
|
||||||
|
return
|
||||||
|
}
|
||||||
if err != nil {
|
if err != nil {
|
||||||
httpapi.Write(ctx, rw, http.StatusInternalServerError, codersdk.Response{
|
httpapi.Write(ctx, rw, http.StatusInternalServerError, codersdk.Response{
|
||||||
Message: "Internal error fetching workspace agent.",
|
Message: "Internal error fetching workspace agent.",
|
||||||
|
|
Loading…
Reference in New Issue