Skip to content

Commit bc9cfb8

Browse files
ThomasK33ThomasK33
committed
review improvements
Change-Id: I7868299308cdf4fca36092025bf2692cfe652640 Signed-off-by: Thomas Kosiewski <tk@coder.com>
1 parent b5ed0bd commit bc9cfb8

17 files changed

+74
-110
lines changed

‎cli/testdata/coder_provisioner_list_--output_json.golden

+1-1
Original file line numberDiff line numberDiff line change
@@ -7,7 +7,7 @@
77
"last_seen_at": "====[timestamp]=====",
88
"name": "test",
99
"version": "v0.0.0-devel",
10-
"api_version": "1.4",
10+
"api_version": "1.5",
1111
"provisioners": [
1212
"echo"
1313
],

‎coderd/coderd.go

+2-12
Original file line numberDiff line numberDiff line change
@@ -804,11 +804,6 @@ func New(options *Options) *API {
804804
DB: options.Database,
805805
Optional: false,
806806
})
807-
// Same as above but optional
808-
workspaceAgentInfoOptional := httpmw.ExtractWorkspaceAgentAndLatestBuild(httpmw.ExtractWorkspaceAgentAndLatestBuildConfig{
809-
DB: options.Database,
810-
Optional: true,
811-
})
812807

813808
// API rate limit middleware. The counter is local and not shared between
814809
// replicas or instances of this middleware.
@@ -1029,7 +1024,6 @@ func New(options *Options) *API {
10291024
r.Route("/external-auth", func(r chi.Router) {
10301025
r.Use(
10311026
apiKeyMiddleware,
1032-
workspaceAgentInfoOptional,
10331027
)
10341028
// Get without a specific external auth ID will return all external auths.
10351029
r.Get("/", api.listUserExternalAuths)
@@ -1265,12 +1259,8 @@ func New(options *Options) *API {
12651259
r.Get("/", api.workspaceByOwnerAndName)
12661260
r.Get("/builds/{buildnumber}", api.workspaceBuildByBuildNumber)
12671261
})
1268-
r.With(
1269-
workspaceAgentInfoOptional,
1270-
).Get("/gitsshkey", api.gitSSHKey)
1271-
r.With(
1272-
workspaceAgentInfoOptional,
1273-
).Put("/gitsshkey", api.regenerateGitSSHKey)
1262+
r.Get("/gitsshkey", api.gitSSHKey)
1263+
r.Put("/gitsshkey", api.regenerateGitSSHKey)
12741264
r.Route("/notifications", func(r chi.Router) {
12751265
r.Route("/preferences", func(r chi.Router) {
12761266
r.Get("/", api.userNotificationPreferences)

‎coderd/database/dbauthz/dbauthz_test.go

+1-1
Original file line numberDiff line numberDiff line change
@@ -3988,7 +3988,7 @@ func (s *MethodTestSuite) TestSystemFunctions() {
39883988
check.Args(database.InsertWorkspaceAgentParams{
39893989
ID: uuid.New(),
39903990
Name: "dev",
3991-
APIKeyScope: database.ApiKeyScopeEnumDefault,
3991+
APIKeyScope: database.AgentKeyScopeEnumAll,
39923992
}).Asserts(rbac.ResourceSystem, policy.ActionCreate)
39933993
}))
39943994
s.Run("InsertWorkspaceApp", s.Subtest(func(db database.Store, check *expects) {

‎coderd/database/dbgen/dbgen.go

+1-1
Original file line numberDiff line numberDiff line change
@@ -210,7 +210,7 @@ func WorkspaceAgent(t testing.TB, db database.Store, orig database.WorkspaceAgen
210210
MOTDFile: takeFirst(orig.TroubleshootingURL, ""),
211211
DisplayApps: append([]database.DisplayApp{}, orig.DisplayApps...),
212212
DisplayOrder: takeFirst(orig.DisplayOrder, 1),
213-
APIKeyScope: takeFirst(orig.APIKeyScope, database.ApiKeyScopeEnumDefault),
213+
APIKeyScope: takeFirst(orig.APIKeyScope, database.AgentKeyScopeEnumAll),
214214
})
215215
require.NoError(t, err, "insert workspace agent")
216216
return agt

‎coderd/database/dump.sql

+7-7
Some generated files are not rendered by default. Learn more about customizing how changed files appear on GitHub.

‎coderd/database/migrations/000320_add_api_key_scope_to_workspace_agents.down.sql

+1-1
Original file line numberDiff line numberDiff line change
@@ -3,4 +3,4 @@ ALTER TABLE workspace_agents
33
DROP COLUMN IF EXISTS api_key_scope;
44

55
-- Drop the enum type for API key scope
6-
DROP TYPE IF EXISTS api_key_scope_enum;
6+
DROP TYPE IF EXISTS agent_key_scope_enum;

‎coderd/database/migrations/000320_add_api_key_scope_to_workspace_agents.up.sql

+4-4
Original file line numberDiff line numberDiff line change
@@ -1,10 +1,10 @@
11
-- Create the enum type for API key scope
2-
CREATE TYPE api_key_scope_enum AS ENUM ('default', 'no_user_data');
2+
CREATE TYPE agent_key_scope_enum AS ENUM ('all', 'no_user_data');
33

44
-- Add the api_key_scope column to the workspace_agents table
5-
-- It defaults to 'default' to maintain existing behavior for current agents.
5+
-- It defaults to 'all' to maintain existing behavior for current agents.
66
ALTER TABLE workspace_agents
7-
ADD COLUMN api_key_scope api_key_scope_enum NOT NULL DEFAULT 'default';
7+
ADD COLUMN api_key_scope agent_key_scope_enum NOT NULL DEFAULT 'all';
88

99
-- Add a comment explaining the purpose of the column
10-
COMMENT ON COLUMN workspace_agents.api_key_scope IS 'Defines the scope of the API key associated with the agent. ''default'' allows access to everything, ''no_user_data'' restricts it to exclude user data.';
10+
COMMENT ON COLUMN workspace_agents.api_key_scope IS 'Defines the scope of the API key associated with the agent. ''all'' allows access to everything, ''no_user_data'' restricts it to exclude user data.';

‎coderd/database/models.go

+24-24
Some generated files are not rendered by default. Learn more about customizing how changed files appear on GitHub.

‎coderd/database/queries.sql.go

+1-1
Some generated files are not rendered by default. Learn more about customizing how changed files appear on GitHub.

‎coderd/externalauth_test.go

+12-11
Original file line numberDiff line numberDiff line change
@@ -713,7 +713,7 @@ func TestExternalAuthCallback(t *testing.T) {
713713
apiKeyScope string
714714
expectsError bool
715715
}{
716-
{apiKeyScope: "default", expectsError: false},
716+
{apiKeyScope: "all", expectsError: false},
717717
{apiKeyScope: "no_user_data", expectsError: true},
718718
} {
719719
t.Run(tt.apiKeyScope, func(t *testing.T) {
@@ -746,6 +746,15 @@ func TestExternalAuthCallback(t *testing.T) {
746746
token, err := agentClient.ExternalAuth(t.Context(), agentsdk.ExternalAuthRequest{
747747
Match: "github.com/asd/asd",
748748
})
749+
750+
if tt.expectsError {
751+
require.Error(t, err)
752+
var sdkErr *codersdk.Error
753+
require.ErrorAs(t, err, &sdkErr)
754+
require.Equal(t, http.StatusForbidden, sdkErr.StatusCode())
755+
return
756+
}
757+
749758
require.NoError(t, err)
750759
require.NotEmpty(t, token.URL)
751760

@@ -756,13 +765,8 @@ func TestExternalAuthCallback(t *testing.T) {
756765
Match: "github.com/asd/asd",
757766
Listen: true,
758767
})
759-
if tt.expectsError {
760-
assert.Error(t, err)
761-
close(tokenChan)
762-
} else {
763-
assert.NoError(t, err)
764-
tokenChan <- token
765-
}
768+
assert.NoError(t, err)
769+
tokenChan <- token
766770
}()
767771

768772
time.Sleep(250 * time.Millisecond)
@@ -771,9 +775,6 @@ func TestExternalAuthCallback(t *testing.T) {
771775
require.Equal(t, http.StatusTemporaryRedirect, resp.StatusCode)
772776

773777
token = <-tokenChan
774-
if tt.expectsError {
775-
return
776-
}
777778
require.Equal(t, "access_token", token.Username)
778779

779780
token, err = agentClient.ExternalAuth(t.Context(), agentsdk.ExternalAuthRequest{

‎coderd/gitsshkey_test.go

+6-1
Original file line numberDiff line numberDiff line change
@@ -2,6 +2,7 @@ package coderd_test
22

33
import (
44
"context"
5+
"net/http"
56
"testing"
67

78
"github.com/google/uuid"
@@ -12,6 +13,7 @@ import (
1213
"github.com/coder/coder/v2/coderd/coderdtest"
1314
"github.com/coder/coder/v2/coderd/database"
1415
"github.com/coder/coder/v2/coderd/gitsshkey"
16+
"github.com/coder/coder/v2/codersdk"
1517
"github.com/coder/coder/v2/codersdk/agentsdk"
1618
"github.com/coder/coder/v2/provisioner/echo"
1719
"github.com/coder/coder/v2/testutil"
@@ -134,7 +136,7 @@ func TestAgentGitSSHKey_APIKeyScopes(t *testing.T) {
134136
apiKeyScope string
135137
expectError bool
136138
}{
137-
{apiKeyScope: "default", expectError: false},
139+
{apiKeyScope: "all", expectError: false},
138140
{apiKeyScope: "no_user_data", expectError: true},
139141
} {
140142
t.Run(tt.apiKeyScope, func(t *testing.T) {
@@ -165,6 +167,9 @@ func TestAgentGitSSHKey_APIKeyScopes(t *testing.T) {
165167

166168
if tt.expectError {
167169
require.Error(t, err)
170+
var sdkErr *codersdk.Error
171+
require.ErrorAs(t, err, &sdkErr)
172+
require.Equal(t, http.StatusForbidden, sdkErr.StatusCode())
168173
} else {
169174
require.NoError(t, err)
170175
}

‎coderd/httpmw/workspaceagent.go

+1-1
Original file line numberDiff line numberDiff line change
@@ -118,7 +118,7 @@ func ExtractWorkspaceAgentAndLatestBuild(opts ExtractWorkspaceAgentAndLatestBuil
118118
OwnerID: row.WorkspaceTable.OwnerID,
119119
TemplateID: row.WorkspaceTable.TemplateID,
120120
VersionID: row.WorkspaceBuild.TemplateVersionID,
121-
BlockUserData: row.WorkspaceAgent.APIKeyScope == database.ApiKeyScopeEnumNoUserData,
121+
BlockUserData: row.WorkspaceAgent.APIKeyScope == database.AgentKeyScopeEnumNoUserData,
122122
}),
123123
)
124124
if err != nil {

‎coderd/provisionerdserver/provisionerdserver.go

+3-3
Original file line numberDiff line numberDiff line change
@@ -2003,9 +2003,9 @@ func InsertWorkspaceResource(ctx context.Context, db database.Store, jobID uuid.
20032003
}
20042004
}
20052005

2006-
apiKeyScope := database.ApiKeyScopeEnumDefault
2007-
if prAgent.ApiKeyScope == string(database.ApiKeyScopeEnumNoUserData) {
2008-
apiKeyScope = database.ApiKeyScopeEnumNoUserData
2006+
apiKeyScope := database.AgentKeyScopeEnumAll
2007+
if prAgent.ApiKeyScope == string(database.AgentKeyScopeEnumNoUserData) {
2008+
apiKeyScope = database.AgentKeyScopeEnumNoUserData
20092009
}
20102010

20112011
agentID := uuid.New()

‎coderd/rbac/authz_internal_test.go

+2-30
Original file line numberDiff line numberDiff line change
@@ -4,7 +4,6 @@ import (
44
"context"
55
"encoding/json"
66
"fmt"
7-
"slices"
87
"sync"
98
"testing"
109

@@ -1055,51 +1054,24 @@ func TestAuthorizeScope(t *testing.T) {
10551054
},
10561055
)
10571056

1058-
// This scope can only create workspaces
10591057
meID := uuid.New()
10601058
user = Subject{
10611059
ID: meID.String(),
10621060
Roles: Roles{
10631061
must(RoleByName(RoleMember())),
10641062
must(RoleByName(ScopedRoleOrgMember(defOrg))),
10651063
},
1066-
Scope: Scope{
1067-
Role: Role{
1068-
Identifier: RoleIdentifier{Name: "no_personal_data"},
1069-
DisplayName: "No Personal Data",
1070-
Site: append(
1071-
// Workspace dormancy and workspace are omitted.
1072-
// Workspace is specifically handled based on the opts.NoOwnerWorkspaceExec
1073-
allPermsExcept(ResourceUser),
1074-
// This adds back in the Workspace permissions.
1075-
Permissions(map[string][]policy.Action{
1076-
ResourceUser.Type: {policy.ActionRead},
1077-
})...),
1078-
Org: map[string][]Permission{},
1079-
User: []Permission{},
1080-
},
1081-
// Empty string allow_list is allowed for actions like 'create'
1082-
AllowIDList: []string{meID.String()},
1083-
},
1064+
Scope: must(ScopeNoUserData.Expand()),
10841065
}
1085-
10861066
testAuthorize(t, "ReadPersonalUser", user,
1087-
// All these cases will fail because a resource ID is set.
10881067
cases(func(c authTestCase) authTestCase {
1089-
c.actions = slices.DeleteFunc(ResourceUser.AvailableActions(), func(action policy.Action) bool {
1090-
return action == policy.ActionRead
1091-
})
1068+
c.actions = ResourceUser.AvailableActions()
10921069
c.allow = false
10931070
c.resource.ID = meID.String()
10941071
return c
10951072
}, []authTestCase{
10961073
{resource: ResourceUser.WithOwner(meID.String()).InOrg(defOrg).WithID(meID)},
10971074
}),
1098-
1099-
// Test create allowed by scope:
1100-
[]authTestCase{
1101-
{resource: ResourceUser.WithOwner(meID.String()).InOrg(defOrg).WithID(meID), actions: []policy.Action{policy.ActionRead}, allow: true},
1102-
},
11031075
)
11041076
}
11051077

0 commit comments

Comments
 (0)