diff --git a/controller/internal/service/client/v1/client_service.go b/controller/internal/service/client/v1/client_service.go index a3bb726a6..5bece283b 100644 --- a/controller/internal/service/client/v1/client_service.go +++ b/controller/internal/service/client/v1/client_service.go @@ -329,10 +329,10 @@ func (s *ClientService) CreateLease(ctx context.Context, req *cpb.CreateLeaseReq return nil, err } - if len(jlease.Spec.SharedWith) > jumpstarterdevv1alpha1.MaxSharedWithEntries { - return nil, status.Errorf(codes.InvalidArgument, "shared_with list exceeds maximum of %d entries", jumpstarterdevv1alpha1.MaxSharedWithEntries) - } if len(jlease.Spec.SharedWith) > 0 { + // Deduplicate first, then enforce the limit against the deduplicated list, + // so a request with repeated names that collapses to a list within the + // limit is accepted rather than rejected on its raw length. var deduped []string for _, name := range jlease.Spec.SharedWith { if name == jclient.Name { @@ -350,6 +350,9 @@ func (s *ClientService) CreateLease(ctx context.Context, req *cpb.CreateLeaseReq } deduped = append(deduped, name) } + if len(deduped) > jumpstarterdevv1alpha1.MaxSharedWithEntries { + return nil, status.Errorf(codes.InvalidArgument, "shared_with list exceeds maximum of %d entries", jumpstarterdevv1alpha1.MaxSharedWithEntries) + } jlease.Spec.SharedWith = deduped } diff --git a/controller/internal/service/client/v1/client_service_test.go b/controller/internal/service/client/v1/client_service_test.go index 511da9ae7..19b843f74 100644 --- a/controller/internal/service/client/v1/client_service_test.go +++ b/controller/internal/service/client/v1/client_service_test.go @@ -4,14 +4,20 @@ import ( "context" "strings" "testing" + "time" jumpstarterdevv1alpha1 "github.com/jumpstarter-dev/jumpstarter/controller/api/v1alpha1" cpb "github.com/jumpstarter-dev/jumpstarter/controller/internal/protocol/jumpstarter/client/v1" + "github.com/jumpstarter-dev/jumpstarter/controller/internal/service/auth" "google.golang.org/grpc/codes" "google.golang.org/grpc/status" + "google.golang.org/protobuf/types/known/durationpb" corev1 "k8s.io/api/core/v1" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/apimachinery/pkg/runtime" + "k8s.io/apiserver/pkg/authentication/authenticator" + "k8s.io/apiserver/pkg/authentication/user" + "k8s.io/apiserver/pkg/authorization/authorizer" kclient "sigs.k8s.io/controller-runtime/pkg/client" "sigs.k8s.io/controller-runtime/pkg/client/fake" ) @@ -611,3 +617,110 @@ func TestApplySharedWithChanges(t *testing.T) { } }) } + +// stubContextAuthenticator satisfies authentication.ContextAuthenticator and +// always authenticates successfully with the supplied response. +type stubContextAuthenticator struct{ resp *authenticator.Response } + +func (s stubContextAuthenticator) AuthenticateContext(_ context.Context) (*authenticator.Response, bool, error) { + return s.resp, true, nil +} + +// stubAttributesGetter satisfies authorization.ContextAttributesGetter and +// returns fixed attributes identifying the authenticated Client object. +type stubAttributesGetter struct{ attrs authorizer.Attributes } + +func (s stubAttributesGetter) ContextAttributes(_ context.Context, _ user.Info) (authorizer.Attributes, error) { + return s.attrs, nil +} + +// stubAuthorizer satisfies authorizer.Authorizer and always allows. +type stubAuthorizer struct{} + +func (stubAuthorizer) Authorize(_ context.Context, _ authorizer.Attributes) (authorizer.Decision, string, error) { + return authorizer.DecisionAllow, "", nil +} + +// authedClientService builds a ClientService whose auth layer authenticates +// every request as owner/namespace, backed by fc for all object lookups. +func authedClientService(owner, namespace string, fc kclient.Client) *ClientService { + resp := &authenticator.Response{ + User: &user.DefaultInfo{Name: "system:serviceaccount:" + namespace + ":" + owner}, + } + attrs := authorizer.AttributesRecord{ + User: resp.User, + Resource: "Client", + Name: owner, + Namespace: namespace, + ResourceRequest: true, + } + a := auth.NewAuth(fc, stubContextAuthenticator{resp: resp}, stubAuthorizer{}, stubAttributesGetter{attrs: attrs}) + return &ClientService{Client: fc, Auth: *a} +} + +func namedClients(namespace string, names ...string) []kclient.Object { + objs := make([]kclient.Object, 0, len(names)) + for _, name := range names { + objs = append(objs, &jumpstarterdevv1alpha1.Client{ + ObjectMeta: metav1.ObjectMeta{Name: name, Namespace: namespace}, + }) + } + return objs +} + +func createLeaseReq(namespace string, sharedWith []string) *cpb.CreateLeaseRequest { + return &cpb.CreateLeaseRequest{ + Parent: "namespaces/" + namespace, + Lease: &cpb.Lease{ + Selector: "board=rpi4", + Duration: durationpb.New(time.Hour), + SharedWith: sharedWith, + }, + } +} + +func TestCreateLeaseSharedWithDedupBeforeLimit(t *testing.T) { + const ns = "default" + + t.Run("raw length over limit but dedup fits is accepted", func(t *testing.T) { + // 12 raw entries, all "alice", dedup to a single entry (<= max). + objs := namedClients(ns, "owner", "alice") + svc := authedClientService("owner", ns, testFakeClient(objs...)) + + var shared []string + for range 12 { + shared = append(shared, "alice") + } + + result, err := svc.CreateLease(context.Background(), createLeaseReq(ns, shared)) + if err != nil { + t.Fatalf("expected dedup within limit to be accepted, got error: %v", err) + } + if len(result.SharedWith) != 1 || result.SharedWith[0] != "alice" { + t.Fatalf("expected deduped shared_with [alice], got %v", result.SharedWith) + } + }) + + t.Run("dedup still exceeding limit is rejected", func(t *testing.T) { + // 11 unique clients plus a duplicate of the first: 12 raw, 11 deduped, + // which still exceeds the maximum of 10. + var names []string + for i := range 11 { + names = append(names, "client"+string(rune('a'+i))) + } + objs := append(namedClients(ns, "owner"), namedClients(ns, names...)...) + svc := authedClientService("owner", ns, testFakeClient(objs...)) + + shared := append([]string{}, names...) + shared = append(shared, names[0]) // duplicate -> 12 raw / 11 deduped + + _, err := svc.CreateLease(context.Background(), createLeaseReq(ns, shared)) + if err == nil { + t.Fatal("expected deduped list exceeding the limit to be rejected") + } + st, ok := status.FromError(err) + if !ok || st.Code() != codes.InvalidArgument { + t.Fatalf("expected InvalidArgument, got %v", err) + } + }) +}