Skip to content
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.

Sign up for GitHub

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

[installer]: add validation rules to blockNewUsers in config block #13126

Merged
merged 1 commit into from
Sep 21, 2022
Merged

[installer]: add validation rules to blockNewUsers in config block #13126

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 3 additions & 2 deletions install/installer/pkg/config/v1/config.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -403,8 +403,9 @@ const (
)

type BlockNewUsers struct {
Enabled bool `json:"enabled"`
Passlist []string `json:"passlist"`
Enabled bool `json:"enabled"`
// Passlist []string `json:"passlist" validate:"min=1,unique,dive,fqdn"`
Passlist []string `json:"passlist" validate:"block_new_users_passlist"`
}

// AuthProviderConfigs this only contains what is necessary for validation
Expand Down
33 changes: 33 additions & 0 deletions install/installer/pkg/config/v1/validation.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@ package config

import (
"fmt"
"regexp"

"github.com/gitpod-io/gitpod/installer/pkg/cluster"
"github.com/gitpod-io/gitpod/installer/pkg/config/v1/experimental"
Expand Down Expand Up @@ -65,6 +66,38 @@ func (v version) LoadValidationFuncs(validate *validator.Validate) error {
_, ok := LogLevelList[LogLevel(fl.Field().String())]
return ok
},
"block_new_users_passlist": func(fl validator.FieldLevel) bool {
if !fl.Parent().FieldByName("Enabled").Bool() {
// Not enabled - it's valid
return true
}

if fl.Field().Len() == 0 {
// No exceptions
return false
}

// Use same regex as "fqdn"
// @link https://github.com/go-playground/validator/blob/c7e0172e0fd176bdc521afb5186818a7db6b77ac/regexes.go#L52
fqdnRegexStringRFC1123 := `^([a-zA-Z0-9]{1}[a-zA-Z0-9-]{0,62})(\.[a-zA-Z0-9]{1}[a-zA-Z0-9-]{0,62})*?(\.[a-zA-Z]{1}[a-zA-Z0-9]{0,62})\.?$`
fqdnRegexRFC1123 := regexp.MustCompile(fqdnRegexStringRFC1123)

for i := 0; i < fl.Field().Len(); i++ {
val := fl.Field().Index(i).String()

if val == "" {
// Empty value
return false
}

// Check that it validates as a fully-qualified domain name
valid := fqdnRegexRFC1123.MatchString(val)
if !valid {
return false
}
}
return true
},
}

for k, v := range experimental.ValidationChecks {
Expand Down
2 changes: 2 additions & 0 deletions install/installer/pkg/config/validation.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -55,6 +55,8 @@ func Validate(version ConfigVersion, cfg interface{}) (r *ValidationResult, err
res.Fatal = append(res.Fatal, fmt.Sprintf("Field '%s' is %s '%s'", v.Namespace(), tag, v.Param()))
case "startswith":
res.Fatal = append(res.Fatal, fmt.Sprintf("Field '%s' must start with '%s'", v.Namespace(), v.Param()))
case "block_new_users_passlist":
res.Fatal = append(res.Fatal, fmt.Sprintf("Field '%s' failed. If 'Enabled = true', there must be at least one fully-qualified domain name in the passlist", v.Namespace()))
Copy link
Contributor

@mustard-mh mustard-mh Sep 21, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

Maybe Warning is better, for the existing self-hosted

SH (for my case) I only want people I invited to join, admin's may don't want accounts to be created automatically to save seats

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No, I think fatal is correct. If you don't have any domains in the passlist then you end up in a broken state where the initial admin user gets blocked and you've basically bricked the installation. The only way I've found to resolve this is by deleting the database (I guess it's possible to do by deleting specific DB records, but I don't know those)

Copy link
Contributor

Choose a reason for hiding this comment

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

It's good for new SH installation. But for existing SH, they need to LeanMore and change their config.yaml

And it blocks my case -- an empty passlist.

(My case also not allow me to use the same domain of email, so it's useless for my case like small company?)

Copy link
Contributor

Choose a reason for hiding this comment

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

We can fix it by making first account available?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, if there's an exception made to never block the first account then that would remove this problem completely.

I'd propose keeping the test as a fatal error, but it would remove the requirement for option 2 in the original test scenarios:

If blockNewUsers.enabled is set to true and blockNewUser.passlist is set to [] - this should fail

default:
// General error message
res.Fatal = append(res.Fatal, fmt.Sprintf("Field '%s' failed %s validation", v.Namespace(), v.Tag()))
Expand Down