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

Deep linking via pre_loginpage_hook() doesn't fully reset the session resulting in role access / block bugs #117

Closed
kevinorlowski opened this issue Jun 26, 2017 · 15 comments
Assignees
Labels

Comments

@kevinorlowski
Copy link

(we're running Moodle v3.1.5+)

Recently implemented your plugin in conjunction with our IdP appliance SecureAuth. Initial testing was great but we have since noticed some quirky behavior. When logging into the Moodle site itself (front page) everything appears to work perfectly the only time we have seen this issue to date is when sending unauthenticated users directly to a course deep link. (i.e. www.domain.edu/course/view.php?id=XXXXX)

If users attempt to access a course deep link without first being authenticated they are passed to our IdP, after successfully authenticating they are passed back to Moodle. They are logged in and the site / system matches the user appropriately at the system level but the course shell itself displays as if the user was a guest (certain blocks might not display and permissions / access to certain functions are reduced). If the user hits refresh in the browser the course picks up the user correctly and the user then exists on the course page as themselves and not a guest.

Been doing some SAML traces and it appears like when first sent in via the order of operations mentioned above Moodle eventually makes a POST to:

/lib/ajax/service.php?sesskey=XXXXXX

initially from the trace:
GET
sesskey: GgE3QxNjsi
POST
[{"index":0,"methodname":"core_fetch_notifications","args":{"contextid":1}}]:

After hitting refresh Moodle makes the same POST but the trace is different:
GET
sesskey: GgE3QxNjsi
POST
[{"index":0,"methodname":"core_fetch_notifications","args":{"contextid":517545}}]:

contextid 1 is in the DB at a context level of '10' (SYSTEM) and contextid 517545 is at a context level of '50' (COURSE)

It seems like when you are initially passed through the assertion directly to the course page the system knows the user but the course does not and if you navigate away and then back or hit refresh then the system AND the course are given the appropriate user information.

I have been trying like hell to configure my way around the issue. Tried tweaking guest access policy settings, IdP user mapping settings, etc. I can modify the behavior a bit but I have yet to land on the end user experience of deep linking to a course, getting sent to our IdP, authenticating successfully and then landing in the course as them (not guest) without any more manual steps needed.

Many thanks for your time and consideration!

@brendanheywood
Copy link
Contributor

There is possibly a couple things going on. I think the lib/ajax/service.php stuff above is a red herring, that is an unrelated ajax call to get recent notifications for the top right corner. By the time you hit that in theory you should already be authenticated.

In the chrome dev tools can you please turn on 'preserve log' in the network tab and trace exactly what is going on in the main http load. Please dump a screen shot of that network tab and a couple screen shots of the broken and then working state. Feel free to redact as you see fit

@kevinorlowski
Copy link
Author

Thanks for taking the time to look into this. Attached are some screenshots of the Chrome dev tools traces as well as some of the course page itself (the initial load and after the refresh). I also exported the traces from the initial load and post refresh to HAR files that I have zipped and also attached.

If you need anything else just ask. Your time is much appreciated!

course_after_refresh_trace
course_as_guest_trace
course_page_after_refresh
course_page_as_guest

trace_HAR_files.zip

@kevinorlowski
Copy link
Author

Ah, I forgot it renamed all the files upon upload. Order of those images top to bottom is:

  • trace after refresh
  • trace of initial load
  • course page after refresh
  • course page at initial load

@brendanheywood
Copy link
Contributor

Hmm, I'm not really sure from looking at that. It's very odd that some components like the crumb trail and right column are not rendered, but other things like the admin panel are. I would expect a failure to be more an all-or-nothing issue. It could be a bug in this auth plugin or perhaps some weird interaction with other code in your site. I've not seen anything like that in any site which is using it - and it would be a critical show stopper. I'm not sure how much help I can be without getting my hands dirty, I could give you some pointers of where and how to try and debug that yourself or you could get some commercial support?

@kevinorlowski
Copy link
Author

We don't have a ton of internal resources to deep dive on something like this but its worth a shot as this plugin would be a big win for us (trying to move to our new IdP campus wide) and its the best saml plugin I've found to date.

If you could supply those pointers as to where to start looking we'll do our best to poke around and report back if we find anything.

Thanks again!

@brendanheywood
Copy link
Contributor

hi @kevinorlowski,

So two simple things you can do to cut the problem space down a bit:

  1. Do a full login and reload until the page is good. Then in chrome dev tool go to Application > Cookies and you should see a few cookies like this:

image

Now delete just the moodle cookies. Then reload the page, you should still be logged in but it may or may not break the page as above. If this doesn't break it, then try it again and this time clear both the moodle cookie as well as the simplesaml cookies and reload, again you should still stay logged in after some redirects, and see if that breaks.

  1. In auth.php there is a method called pre_loginpage_hook()

https://github.com/catalyst/moodle-auth_saml2/blob/MOODLE_33PLUS/auth.php#L177-L193

Try commenting this whole method out, and do a full moodle purge cache as well as a browser cookie clear, then retest everything. Login should still work but if you watch carefully there will be an extra set of http redirects so it will be slightly slower. If the page works fully with that method commented out then that clearly shows it's a bug in this plugin. But also you might be happy living with the working but slightly slower login flow so you can continue testing and deployment and not be held up waiting for this bug to be isolated and fixed.

@kevinorlowski
Copy link
Author

Your hunch proved fruitful.

Working through the initial cookie tests, removing the MoodleID and Session cookies 'broke' the site and we witnessed the same mixed guest page rendering behavior. Deleting all 4 cookies got us the same behavior as well.

We tried commenting out pre_loginpage_hook() in our dev site, purged all Moodle caches and browser cookies and were able to get the behavior we were seeking. Browsing directly to a course link logged us in with proper roles and page rendering at the system and course level. And we really didn't notice an degradation in speed (any that was easily perceived anyway).

We want to do some more testing but we are going to consider going to prod with this commented out if those tests go well.

I will report back if we discover anything new during the additional tests.

Thanks!

@brendanheywood brendanheywood changed the title Deep linking directly to a Moodle course initially puts user in the course as a guest after IdP authentication Deep linking via pre_loginpage_hook() doesn't fully reset the session resulting in role access / block bugs Jul 2, 2017
@brendanheywood
Copy link
Contributor

thanks @kevinorlowski that's great to help isolate it. I've renamed this issue and we'll try and get to the bottom of this shortly

@chrisshearing
Copy link

I've implemented the saml2 auth (connecting to adfs 3.0) this week on our Moodle 3.2 dev environment, in preparation for rolling this out to our live Moodle instance in summer and ran into this same problem after setting Dual login to No.

The symptom was our homepage after a successful login wouldn't load fully - blocks missing etc (reading this bug report its likely they were set as guest) until a refresh of the page, when everything came back,

With Dual login set to yes - clicking the login via SAML2 worked perfectly,

I've also commented out the pre_loginpage_hook() function and the homepage is loading correctly after an initial login now,

I thought you might find it useful to know of another instance of what is likely to be this bug and we are available to test a bugfix if needed.

@brendanheywood
Copy link
Contributor

thanks @chrisshearing

@brendanheywood
Copy link
Contributor

@kevinorlowski can you still replicate this on demand?

roperto pushed a commit that referenced this issue Aug 17, 2017
Regression #117 after this commit, in case viewing a course we are overriding
the course context with the system context (for example).

This reverts commit 68d62cc.
roperto pushed a commit that referenced this issue Aug 17, 2017
Regression #117 after this commit, in case viewing a course we are overriding the course context with the system context (for example).

This reverts commit 68d62cc.
@roperto
Copy link
Contributor

roperto commented Aug 17, 2017

hi @kevinorlowski and @chrisshearing ,

After discussing with @brendanheywood we decided to revert a fix for #103 that we believe to have caused this issue.

Please let us know if it is working now, in case you still have problems please reopen this issue.

Cheers,

Daniel

@kevinorlowski
Copy link
Author

@roperto

Sorry for the late response I have been on vacation. We just grabbed this latest version today to try some testing and had a few issues getting it in place.

First the plugin->requires line in version.php has the old version number of this plugin and not the version of moodle required so we had to tweak that just to get it to install.

Then once installed we are getting:
Fatal error: Call to undefined function display_auth_lock_options() in /web/app/courses/auth/saml2/settings.php on line 179

based on other code we tried swapping to print_auth_lock_options() but that didn't seem to work.

We'll revert for now. Thanks for your time on this.

@kevinorlowski
Copy link
Author

@roperto

Apologies, please disregard my previous message. My vacation clouded brain grabbed the wrong branch. Once I grabbed the appropriate one it installed fine and after some initial testing things look good on our end.

I'll update if anything comes up. Many thanks again for your time.

@roperto
Copy link
Contributor

roperto commented Aug 29, 2017

Hi @kevinorlowski thank you very much for the feedback, I am happy it worked out.

Let us know if something else comes up.

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

No branches or pull requests

4 participants