Skip to content
This repository was archived by the owner on Oct 22, 2025. It is now read-only.

Conversation

@Akanshu-2u
Copy link

Description:

The get_user_if_exists function lacks visibility into authentication failures. We can't debug why "user already exists" errors happen in devstack but not in stage.

Solution:

  • Added comprehensive debugging with new toggle IGNORE_LOGGED_IN_USER_ON_MISMATCH.

  • Added custom attributes to track authentication flow and detect issues:

        - Username mismatches between details and existing users.
        - User lookup success/failure states.
        - Database errors during user retrieval.
    
  • Improved logging with detailed info and warning messages.

  • Removed unnecessary pass statement for cleaner code.

JIRA Ticket:

BOMS-3

Reference PR:

PR

Reference pr for testing legacy get_user_if_exists code with current test_file:

openedx#395

Akanshu-2u and others added 21 commits July 24, 2025 12:43
Co-authored-by: Robert Raposa <rraposa@gmail.com>
Co-authored-by: Robert Raposa <rraposa@gmail.com>
@Akanshu-2u Akanshu-2u requested review from Copilot and robrap September 3, 2025 07:30
Copy link

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull Request Overview

This PR enhances the get_user_if_exists pipeline function with comprehensive debugging capabilities to diagnose authentication failures. The primary goal is to add visibility into username mismatches between logged-in users and social auth details to resolve issues where "user already exists" errors occur in devstack but not in stage.

Key changes:

  • Added new toggle IGNORE_LOGGED_IN_USER_ON_MISMATCH to control behavior when username mismatches occur
  • Enhanced pipeline function with custom attributes for monitoring authentication flow states
  • Added comprehensive test coverage with DDT framework for multiple scenario testing

Reviewed Changes

Copilot reviewed 8 out of 8 changed files in this pull request and generated 2 comments.

Show a summary per file
File Description
requirements/test.txt Updated dependency versions and added ddt, responses, typing_extensions for enhanced testing
requirements/test.in Added new test dependencies: ddt, responses, typing_extensions
auth_backends/tests/test_pipeline.py Comprehensive rewrite with DDT-based tests covering all username mismatch scenarios
auth_backends/tests/test_backends.py Updated test helper methods to work with responses library
auth_backends/pipeline.py Enhanced get_user_if_exists with debugging capabilities and toggle logic
auth_backends/backends.py Removed unnecessary pylint disable comment
auth_backends/init.py Version bump to 4.6.1
CHANGELOG.rst Added changelog entry for version 4.6.1

Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.

'access_token': access_token
})
return 200, headers, body
return (200, {}, body)
Copy link

Copilot AI Sep 3, 2025

Choose a reason for hiding this comment

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

The return statement removes the headers parameter completely, which could break functionality expecting specific headers. The original headers parameter should be preserved.

Copilot uses AI. Check for mistakes.
'is_new': False,
'user': User.objects.get(username=username)
}
except User.DoesNotExist:
Copy link

Copilot AI Sep 3, 2025

Choose a reason for hiding this comment

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

The empty pass statement with removed comment makes the control flow unclear. Consider adding a brief comment explaining that this exception is expected when no user exists.

Suggested change
except User.DoesNotExist:
except User.DoesNotExist:
# Expected exception: no user exists with the given username.

Copilot uses AI. Check for mistakes.
Copy link

@jcapphelix jcapphelix left a comment

Choose a reason for hiding this comment

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

It's a temp fork, so you are good to merge.

Please note that if you plan to use tag (in installation, you'll need to create it)

Or you can use commit hash after you merge.

@Akanshu-2u Akanshu-2u merged commit 0b97daa into edx:master Sep 3, 2025
9 checks passed
@robrap robrap deleted the akanshu/bug-in-third-party-authentication-email-and-name-update branch September 11, 2025 03:25
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants