-
Notifications
You must be signed in to change notification settings - Fork 1.3k
[public-api] Validate Workspace IDs #15423
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -7,8 +7,9 @@ package apiv1 | |
import ( | ||
"context" | ||
"fmt" | ||
|
||
connect "github.com/bufbuild/connect-go" | ||
"github.com/gitpod-io/gitpod/common-go/log" | ||
"github.com/gitpod-io/gitpod/common-go/namegen" | ||
v1 "github.com/gitpod-io/gitpod/components/public-api/go/experimental/v1" | ||
"github.com/gitpod-io/gitpod/components/public-api/go/experimental/v1/v1connect" | ||
protocol "github.com/gitpod-io/gitpod/gitpod-protocol" | ||
|
@@ -31,14 +32,19 @@ type WorkspaceService struct { | |
} | ||
|
||
func (s *WorkspaceService) GetWorkspace(ctx context.Context, req *connect.Request[v1.GetWorkspaceRequest]) (*connect.Response[v1.GetWorkspaceResponse], error) { | ||
logger := ctxlogrus.Extract(ctx) | ||
workspaceID, err := validateWorkspaceID(req.Msg.GetWorkspaceId()) | ||
if err != nil { | ||
return nil, err | ||
} | ||
|
||
logger := ctxlogrus.Extract(ctx).WithField("workspace_id", workspaceID) | ||
|
||
conn, err := getConnection(ctx, s.connectionPool) | ||
if err != nil { | ||
return nil, err | ||
} | ||
|
||
workspace, err := conn.GetWorkspace(ctx, req.Msg.GetWorkspaceId()) | ||
workspace, err := conn.GetWorkspace(ctx, workspaceID) | ||
if err != nil { | ||
logger.WithError(err).Error("Failed to get workspace.") | ||
return nil, proxy.ConvertError(err) | ||
|
@@ -71,13 +77,18 @@ func (s *WorkspaceService) GetWorkspace(ctx context.Context, req *connect.Reques | |
} | ||
|
||
func (s *WorkspaceService) GetOwnerToken(ctx context.Context, req *connect.Request[v1.GetOwnerTokenRequest]) (*connect.Response[v1.GetOwnerTokenResponse], error) { | ||
logger := ctxlogrus.Extract(ctx) | ||
workspaceID, err := validateWorkspaceID(req.Msg.GetWorkspaceId()) | ||
if err != nil { | ||
return nil, err | ||
} | ||
|
||
logger := ctxlogrus.Extract(ctx).WithField("workspace_id", workspaceID) | ||
conn, err := getConnection(ctx, s.connectionPool) | ||
if err != nil { | ||
return nil, err | ||
} | ||
|
||
ownerToken, err := conn.GetOwnerToken(ctx, req.Msg.GetWorkspaceId()) | ||
ownerToken, err := conn.GetOwnerToken(ctx, workspaceID) | ||
|
||
if err != nil { | ||
logger.WithError(err).Error("Failed to get owner token.") | ||
|
@@ -123,24 +134,35 @@ func (s *WorkspaceService) ListWorkspaces(ctx context.Context, req *connect.Requ | |
} | ||
|
||
func (s *WorkspaceService) UpdatePort(ctx context.Context, req *connect.Request[v1.UpdatePortRequest]) (*connect.Response[v1.UpdatePortResponse], error) { | ||
workspaceID, err := validateWorkspaceID(req.Msg.GetWorkspaceId()) | ||
if err != nil { | ||
return nil, err | ||
} | ||
|
||
conn, err := getConnection(ctx, s.connectionPool) | ||
if err != nil { | ||
return nil, err | ||
} | ||
if req.Msg.Port.Policy == v1.PortPolicy_PORT_POLICY_PRIVATE { | ||
_, err = conn.OpenPort(ctx, req.Msg.GetWorkspaceId(), &protocol.WorkspaceInstancePort{ | ||
|
||
switch req.Msg.GetPort().GetPolicy() { | ||
case v1.PortPolicy_PORT_POLICY_PRIVATE: | ||
_, err = conn.OpenPort(ctx, workspaceID, &protocol.WorkspaceInstancePort{ | ||
Port: float64(req.Msg.Port.Port), | ||
Visibility: protocol.PortVisibilityPrivate, | ||
}) | ||
} else if req.Msg.Port.Policy == v1.PortPolicy_PORT_POLICY_PUBLIC { | ||
_, err = conn.OpenPort(ctx, req.Msg.GetWorkspaceId(), &protocol.WorkspaceInstancePort{ | ||
case v1.PortPolicy_PORT_POLICY_PUBLIC: | ||
_, err = conn.OpenPort(ctx, workspaceID, &protocol.WorkspaceInstancePort{ | ||
Port: float64(req.Msg.Port.Port), | ||
Visibility: protocol.PortVisibilityPublic, | ||
}) | ||
default: | ||
return nil, connect.NewError(connect.CodeInvalidArgument, fmt.Errorf("Unknown port policy specified.")) | ||
} | ||
if err != nil { | ||
log.WithField("workspace_id", workspaceID).Error("Failed to update port") | ||
return nil, proxy.ConvertError(err) | ||
} | ||
Comment on lines
+147
to
164
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Drive-by cleanup: this ensures that an unknown port policy fails on the API layer. Previously, it would skip both branches and return OK, without actually doing anything. |
||
|
||
return connect.NewResponse( | ||
&v1.UpdatePortResponse{}, | ||
), nil | ||
|
@@ -281,3 +303,16 @@ func parseGitpodTimestamp(input string) (*timestamppb.Timestamp, error) { | |
} | ||
return timestamppb.New(parsed), nil | ||
} | ||
|
||
func validateWorkspaceID(id string) (string, error) { | ||
if id == "" { | ||
return "", connect.NewError(connect.CodeInvalidArgument, fmt.Errorf("Empty workspace id specified")) | ||
} | ||
|
||
err := namegen.ValidateWorkspaceID(id) | ||
if err != nil { | ||
return "", connect.NewError(connect.CodeInvalidArgument, err) | ||
} | ||
|
||
return id, nil | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -6,6 +6,7 @@ package apiv1 | |
|
||
import ( | ||
"context" | ||
"github.com/gitpod-io/gitpod/common-go/namegen" | ||
"net/http" | ||
"net/http/httptest" | ||
"testing" | ||
|
@@ -26,72 +27,60 @@ import ( | |
) | ||
|
||
func TestWorkspaceService_GetWorkspace(t *testing.T) { | ||
const ( | ||
bearerToken = "bearer-token-for-tests" | ||
foundWorkspaceID = "easycz-seer-xl8o1zacpyw" | ||
) | ||
|
||
type Expectation struct { | ||
Code connect.Code | ||
Response *v1.GetWorkspaceResponse | ||
} | ||
workspaceID := workspaceTestData[0].Protocol.Workspace.ID | ||
|
||
scenarios := []struct { | ||
name string | ||
WorkspaceID string | ||
Workspaces map[string]protocol.WorkspaceInfo | ||
Expect Expectation | ||
}{ | ||
{ | ||
name: "returns a workspace when workspace is found by ID", | ||
WorkspaceID: foundWorkspaceID, | ||
Workspaces: map[string]protocol.WorkspaceInfo{ | ||
foundWorkspaceID: workspaceTestData[0].Protocol, | ||
}, | ||
Expect: Expectation{ | ||
Response: &v1.GetWorkspaceResponse{ | ||
Result: workspaceTestData[0].API, | ||
}, | ||
}, | ||
}, | ||
{ | ||
name: "not found when workspace is not found by ID", | ||
WorkspaceID: "some-not-found-workspace-id", | ||
Expect: Expectation{ | ||
Code: connect.CodeNotFound, | ||
}, | ||
}, | ||
} | ||
t.Run("invalid argument when workspace ID is missing", func(t *testing.T) { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Changing the tests into individual sub-tests made it much easier to follow the test and debug. |
||
_, client := setupWorkspacesService(t) | ||
|
||
for _, test := range scenarios { | ||
t.Run(test.name, func(t *testing.T) { | ||
serverMock, client := setupWorkspacesService(t) | ||
_, err := client.GetWorkspace(context.Background(), connect.NewRequest(&v1.GetWorkspaceRequest{ | ||
WorkspaceId: "", | ||
})) | ||
require.Error(t, err) | ||
require.Equal(t, connect.CodeInvalidArgument, connect.CodeOf(err)) | ||
}) | ||
|
||
serverMock.EXPECT().GetWorkspace(gomock.Any(), gomock.Any()).DoAndReturn(func(ctx context.Context, id string) (res *protocol.WorkspaceInfo, err error) { | ||
w, ok := test.Workspaces[id] | ||
if !ok { | ||
return nil, &jsonrpc2.Error{ | ||
Code: 404, | ||
Message: "not found", | ||
} | ||
} | ||
return &w, nil | ||
}) | ||
t.Run("invalid argument when workspace ID does not validate", func(t *testing.T) { | ||
_, client := setupWorkspacesService(t) | ||
|
||
resp, err := client.GetWorkspace(context.Background(), connect.NewRequest(&v1.GetWorkspaceRequest{ | ||
WorkspaceId: test.WorkspaceID, | ||
})) | ||
requireErrorCode(t, test.Expect.Code, err) | ||
if test.Expect.Response != nil { | ||
requireEqualProto(t, test.Expect.Response, resp.Msg) | ||
} | ||
_, err := client.GetWorkspace(context.Background(), connect.NewRequest(&v1.GetWorkspaceRequest{ | ||
WorkspaceId: "some-random-not-valid-workspace-id", | ||
})) | ||
require.Error(t, err) | ||
require.Equal(t, connect.CodeInvalidArgument, connect.CodeOf(err)) | ||
}) | ||
|
||
t.Run("not found when workspace does not exist", func(t *testing.T) { | ||
serverMock, client := setupWorkspacesService(t) | ||
|
||
serverMock.EXPECT().GetWorkspace(gomock.Any(), workspaceID).Return(nil, &jsonrpc2.Error{ | ||
Code: 404, | ||
Message: "not found", | ||
}) | ||
} | ||
|
||
_, err := client.GetWorkspace(context.Background(), connect.NewRequest(&v1.GetWorkspaceRequest{ | ||
WorkspaceId: workspaceID, | ||
})) | ||
require.Error(t, err) | ||
require.Equal(t, connect.CodeNotFound, connect.CodeOf(err)) | ||
}) | ||
|
||
t.Run("returns a workspace when it exists", func(t *testing.T) { | ||
serverMock, client := setupWorkspacesService(t) | ||
|
||
serverMock.EXPECT().GetWorkspace(gomock.Any(), workspaceID).Return(&workspaceTestData[0].Protocol, nil) | ||
|
||
resp, err := client.GetWorkspace(context.Background(), connect.NewRequest(&v1.GetWorkspaceRequest{ | ||
WorkspaceId: workspaceID, | ||
})) | ||
require.NoError(t, err) | ||
|
||
requireEqualProto(t, workspaceTestData[0].API, resp.Msg.GetResult()) | ||
}) | ||
} | ||
|
||
func TestWorkspaceService_GetOwnerToken(t *testing.T) { | ||
const ( | ||
bearerToken = "bearer-token-for-tests" | ||
foundWorkspaceID = "easycz-seer-xl8o1zacpyw" | ||
ownerToken = "some-owner-token" | ||
) | ||
|
@@ -118,7 +107,7 @@ func TestWorkspaceService_GetOwnerToken(t *testing.T) { | |
}, | ||
{ | ||
name: "not found when workspace is not found by ID", | ||
WorkspaceID: "some-not-found-workspace-id", | ||
WorkspaceID: mustGenerateWorkspaceID(t), | ||
Expect: Expectation{ | ||
Code: connect.CodeNotFound, | ||
}, | ||
|
@@ -446,3 +435,12 @@ func requireErrorCode(t *testing.T, expected connect.Code, err error) { | |
actual := connect.CodeOf(err) | ||
require.Equal(t, expected, actual, "expected code %s, but got %s from error %v", expected.String(), actual.String(), err) | ||
} | ||
|
||
func mustGenerateWorkspaceID(t *testing.T) string { | ||
t.Helper() | ||
|
||
wsid, err := namegen.GenerateWorkspaceID() | ||
require.NoError(t, err) | ||
|
||
return wsid | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
#10491 (comment)