SafeExamBrowser / seb-win-refactoring

Safe Exam Browser for Windows.
https://www.safeexambrowser.org/news_en.html
Mozilla Public License 2.0
179 stars 117 forks source link

Initial test – three questions #2

Closed Krisell closed 4 years ago

Krisell commented 4 years ago

During initial testing of SEB 3.0, I noticed three things. I don't know if these are known issues:

1) Using Microsoft Edge, sebs:// links don't trigger a start of SEB (nothing happens). Using Chrome, SEB is launched. 2) Using a sebs://-link from Chrome starts SEB but triggers a "Configuration Error" message saying the resource is not supported, and SEB is quit. If I download the exact contents of the same URL and save it as an .seb-file, it all works as expected, so there's nothing wrong with the actual configuration.

Let me know if these are not known issues and if you want me to post a link that behaves in this way for me.

3) Cookies seem to be cleared between launches, as chosen language is not remembered.

Krisell commented 4 years ago

Update: The first issue is probably not related to SEB 3.0. I installed SEB 2.4 again and the issue remained, and logging out and back in resolved it, even for 3.0.

dbuechel commented 4 years ago

Thanks for testing and your feedback! The exact reason for the configuration error can be found in the log file of the runtime, see %LocalAppData%\SafeExamBrowser\Logs. Feel free to post it here so we may investigate it. Regarding the cookies, there are new settings in the "Browser" and "Exam" tab where you can control whether SEB keeps cookies and the browser cache between sessions.

Krisell commented 4 years ago

Thank you for your prompt reply! I might not be able to try again until next week, but I will get back to you then.

Krisell commented 4 years ago

I made some quick checks, and I can verify that the cookie problem was resolved by configuration settings. Are these configuration additions backward "compatible" (i.e. they don't break previous versions for any platform)?

The sebs:// issue seems to be that the hosting I'm using (Google Cloud Platform storage bucket) responds with 400 for HEAD-requests, even though the file exists and GET-requests return 200. I'm not sure if the issue is at Google or in SEB, but is it necessary to send the HEAD-request first in the NetworkResourceLoader?

I'll be able to post the full log file on Monday if you still need that.

A fourth question, is there a setting to not show the large left menu after SEB is launched (it covers a third of the screen and requires a click-away to close)?

Krisell commented 4 years ago

Edit: I just realized the HEAD-issue is probably because I don't set any CORS headers. I'll try to change that on Monday.

dbuechel commented 4 years ago

Aha, I see. Well, for performance reasons I would prefer to keep the mechanism as it is now, that particular method only checks whether the loader can access the resource (with a GET request, the resource would then be downloaded twice).

Regarding the large left menu: This is a new feature of SEB 3.x called the Action Center (or side menu) and can be controlled in the "User Interface" tab.

Krisell commented 4 years ago

That sounds reasonable, setting CORS-headers and allowing HEAD requests are not an issue.

Thanks, have a nice weekend and stay safe!

dbuechel commented 4 years ago

Thanks, same to you. Let me know whether it worked out regarding the HEAD issue when you have the time to investigate it.

Krisell commented 4 years ago

It does seem like this Google storage unfortunately does not support HEAD-requests, I've found other reports of similar problems.

We might be able to work around this, but I'd like to suggest skipping the HEAD-request and making a GET-request instead and also caching the downloaded data such that no additional request need to be made when the actual configuration is loaded. It does sound unnecessary, both for the individual client and for school networks, to make two requests for every student, even though the first is only a HEAD-request without any body in the response.

dbuechel commented 4 years ago

Hm, are you absolutely sure that it isn't a configuration issue with your server? HEAD requests should work just fine, this literally is their purpose (to retrieve meta information about a resource, in this case to check whether it exists).

Caching the resource is out of the question, since I can right off the top of my head come up with a scenario where caching could cause serious issues (say a school uses the same SEB instance during a day for multiple exams and always serves the exam configurations from the same URL, even though they are different). I could implement a fallback GET request in case the HEAD fails, but that would be unnecessary overhead in all those cases where a resource really doesn't exist...

Krisell commented 4 years ago

I'm testing this using Google Firebase storage, and the HEAD problem seems to be reported: https://github.com/firebase/firebase-js-sdk/issues/2623

The response sounds like it will be fixed in the future, but that could take time. Also, chances are there are other storage hosting providers (today or in the future) with the same issue.

The caching I referred to was simply between the two, almost simultaneous, requests (checking file and loading configuration), not longer than that.

dbuechel commented 4 years ago

Okay, I have implemented a fallback GET request in case HEAD fails. But I did this quite reluctantly, since I do not like having to fix issues of other libraries in my code...

I would welcome it if you could let me know once the issue is resolved in this Google library, so that I can potentially remove the fallback.

dbuechel commented 4 years ago

Please test the latest build and let me know whether it works.

Krisell commented 4 years ago

Thank you @dbuechel ! I absolutely understand your reasoning. I'll try it out once I can access a Windows computer (later today), and report back.

Another note – the url might not point to an actual file but rather at backend code generating the configuration on the fly. Sending two requests might cause the backend to generate the data twice, using more server resources than intended. That can definitely be solved in the backend, but I just wanted to point out another possible drawback with using multiple requests.

dbuechel commented 4 years ago

Okay, great.

Another note – the url might not point to an actual file but rather at backend code generating the configuration on the fly. Sending two requests might cause the backend to generate the data twice, using more server resources than intended. That can definitely be solved in the backend, but I just wanted to point out another possible drawback with using multiple requests.

That would be the right place to cache, IMHO 😉

Krisell commented 4 years ago

That would be the right place to cache, IMHO 😉

Perhaps you're right, but remember that two subsequent requests to a URL might hit two completely different servers under a load balancer, so in that case, a central cache store need to be involved, which is probably slower than generating the data again.

I obviously don't have a lot of knowledge of your implementation, but currently I don't see why the TryLoad function simply can't return the data received in the IsAvailable function fired just before.

dbuechel commented 4 years ago

The reason is that there are multiple use cases involved here, one of them is the following: In case a resource needs authentication (response code 401 "Unauthorized"), SEB should load the resource in the browser so that a user may authenticate themselves, whereupon SEB will download the resource and reconfigure itself. In this and the other use cases, I deemed it more efficient to use HEAD requests to determine the status of a resouce, as this again literally is the purpose of HEAD requests.

Krisell commented 4 years ago

I downloaded https://github.com/SafeExamBrowser/seb-win-refactoring/releases/download/3.0.0-beta/SEB_3.0.0.118_SetupBundle.exe but that does not seem to include these changes. I get the same error and there are no mentions of HEAD request was not successful, trying GET request for in the log file.

Is there another link I can use to get the latest build?

dbuechel commented 4 years ago

Yes, please use the direct link to the build server: https://sebdev-let.ethz.ch/project/appveyor/seb-win-refactoring/build/job/u6p88j9miqra29dg/artifacts

Krisell commented 4 years ago

Yes, please use the direct link to the build server: https://sebdev-let.ethz.ch/project/appveyor/seb-win-refactoring/build/job/u6p88j9miqra29dg/artifacts

Thanks. It works!

2020-03-30 14:28:30.932 [06] - DEBUG: [NetworkResourceLoader] Sending HEAD request for 'https://configuration-data'...
2020-03-30 14:28:31.251 [06] - DEBUG: [NetworkResourceLoader] Received response '400 - Bad Request'.
2020-03-30 14:28:31.255 [06] - DEBUG: [NetworkResourceLoader] HEAD request was not successful, trying GET request for 'https://configuration-data'...
2020-03-30 14:28:31.444 [06] - DEBUG: [NetworkResourceLoader] Received response '200 - OK'.
2020-03-30 14:28:31.447 [06] - DEBUG: [NetworkResourceLoader] Can load 'sebs://configuration-data' as it references an existing network resource.
2020-03-30 14:28:31.454 [06] - DEBUG: [NetworkResourceLoader] Sending GET request for 'https://configuration-data'...
2020-03-30 14:28:31.584 [06] - DEBUG: [NetworkResourceLoader] Received response '200 - OK'.
2020-03-30 14:28:31.586 [06] - DEBUG: [NetworkResourceLoader] Trying to extract response data...
2020-03-30 14:28:31.592 [06] - DEBUG: [NetworkResourceLoader] Created 'System.IO.MemoryStream' for 19,843 KB data of response body.

I still want to point out that performing 2–3 identical requests (3 if HEAD fails) within a few milliseconds might affect performance in school networks where hundreds of students does this simultaneously. Caching on the server has no effect on that.

Krisell commented 4 years ago

Are you sure by the way that the HTTP-layer you're using does not already respect cache-headers, thus not sending the second request if the first responded with cache-headers?

dbuechel commented 4 years ago

I still want to point out that performing 2–3 identical requests (3 if HEAD fails) within a few milliseconds might affect performance in school networks where hundreds of students does this simultaneously. Caching on the server has no effect on that.

Uhm, isn't it the case that there only now might be 3 requests solely because your library does not correctly handle HEAD requests...? 😕

Regarding your other point, I don't think there is any caching in the .NET API I am using, I however did not investigate this in great detail. There is a ton of work involved in writing a feature-rich application like SEB single-handed, I simply don't have the capacity to test out every minute detail. If you'd like to know it exactly, feel free to debug / test it yourself. I certainly aspire to improve the code whenever and wherever I can...

dbuechel commented 4 years ago

If there aren't any other issues, I suggest we close this one for now?

Krisell commented 4 years ago

Regarding your other point, I don't think there is any caching in the .NET API I am using, I however did not investigate this in great detail. There is a ton of work involved in writing a feature-rich application like SEB single-handed, I simply don't have the capacity to test out every minute detail. If you'd like to know it exactly, feel free to debug / test it yourself. I certainly aspire to improve the code whenever and wherever I can...

I'll take a look at the caching when I get the chance. It should be easy to test considering that there are two identical requests now if I reject the initial HEAD.

If there aren't any other issues, I suggest we close this one for now?

Absolutely, closed.

Krisell commented 4 years ago

Setting cache-headers on the response did not stop SEB from making additional identical requests.

dbuechel commented 4 years ago

Great, thanks for investigating!