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

feat: add minimum password length and password requirements config #2885

Open
wants to merge 2 commits into
base: develop
Choose a base branch
from

Conversation

silentworks
Copy link
Contributor

What kind of change does this PR introduce?

This feature can be found in the Supabase Hosted Dashboard but doesn't have a way to configure it in the config.toml.

What is the current behavior?

No way to set Password minimum length and Password requirements

What is the new behavior?

Can now set Password minimum length and Password requirements in the config.toml file.

Additional context

Add any other context or screenshots.

@@ -18,6 +18,8 @@ type (
SiteUrl string `toml:"site_url"`
AdditionalRedirectUrls []string `toml:"additional_redirect_urls"`
JwtExpiry uint `toml:"jwt_expiry"`
MinimumPasswordLength uint `toml:"minimum_password_length"`
PasswordRequirements string `toml:"password_requirements"`
Copy link
Contributor

Choose a reason for hiding this comment

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

Since we started supporting config push command, adding new auth config also requires mapping to the api request / response fields https://github.com/supabase/cli/blob/develop/pkg/config/auth.go#L186.

Once that's mapped, remember to update the diff snapshots in pkg/config/testdata to resolve the failing tests.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@sweatybridge I think this is going to be an issue for me as I don't know what these keys are on the hosted platform.

Copy link
Contributor

Choose a reason for hiding this comment

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

No worries, I can take over from here next week.

Fyi, all fields are more or less documented in the openapi spec.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I figured it out. The only part is the testdata I'm having trouble with now.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@sweatybridge Are the diff files manually created or is there a command to run to generate them?

Copy link
Contributor

Choose a reason for hiding this comment

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

Deleting a specific diff file in testdata and rerun the corresponding test will regenerate it.

# Passwords that do not have at least one of each will be rejected as weak.
# Letters and digits: abcdefghijklmnopqrstuvwxyzABCDEFGHIJKLMNOPQRSTUVWXYZ:0123456789
# Lowercase, uppercase letters and digits: abcdefghijklmnopqrstuvwxyz:ABCDEFGHIJKLMNOPQRSTUVWXYZ:0123456789
# Lowercase, uppercase letters, digits and symbols: abcdefghijklmnopqrstuvwxyz:ABCDEFGHIJKLMNOPQRSTUVWXYZ:0123456789:!@#$%^&*()_+-=[]{};'\\\\:\"|<>?,./`~
Copy link
Contributor

Choose a reason for hiding this comment

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

We should make this an enum and validate it, similar to frontend.

Screenshot 2024-11-18 at 3 11 03 PM

@@ -192,6 +194,8 @@ func (a *auth) ToUpdateAuthConfigBody() v1API.UpdateAuthConfigBody {
SecurityManualLinkingEnabled: &a.EnableManualLinking,
DisableSignup: cast.Ptr(!a.EnableSignup),
ExternalAnonymousUsersEnabled: &a.EnableAnonymousSignIns,
PasswordMinLength: cast.UintToIntPtr(&a.MinimumPasswordLength),
PasswordRequiredCharacters: (*v1API.UpdateAuthConfigBodyPasswordRequiredCharacters)(&a.PasswordRequirements),
Copy link
Contributor

Choose a reason for hiding this comment

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

Also need to update fromRemoteAuthConfig for the reverse mapping.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants