public
/
pyceo
27
1
Fork 3
Code Issues 25 Pull Requests 5 Projects Releases 1 Wiki Activity

(#101) Validate usernames across cli & tui #115

Merged
o32patel merged 8 commits from 101-username-validation into master 2024-01-22 13:15:43 -05:00
Conversation 6 Commits 8 Files Changed 8 +89 -34
o32patel commented 2024-01-16 08:34:04 -05:00
Member
Copy Link

Current changes should address issues raised by @merenber in #114 excluding #114 (comment) (both CLI and TUI validation)

Current changes should address issues raised by @merenber in #114 excluding https://git.csclub.uwaterloo.ca/public/pyceo/pulls/114#issuecomment-14153 (both CLI and TUI validation)
o32patel added 1 commit 2024-01-16 08:34:06 -05:00
o32patel self-assigned this 2024-01-16 08:34:12 -05:00
merenber requested changes 2024-01-16 22:38:32 -05:00
merenber left a comment
Owner
Copy Link

A few minor comments, but looks great overall. Thank you for taking the initiative to add this feature!

A few minor comments, but looks great overall. Thank you for taking the initiative to add this feature!
ceo_common/utils.py
@ -53,1 +56,4 @@
return datetime.datetime.now()
class UsernameValidationResult:
merenber commented 2024-01-16 22:35:14 -05:00
Owner
Copy Link

This can be simplified with Python's dataclass decorator:

from dataclasses import dataclass

@dataclass
class UsernameValidationResult:
    is_valid: bool
    error_message: str = ''
This can be simplified with Python's dataclass decorator: ```python from dataclasses import dataclass @dataclass class UsernameValidationResult: is_valid: bool error_message: str = '' ```
o32patel marked this conversation as resolved
tests/ceo_common/test_utils.py Outdated
@ -0,0 +2,4 @@
def test_validate_username():
assert utils.validate_username('').__eq__(utils.UsernameValidationResult(False, 'Username must not be empty'))
merenber commented 2024-01-16 22:36:45 -05:00
Owner
Copy Link

Why are we using __eq__ here? Why not just ==?

Why are we using `__eq__` here? Why not just `==`?
o32patel marked this conversation as resolved
o32patel added 3 commits 2024-01-16 22:43:16 -05:00
o32patel commented 2024-01-16 22:48:53 -05:00
Author
Member
Copy Link

Since the TUI verifies the usernames for every input, I coped that behaviour over to the CLI parts, should they also be applied to the API? Or is it fine to leave it as only verifiying for new users?

Since the TUI verifies the usernames for every input, I coped that behaviour over to the CLI parts, should they also be applied to the API? Or is it fine to leave it as only verifiying for new users?
o32patel added 1 commit 2024-01-16 23:11:34 -05:00
continuous-integration/drone/pr Build is failing Details
adc31519c0
Simplify UsernameValidationResult and tests
merenber commented 2024-01-17 21:38:20 -05:00
Owner
Copy Link

Since the TUI verifies the usernames for every input, I coped that behaviour over to the CLI parts, should they also be applied to the API? Or is it fine to leave it as only verifiying for new users?

The same username validation logic should be applied to the /members POST endpoint:
https://git.csclub.uwaterloo.ca/public/pyceo/src/branch/master/ceod/api/members.py#L26

> Since the TUI verifies the usernames for every input, I coped that behaviour over to the CLI parts, should they also be applied to the API? Or is it fine to leave it as only verifiying for new users? The same username validation logic should be applied to the /members POST endpoint: https://git.csclub.uwaterloo.ca/public/pyceo/src/branch/master/ceod/api/members.py#L26
merenber commented 2024-01-17 21:40:41 -05:00
Owner
Copy Link

I just checked the CI logs, it looks like the tests are failing because some of the tests use underscores in the usernames ... ugh. I think it's OK to allow underscores in the username regex for now. Someone can open a separate PR later to clean that up.

I just checked the CI logs, it looks like the tests are failing because some of the tests use underscores in the usernames ... ugh. I think it's OK to allow underscores in the username regex for now. Someone can open a separate PR later to clean that up.
o32patel commented 2024-01-17 21:54:54 -05:00
Author
Member
Copy Link

Since the TUI verifies the usernames for every input, I coped that behaviour over to the CLI parts, should they also be applied to the API? Or is it fine to leave it as only verifiying for new users?

The same username validation logic should be applied to the /members POST endpoint:
https://git.csclub.uwaterloo.ca/public/pyceo/src/branch/master/ceod/api/members.py#L26

Cool, should be done in 3f751ccece

> > Since the TUI verifies the usernames for every input, I coped that behaviour over to the CLI parts, should they also be applied to the API? Or is it fine to leave it as only verifiying for new users? > > The same username validation logic should be applied to the /members POST endpoint: > https://git.csclub.uwaterloo.ca/public/pyceo/src/branch/master/ceod/api/members.py#L26 Cool, should be done in 3f751ccece
o32patel added 1 commit 2024-01-17 21:56:54 -05:00
continuous-integration/drone/pr Build is passing Details
c48cce3826
Allow underscores in usernames to pass ci
o32patel commented 2024-01-17 21:57:15 -05:00
Author
Member
Copy Link

And everything should be good now

And everything should be good now
o32patel changed title from WIP: (#101) Validate usernames across cli & tui to (#101) Validate usernames across cli & tui 2024-01-17 21:57:18 -05:00
o32patel requested review from merenber 2024-01-18 19:08:14 -05:00
merenber requested changes 2024-01-19 18:55:51 -05:00
ceo/cli/members.py Outdated
@ -105,6 +112,11 @@ def print_user_lines(d: Dict):
@members.command(short_help='Get info about a user')
@click.argument('username')
def get(username):
# Verify that the username is valid before requesting data from API
merenber commented 2024-01-19 18:54:42 -05:00
Owner
Copy Link

We don't need to validate the username for get/modify/renew/pwreset/delete, because the client will get a 404 if the username is invalid. We only need to validate the username when adding new members.

We don't need to validate the username for get/modify/renew/pwreset/delete, because the client will get a 404 if the username is invalid. We only need to validate the username when adding new members.
o32patel commented 2024-01-22 01:00:38 -05:00
Author
Member
Copy Link

Should this also be done for the TUI? Currently it validates under Controller.get_username_from_view() which is shared across all/most options

Should this also be done for the TUI? Currently it validates under Controller.get_username_from_view() which is shared across all/most options
merenber commented 2024-01-22 07:44:48 -05:00
Owner
Copy Link

Nah, I think that should be fine. I just didn't want to duplicate code if it wasn't necessary.

Nah, I think that should be fine. I just didn't want to duplicate code if it wasn't necessary.
merenber commented 2024-01-19 19:50:36 -05:00
Owner
Copy Link

Could we also please add a few unit tests for the API (under tests/ceod/api)? Just as a sanity check.

Could we also please add a few unit tests for the API (under tests/ceod/api)? Just as a sanity check.
o32patel added 1 commit 2024-01-22 00:59:06 -05:00
o32patel added 1 commit 2024-01-22 01:37:31 -05:00
continuous-integration/drone/pr Build is passing Details
d11bdc1a6f
add ceod unit tests for valid usernames
o32patel reviewed 2024-01-22 01:38:50 -05:00
tests/ceod/api/test_members.py
@ -135,6 +135,40 @@ def test_api_create_user_without_forwarding_addresses(cfg, client):
assert data['error'] == "BadRequest: Attribute 'forwarding_addresses' is missing or empty"
def test_api_create_user_without_valid_username(cfg, client):
o32patel commented 2024-01-22 01:38:50 -05:00
Author
Member
Copy Link

test_api_create_user_without_valid_username should probably be expanded on with the _ fix mentioned here: #115 (comment)

test_api_create_user_without_valid_username should probably be expanded on with the _ fix mentioned here: https://git.csclub.uwaterloo.ca/public/pyceo/pulls/115#issuecomment-14170
merenber approved these changes 2024-01-22 07:48:58 -05:00
o32patel merged commit de23296413 into master 2024-01-22 13:15:43 -05:00
o32patel deleted branch 101-username-validation 2024-01-22 13:15:43 -05:00
Sign in to join this conversation.
Reviewers
No reviewers
merenber
Labels
Clear labels   
priority
high
High priority

  
priority
low
Low priority

  
priority
medium
Medium priority

  
priority
very high
Very high priority

  
BUG

   
Feature

   
High Priority

   
Low Priority

   
Medium Priority
No Label
priority
high
priority
low
priority
medium
priority
very high
BUG
Feature
High Priority
Low Priority
Medium Priority
Milestone
Clear milestone
No items
No Milestone
Projects
Clear projects
No project
Assignees
Clear assignees
No Assignees
o32patel
2 Participants
Notifications
Due Date
The due date is invalid or out of range. Please use the format 'yyyy-mm-dd'.

No due date set.

Dependencies

No dependencies set.

Reference: public/pyceo#115
No description provided.
Delete Branch "101-username-validation"

Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?