Merge pull request #149 from RafayLabs/even-more-fixes

Fix errors, unique filter, oidc

_kratos/identity.schema.json

@@ -17,11 +17,6 @@
           "type": "string",
           "pattern": "^.*$"
         },
-        "description": {
-          "title": "Description",
-          "type": "string",
-          "pattern": "^.*$"
-        },
         "email": {
           "type": "string",
           "format": "email",

persistence/migrations/admindb/000001_authsrv_partner.up.sql

@@ -28,6 +28,8 @@ ALTER TABLE authsrv_partner OWNER TO admindbuser;
 
 ALTER TABLE ONLY authsrv_partner ADD CONSTRAINT authsrv_partner_pkey PRIMARY KEY (id);
 
+CREATE UNIQUE index authsrv_partner_unique_name ON authsrv_partner (name) WHERE trash IS false;
+
 CREATE INDEX authsrv_partner_name_b6a8d21f ON authsrv_partner USING btree (name);
 
 CREATE INDEX authsrv_partner_name_b6a8d21f_like ON authsrv_partner USING btree (name varchar_pattern_ops);

persistence/migrations/admindb/000003_authsrv_project.up.sql

@@ -14,6 +14,9 @@ ALTER TABLE authsrv_project OWNER TO admindbuser;
 
 ALTER TABLE ONLY authsrv_project ADD CONSTRAINT authsrv_project_pkey PRIMARY KEY (id);
 
+-- update when we have more than one org
+CREATE UNIQUE index authsrv_project_unique_name ON authsrv_project (name) WHERE trash IS false;
+
 CREATE INDEX authsrv_project_name_1b8dd279 ON authsrv_project USING btree (name);
 
 CREATE INDEX authsrv_project_name_1b8dd279_like ON authsrv_project USING btree (name varchar_pattern_ops);
@@ -28,4 +31,4 @@ ALTER TABLE ONLY authsrv_project
 
 ALTER TABLE ONLY authsrv_project
     ADD CONSTRAINT authsrv_project_partner_id_3d505b76_fk_authsrv_partner_id FOREIGN KEY (partner_id) 
-    REFERENCES authsrv_partner(id) DEFERRABLE INITIALLY DEFERRED;
+    REFERENCES authsrv_partner(id) DEFERRABLE INITIALLY DEFERRED;

persistence/migrations/admindb/000029_authsrv_oidc_provider.up.sql

@@ -25,9 +25,9 @@ ALTER TABLE authsrv_oidc_provider OWNER TO admindbuser;
 
 ALTER TABLE ONLY authsrv_oidc_provider ADD CONSTRAINT authsrv_oidc_provider_pkey PRIMARY KEY (id);
 
-ALTER TABLE ONLY authsrv_oidc_provider ADD CONSTRAINT authsrv_oidc_provider_id_name_key UNIQUE (id,name);
+CREATE UNIQUE index authsrv_oidc_provider_issuer_url ON authsrv_oidc_provider (issuer_url) WHERE trash IS false;
 
-ALTER TABLE ONLY authsrv_oidc_provider ADD CONSTRAINT authsrv_oidc_provider_issuer_url_key UNIQUE (issuer_url);
+CREATE UNIQUE index authsrv_oidc_provider_name ON authsrv_oidc_provider (name) WHERE trash IS false;
 
 CREATE INDEX authsrv_oidc_provider_organization_id_4219d6ee ON authsrv_oidc_provider USING btree (organization_id);
 

pkg/controller/strategicpatch/patch.go

@@ -685,7 +685,7 @@ func diffListsOfScalars(original, modified []interface{}, diffOptions DiffOption
 			}
 			modifiedIndex++
 		default:
-			return nil, nil, fmt.Errorf("Unexpected returned value from compareListValuesAtIndex: %v and %v", originalV, modifiedV)
+			return nil, nil, fmt.Errorf("unexpected returned value from compareListValuesAtIndex: %v and %v", originalV, modifiedV)
 		}
 	}
 
@@ -1022,7 +1022,7 @@ func validatePatchWithSetOrderList(patchList, setOrderList interface{}, mergeKey
 	// If patchIndex is inbound but setOrderIndex if out of bound mean there are items mismatching between the patch list and setElementOrder list.
 	// the second check is is a sanity check, and should always be true if the first is true.
 	if patchIndex < len(nonDeleteList) && setOrderIndex >= len(typedSetOrderList) {
-		return fmt.Errorf("The order in patch list:\n%v\n doesn't match %s list:\n%v\n", typedPatchList, setElementOrderDirectivePrefix, setOrderList)
+		return fmt.Errorf("order in patch list:\n%v\n doesn't match %s list:\n%v\n", typedPatchList, setElementOrderDirectivePrefix, setOrderList)
 	}
 	typedPatchList = append(nonDeleteList, toDeleteList...)
 	return nil

pkg/sentry/authz/authz.go

@@ -411,7 +411,7 @@ func GetAuthorization(ctx context.Context, req *sentryrpc.GetUserAuthorizationRe
 				return nil, err
 			}
 			if !active {
-				return nil, fmt.Errorf("Error: kubeconfig user deactivated")
+				return nil, fmt.Errorf("kubeconfig user deactivated")
 			}
 		}
 
@@ -420,7 +420,7 @@ func GetAuthorization(ctx context.Context, req *sentryrpc.GetUserAuthorizationRe
 		if err != nil && err != constants.ErrNotFound {
 			return nil, err
 		} else if err == nil && kr.RevokedAt.AsTime().Unix() >= req.CertIssueSeconds {
-			return nil, fmt.Errorf("Error: kubeconfig revoked")
+			return nil, fmt.Errorf("kubeconfig revoked")
 		}
 	}
 

pkg/sentry/register/register.go

@@ -250,7 +250,7 @@ func prepareConfig(config *Config) error {
 	}
 
 	if config.TemplateToken == "" && config.Mode == modeClient {
-		return fmt.Errorf("TemplateToken cannot be empty")
+		return fmt.Errorf("template token cannot be empty")
 	}
 
 	if config.PrivateKey == nil {

pkg/service/idp.go

@@ -123,10 +123,10 @@ func (s *idpService) Create(ctx context.Context, idp *systemv3.Idp) (*systemv3.I
 
 	// validate name and domain
 	if len(name) == 0 {
-		return &systemv3.Idp{}, fmt.Errorf("EMPTY NAME")
+		return &systemv3.Idp{}, fmt.Errorf("empty name for idp provider")
 	}
 	if len(domain) == 0 {
-		return &systemv3.Idp{}, fmt.Errorf("EMPTY DOMAIN")
+		return &systemv3.Idp{}, fmt.Errorf("empty domain for idp provider")
 	}
 
 	partnerId, organizationId, err := s.getPartnerOrganization(ctx, idp)
@@ -142,13 +142,13 @@ func (s *idpService) Create(ctx context.Context, idp *systemv3.Idp) (*systemv3.I
 		&models.Idp{},
 	)
 	if i != nil {
-		return nil, fmt.Errorf("Idp %q already exists", idp.GetMetadata().GetName())
+		return nil, fmt.Errorf("idp %q already exists", idp.GetMetadata().GetName())
 	}
 
 	e := &models.Idp{}
 	dao.GetX(ctx, s.db, "domain", domain, e)
 	if e.Domain == domain {
-		return &systemv3.Idp{}, fmt.Errorf("DUPLICATE DOMAIN")
+		return &systemv3.Idp{}, fmt.Errorf("duplicate idp domain")
 	}
 
 	entity := &models.Idp{

pkg/service/metro_test.go

@@ -98,7 +98,7 @@ func TestMetroDeleteNonExist(t *testing.T) {
 	puuid := uuid.New().String()
 
 	mock.ExpectQuery(`SELECT "metro"."id", "metro"."name", .* FROM "cluster_metro" AS "metro" WHERE`).
-		WithArgs().WillReturnError(fmt.Errorf("No data available"))
+		WithArgs().WillReturnError(fmt.Errorf("no data available"))
 
 	metro := &infrav3.Location{
 		Metadata: &commonv3.Metadata{Id: puuid, Name: "metro-" + puuid},

pkg/service/oidc_provider.go

@@ -83,15 +83,15 @@ func (s *oidcProvider) getPartnerOrganization(ctx context.Context, provider *sys
 func (s *oidcProvider) Create(ctx context.Context, provider *systemv3.OIDCProvider) (*systemv3.OIDCProvider, error) {
 	name := provider.GetMetadata().GetName()
 	if len(name) == 0 {
-		return &systemv3.OIDCProvider{}, fmt.Errorf("EMPTY NAME")
+		return &systemv3.OIDCProvider{}, fmt.Errorf("empty name for provider")
 	}
 	scopes := provider.GetSpec().GetScopes()
-	if scopes == nil || len(scopes) == 0 {
-		return &systemv3.OIDCProvider{}, fmt.Errorf("NO SCOPES")
+	if len(scopes) == 0 {
+		return &systemv3.OIDCProvider{}, fmt.Errorf("no scopes present")
 	}
 	issUrl := provider.GetSpec().GetIssuerUrl()
 	if len(issUrl) == 0 {
-		return &systemv3.OIDCProvider{}, fmt.Errorf("EMPTY ISSUER URL")
+		return &systemv3.OIDCProvider{}, fmt.Errorf("empty issuer url")
 	}
 
 	partnerId, organizationId, err := s.getPartnerOrganization(ctx, provider)
@@ -107,19 +107,20 @@ func (s *oidcProvider) Create(ctx context.Context, provider *systemv3.OIDCProvid
 		&models.OIDCProvider{},
 	)
 	if p != nil {
-		return nil, fmt.Errorf("OIDC provider %q already exists", name)
+		return nil, fmt.Errorf("provider %q already exists", name)
 	}
 
 	p, _ = dao.GetM(ctx, s.db, map[string]interface{}{
 		"issuer_url":      issUrl,
 		"partner_id":      partnerId,
 		"organization_id": organizationId,
+		"trash":           false,
 	}, &models.OIDCProvider{})
 	if p != nil {
-		return nil, fmt.Errorf("DUPLICATE ISSUER URL")
+		return nil, fmt.Errorf("duplicate issuer url")
 	}
 	if !validateURL(issUrl) {
-		return &systemv3.OIDCProvider{}, fmt.Errorf("INVALID ISSUER URL")
+		return &systemv3.OIDCProvider{}, fmt.Errorf("invalid issuer url")
 	}
 
 	mapUrl := provider.Spec.GetMapperUrl()
@@ -127,13 +128,13 @@ func (s *oidcProvider) Create(ctx context.Context, provider *systemv3.OIDCProvid
 	tknUrl := provider.Spec.GetTokenUrl()
 
 	if len(mapUrl) != 0 && !validateURL(mapUrl) {
-		return &systemv3.OIDCProvider{}, fmt.Errorf("INVALID MAPPER URL")
+		return &systemv3.OIDCProvider{}, fmt.Errorf("invalid mapper url")
 	}
 	if len(authUrl) != 0 && !validateURL(authUrl) {
-		return &systemv3.OIDCProvider{}, fmt.Errorf("INVALID AUTH URL")
+		return &systemv3.OIDCProvider{}, fmt.Errorf("invalid auth url")
 	}
 	if len(tknUrl) != 0 && !validateURL(tknUrl) {
-		return &systemv3.OIDCProvider{}, fmt.Errorf("INVALID TOKEN URL")
+		return &systemv3.OIDCProvider{}, fmt.Errorf("invalid token url")
 	}
 
 	entity := &models.OIDCProvider{
@@ -325,15 +326,15 @@ func (s *oidcProvider) List(ctx context.Context) (*systemv3.OIDCProviderList, er
 func (s *oidcProvider) Update(ctx context.Context, provider *systemv3.OIDCProvider) (*systemv3.OIDCProvider, error) {
 	name := provider.GetMetadata().GetName()
 	if len(name) == 0 {
-		return &systemv3.OIDCProvider{}, status.Error(codes.InvalidArgument, "EMPTY NAME")
+		return &systemv3.OIDCProvider{}, status.Error(codes.InvalidArgument, "empty name")
 	}
 	scopes := provider.GetSpec().GetScopes()
-	if scopes == nil || len(scopes) == 0 {
-		return &systemv3.OIDCProvider{}, fmt.Errorf("NO SCOPES")
+	if len(scopes) == 0 {
+		return &systemv3.OIDCProvider{}, fmt.Errorf("no scopes")
 	}
 	issUrl := provider.GetSpec().GetIssuerUrl()
 	if len(issUrl) == 0 {
-		return &systemv3.OIDCProvider{}, fmt.Errorf("EMPTY ISSUER URL")
+		return &systemv3.OIDCProvider{}, fmt.Errorf("empty issuer url")
 	}
 
 	partnerId, organizationId, err := s.getPartnerOrganization(ctx, provider)
@@ -345,7 +346,7 @@ func (s *oidcProvider) Update(ctx context.Context, provider *systemv3.OIDCProvid
 	_, err = dao.GetByName(ctx, s.db, name, existingP)
 	if err != nil {
 		if errors.Is(err, sql.ErrNoRows) {
-			return &systemv3.OIDCProvider{}, status.Errorf(codes.InvalidArgument, "OIDC PROVIDER %q NOT EXIST", name)
+			return &systemv3.OIDCProvider{}, status.Errorf(codes.InvalidArgument, "oidc provider %q not exist", name)
 		} else {
 			return &systemv3.OIDCProvider{}, status.Error(codes.Internal, codes.Internal.String())
 		}
@@ -356,16 +357,16 @@ func (s *oidcProvider) Update(ctx context.Context, provider *systemv3.OIDCProvid
 	tknUrl := provider.Spec.GetTokenUrl()
 
 	if !validateURL(issUrl) {
-		return &systemv3.OIDCProvider{}, fmt.Errorf("INVALID ISSUER URL")
+		return &systemv3.OIDCProvider{}, fmt.Errorf("invalid issuer url")
 	}
 	if len(mapUrl) != 0 && !validateURL(mapUrl) {
-		return &systemv3.OIDCProvider{}, fmt.Errorf("INVALID MAPPER URL")
+		return &systemv3.OIDCProvider{}, fmt.Errorf("invalid mapper url")
 	}
 	if len(authUrl) != 0 && !validateURL(authUrl) {
-		return &systemv3.OIDCProvider{}, fmt.Errorf("INVALID AUTH URL")
+		return &systemv3.OIDCProvider{}, fmt.Errorf("invalid auth url")
 	}
 	if len(tknUrl) != 0 && !validateURL(tknUrl) {
-		return &systemv3.OIDCProvider{}, fmt.Errorf("INVALID TOKEN URL")
+		return &systemv3.OIDCProvider{}, fmt.Errorf("invalid token url")
 	}
 
 	entity := &models.OIDCProvider{
@@ -388,7 +389,9 @@ func (s *oidcProvider) Update(ctx context.Context, provider *systemv3.OIDCProvid
 	}
 	_, err = dao.Update(ctx, s.db, existingP.Id, entity)
 	if err != nil {
-		return &systemv3.OIDCProvider{}, err
+		_log.Errorf("Unable to create oidc provider: %s", err)
+		// TODO: catch already existing issuer url and return exact error
+		return &systemv3.OIDCProvider{}, fmt.Errorf("unable to create oidc provider")
 	}
 
 	rclaims, _ := structpb.NewStruct(entity.RequestedClaims)

pkg/service/organization_test.go

@@ -97,7 +97,7 @@ func TestOrganizationDeleteNonExist(t *testing.T) {
 	ouuid := uuid.New().String()
 
 	mock.ExpectQuery(`SELECT "organization"."id", "organization"."name", .* FROM "authsrv_organization" AS "organization" WHERE`).
-		WithArgs().WillReturnError(fmt.Errorf("No data available"))
+		WithArgs().WillReturnError(fmt.Errorf("no data available"))
 
 	organization := &systemv3.Organization{
 		Metadata: &v3.Metadata{Id: ouuid, Name: "organization-" + ouuid},

pkg/service/partner_test.go

@@ -109,7 +109,7 @@ func TestPartnerDeleteNonExist(t *testing.T) {
 	puuid := uuid.New().String()
 
 	mock.ExpectQuery(`SELECT "partner"."id", "partner"."name", .* FROM "authsrv_partner" AS "partner" WHERE`).
-		WithArgs().WillReturnError(fmt.Errorf("No data available"))
+		WithArgs().WillReturnError(fmt.Errorf("no data available"))
 
 	partner := &systemv3.Partner{
 		Metadata: &v3.Metadata{Id: puuid, Name: "partner-" + puuid},

pkg/service/project.go

@@ -64,6 +64,11 @@ func (s *projectService) Create(ctx context.Context, project *systemv3.Project)
 		return nil, err
 	}
 
+	p, _ := dao.GetIdByNamePartnerOrg(ctx, s.db, project.GetMetadata().GetName(), uuid.NullUUID{}, uuid.NullUUID{}, &models.Project{})
+	if p != nil {
+		return nil, fmt.Errorf("project '%v' already exists", project.GetMetadata().GetName())
+	}
+
 	//convert v3 spec to internal models
 	proj := models.Project{
 		Name:           project.GetMetadata().GetName(),

pkg/service/project_test.go

@@ -106,7 +106,7 @@ func TestProjectDeleteNonExist(t *testing.T) {
 	puuid := uuid.New().String()
 
 	mock.ExpectQuery(`SELECT "project"."id", "project"."name", .* FROM "authsrv_project" AS "project" WHERE`).
-		WithArgs().WillReturnError(fmt.Errorf("No data available"))
+		WithArgs().WillReturnError(fmt.Errorf("no data available"))
 
 	project := &systemv3.Project{
 		Metadata: &v3.Metadata{Id: puuid, Name: "project-" + puuid},

pkg/service/role_test.go

@@ -300,7 +300,7 @@ func TestRoleDeleteNonExist(t *testing.T) {
 	ouuid := uuid.New().String()
 
 	mock.ExpectQuery(`SELECT "resourcerole"."id", "resourcerole"."name", .* FROM "authsrv_resourcerole" AS "resourcerole" WHERE`).
-		WithArgs().WillReturnError(fmt.Errorf("No data available"))
+		WithArgs().WillReturnError(fmt.Errorf("no data available"))
 
 	role := &rolev3.Role{
 		Metadata: &v3.Metadata{Partner: "partner-" + puuid, Organization: "org-" + ouuid, Name: "role-" + ruuid},

pkg/service/user.go

@@ -975,12 +975,12 @@ func (s *userService) UpdateIdpUserGroupPolicy(ctx context.Context, op, id, trai
 	}
 	err = json.Unmarshal([]byte(traits), &userInfo)
 	if err != nil {
-		return fmt.Errorf("Encountered error unmarshing payload to userInfo: %s", err)
+		return fmt.Errorf("encountered error unmarshing payload to userInfo: %s", err)
 	}
 	// TODO: Revisit to only run by IDP users and not by any other
 	// user
 	if len(userInfo.IdpGroups) == 0 {
-		return fmt.Errorf("Empty idp groups for user with id %s", id)
+		return fmt.Errorf("empty idp groups for user with id %s", id)
 	}
 
 	// Get existing user group so that the update does not wipe them out
@@ -992,7 +992,7 @@ func (s *userService) UpdateIdpUserGroupPolicy(ctx context.Context, op, id, trai
 		}
 	}
 	if err != nil {
-		return fmt.Errorf("Empty to find existing groups for user with id %s", id)
+		return fmt.Errorf("empty to find existing groups for user with id %s", id)
 	}
 	user = &userv3.User{
 		Metadata: &v3.Metadata{
@@ -1025,7 +1025,7 @@ func (s *userService) UpdateIdpUserGroupPolicy(ctx context.Context, op, id, trai
 			return err
 		}
 	default:
-		return fmt.Errorf("Unsupported %s operation in payload", op)
+		return fmt.Errorf("unsupported %s operation in payload", op)
 	}
 	return nil
 }

server/user.go

@@ -8,7 +8,6 @@ import (
 	"github.com/RafayLabs/rcloud-base/pkg/query"
 	"github.com/RafayLabs/rcloud-base/pkg/service"
 	rpcv3 "github.com/RafayLabs/rcloud-base/proto/rpc/user"
-	commonv3 "github.com/RafayLabs/rcloud-base/proto/types/commonpb/v3"
 	v3 "github.com/RafayLabs/rcloud-base/proto/types/commonpb/v3"
 	userpbv3 "github.com/RafayLabs/rcloud-base/proto/types/userpb/v3"
 	"google.golang.org/protobuf/types/known/timestamppb"
@@ -41,7 +40,7 @@ func (s *userServer) CreateUser(ctx context.Context, req *userpbv3.User) (*userp
 	return updateUserStatus(req, resp, err), err
 }
 
-func (s *userServer) GetUsers(ctx context.Context, req *commonv3.QueryOptions) (*userpbv3.UserList, error) {
+func (s *userServer) GetUsers(ctx context.Context, req *v3.QueryOptions) (*userpbv3.UserList, error) {
 	return s.us.List(ctx, query.WithOptions(req))
 }
 
@@ -73,7 +72,7 @@ func (s *userServer) UpdateUser(ctx context.Context, req *userpbv3.User) (*userp
 	return updateUserStatus(req, resp, err), err
 }
 
-func (s *userServer) DownloadCliConfig(ctx context.Context, req *rpcv3.CliConfigRequest) (*commonv3.HttpBody, error) {
+func (s *userServer) DownloadCliConfig(ctx context.Context, req *rpcv3.CliConfigRequest) (*v3.HttpBody, error) {
 	sessData, ok := service.GetSessionDataFromContext(ctx)
 	if !ok {
 		return nil, fmt.Errorf("unable to retrieve session data")
@@ -92,7 +91,7 @@ func (s *userServer) DownloadCliConfig(ctx context.Context, req *rpcv3.CliConfig
 		return nil, err
 	}
 
-	return &commonv3.HttpBody{
+	return &v3.HttpBody{
 		ContentType: "application/json",
 		Data:        bb,
 	}, nil