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

fixed username validator #239

Merged
merged 2 commits into from
Nov 11, 2024
Merged

fixed username validator #239

merged 2 commits into from
Nov 11, 2024

Conversation

Roaster05
Copy link
Contributor

The username validator previously implemented only allowed alphanumeric characters, which resulted in rejecting valid usernames accepted by the server, such as harbor-cli. This inconsistency also caused failures in login tests.

To resolve this, the current implementation of the validator has been aligned with the server's username validation criteria. You can view the server-side validator here.

This PR references issue #234

pattern := `^[a-zA-Z0-9]{1,255}$`
re := regexp.MustCompile(pattern)
return re.MatchString(username)
return len(username) >= 1 && len(username) <= 255 && !strings.ContainsAny(username, `,"~#%$`)
Copy link
Contributor

Choose a reason for hiding this comment

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

Yes, it meets the username validators, but it also satisfy certain requirements outlined in the issue #195.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Just to confirm, should I add space trimming and an empty-check to the username validation here to fully meet issue #195 requirements, or handle that separately?

Copy link
Contributor

Choose a reason for hiding this comment

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

I think it would be better to validate and trim spaces in the input fields here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@JianMinTang @bupd is it fine now?

Roaster05 and others added 2 commits November 7, 2024 11:46
Signed-off-by: Roaster05 <[email protected]>
Signed-off-by: Roaster05 <[email protected]>
Signed-off-by: Roaster05 <[email protected]>
@bupd bupd added the Priority: Critical Impacts essential functionality, degrading the user experie label Nov 11, 2024
Copy link
Contributor

@bupd bupd left a comment

Choose a reason for hiding this comment

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

LGTM

@Vad1mo Vad1mo merged commit d059ef2 into goharbor:main Nov 11, 2024
1 check passed
Vad1mo added a commit that referenced this pull request Nov 11, 2024
Vad1mo added a commit that referenced this pull request Nov 11, 2024
@Vad1mo
Copy link
Member

Vad1mo commented Nov 11, 2024

reverting back as test fail.

pattern := `^[a-zA-Z0-9]{1,255}$`
re := regexp.MustCompile(pattern)
return re.MatchString(username)
username = strings.TrimSpace(username)
Copy link
Contributor

@Standing-Man Standing-Man Nov 14, 2024

Choose a reason for hiding this comment

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

this code may not work as expected. If you enter harbor-cli with extra spaces, the strings.TrimSpace(username) doesn't properly store the value in the login.LoginView variable. Could you please fix this if you still want to continue? @Roaster05

@bupd bupd self-requested a review November 25, 2024 07:02
@bupd bupd mentioned this pull request Nov 26, 2024
Copy link
Contributor

@bupd bupd left a comment

Choose a reason for hiding this comment

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

.

qcserestipy pushed a commit to qcserestipy/harbor-cli that referenced this pull request Dec 22, 2024
qcserestipy pushed a commit to qcserestipy/harbor-cli that referenced this pull request Dec 22, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Priority: Critical Impacts essential functionality, degrading the user experie
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants