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

identity: forbid underscores in GitHub logins #1414

Merged
merged 1 commit into from Oct 19, 2019

Conversation

@wchargin
Copy link
Member

wchargin commented Oct 18, 2019

Summary:
GitHub logins may not have underscores, because underscores are not
valid characters in DNS labels. We already have a good-enough regular
expression for validating GitHub usernames; this commit updates the
alias parser to use that.

Discourse usernames are more permissive than what is listed here, but we
leave that unchanged for now.

Test Plan:
Unit tests updated.

wchargin-branch: alias-no-underscore

@wchargin wchargin requested a review from Beanow Oct 18, 2019
@wchargin wchargin force-pushed the wchargin-alias-no-underscore branch from aa3e6f6 to fe505a3 Oct 18, 2019
@Beanow
Beanow approved these changes Oct 18, 2019
Copy link
Member

Beanow left a comment

I feel like aliases and validation like this, will pretty soon warrant it's own module. But currently this looks strictly better. So, happy to have this 👍

@wchargin wchargin force-pushed the wchargin-alias-anchor branch 2 times, most recently from 4805b95 to 5d2f057 Oct 19, 2019
@wchargin wchargin changed the base branch from wchargin-alias-anchor to master Oct 19, 2019
Summary:
GitHub logins may not have underscores, because underscores are not
valid characters in DNS labels. We already have a good-enough regular
expression for validating GitHub usernames; this commit updates the
alias parser to use that.

Discourse usernames are more permissive than what is listed here, but we
leave that unchanged for now.

Test Plan:
Unit tests updated.

wchargin-branch: alias-no-underscore
@wchargin wchargin force-pushed the wchargin-alias-no-underscore branch from fe505a3 to c4ac1df Oct 19, 2019
@wchargin

This comment has been minimized.

Copy link
Member Author

wchargin commented Oct 19, 2019

Yep! Hence the doc comment about this being a short-term implementation
that doesn’t need to scale. Thanks for the review!

@wchargin wchargin merged commit f577ae7 into master Oct 19, 2019
2 checks passed
2 checks passed
ci/circleci: publish-1 Your tests passed on CircleCI!
Details
ci/circleci: test Your tests passed on CircleCI!
Details
@wchargin wchargin deleted the wchargin-alias-no-underscore branch Oct 19, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

2 participants
You can’t perform that action at this time.