Corrects a bug experienced with external auth enabled #1547
Conversation
|
Hi @carsongee what's the plan with this PR? It's been untouched for 16 days with no reviews. We're trying really hard to have all open PRs be PRs in progress, and to close PRs that still need work or that aren't planning on being merged soon. |
|
Waiting for edx to merge, or I can merge it.
|
|
There's a few issues here. It hasn't been reviewed at all by edX staff, another is tests haven't been run. I'll get MITx employees added to the whitelist so these PRs will get an automatic Jenkins build. But some edX employees should be tagged in the PRs so that reviews can happen. |
|
This is surrounding auth - @jbau @cpennington or @brianhw would any of you be willing/able to review this PR? |
|
I'd love to know in what context the user is being sent in as |
|
Here is the stack trace, it is possible that this should be solved in the external_auth module, but after trying for some time to fix it up there without luck, it was far simpler to fix it here, plus it seemed like a good idea to do parameter existence checking anyway. 2013-10-29 13:26:46,438 ERROR 20750 [django.request] base.py:215 - Internal Server Error: / |
|
Is it possible that the login(request,user) call at external_auth/views.py line 209 is failing? If so, there doesn't seem to be anything in the code to check this. The particular user should appear in the log in a previous line (i.e. "Login success" with username and email). Is there an issue with that user? |
|
I wonder if what happened is that the user changed their password from the auto created one, and so fails this line: |
|
So, this occurs only on the front page when you have a course that has beta testers, and only happens on the first load (e.g. before clicking login). So there literally isn't a user object yet (since the code in the PR solves the problem by checking for None), but they don't appear to be fully authenticated by the time this code gets run. A refresh of the page clears the error and the user object is properly populated. |
|
I think I merged a change to fix a bug that sounds like this some point in the past. Is it possible that your branch doesn’t have that change? Let me dig it up On Nov 22, 2013, at 10:20 AM, Carson Gee notifications@github.com wrote:
|
|
(If authenticate returned None at 197, then I would expect the test at 198 would be true and it wouldn't get to 223 as the trace above purports.) |
|
Here’s the PR I was talking about. https://github.com/edx/edx-platform/pull/959 On Nov 22, 2013, at 10:22 AM, Jason Bau jbau@stanford.edu wrote:
|
|
Right. of course, missed that. On Nov 22, 2013, at 10:23 AM, brianhw notifications@github.com wrote:
|
|
Let me try and reproduce on a newer version and get back to you...it takes a bit to setup the conditions. |
|
The way django authentication works, and given our middleware setup, we should always have either a real user or an |
|
It looks like that did the trick, but I still think there should probably be some user object checking somewhere. If not here, at least at has_access since we aren't using request.user and thus any function can pass in whatever they want. My two cents. |
|
Adding a check to has_access seems reasonable. |
|
Actually, I stand corrected, the error is still present. |
|
Sorry, I'm still getting used to the new ansible supervisord stuff, and the reload to older code didn't work quite right. Here is a fresh stack trace from commit 93dddf8: |
|
Ok, so finally tracked it down. It looks like it is coming from student.views.index which has user=None by default, and ssl_login is returning the partial of that without user set. So index is then calling get_courses(None, domain=domain) and that triggers the failure. So, where to fix? In student.views.index? or lower in the call stack? like get_courses, or down at the bottom where this is? Or everywhere? |
|
The naive fix could be to default to django.contrib.auth.models.AnonymousUser. On Nov 22, 2013, at 11:30 AM, Carson Gee notifications@github.com wrote:
|
|
So should I just change the params in students.views.index to user=django.contrib.auth.models.AnonymousUser ? I just don't know what side effects that would have down the road and why it is set to None now or what side effects that would have. |
|
Well, I’d argue (as @cpennington pointed out) that user in django context should never be None, so this is a bug fix that other code should, by convention, be tolerating. Really, though, all user is used for is And I do think that user object checks in access.py are reasonable as well—belt and suspenders… On Nov 22, 2013, at 12:02 PM, Carson Gee notifications@github.com wrote:
|
|
rebased and updated |
|
can you write a test in the manner of #959 that shows before=500, after=OK? |
|
I added a test for has_access, but it is extremely difficult to do for student.views.index because it has to have a course that has beta users to hit the has_access code path. I feel like the has_access test is adequate, but I'm open to suggestions. As an FYI, there is a bit of a bug in the branding test.py as it uses self.factory but there is no request factory so it will always throw an attribute error exception, and I would guess that isn't the intent. I only noticed due to my failed attempts to reproduce the error I'm seeing in production in that test class. |
|
Thanks for pointing out the bug with factory. I'll fix. Also, doesn't https://github.com/edx/edx-platform/blob/7668ef6f31f9d88a7eeb9174b56b73af6ccb74a2/lms/djangoapps/branding/tests.py#L26 set up a course with a beta tester (or close to it?) You can just add something there, no? |
|
Setting up a course w/ a beta tester should be fairly straightforward these days. I just cleaned up some test code of the BetaTesterRole (in rc/2013-11-21) that does just that. Basically, you can use a CourseFactory with arguments to add the beta parameter, and then there is (or will be, once that rc branch merges) a factory for creating beta test users for a course. |
|
It turns out I just needed to have start dates enabled and the beta user check happens, so I replaced the existing start dates test with a new test that fails without the new parameter checking/default fixes. |
|
ping @jbau @cpennington @brianhw The tests appear to have failed on something unrelated and I don't know how to rerun them, but this should be good to go. As an additional note, I can't merge stuff. |
|
@carsongee Your branch will auto-build whenever changes are detected. So an easy way to trigger a new build is to rebase your branch and force-push it. I will manually kick off a build for you now. |
|
Thanks @sarina that is very helpful. I'll do that next time. |
|
@carsongee can you rebase this branch? I think it's picked up some extraneous commits. |
|
No problem, rebased. |
|
Great! I think it makes sense for @jbau @cpennington @brianhw to continue the review on this PR. I'll hound them all more after the data jam finishes up. |
lms/djangoapps/branding/tests.py
Outdated
There was a problem hiding this comment.
Please use assertRaises, rather than catching the exception and then calling fail
There was a problem hiding this comment.
Sorry, I misread. Please use except AttributeError as ex, as that is more forward compatible.
There was a problem hiding this comment.
Hrm... In fact, I think I would just leave this clause out, and just let the attribute error propagate, and let that fail the test. Your failure message isn't really adding any new information that the stack trace wouldn't give you.
There was a problem hiding this comment.
Except if there is a particular message that comes with the AttributeError, in which case I would restore the assertRaisesRegexp call.
|
Please squash this branch before merging. |
There was a problem hiding this comment.
Would be good to add this behavior to the docstring as well.
…iews.index Replaced existing test_none_user_index_access_with_startdate_fails test with new test now that the tested function has changed and was causing the original test to fail.
|
I removed the exception catches, added to the doc string, rebased, and squashed. |
|
LGTM 👍 |
|
@carsongee do you still need Will's input on something? I'm not sure from your comment 3 days ago what you needed him to look at. @jbau successfully defended his thesis last week (huzzah ✨ 🎉 🍰 !!!!) and will give this a once-over this week. |
|
Congrats @jbau, and no I went with the preferred method of just letting the exceptions fail the tests instead of catching and throwing fail messages to make it clearer. |
|
thanks @carsongee on the PR: |
Corrects a bug experienced with external auth enabled
* Revert "Fix initialize end-date for self-paced openedx#1544 (openedx#1546)" This reverts commit 8d47dc9cf42c09857efb813e682d1c2f780abd91. * Fix end-date display openedx#1547
6 participants
Simply adds a boundary check to see if the user exists
before attempting to access it's members.