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.

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

Conversation

mrsimonemms
Copy link
Contributor

Description

If a user selects the option to limit their user registrations (which they should if on public internet), it is possible to get their Gitpod instance into a weird state where the initial admin user is blocked if they had configured no items in the passlist.

This adds a validation check to ensure that is not possible.

Related Issue(s)

Fixes #13114

How to test

Four scenarios to check:

  1. If blockNewUsers.enabled is set to false and blockNewUsers.passlist is set to [] - this should pass
  2. If blockNewUsers.enabled is set to true and blockNewUser.passlist is set to [] - this should fail
  3. If blockNewUsers.enabled is set to true and blockNewUser.passlist is set to ["somedomain"] - this should fail
  4. If blockNewUsers.enabled is set to true and blockNewUser.passlist is set to ["somedomain.com"] - this should pass

Use go run . validate config -c /path/to/config.yaml to execute without building the binary

Release Notes

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

Documentation

Werft options:

  • /werft with-preview
  • /werft with-integration-tests=all
    Valid options are all, workspace, webapp, ide

@mrsimonemms mrsimonemms force-pushed the mrsimonemms/add-validation-check-13114 branch from b63291c to d8c8a7c Compare September 20, 2022 13:13
@mrsimonemms mrsimonemms marked this pull request as ready for review September 20, 2022 13:13
@mrsimonemms mrsimonemms requested a review from a team September 20, 2022 13:13
@github-actions github-actions bot added the team: delivery Issue belongs to the self-hosted team label Sep 20, 2022
Copy link
Contributor

@Pothulapati Pothulapati left a comment

Choose a reason for hiding this comment

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

LGTM, and works as expected

@roboquat roboquat merged commit b422b93 into main Sep 21, 2022
@roboquat roboquat deleted the mrsimonemms/add-validation-check-13114 branch September 21, 2022 06:44
@@ -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

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
release-note size/M team: delivery Issue belongs to the self-hosted team
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add validation check to ensure that, if limiting user registrations, there is at least one exception
4 participants