Skip to content
Merged
2 changes: 1 addition & 1 deletion lib/web/apiserver.go
Original file line number Diff line number Diff line change
Expand Up @@ -845,7 +845,7 @@ func (h *Handler) bindDefaultEndpoints() {
// User Status (used by client to check if user session is valid)
h.GET("/webapi/user/status", h.WithAuth(h.getUserStatus))

h.GET("/webapi/roles", h.WithAuth(h.getRolesHandle))
h.GET("/webapi/roles", h.WithAuth(h.listRolesHandle))
h.POST("/webapi/roles", h.WithAuth(h.createRoleHandle))
h.PUT("/webapi/roles/:name", h.WithAuth(h.updateRoleHandle))
h.DELETE("/webapi/roles/:name", h.WithAuth(h.deleteRole))
Expand Down
51 changes: 50 additions & 1 deletion lib/web/resources.go
Original file line number Diff line number Diff line change
Expand Up @@ -78,12 +78,19 @@ func (h *Handler) checkAccessToRegisteredResource(w http.ResponseWriter, r *http
}, nil
}

func (h *Handler) getRolesHandle(w http.ResponseWriter, r *http.Request, params httprouter.Params, ctx *SessionContext) (interface{}, error) {
func (h *Handler) listRolesHandle(w http.ResponseWriter, r *http.Request, params httprouter.Params, ctx *SessionContext) (interface{}, error) {
clt, err := ctx.GetClient()
if err != nil {
return nil, trace.Wrap(err)
}

values := r.URL.Query()
// If limit exists as a query parameter, this means its coming from a "new" webui
// and can return the new paginated response.
// TODO(gzdunek): DELETE IN 17.0.0: remove "getRoles".
if values.Has("limit") {
return listRoles(clt, values)
}
return getRoles(clt)
}

Expand All @@ -96,6 +103,39 @@ func getRoles(clt resourcesAPIGetter) ([]ui.ResourceItem, error) {
return ui.NewRoles(roles)
}

func listRoles(clt resourcesAPIGetter, values url.Values) (*listResourcesWithoutCountGetResponse, error) {
limit, err := QueryLimitAsInt32(values, "limit", defaults.MaxIterationLimit)
if err != nil {
return nil, trace.Wrap(err)
}

roles, err := clt.ListRoles(context.TODO(), &proto.ListRolesRequest{
Limit: limit,
StartKey: values.Get("startKey"),
Filter: &types.RoleFilter{
SearchKeywords: client.ParseSearchKeywords(values.Get("search"), ' '),
},
})
if err != nil {
return nil, trace.Wrap(err)
}

var typeRoles []types.Role
for _, role := range roles.GetRoles() {
typeRoles = append(typeRoles, role)
}

uiRoles, err := ui.NewRoles(typeRoles)
if err != nil {
return nil, trace.Wrap(err)
}

return &listResourcesWithoutCountGetResponse{
Items: uiRoles,
StartKey: roles.GetNextKey(),
}, nil
Comment on lines +133 to +136
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We need to be able to handle requests from an old webclient (in the case of two different proxies running together) which would still expect a response that is only an array of items, rather than this paginated object.

Without having to resort to a larger refactor that can version this endpoint, we can just use the existence of one of these new parameters to determine between old/new clients. For example "if limit exists, send new response. otherwise, send only uiRoles"

We did something similar here

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also, make sure the TODO i left in useKeyBasedPagination would fix this from the frontend side as well please (new webclient being able to handle "old" response types).

Copy link
Copy Markdown
Contributor Author

@gzdunek gzdunek Apr 4, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh, I completely forgot that we can have proxies running different versions.
I added a check on the parameters to determine what response type should be returned. It's not beautiful but works.

Also, make sure the TODO i left in useKeyBasedPagination would fix this from the frontend side as well please

The thing I could do to allow new webclient being able to handle "old" response types, would be to wrap the response in the service layer if I detect that I didn't receive the paginated object. But I'm not sure if it makes any sense, things like limit or search would not work at all.

I don't know how to handle this, to be honest. If I have a server-side table, how can I make it work with both paginated and non-paginated responses?
Maybe instead of modifying the existing endpoint I should add a new one? At least the request would fail with 404 error instead of something like res.map is not a function.
EDIT: I could also throw an error in resource.ts saying ...please update your Teleport Proxy to version ... if I detect a non-paginated response.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't know how to handle this, to be honest. If I have a server-side table, how can I make it work with both paginated and non-paginated responses?

The worst case scenario is we receive a very large dataset from the "old" api. I don't have any answer for this yet. An upcoming project will be to add a properly versioned API. I don't know exactly how it would work yet for newer web clients receiving 404s but my guess is there would have to be a fallback to a previous version or just accept the 404. I want to discuss this with the team with an RFD once we get the greenlight for the project.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actually, we can support the "new webclient being able to handle old response types" case too.
I added a compatibility layer in resource.ts: if we detect that we received a non-paginated response, we can "paginate" it locally, in the browser. I added support for search, limit and startKey.
Let me know what you think about this.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i LOVE this. amazing work!!

}

func (h *Handler) deleteRole(w http.ResponseWriter, r *http.Request, params httprouter.Params, ctx *SessionContext) (interface{}, error) {
clt, err := ctx.GetClient()
if err != nil {
Expand Down Expand Up @@ -505,6 +545,13 @@ type listResourcesGetResponse struct {
TotalCount int `json:"totalCount"`
}

type listResourcesWithoutCountGetResponse struct {
// Items is a list of resources retrieved.
Items interface{} `json:"items"`
// StartKey is the position to resume search events.
StartKey string `json:"startKey"`
}

type checkAccessToRegisteredResourceResponse struct {
// HasResource is a flag to indicate if user has any access
// to a registered resource or not.
Expand All @@ -516,6 +563,8 @@ type resourcesAPIGetter interface {
GetRole(ctx context.Context, name string) (types.Role, error)
// GetRoles returns a list of roles
GetRoles(ctx context.Context) ([]types.Role, error)
// ListRoles returns a paginated list of roles.
ListRoles(ctx context.Context, req *proto.ListRolesRequest) (*proto.ListRolesResponse, error)
// UpsertRole creates or updates role
UpsertRole(ctx context.Context, role types.Role) (types.Role, error)
// GetGithubConnectors returns all configured Github connectors
Expand Down
32 changes: 22 additions & 10 deletions lib/web/resources_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -300,22 +300,25 @@ version: v2
func TestGetRoles(t *testing.T) {
m := &mockedResourceAPIGetter{}

m.mockGetRoles = func(ctx context.Context) ([]types.Role, error) {
m.mockListRoles = func(ctx context.Context, req *proto.ListRolesRequest) (*proto.ListRolesResponse, error) {
role, err := types.NewRole("test", types.RoleSpecV6{
Allow: types.RoleConditions{
Logins: []string{"test"},
},
})
require.NoError(t, err)

return []types.Role{role}, nil
return &proto.ListRolesResponse{
Roles: []*types.RoleV6{role.(*types.RoleV6)},
NextKey: "",
}, nil
}

// Test response is converted to ui objects.
roles, err := getRoles(m)
roles, err := listRoles(m, url.Values{})
require.NoError(t, err)
require.Len(t, roles, 1)
require.Contains(t, roles[0].Content, "name: test")
require.Len(t, roles.Items, 1)
require.Contains(t, roles.Items.([]ui.ResourceItem)[0].Content, "name: test")
}

func TestRoleCRUD(t *testing.T) {
Expand Down Expand Up @@ -402,15 +405,16 @@ func TestRoleCRUD(t *testing.T) {
_, err = pack.clt.Delete(ctx, pack.clt.Endpoint("webapi", "roles", expected.GetName()))
require.NoError(t, err, "unexpected error deleting role")

resp, err = pack.clt.Get(ctx, pack.clt.Endpoint("webapi", "roles"), nil)
resp, err = pack.clt.Get(ctx, pack.clt.Endpoint("webapi", "roles"), url.Values{"limit": []string{"15"}})
assert.NoError(t, err, "unexpected error listing role")

var items []ui.ResourceItem
require.NoError(t, json.Unmarshal(resp.Bytes(), &items), "invalid resource item received")
var getResponse listResourcesWithoutCountGetResponse
require.NoError(t, json.Unmarshal(resp.Bytes(), &getResponse), "invalid resource item received")
assert.Equal(t, http.StatusOK, resp.Code(), "unexpected status code getting roles")

for _, item := range items {
assert.NotEqual(t, "test-role", item.Name, "expected test-role to be deleted")
assert.Equal(t, "", getResponse.StartKey)
for _, item := range getResponse.Items.([]interface{}) {
assert.NotEqual(t, "test-role", item.(map[string]interface{})["name"], "expected test-role to be deleted")
}
}

Expand Down Expand Up @@ -609,6 +613,7 @@ func TestListResources(t *testing.T) {
type mockedResourceAPIGetter struct {
mockGetRole func(ctx context.Context, name string) (types.Role, error)
mockGetRoles func(ctx context.Context) ([]types.Role, error)
mockListRoles func(ctx context.Context, req *proto.ListRolesRequest) (*proto.ListRolesResponse, error)
mockUpsertRole func(ctx context.Context, role types.Role) (types.Role, error)
mockGetGithubConnectors func(ctx context.Context, withSecrets bool) ([]types.GithubConnector, error)
mockGetGithubConnector func(ctx context.Context, id string, withSecrets bool) (types.GithubConnector, error)
Expand All @@ -634,6 +639,13 @@ func (m *mockedResourceAPIGetter) GetRoles(ctx context.Context) ([]types.Role, e
return nil, trace.NotImplemented("mockGetRoles not implemented")
}

func (m *mockedResourceAPIGetter) ListRoles(ctx context.Context, req *proto.ListRolesRequest) (*proto.ListRolesResponse, error) {
if m.mockListRoles != nil {
return m.mockListRoles(ctx, req)
}
return nil, trace.NotImplemented("mockListRoles not implemented")
}

func (m *mockedResourceAPIGetter) UpsertRole(ctx context.Context, role types.Role) (types.Role, error) {
if m.mockUpsertRole != nil {
return m.mockUpsertRole(ctx, role)
Expand Down
7 changes: 4 additions & 3 deletions web/packages/design/src/DataTable/Pager/ServerSidePager.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -23,9 +23,9 @@ import { CircleArrowLeft, CircleArrowRight } from 'design/Icon';

import { StyledArrowBtn } from './StyledPager';

export function ServerSidePager({ nextPage, prevPage }: Props) {
const isNextDisabled = !nextPage;
const isPrevDisabled = !prevPage;
export function ServerSidePager({ nextPage, prevPage, isLoading }: Props) {
const isNextDisabled = !nextPage || isLoading;
const isPrevDisabled = !prevPage || isLoading;

return (
<Flex justifyContent="flex-end" width="100%">
Expand All @@ -52,6 +52,7 @@ export function ServerSidePager({ nextPage, prevPage }: Props) {
}

export type Props = {
isLoading: boolean;
nextPage: (() => void) | null;
prevPage: (() => void) | null;
};
8 changes: 7 additions & 1 deletion web/packages/design/src/DataTable/Table.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -154,6 +154,7 @@ export function Table<T>({
prevPage={fetching.onFetchPrev}
pagination={state.pagination}
serversideProps={serversideProps}
fetchStatus={fetching.fetchStatus}
/>
);
}
Expand Down Expand Up @@ -325,6 +326,7 @@ function ServersideTable<T>({
className,
style,
serversideProps,
fetchStatus,
}: ServersideTableProps<T>) {
return (
<>
Expand All @@ -335,7 +337,11 @@ function ServersideTable<T>({
</StyledTable>
{(nextPage || prevPage) && (
<StyledPanel showTopBorder={true}>
<ServerSidePager nextPage={nextPage} prevPage={prevPage} />
<ServerSidePager
nextPage={nextPage}
prevPage={prevPage}
isLoading={fetchStatus === 'loading'}
/>
</StyledPanel>
)}
</>
Expand Down
1 change: 1 addition & 0 deletions web/packages/design/src/DataTable/types.ts
Original file line number Diff line number Diff line change
Expand Up @@ -178,4 +178,5 @@ export type ServersideTableProps<T> = BasicTableProps<T> & {
prevPage: () => void;
pagination: State<T>['state']['pagination'];
serversideProps: State<T>['serversideProps'];
fetchStatus?: FetchStatus;
};
30 changes: 18 additions & 12 deletions web/packages/shared/components/Search/SearchPanel.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -33,14 +33,16 @@ export function SearchPanel({
filter,
showSearchBar,
disableSearch,
hideAdvancedSearch,
extraChildren,
}: {
updateQuery(s: string): void;
updateSearch(s: string): void;
pageIndicators: { from: number; to: number; total: number };
pageIndicators?: { from: number; to: number; total: number };
filter: ResourceFilter;
showSearchBar: boolean;
disableSearch: boolean;
hideAdvancedSearch?: boolean;
extraChildren?: JSX.Element;
}) {
const [query, setQuery] = useState(filter.search || filter.query || '');
Expand Down Expand Up @@ -82,22 +84,26 @@ export function SearchPanel({
>
{showSearchBar && (
<InputSearch searchValue={query} setSearchValue={setQuery}>
<AdvancedSearchToggle
isToggled={isAdvancedSearch}
onToggle={onToggle}
px={3}
/>
{!hideAdvancedSearch && (
<AdvancedSearchToggle
isToggled={isAdvancedSearch}
onToggle={onToggle}
px={3}
/>
)}
</InputSearch>
)}
</StyledFlex>
</Flex>
<Flex alignItems="center">
<PageIndicatorText
from={pageIndicators.from}
to={pageIndicators.to}
count={pageIndicators.total}
/>
{extraChildren && extraChildren}
{pageIndicators && (
<PageIndicatorText
from={pageIndicators.from}
to={pageIndicators.to}
count={pageIndicators.total}
/>
)}
{extraChildren}
</Flex>
</Flex>
</StyledPanel>
Expand Down
7 changes: 6 additions & 1 deletion web/packages/teleport/src/Bots/EditBot.test.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -18,11 +18,13 @@

import { render, screen, userEvent } from 'design/utils/testing';

import { waitFor } from '@testing-library/react';

import { EditBot } from 'teleport/Bots/EditBot';
import { EditBotProps } from 'teleport/Bots/types';

const makeProps = (overrides: Partial<EditBotProps> = {}): EditBotProps => ({
allRoles: [],
fetchRoles: jest.fn().mockResolvedValueOnce([]),
attempt: { status: '' },
name: 'bot-007',
onClose: () => {},
Expand All @@ -35,6 +37,7 @@ const makeProps = (overrides: Partial<EditBotProps> = {}): EditBotProps => ({
test('renders', async () => {
const props = makeProps({ selectedRoles: ['foo-role'] });
render(<EditBot {...props} />);
await waitFor(() => expect(props.fetchRoles).toHaveBeenCalledTimes(1));

expect(screen.getByText('Edit Bot')).toBeInTheDocument();
expect(screen.getByRole('button', { name: 'Save' })).toBeInTheDocument();
Expand Down Expand Up @@ -72,6 +75,7 @@ test('edit calls onedit cb', async () => {
test('disables buttons when processing', async () => {
const props = makeProps({ attempt: { status: 'processing' } });
render(<EditBot {...props} />);
await waitFor(() => expect(props.fetchRoles).toHaveBeenCalledTimes(1));

expect(screen.queryByRole('button', { name: 'Save' })).toBeDisabled();
expect(screen.queryByRole('button', { name: 'Cancel' })).toBeDisabled();
Expand All @@ -82,6 +86,7 @@ test('displays error text', async () => {
attempt: { status: 'failed', statusText: 'error editing' },
});
render(<EditBot {...props} />);
await waitFor(() => expect(props.fetchRoles).toHaveBeenCalledTimes(1));

expect(screen.getByText('error editing')).toBeInTheDocument();
});
22 changes: 12 additions & 10 deletions web/packages/teleport/src/Bots/EditBot.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -27,26 +27,21 @@ import Dialog, {

import FieldInput from 'shared/components/FieldInput';
import { requiredField } from 'shared/components/Validation/rules';
import FieldSelect from 'shared/components/FieldSelect';
import { FieldSelectAsync } from 'shared/components/FieldSelect';
import Validation from 'shared/components/Validation';
import { Option } from 'shared/components/Select';

import { EditBotProps } from 'teleport/Bots/types';

export function EditBot({
allRoles,
fetchRoles,
attempt,
name,
onClose,
onEdit,
selectedRoles,
setSelectedRoles,
}: EditBotProps) {
const selectOptions: Option[] = allRoles.map(r => ({
value: r,
label: r,
}));

return (
<Dialog disableEscapeKeyDown={false} onClose={onClose} open={true}>
<DialogHeader>
Expand All @@ -65,7 +60,7 @@ export function EditBot({
readonly={true}
onChange={() => {}}
/>
<FieldSelect
<FieldSelectAsync
menuPosition="fixed"
label="Bot Roles"
rule={requiredField('At least one role is required')}
Expand All @@ -79,9 +74,16 @@ export function EditBot({
label: r,
}))}
onChange={(values: Option[]) =>
setSelectedRoles(values.map(v => v.value))
setSelectedRoles(values?.map(v => v.value) || [])
}
options={selectOptions}
loadOptions={async input => {
const roles = await fetchRoles(input);
return roles.map(r => ({
value: r,
label: r,
}));
}}
noOptionsMessage={() => 'No roles found'}
elevated={true}
/>
</DialogContent>
Expand Down
2 changes: 1 addition & 1 deletion web/packages/teleport/src/Bots/List/BotList.test.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -31,7 +31,7 @@ const makeProps = (): BotListProps => ({
onClose: () => {},
onDelete: () => {},
onEdit: () => {},
roles: [],
fetchRoles: async () => [],
selectedBot: null,
selectedRoles: [],
setSelectedBot: () => {},
Expand Down
Loading