Conversation
jmaupetit
force-pushed
the
add-acl-proxy
branch
2 times, most recently
from
8921649 to
eb86f34
Compare
We need fine-grained control over dashboard permissions to ensure users cannot access variable values they are not expected to. As it appears that we cannot forbid dashboard input variable values passed via a GET request, we've designed a FastAPI-based application that proxy dashboards requests and check input values given grafana logged user. The acl-proxy application is used as a companion of our grafana instance along with the Caddy2 web proxy using an x-accel-redirect workflow.
jmaupetit
force-pushed
the
add-acl-proxy
branch
2 times, most recently
from
af82846 to
5a59b8a
Compare
CircleCI now automatically checks the acl-proxy application compliance with our standards for a python application. It also handles docker image build and publication.
jmaupetit
force-pushed
the
add-acl-proxy
branch
from
5a59b8a to
215fbd2
Compare
|
I think I'll postpone the documentation and tray implementations in a separated PR so that the review will be easier. |
| if ( | ||
| all((school, course, session)) | ||
| and f"course-v1:{school}+{course}+{session}" not in user_course_keys | ||
| ): | ||
| return forbidden |
There was a problem hiding this comment.
I think we can be more pedantic on this and check for school | course | session in user_course_keys instead of considering the triplet only.
| and f"course-v1:{school}+{course}+{session}" not in user_course_keys | ||
| ): | ||
| return forbidden | ||
|
|
There was a problem hiding this comment.
Also, maybe we should write "rules" instead of series of conditions. Suggestions are welcome on this.
| try: | ||
| valid = validate_email(email) | ||
| except EmailNotValidError as error: | ||
| raise EdxException(f"Invalid input email {email}") from error |
There was a problem hiding this comment.
It's a very tiny detail...
| raise EdxException(f"Invalid input email {email}") from error | |
| raise EdxException(f"Invalid input email: {email}") from error |
SergioSim
left a comment
SergioSim
left a comment
There was a problem hiding this comment.
This is awesome! So many new tools/packages to discover) Thank you)
Have only one minor comment regarding the query)
Also some delicate grafana routes might be /api/ds/query and /api/datasources/proxy/1/_msearch (they seem to accept any kind of user input).
| edx_course_keys_sql_request = ( # nosec | ||
| "SELECT DISTINCT `student_courseaccessrole`.`course_id` " | ||
| "FROM `student_courseaccessrole` WHERE (" | ||
| " `student_courseaccessrole`.`user_id` = (" | ||
| " SELECT id from auth_user " | ||
| f' WHERE email="{valid.email}" ' | ||
| ' AND `student_courseaccessrole`.`role` IN ("staff", "instructor")' | ||
| " )" | ||
| ")" | ||
| ) |
There was a problem hiding this comment.
Email validation is good but maybe we could use in addition the sqlachemy query builder or Textual SQL with Bound Parameters ?
Apropos, the seemingly invalid email: \u0074eacher@example.org is considered valid by validate_email but I'm not sure if it is possible to submit such an email.
There was a problem hiding this comment.
Email validation is good but maybe we could use in addition the sqlachemy query builder or Textual SQL with Bound Parameters ?
I've considered using SQL Alchemy but it seemed a bit convoluted to add such dependency for a single SQL request...
Apropos, the seemingly invalid email: \u0074eacher@example.org is considered valid by validate_email but I'm not sure if it is possible to submit such an email.
Using unicode characters in email addresses is invalid?
There was a problem hiding this comment.
Having a second look, indeed the \u0074eacher@example.org is not a threat.
Python does a good job by escaping this kind of input and the validation fails way before the query - at the pydantic email validation step.
However, there is one more edge case - case sensitivity. Grafana treats emails in a case-sensitive manner. We can have a teacher with "teacher@example.org" and a student changing his email to "Teacher@example.org".
3 participants
Purpose
As grafana has no fine-grained control over variable template values, we need a proxy app that will look for such permissions to allow or deny user requests.
Proposal
x-accel-redirectendpointswrite documentationpostponedimplement application traypostponednginxdraft implementationEdited to postpone docs and tray.