Closed cinnaco closed 5 years ago
First off, thank you for this major PR with substantial improvements to the web app. The improvements you have made are quite impressive.
I have noticed some small improvements that can be made.
In /server/controlPanel/workshopControlPanel.js
for the sendQuery
function I believe you can user use db.multi
instead of two separate commands. If there is a good reason to keep them separated that I cannot see it would then be better to use a task. Either of these methods do not have the problems of releasing and recreating the connection of multiple individual queries. This information can be seen here
When choosing a language there is a large space as seen here
line 112 of client/javascripts/controllers/workshopController.js
needs to be reworded.
It is worth noting that I do not believe the luegg.directives
is needed in the main controller in the current architecture of the webapp. I believe it can be localized to the workshop controller. However, with the time constraints I believe it is not worth the time for research into the pros and cons of this change and thus should be left as it is, as it causes on minor load time increases. However, it may be a good idea to make an issue or note on the wiki so this addition can be refactored after our final milestone tomorrow.
Thanks @KevinKelly25, I appreciate your review on this PR. Here are some of the changes I have made based on your comment:
db.oneOrNone()
and db.any()
has been replaced with db.task()
.onbeforeunload
attribute and a DOM reference to an AngularJS function, which makes an HTTP
POST
request to the route.workshopController.js
has been reworded for clarity.luegg.directives
should be of focus after the end of the semester. I will create an issue for this shortly.@KevinKelly25 and @michaeltorres1, please retest this PR since several versions of dev
have been merged into this branch. I experience no issues on my end, at this point.
@cinnaco, I have tested your PR again after you merged it with dev
. Everything works fine on my end as well.
It all works fine for me now. Thank you for the update.
Introduction
This pull request provides the ability for users to write queries and run them against schemas which they have access to. The database will send its response back to the user in a tabular format where applicable.
Using the UI
The user interface is comprised of two
textarea
sections, a command history log on top and the query composer on the bottom. The user can select the language based on the type of request they intend to send to their class's database. TheSQL
button is assumed by default and should be used when sending most DDL, DML, DCL and TCL queries. ThePL/pgSQL
button must be selected if the input from the user utilizes procedural extensions, so it is parsed correctly. As expected, this option must be selected when writing functions and triggers. The user should be able to write multiple queries in the query composertextarea
and they will be individually executed in order.Results from the database are parsed using JavaScript and are formatted into tables which closely resembles the user interface
psql
provides.Other UI Notes
This pull request has been extensively tested using the three most popular browsers due to the non-standard treatment of the
textarea
tag and related CSS. It should be noted thattextarea
sections are resizable on Mozilla FireFox and Google Chrome, but not Microsoft Edge. Besides this, all three browsers behave in the same manner on this page.Additionally, if you test this pull request using Edge, only version
44.17763.1.0
included with Windows 10 v1809, works as intended. Edge42.17134.1.0
included with Windows 10 v1803, does not wrap the text correctly in the command history. This is not a concern since the newer version of Edge does not exhibit this behavior.In order to keep the command history
textarea
automatically scrolled to the most recent results, I have incorporated a new module. Here is the procedure for installing angularjs-scroll-glue. You may have to delete yourpackage-lock.json
and runnpm install
again to successfully install the module.Security
Every query and request sent to the database using this page is executed under the identity of the web user logged in. I believe it was appropriate to use
SET ROLE
instead ofSET AUTHORIZATION
since it is not the case that the end user initiated the connection, from a server-side programmatic standpoint. I have tested the most obvious attempt to gain unauthorized access to other schemas, by attempting toSET ROLE
back topostgres
. As intended, the user is unable to make this change since thecurrent_user
variable is set to the user's login name every time theRun Code
button is clicked.I want to clarify that while I have made a reasonable attempt to safeguard against some security concerns, it is by no means comprehensive. This is out of scope of this pull request, but there may be some objects which are accessible to users, but in the interest of security, should not be allowed to view. For example, students should be restricted to view the
pg_database
table, as its information can be used to exploit an issue I mentioned in my comment on PR #99. Other tables to consider would beinformation_schema
andpg_catalog
.Conclusion
The major difficulty presented by the objective of this pull request, was formatting the results sent back from the database. Since
textarea
elements do not interpolate HTML, I took an algorithmic approach to parse the results for any given realistic result set. Test this pull request as if you were using thepsql
command line interface with both legitimate and malicious intent. I would appreciate your comments on any shortcomings you experience.