-
Notifications
You must be signed in to change notification settings - Fork 55
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
Add validation to org subdomain during signup, and add a list of reserved subdomains. #3066
Conversation
4506104
to
b06786f
Compare
@@ -12,6 +12,33 @@ type SignupOrg struct { | |||
Subdomain string `json:"subDomain"` | |||
} | |||
|
|||
var reservedSubDomains = []string{ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@ssoroka also had a list of reserved subdomains. Perhaps you should work together on this
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm happy to add or edit this list! I should have mentioned that in the PR description.
Please do suggest changes to this list and I can update it.
b06786f
to
6f60a09
Compare
validate.StringRule{ | ||
Name: "token", | ||
Value: r.Token, | ||
MinLength: 10, | ||
MaxLength: 10, | ||
CharacterRanges: validate.AlphaNumeric, | ||
}, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
way less elegant :P
validate.Required("name", r.Name), | ||
validate.Required("subDomain", r.Subdomain), | ||
validate.ReservedStrings("subDomain", r.Subdomain, reservedSubDomains), | ||
validate.StringRule{ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
not necessarily a blocker, but it's missing the can't-start-with-dash case
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I added first character validation to the StringRule
and added it here as well
6f60a09
to
e373105
Compare
So that the openAPI doc is generated correctly. The validation rules continue to work the same way.
Functions with more than 3 arguments are hard to use, especially when some of the args have the same type. From the caller it's not easy to see what the values represent. Using a string makes the code more obvious because we can see the value 10 is for length. The field names help document what the values mean. Also remove a TODO, the password critiera are validated in the access package.
Add a new validation rule for restricting strings, and use it to validate the org subdomain. Also add min/max length and character restrictions.
92d1740
to
b63ed2c
Compare
Summary
This PR adds validation to the org subdomain, including min/max length, character restrictions, and a list of reserved words that we should not allow to be subdomains.
Best viewed by individual commit, more details in commit messages.