fix: cleanup of toggle and few custom attributes #405

Akanshu-2u · 2025-10-13T07:28:10Z

Description:

This PR focusses on the removal of the toggles and custom attributes from both EdXOAuth2.start() and update_email().

Solution:

Removed ENABLE_OAUTH_SESSION_CLEANUP from EdXOAuth2.start().
Removed SKIP_UPDATE_EMAIL_ON_USERNAME_MISMATCH from update_email() .
Custom-attributes in EdXOAuth2.start():
- Removed:
  - session_cleanup.has_request : Tracks whether a request object is available during OAuth start. This is a technical implementation detail that doesn't provide actionable insights.
  - session_cleanup.logged_out_username: Records the username that was logged out during session cleanup. Storing usernames in monitoring data may violate privacy policies.
- Retained:
  - session_cleanup.logout_required: Tracks whether a user was authenticated before session cleanup . Critical for detecting potential session hijacking and concurrent login attempts.
Custom-attributes in update_email():
- Removed:
  - update_email.details_username - Privacy risk: stores personal identifiers in monitoring data.
  - update_email.user_username - Security risk: exposes usernames in monitoring dashboards.
  - update_email.details_has_email - Low value: technical detail creating monitoring noise.
- Retained:
  - update_email.username_mismatch - Critical for security monitoring and detecting potential account takeover attempts.
  - update_email.email_updated - Essential for tracking successful email updates and operational health.

Related PRs:

JIRA Ticket:

BOMS-3

robrap

I didn't review tests yet, but I provided some comments. Please update the version and changelog as well. It would be great to land this soon. Thank you.

Note that the changelog can be a very brief summary.

robrap · 2025-10-15T15:01:28Z

auth_backends/backends.py

-            # .. custom_attribute_name: session_cleanup.logged_out_username
-            # .. custom_attribute_description: Records the username that was logged out
-            #    during session cleanup for tracking and debugging purposes.
-            set_custom_attribute('session_cleanup.logged_out_username', existing_username)


Let's also remove session_cleanup.logout_performed. I think session_cleanup.logout_required is close enough and has a simpler implementation.

Addressed. Removed session_cleanup.logout_performed and updated test file.

robrap · 2025-10-15T15:03:48Z

auth_backends/pipeline.py

        # .. custom_attribute_description: Tracks whether there's a mismatch between
        #    the username in the social details and the user's actual username.
        #    True if usernames don't match, False if they match.
        set_custom_attribute('update_email.username_mismatch', username_mismatch)


This is highly unexpected, so let's drop this. I think we can leave the warning, but update its text. I'll add another comment.

Addressed. Dropped the update_email.username_mismatch custom attribute and modified test file.

robrap · 2025-10-15T15:06:22Z

auth_backends/pipeline.py

-            # Log warning and set additional custom attributes for mismatches
+            # Log warning about the mismatch
            logger.warning(
                "Username mismatch during email update. User username: %s, Details username: %s",


Let's combine the two warning logs into something like the following:

Suggested change

"Username mismatch during email update. User username: %s, Details username: %s",

"Unexpected username mismatch during email update. Skipping email update for user %s. User username: %s, Details username: %s",

Addressed. Combined two loggers into one as per suggestion.

robrap · 2025-10-15T15:06:55Z

auth_backends/pipeline.py

+            # Skip email update due to username mismatch
+            logger.warning(
+                "Skipping email update for user %s due to username mismatch",
+                user_username
+            )


This can be removed in favor of the first log message.

robrap

Minor clean-up request. Thank you.

robrap · 2025-10-16T18:28:39Z

auth_backends/tests/test_backends.py

-                    call('session_cleanup.toggle_enabled', False),
-                    call('session_cleanup.logout_performed', False)
-                ], any_order=True)
+            mock_logger.info.assert_called_with(


I think this is the only assertion needed in the conditional. You should be able to remove the else block, and move all other assertions after (or before) the conditional, with the caveat of updating one assertion to:

mock_set_attr.assert_called_once_with('session_cleanup.logout_required', user_authenticated)

robrap · 2025-10-17T17:48:02Z

auth_backends/tests/test_backends.py

            self.assertNotEqual(request.session.session_key, initial_session_key)
            self.assertTrue(request.user.is_anonymous)


In this comment, I meant that I think these could move outside the conditional as well. Is that right?

No, they should not be moved outside the conditional.

These assertions test the effects of logout(request), which only executes when user_authenticated=True. When user_authenticated=False, no logout happens, so the session key doesn't change and the user was already anonymous - moving the assertions outside would test nothing changed instead of logout worked correctly.

@Akanshu-2u: I personally would have moved the assertions, because they should be true for both cases. From one perspective, the meaning of the assertions changes from "nothing changed" to "logout worked" depending on the test. But, from another perspective, the same assertions are true in both cases, because whether or not whether or not you start with an authenticated user, you end with an anonymous user.

It's fine to leave as-is. Or, we could follow-up with a minor PR that doesn't need to adjust changelog or version, because it would just affect tests. Either way.

robrap · 2025-10-17T17:48:39Z

auth_backends/tests/test_backends.py

-        else:
-            mock_set_attr.assert_called_once_with('session_cleanup.logout_required', False)
-
-            mock_logger.info.assert_not_called()


Oops. I missed this useful assertion. You can restore this. Sorry. But the other can stay outside the conditional.

Added the assertion. Thank you.

feanil

Thanks for the further cleanup, trusting Robert's review here, I can merge and release this one the last test request has been addressed.

robrap · 2025-10-17T19:43:30Z

@Akanshu-2u: In addition to the last change, can you confirm that you've tested this locally (or sandbox) and confirm that all works as expected? Switching the toggle on/off in the other service (e.g. discovery) should provide no change, and you should always get the logout from start. Thanks.

robrap · 2025-10-20T19:08:40Z

auth_backends/pipeline.py

-
        if username_mismatch:
-            # Log warning and set additional custom attributes for mismatches
+            # Log warning about the mismatch


@Akanshu-2u: You can remove this comment. It doesn't add anything.

Akanshu-2u · 2025-10-21T04:22:50Z

@Akanshu-2u: In addition to the last change, can you confirm that you've tested this locally (or sandbox) and confirm that all works as expected? Switching the toggle on/off in the other service (e.g. discovery) should provide no change, and you should always get the logout from start. Thanks.

Yes, I have tested the current changes locally as well as on sandbox. For both the cases as expeted user is getting logged out from start. So changes are working well.
Checked the custom attributes in monitorings so no reference for toggle is currently present, hence the toggle switch provides no change.
Thank you!

fix: cleanup of toggle and few custom attributes

d6c96a0

robrap reviewed Oct 15, 2025

View reviewed changes

Akanshu-2u added 3 commits October 16, 2025 06:50

fix: removed custom attribute and updated logger and tests

7c9f209

fix: fixed the pylint and pycodestyle issues

54927df

fix: updated version and added in changelog

c26ddfb

robrap reviewed Oct 16, 2025

View reviewed changes

fix: removed else part and moved assertions out of conditional

6a79f2a

Akanshu-2u requested a review from robrap October 17, 2025 11:31

robrap reviewed Oct 17, 2025

View reviewed changes

feanil reviewed Oct 17, 2025

View reviewed changes

robrap reviewed Oct 20, 2025

View reviewed changes

fix: removed unwanted comment and added imp assert call

6550570

Akanshu-2u requested a review from robrap October 21, 2025 04:33

feanil approved these changes Oct 21, 2025

View reviewed changes

feanil merged commit f8e27d9 into openedx:master Oct 21, 2025
12 checks passed

robrap deleted the aaich/BOMS-3-cleanup branch October 21, 2025 20:35

	"Username mismatch during email update. User username: %s, Details username: %s",
	"Unexpected username mismatch during email update. Skipping email update for user %s. User username: %s, Details username: %s",

		self.assertNotEqual(request.session.session_key, initial_session_key)
		self.assertTrue(request.user.is_anonymous)

fix: cleanup of toggle and few custom attributes #405

fix: cleanup of toggle and few custom attributes #405

Uh oh!

Conversation

Akanshu-2u commented Oct 13, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description:

Solution:

Custom-attributes in EdXOAuth2.start():

Custom-attributes in update_email():

Related PRs:

JIRA Ticket:

Uh oh!

robrap left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

robrap left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

feanil left a comment

Choose a reason for hiding this comment

Uh oh!

robrap commented Oct 17, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Akanshu-2u commented Oct 21, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Akanshu-2u commented Oct 13, 2025 •

edited

Loading

Custom-attributes in `EdXOAuth2.start()`:

Custom-attributes in `update_email()`:

Akanshu-2u commented Oct 21, 2025 •

edited

Loading