From 7938f306682ba03ca8227ab8a5ccd4f8f2b936a6 Mon Sep 17 00:00:00 2001 From: akshay196-rafay <93963751+akshay196-rafay@users.noreply.github.com> Date: Mon, 7 Mar 2022 19:06:57 +0530 Subject: [PATCH] Update IdP and OIDC Provider (#40) * Update creating oidc provider * Validate partner and organization by checking in db tables. * Validate empty scopes * Fix oidc provider update * Fix application Host url in ACS URL of Idp --- internal/models/oidc_provider.go | 2 +- main.go | 7 ++- pkg/service/idp.go | 57 ++++++++++++++----- pkg/service/oidc_provider.go | 96 ++++++++++++++++++++------------ 4 files changed, 108 insertions(+), 54 deletions(-) diff --git a/internal/models/oidc_provider.go b/internal/models/oidc_provider.go index 1d03901..1d5e325 100644 --- a/internal/models/oidc_provider.go +++ b/internal/models/oidc_provider.go @@ -23,7 +23,7 @@ type OIDCProvider struct { MapperFilename string `bun:"mapper_filename"` ClientId string `bun:"client_id,notnull"` ClientSecret string `bun:"client_secret,notnull"` - Scopes []string `bun:"scopes,notnull"` + Scopes []string `bun:"scopes,array,notnull"` IssuerURL string `bun:"issuer_url,notnull"` AuthURL string `bun:"auth_url"` TokenURL string `bun:"token_url"` diff --git a/main.go b/main.go index bcb2f43..ca38eb6 100644 --- a/main.go +++ b/main.go @@ -160,10 +160,10 @@ func setup() { viper.BindEnv(debugPortEnv) viper.BindEnv(dbAddrEnv) viper.BindEnv(dbNameEnv) - viper.BindEnv(dbPasswordEnv) + viper.BindEnv(dbUserEnv) viper.BindEnv(dbPasswordEnv) viper.BindEnv(devEnv) - viper.BindEnv(dbUserEnv) + viper.BindEnv(apiAddrEnv) viper.BindEnv(kratosSchemeEnv) viper.BindEnv(kratosAddrEnv) viper.BindEnv(bootstrapKEKEnv) @@ -185,6 +185,7 @@ func setup() { dbName = viper.GetString(dbNameEnv) dbUser = viper.GetString(dbUserEnv) dbPassword = viper.GetString(dbPasswordEnv) + apiAddr = viper.GetString(apiAddrEnv) dev = viper.GetBool(devEnv) kratosScheme = viper.GetString(kratosSchemeEnv) kratosAddr = viper.GetString(kratosAddrEnv) @@ -245,7 +246,7 @@ func setup() { gs = service.NewGroupService(db, as) rs = service.NewRoleService(db, as) rrs = service.NewRolepermissionService(db) - is = service.NewIdpService(db, apiAddrEnv) + is = service.NewIdpService(db, apiAddr) oidcs = service.NewOIDCProviderService(db, kratosUrl) //sentry related services diff --git a/pkg/service/idp.go b/pkg/service/idp.go index e798c3a..6809df9 100644 --- a/pkg/service/idp.go +++ b/pkg/service/idp.go @@ -16,6 +16,7 @@ import ( "github.com/RafaySystems/rcloud-base/internal/models" "github.com/RafaySystems/rcloud-base/internal/persistence/provider/pg" + "github.com/RafaySystems/rcloud-base/internal/utils" commonv3 "github.com/RafaySystems/rcloud-base/proto/types/commonpb/v3" systemv3 "github.com/RafaySystems/rcloud-base/proto/types/systempb/v3" "github.com/google/uuid" @@ -36,18 +37,20 @@ type IdpService interface { type idpService struct { dao pg.EntityDAO appHost string + l utils.Lookup } func NewIdpService(db *bun.DB, hostUrl string) IdpService { return &idpService{ dao: pg.NewEntityDAO(db), appHost: hostUrl, + l: utils.NewLookup(db), } } func generateAcsURL(id string, hostUrl string) string { b, _ := url.Parse(hostUrl) - return fmt.Sprintf("%s://%s/auth/v3/sso/acs/%s", b.Scheme, b.Host, id) + return fmt.Sprintf("%s/auth/v3/sso/acs/%s", b.String(), id) } // generateSpCert generates self signed certificate. Returns cert and @@ -104,6 +107,20 @@ func generateSpCert(host string) (string, string, error) { return string(cPEMBytes), string(privPEMBytes), nil } +func (s *idpService) getPartnerOrganization(ctx context.Context, provider *systemv3.Idp) (uuid.UUID, uuid.UUID, error) { + partner := provider.GetMetadata().GetPartner() + org := provider.GetMetadata().GetOrganization() + partnerId, err := s.l.GetPartnerId(ctx, partner) + if err != nil { + return uuid.Nil, uuid.Nil, err + } + organizationId, err := s.l.GetOrganizationId(ctx, org) + if err != nil { + return partnerId, uuid.Nil, err + } + return partnerId, organizationId, nil +} + func (s *idpService) Create(ctx context.Context, idp *systemv3.Idp) (*systemv3.Idp, error) { name := idp.Metadata.GetName() domain := idp.Spec.GetDomain() @@ -115,11 +132,23 @@ func (s *idpService) Create(ctx context.Context, idp *systemv3.Idp) (*systemv3.I if len(domain) == 0 { return &systemv3.Idp{}, fmt.Errorf("EMPTY DOMAIN") } - e := &models.Idp{} - s.dao.GetByName(ctx, name, e) - if e.Name == name { - return &systemv3.Idp{}, fmt.Errorf("DUPLICATE NAME") + + partnerId, organizationId, err := s.getPartnerOrganization(ctx, idp) + if err != nil { + return nil, fmt.Errorf("unable to get partner and org id") } + i, _ := s.dao.GetIdByNamePartnerOrg( + ctx, + idp.GetMetadata().GetName(), + uuid.NullUUID{UUID: partnerId, Valid: true}, + uuid.NullUUID{UUID: organizationId, Valid: true}, + &models.Idp{}, + ) + if i != nil { + return nil, fmt.Errorf("Idp %q already exists", idp.GetMetadata().GetName()) + } + + e := &models.Idp{} s.dao.GetX(ctx, "domain", domain, e) if e.Domain == domain { return &systemv3.Idp{}, fmt.Errorf("DUPLICATE DOMAIN") @@ -129,6 +158,8 @@ func (s *idpService) Create(ctx context.Context, idp *systemv3.Idp) (*systemv3.I Name: name, Description: idp.Metadata.GetDescription(), CreatedAt: time.Now(), + PartnerId: partnerId, + OrganizationId: organizationId, IdpName: idp.Spec.GetIdpName(), Domain: domain, SsoURL: idp.Spec.GetSsoUrl(), @@ -150,7 +181,7 @@ func (s *idpService) Create(ctx context.Context, idp *systemv3.Idp) (*systemv3.I entity.SpCert = spcert entity.SpKey = spkey } - _, err := s.dao.Create(ctx, entity) + _, err = s.dao.Create(ctx, entity) if err != nil { return &systemv3.Idp{}, err } @@ -160,10 +191,8 @@ func (s *idpService) Create(ctx context.Context, idp *systemv3.Idp) (*systemv3.I ApiVersion: apiVersion, Kind: "Idp", Metadata: &commonv3.Metadata{ - Name: entity.Name, - Organization: entity.OrganizationId.String(), - Partner: entity.PartnerId.String(), - Id: entity.Id.String(), + Name: entity.Name, + Id: entity.Id.String(), }, Spec: &systemv3.IdpSpec{ IdpName: entity.IdpName, @@ -333,15 +362,13 @@ func (s *idpService) Update(ctx context.Context, idp *systemv3.Idp) (*systemv3.I return &systemv3.Idp{}, err } - acsURL := generateAcsURL(entity.Id.String(), s.appHost) + acsURL := generateAcsURL(idp.GetMetadata().GetId(), s.appHost) rv := &systemv3.Idp{ ApiVersion: apiVersion, Kind: "Idp", Metadata: &commonv3.Metadata{ - Name: entity.Name, - Organization: entity.OrganizationId.String(), - Partner: entity.PartnerId.String(), - Id: entity.Id.String(), + Name: entity.Name, + Id: idp.GetMetadata().GetId(), }, Spec: &systemv3.IdpSpec{ IdpName: entity.IdpName, diff --git a/pkg/service/oidc_provider.go b/pkg/service/oidc_provider.go index fc591ef..e50d568 100644 --- a/pkg/service/oidc_provider.go +++ b/pkg/service/oidc_provider.go @@ -10,6 +10,7 @@ import ( "github.com/RafaySystems/rcloud-base/internal/models" "github.com/RafaySystems/rcloud-base/internal/persistence/provider/pg" + "github.com/RafaySystems/rcloud-base/internal/utils" commonv3 "github.com/RafaySystems/rcloud-base/proto/types/commonpb/v3" systemv3 "github.com/RafaySystems/rcloud-base/proto/types/systempb/v3" "github.com/google/uuid" @@ -31,12 +32,14 @@ type OIDCProviderService interface { type oidcProvider struct { dao pg.EntityDAO kratosUrl string + l utils.Lookup } func NewOIDCProviderService(db *bun.DB, kratosUrl string) OIDCProviderService { return &oidcProvider{ dao: pg.NewEntityDAO(db), kratosUrl: kratosUrl, + l: utils.NewLookup(db), } } @@ -50,16 +53,43 @@ func validateURL(rawURL string) error { return err } +func (s *oidcProvider) getPartnerOrganization(ctx context.Context, provider *systemv3.OIDCProvider) (uuid.UUID, uuid.UUID, error) { + partner := provider.GetMetadata().GetPartner() + org := provider.GetMetadata().GetOrganization() + partnerId, err := s.l.GetPartnerId(ctx, partner) + if err != nil { + return uuid.Nil, uuid.Nil, err + } + organizationId, err := s.l.GetOrganizationId(ctx, org) + if err != nil { + return partnerId, uuid.Nil, err + } + return partnerId, organizationId, nil +} + func (s *oidcProvider) Create(ctx context.Context, provider *systemv3.OIDCProvider) (*systemv3.OIDCProvider, error) { - // validate name name := provider.Metadata.GetName() if len(name) == 0 { return &systemv3.OIDCProvider{}, fmt.Errorf("EMPTY NAME") } - e := &models.OIDCProvider{} - s.dao.GetByName(ctx, name, e) - if e.Name == name { - return &systemv3.OIDCProvider{}, fmt.Errorf("DUPLICATE NAME") + scopes := provider.GetSpec().GetScopes() + if scopes == nil || len(scopes) == 0 { + return &systemv3.OIDCProvider{}, fmt.Errorf("EMPTY SCOPES") + } + + partnerId, organizationId, err := s.getPartnerOrganization(ctx, provider) + if err != nil { + return nil, fmt.Errorf("unable to get partner and org id") + } + p, _ := s.dao.GetIdByNamePartnerOrg( + ctx, + provider.GetMetadata().GetName(), + uuid.NullUUID{UUID: partnerId, Valid: true}, + uuid.NullUUID{UUID: organizationId, Valid: true}, + &models.OIDCProvider{}, + ) + if p != nil { + return nil, fmt.Errorf("OIDC provider %q already exists", provider.GetMetadata().GetName()) } mapUrl := provider.Spec.GetMapperUrl() @@ -81,15 +111,17 @@ func (s *oidcProvider) Create(ctx context.Context, provider *systemv3.OIDCProvid } entity := &models.OIDCProvider{ - Name: name, - CreatedAt: time.Time{}, - ModifiedAt: time.Time{}, - ProviderName: provider.Spec.GetProviderName(), - MapperURL: mapUrl, - MapperFilename: provider.Spec.GetMapperFilename(), - ClientId: provider.Spec.GetClientId(), - // ClientSecret is not returning to avoid leaking secret - // TODO: Use FieldMask on ClientSecret field + Name: name, + Description: provider.GetMetadata().GetDescription(), + CreatedAt: time.Time{}, + ModifiedAt: time.Time{}, + PartnerId: partnerId, + OrganizationId: organizationId, + ProviderName: provider.Spec.GetProviderName(), + MapperURL: mapUrl, + MapperFilename: provider.Spec.GetMapperFilename(), + ClientId: provider.Spec.GetClientId(), + ClientSecret: provider.Spec.GetClientSecret(), Scopes: provider.Spec.GetScopes(), IssuerURL: issUrl, AuthURL: authUrl, @@ -97,7 +129,7 @@ func (s *oidcProvider) Create(ctx context.Context, provider *systemv3.OIDCProvid RequestedClaims: provider.Spec.GetRequestedClaims().AsMap(), Predefined: provider.Spec.GetPredefined(), } - _, err := s.dao.Create(ctx, entity) + _, err = s.dao.Create(ctx, entity) if err != nil { return &systemv3.OIDCProvider{}, err } @@ -116,7 +148,6 @@ func (s *oidcProvider) Create(ctx context.Context, provider *systemv3.OIDCProvid MapperUrl: entity.MapperURL, MapperFilename: entity.MapperFilename, ClientId: entity.ClientId, - ClientSecret: entity.ClientSecret, Scopes: entity.Scopes, IssuerUrl: entity.IssuerURL, AuthUrl: entity.AuthURL, @@ -156,7 +187,6 @@ func (s *oidcProvider) GetByID(ctx context.Context, provider *systemv3.OIDCProvi MapperUrl: entity.MapperURL, MapperFilename: entity.MapperFilename, ClientId: entity.ClientId, - ClientSecret: entity.ClientSecret, Scopes: entity.Scopes, IssuerUrl: entity.IssuerURL, AuthUrl: entity.AuthURL, @@ -203,7 +233,6 @@ func (s *oidcProvider) GetByName(ctx context.Context, provider *systemv3.OIDCPro MapperUrl: entity.MapperURL, MapperFilename: entity.MapperFilename, ClientId: entity.ClientId, - ClientSecret: entity.ClientSecret, Scopes: entity.Scopes, IssuerUrl: entity.IssuerURL, AuthUrl: entity.AuthURL, @@ -267,9 +296,18 @@ func (s *oidcProvider) Update(ctx context.Context, provider *systemv3.OIDCProvid if len(name) == 0 { return &systemv3.OIDCProvider{}, status.Error(codes.InvalidArgument, "EMPTY NAME") } + scopes := provider.GetSpec().GetScopes() + if scopes == nil || len(scopes) == 0 { + return &systemv3.OIDCProvider{}, fmt.Errorf("EMPTY SCOPES") + } + + partnerId, organizationId, err := s.getPartnerOrganization(ctx, provider) + if err != nil { + return nil, fmt.Errorf("unable to get partner and org id") + } existingP := &models.OIDCProvider{} - _, err := s.dao.GetByName(ctx, name, existingP) + _, err = s.dao.GetByName(ctx, name, existingP) if err != nil { if errors.Is(err, sql.ErrNoRows) { return &systemv3.OIDCProvider{}, status.Errorf(codes.InvalidArgument, "OIDC PROVIDER %q NOT EXIST", name) @@ -296,22 +334,11 @@ func (s *oidcProvider) Update(ctx context.Context, provider *systemv3.OIDCProvid return &systemv3.OIDCProvider{}, fmt.Errorf("INVALID TOKEN URL") } - orgId, err := uuid.Parse(provider.Metadata.GetOrganization()) - if err != nil { - return &systemv3.OIDCProvider{}, status.Errorf(codes.InvalidArgument, - "ORG ID %q INCORRECT", provider.Metadata.GetOrganization()) - } - partId, err := uuid.Parse(provider.Metadata.GetPartner()) - if err != nil { - return &systemv3.OIDCProvider{}, status.Errorf(codes.InvalidArgument, - "PARTNER ID %q INCORRECT", provider.Metadata.GetPartner()) - } - entity := &models.OIDCProvider{ Name: provider.Metadata.GetName(), Description: provider.Metadata.GetDescription(), - OrganizationId: orgId, - PartnerId: partId, + OrganizationId: organizationId, + PartnerId: partnerId, ModifiedAt: time.Now(), ProviderName: provider.Spec.GetProviderName(), MapperURL: mapUrl, @@ -337,21 +364,20 @@ func (s *oidcProvider) Update(ctx context.Context, provider *systemv3.OIDCProvid Metadata: &commonv3.Metadata{ Name: entity.Name, Description: entity.Description, - Id: entity.Id.String(), + Id: provider.GetMetadata().GetId(), }, Spec: &systemv3.OIDCProviderSpec{ ProviderName: entity.ProviderName, MapperUrl: entity.MapperURL, MapperFilename: entity.MapperFilename, ClientId: entity.ClientId, - ClientSecret: entity.ClientSecret, Scopes: entity.Scopes, IssuerUrl: entity.IssuerURL, AuthUrl: entity.AuthURL, TokenUrl: entity.TokenURL, RequestedClaims: rclaims, Predefined: entity.Predefined, - CallbackUrl: generateCallbackUrl(entity.Id.String(), s.kratosUrl), + CallbackUrl: generateCallbackUrl(provider.GetMetadata().GetId(), s.kratosUrl), }, } return rv, nil