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

Bump djoser to 2.3.1 #175

Open
wants to merge 4 commits into
base: main
Choose a base branch
from
Open

Conversation

marslanabdulrauf
Copy link

What are the relevant tickets?

https://github.com/mitodl/mitxpro/security/dependabot/204

Description (What does it do?)

This PR upgrades djoser to 2.3.1

Copy link

@Anas12091101 Anas12091101 left a comment

Choose a reason for hiding this comment

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

LGTM 👍 Tested in xPRO

@arslanashraf7
Copy link
Contributor

@marslanabdulrauf Could you add testing instructions + a little bit of explanation as to why we are creating this PR for future references?

@marslanabdulrauf
Copy link
Author

Test Steps
Direct Approach:

  1. Updated the djoser dependency in xpro to match the new version.
  2. Ran the application locally to ensure there were no breaking changes.
  3. Verified the authentication flow:
    • Checked user login/logout functionality.
    • Validated user registration and password reset flows.
    • Confirmed email token generation and validation.

Reviewed the test suite and ensured all tests passed successfully.

Testing with Local Changes in mitol-django-authentication:

  1. Clone the mitol-django-authentication repository locally.
  2. Checked out the branch where djoser was updated to >=2.3.0.
  3. In the xpro project:
    • Removed the existing mitol-django-authentication package.
    • Installed the local version of the updated mitol-django-authentication package in the Docker container.
  4. Ran the application locally and verified:
    • Authentication flows (login/logout, registration, password reset).
    • Compatibility of djoser functionality with existing xPro code.

Run the xpro test suite and ensured all tests passed.

Copy link
Contributor

@arslanashraf7 arslanashraf7 left a comment

Choose a reason for hiding this comment

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

I tried to install and test it in xPRO and I am getting lxml mismatch error. How did you folks test it?

Here is what I did:

  1. Generated the new mitol-django-authentication.gz package from this branch
  2. Installed that in xPRO through poetry
  3. Tried to signup. Faced the error.
image

@Anas12091101
Copy link

I tested it by directly updating the package to v2.3.1 in xPRO

@marslanabdulrauf
Copy link
Author

marslanabdulrauf commented Jan 27, 2025

I tried to install and test it in xPRO and I am getting lxml mismatch error. How did you folks test it?

Here is what I did:

  1. Generated the new mitol-django-authentication.gz package from this branch
  2. Installed that in xPRO through poetry
  3. Tried to signup. Faced the error.
image

Hi @arslanashraf7 ,
I have identified the following details regarding the xmlsec dependency issue:

  1. Source of xmlsec Installation:
    The xmlsec package is being installed indirectly through the python3-saml dependency, which was introduced in mitol-django-authentication.
  2. Availability of python3-saml:
    python3-saml was originally included in mitol-django-authentication prior to the Pants migration. You can see this in this commit.
    Version 1.1.0 of mitol-django-authentication included python3-saml, but it was removed in version >=1.2.0 during the Pants refactor (verified. by downloading next versions and checking their setup.py file).
    It was again added and this change was part of this PR when the migration from Pants to Rye was performed. But it was never deployed on Pypi.
  3. Deployment on PyPI:
    The most recent version of mitol-django-authentication deployed to PyPI is 2023.12.19 (deployed on December 19, 2023).
    This version does not include python3-saml in its build files, despite the fact that the package was available in earlier versions <=1.1.0 and in its current dependencies

Cc @blarghmatey @rhysyngsun

@arslanashraf7
Copy link
Contributor

@marslanabdulrauf I have never directly worked with this authentication package so I don't know the exact details of why python3-saml was added initially and/or why it was removed later on and re-added. This could be the package builder tool like pants or rye, where pants removed it from the package (thinking it wasn't used? ) and rye added it back (thinking it was being used in the package?).

The main thing is to identify whether the authentication package really needs to depend on python-saml or not. As per your finding djoser doesn't need it, were you able to identify what else in the authentication package needs python3-saml?

@rhysyngsun or @blarghmatey might be able to weigh in more on this.

@rhysyngsun
Copy link
Collaborator

python-saml I think is only necessary for touchstone integrations, so if that package has code around that that's probably why it's used.

@marslanabdulrauf
Copy link
Author

I have reviewed the codebase and confirmed that while we have a touchstone.py settings file and a saml.py view file, they are not being used in xPro or MITx Online. This likely explains why the last published version of the project, which excludes the python3-saml package, works without any issues.

I also verified this in ol-infrastructure (search result), which confirms that SOCIAL_AUTH_SAML_SP_PRIVATE_KEY is not being utilized in these environments.

Given this, I recommend safely removing python3-saml from the dependencies.

@collinpreston @rhysyngsun Could you confirm if I might be missing anything here? Thanks

@marslanabdulrauf marslanabdulrauf changed the title Bumb djoser to 2.3.1 Bump djoser to 2.3.1 Jan 29, 2025
@rhysyngsun
Copy link
Collaborator

@marslanabdulrauf go for it

@arslanashraf7
Copy link
Contributor

@marslanabdulrauf one of the checks are failing because of saml

@marslanabdulrauf marslanabdulrauf force-pushed the marslan/204-bumb-djoser branch 2 times, most recently from 4159825 to a84a9dc Compare January 30, 2025 08:52
Copy link
Contributor

@arslanashraf7 arslanashraf7 left a comment

Choose a reason for hiding this comment

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

This now works fine for me on MIT xPRO.

However, I was looking at the usages of this package and I found at least 3 other applications using this:

  1. MITx Online (This is similar to xPRO so this should work too)
  2. Bootcamps (Also the registration is similar to xPRO and should work there too)
  3. OCW Studio (I am not sure about how this is being used here)

I would hold the merge until I get the approval from someone in OCW team because the changes in this package here might impact them as well. So, Could you please ask check with someone in OCW before merging this. @umar8hassan @ibrahimjaved12 Could you help ?

@marslanabdulrauf
Copy link
Author

Thank you @arslanashraf7 for highlighting the OCW Studio, I completely missed that. I will check with OCW Team for confirmation before merging this.

@marslanabdulrauf
Copy link
Author

I had a discussion over a huddle with @umar8hassan from the OCW team, and here are our findings:

  • OCW-Studio is using mitol-django-authentication version 2023-12-19, which already excludes python3-saml, but it still uses Touchstone for login.
  • In OCW-Studio, python3-saml is actually being pulled in from social-auth-core[saml] as seen in:

Given this, removing python3-saml from mitol-django-authentication should not affect OCW-Studio, as it's still receiving python3-saml via social-auth-core[saml]

However, since this PR also removes Touchstone-related settings and the saml_metadata view, we need confirmation from the OCW team on whether those settings are required for OCW-Studio.

I’m attaching the latest package build for review: mitol_django_authentication-2025.1.9.tar.gz.

Can someone from OCW verify this?

Cc @pdpinch

@pdpinch
Copy link
Member

pdpinch commented Jan 31, 2025

However, since this PR also removes Touchstone-related settings and the saml_metadata view, we need confirmation from the OCW team on whether those settings are required for OCW-Studio.

@umar8hassan can you take a look at this?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants