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

[ISSUE-218] fix: Change user from None to Anonymous User in authenticated requests #228

Closed

Conversation

OlhaShyliaieva
Copy link

Description

Modified the Lti1p3ApiAuthentication authentication backend. Set up in request AnonymousUser instead of None.
It allows DarkLangMiddleware to work properly without crashing when middleware tries to get a user from the request.

Supporting information

Issue: #218

@openedx-webhooks
Copy link

openedx-webhooks commented Feb 1, 2022

Thanks for the pull request, @OlhaShyliaieva! I've created OSPR-6428 to keep track of it in JIRA, where we prioritize reviews. Please note that it may take us up to several weeks or months to complete a review and merge your PR.

Feel free to add as much of the following information to the ticket as you can:

  • supporting documentation
  • Open edX discussion forum threads
  • timeline information ("this must be merged by XX date", and why that is)
  • partner information ("this is a course on edx.org")
  • any other information that can help Product understand the context for the PR

All technical communication about the code itself will be done via the GitHub pull request interface. As a reminder, our process documentation is here.

Please let us know once your PR is ready for our review and all tests are green.

@regisb
Copy link

regisb commented Feb 1, 2022

@giovannicimolin do you have an idea why you (or edX) did not face this issue before? Unless I'm mistaken it should occur for every LTI 1.3 unit, right?

@giovannicimolin
Copy link
Contributor

@regisb

do you have an idea why you (or edX) did not face this issue before? Unless I'm mistaken it should occur for every LTI 1.3 unit, right?

No idea, I even tried reproducing it on my devstack without success. I worked for me during the implementation phase as well as for @nedbat when he ran the IMS certification tool.


@OlhaShyliaieva Thanks for the contribution! Can you sign the contributor agreement and ping me once it's done? Only then I'll be able to review + move this forward. 😁

@e0d
Copy link
Contributor

e0d commented Feb 4, 2022

@OlhaShyliaieva we've processed an individual CLA for you, but I believe this should be covered under the RaccoonGang Entity CLA. @idegtiarov can you confirm?

@natabene
Copy link
Contributor

natabene commented Feb 4, 2022

@OlhaShyliaieva Thank you for your contribution.

@codecov
Copy link

codecov bot commented Feb 7, 2022

Codecov Report

Merging #228 (c82dbeb) into master (f96d560) will increase coverage by 0.00%.
The diff coverage is 100.00%.

Impacted file tree graph

@@           Coverage Diff           @@
##           master     #228   +/-   ##
=======================================
  Coverage   97.14%   97.14%           
=======================================
  Files          66       66           
  Lines        4696     4697    +1     
=======================================
+ Hits         4562     4563    +1     
  Misses        134      134           
Flag Coverage Δ
unittests 97.14% <100.00%> (+<0.01%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
lti_consumer/__init__.py 100.00% <100.00%> (ø)
...ti_1p3/extensions/rest_framework/authentication.py 100.00% <100.00%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update f96d560...c82dbeb. Read the comment docs.

@idegtiarov
Copy link

Hi @e0d I confirm @OlhaShyliaieva is covered with the RaccoonGang Entity CLA.

@natabene
Copy link
Contributor

natabene commented Feb 7, 2022

@idegtiarov Thanks for confirming. @e0d Please update the file once you have a chance.
@OlhaShyliaieva I think your CLA check should clear with the next commit. Let me know if it doesn't happen.

@giovannicimolin
Copy link
Contributor

@natabene CLA check is green now. 😄

@OlhaShyliaieva Can you add in a minor version bump (here) and add a changelog entry?

@OlhaShyliaieva
Copy link
Author

@giovannicimolin Done

@natabene
Copy link
Contributor

I still see a conflict.

@giovannicimolin
Copy link
Contributor

@OlhaShyliaieva I just tested the change and it's working perfectly. A few changes were merged in while this was in my backlog, so can you update the version bump and changelog? Once you do it I'll approve the PR and merge the changes.

ormsbee pushed a commit to ormsbee/xblock-lti-consumer that referenced this pull request Jun 30, 2022
Modified the Lti1p3ApiAuthentication authentication backend to return
AnonymousUser instead of None. This allows DarkLangMiddleware to work
properly without crashing when middleware tries to get a user info
from the request.

This investigation and patch was originally done by @OlhaShyliaieva in:
  openedx#228

I'm just rebasing it onto the latest version to land this change.
ormsbee pushed a commit to ormsbee/xblock-lti-consumer that referenced this pull request Jul 5, 2022
Modified the Lti1p3ApiAuthentication authentication backend to return
AnonymousUser instead of None. This allows DarkLangMiddleware to work
properly without crashing when middleware tries to get a user info
from the request.

This investigation and patch was originally done by @OlhaShyliaieva in:
  openedx#228

I'm just rebasing it onto the latest version to land this change.
ormsbee pushed a commit that referenced this pull request Jul 5, 2022
…#261)

Modified the Lti1p3ApiAuthentication authentication backend to return
AnonymousUser instead of None. This allows DarkLangMiddleware to work
properly without crashing when middleware tries to get a user info
from the request.

This investigation and patch was originally done by @OlhaShyliaieva in:
  #228

I'm just rebasing it onto the latest version to land this change.
@ormsbee
Copy link
Contributor

ormsbee commented Jul 5, 2022

I merged this change in a different PR: #261

@ormsbee ormsbee closed this Jul 5, 2022
@openedx-webhooks
Copy link

@OlhaShyliaieva Even though your pull request wasn’t merged, please take a moment to answer a two question survey so we can improve your experience in the future.

ormsbee pushed a commit that referenced this pull request Jul 8, 2022
Modified the Lti1p3ApiAuthentication authentication backend to return
AnonymousUser instead of None. This allows DarkLangMiddleware to work
properly without crashing when middleware tries to get a user info
from the request.

This investigation and patch was originally done by @OlhaShyliaieva in:
  #228

This is a Nutmeg backport of the 4.2.2 release. See:
  #261

I'm just rebasing it onto the latest version to land this change.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
open-source-contribution PR author is not from Axim or 2U
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

8 participants