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

Username case error #11716

Merged

Conversation

nick2432
Copy link
Contributor

@nick2432 nick2432 commented Jan 11, 2024

Summary

This pull request addresses the issue of username case sensitivity when using the same password and also handles scenarios involving multiple usernames.

Changes Made

-In the 'backend.py' file, modify the logic to first check the password for case-sensitive usernames. If the user can log in successfully, proceed accordingly; otherwise, switch to using case-insensitive usernames.
-In 'api.py,' I modified the 'try-except' block and added an additional 'except' in the 'SessionViewSet' to handle scenarios involving multiple usernames.

BEFORE

If the usernames are 'h' and 'H' and they share the same password 'n', logging in with either 'h' or 'H' will open the same user account, specifically the one associated with the uppercase 'H'

2024-01-12.11-01-48.mp4

AFTER

When I log in with the 'h' username, it opens the 'h' user account. Similarly, logging in with the 'H' username opens the 'H' user account.

2024-01-12.11-10-16.mp4

Related Issues

Closes #11082

Reviewer guidance


Testing checklist

  • Contributor has fully tested the PR manually
  • If there are any front-end changes, before/after screenshots are included
  • Critical user journeys are covered by Gherkin stories
  • Critical and brittle code paths are covered by unit tests

PR process

  • PR has the correct target branch and milestone
  • PR has 'needs review' or 'work-in-progress' label
  • If PR is ready for review, a reviewer has been added. (Don't use 'Assignees')
  • If this is an important user-facing change, PR or related issue has a 'changelog' label
  • If this includes an internal dependency change, a link to the diff is provided

Reviewer checklist

  • Automated test coverage is satisfactory
  • PR is fully functional
  • PR has been tested for accessibility regressions
  • External dependency files were updated if necessary (yarn and pip)
  • Documentation is updated
  • Contributor is in AUTHORS.md

@github-actions github-actions bot added DEV: backend Python, databases, networking, filesystem... SIZE: small labels Jan 11, 2024
Copy link
Member

@rtibbles rtibbles left a comment

Choose a reason for hiding this comment

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

One small code cleanup, but this looks like the correct logic! Have not tested yet.

for user in users:
if user.check_password(password):
return user
# Allow login without password for learners for facilities that allow this.
Copy link
Member

Choose a reason for hiding this comment

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

Would be good to keep this code comment.

kolibri/core/auth/backends.py Outdated Show resolved Hide resolved
@nick2432
Copy link
Contributor Author

@rtibbles ,I've made the necessary changes.

Copy link
Member

@rtibbles rtibbles left a comment

Choose a reason for hiding this comment

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

This is great. The last thing here would be to add tests for the backend changes in this file: https://github.com/learningequality/kolibri/blob/release-v0.16.x/kolibri/core/auth/test/test_backend.py (so add a test where we add another user with the username mike as well as the user Mike, and ensure that we authenticate as the correct one!).

In addition, a regression test for the multiple matching usernames case for the session endpoint would be great too - existing tests for that are here: https://github.com/learningequality/kolibri/blob/release-v0.16.x/kolibri/core/auth/test/test_api.py#L1054

If this is a step too far, I'm happy to implement these tests, but let me know your preference!

@nick2432
Copy link
Contributor Author

This is great. The last thing here would be to add tests for the backend changes in this file: https://github.com/learningequality/kolibri/blob/release-v0.16.x/kolibri/core/auth/test/test_backend.py (so add a test where we add another user with the username mike as well as the user Mike, and ensure that we authenticate as the correct one!).

In addition, a regression test for the multiple matching usernames case for the session endpoint would be great too - existing tests for that are here: https://github.com/learningequality/kolibri/blob/release-v0.16.x/kolibri/core/auth/test/test_api.py#L1054

If this is a step too far, I'm happy to implement these tests, but let me know your preference!

i will try

@nick2432
Copy link
Contributor Author

@rtibbles , I've added the test. Please take a moment to review, and thank you for giving me the opportunity.

@rtibbles rtibbles self-assigned this Jan 16, 2024
Copy link
Member

@rtibbles rtibbles left a comment

Choose a reason for hiding this comment

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

Just one question on the API test, but this is looking good!

# Assert the expected behavior based on the application's design
self.assertEqual(response_user1.status_code, 200)

response_user2 = self.client.post(
Copy link
Member

Choose a reason for hiding this comment

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

What is different between each of these requests? Should the second one be the title case username instead?

@nick2432
Copy link
Contributor Author

Just one question on the API test, but this is looking good!

yes 😅

@nick2432
Copy link
Contributor Author

@rtibbles done

Copy link
Member

@rtibbles rtibbles left a comment

Choose a reason for hiding this comment

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

Excellent work!

@rtibbles
Copy link
Member

Docs build is fixed on release branch.

@rtibbles rtibbles merged commit 765a54b into learningequality:release-v0.16.x Jan 18, 2024
33 of 34 checks passed
@nick2432 nick2432 deleted the username-case-error branch January 23, 2024 06:04
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
DEV: backend Python, databases, networking, filesystem... SIZE: small
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Attempt case sensitive username disambiguation before case insensitive at login
2 participants