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

fix: extend list of reserved domains in signup #3152

Merged
merged 4 commits into from
Sep 13, 2022
Merged

fix: extend list of reserved domains in signup #3152

merged 4 commits into from
Sep 13, 2022

Conversation

ssoroka
Copy link
Contributor

@ssoroka ssoroka commented Sep 7, 2022

Summary

  • extend list of reserved domains
  • don't allow domains of 5 char or less (would have to be created manually by us if we want it)
  • add test
  • one change we might want to make after this (or in this pr) is to have different error messages for the reserved list vs the reject list. the reserve list should say to sign up with a different domain and/or contact us or something along those lines.

Checklist

  • Wrote appropriate unit tests
  • Considered security implications of the change
  • Updated associated docs where necessary
  • Updated associated configuration where necessary
  • Change is backwards compatible if it needs to be (user can upgrade without manual steps?)
  • Nothing sensitive logged
  • Considered data migrations for smooth upgrades

Copy link
Collaborator

@BruceMacD BruceMacD left a comment

Choose a reason for hiding this comment

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

looks good once lint/tests pass

Copy link
Contributor

@dnephin dnephin left a comment

Choose a reason for hiding this comment

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

The list of words looks great! @pdevine had originally requested a minimum of 3, but personally I think it should be at least 4 or 5. This PR changes it to 6. I'm fine with that, it's easier to relax this later than to make it more strict.

What's the reason for storing this as yaml? Do you think we could store it as a go literal to remove the need for decoding?

Or even a simple "word per line" that would let us parse it with https://pkg.go.dev/bufio#NewScanner ? I guess word per line would not allow for comments unless we also cut off any #... suffix.

@ssoroka
Copy link
Contributor Author

ssoroka commented Sep 9, 2022

Yaml was just easy and the source list was already yaml. Alternatives are okay. We already use the yaml parsing library so it's no new dependencies. The comments are nice

Copy link
Contributor

@dnephin dnephin left a comment

Choose a reason for hiding this comment

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

Maybe worth double checking with others about the min length change, but LGTM

@ssoroka ssoroka enabled auto-merge (squash) September 13, 2022 20:30
@ssoroka ssoroka merged commit 4178903 into main Sep 13, 2022
@ssoroka ssoroka deleted the reserved-list branch September 13, 2022 20:34
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.

3 participants