-
Notifications
You must be signed in to change notification settings - Fork 14
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
feat: Add student role with minimal access in Superset #307
feat: Add student role with minimal access in Superset #307
Conversation
Thanks for the pull request, @nandodev-net! Please note that it may take us up to several weeks or months to complete a review and merge your PR. Feel free to add as much of the following information to the ticket as you can:
All technical communication about the code itself will be done via the GitHub pull request interface. As a reminder, our process documentation is here. Please let us know once your PR is ready for our review and all tests are green. |
You are missing the part where the role should be assigned automatically at login. Lines 177 to 178 in 8a5c236
You can use something similar to this: if roles:
return roles
if not bool("{{SUPERSET_BLOCK_STUDENT_ACCESS}}"):
return ["student", f"student - {language}"]
raise Exception(f"Student {username} tried to access Superset") |
Also, as you are replicating the permissions for the instructor role. Can you create a template that holds the shared permissions for both roles and inject it where it corresponds? |
tutoraspects/templates/aspects/apps/superset/pythonpath/superset_config_docker.py
Show resolved
Hide resolved
tutoraspects/templates/aspects/apps/superset/security/roles.json
Outdated
Show resolved
Hide resolved
f282bb4
to
850199f
Compare
92c8233
to
bd79be8
Compare
tutoraspects/templates/aspects/apps/superset/pythonpath/openedx_sso_security_manager.py
Outdated
Show resolved
Hide resolved
@@ -174,9 +174,13 @@ def _get_user_roles(self, username, language): | |||
return ["instructor", f"instructor-{language}"] | |||
else: | |||
roles = self.extra_get_user_roles(username, decoded_access_token) | |||
if bool("{{SUPERSET_BLOCK_STUDENT_ACCESS}}") and not roles: | |||
if roles: | |||
return roles |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm somewhat concerned about this logic. Having a student
role returned here would override SUPERSET_BLOCK_STUDENT_ACCESS
. Maybe not a big deal, but can we add a check for that and throw an exception if it happens?
…DENT_ACCESS is True.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @nandodev-net , looks good to me!
@nandodev-net 🎉 Your pull request was merged! Please take a moment to answer a two question survey so we can improve your experience in the future. |
Description:
This pull request introduces a new role for students, designed to be similar to the instructor role but with read-only access. The aim is to ensure students can only access and view assets for which they have permissions.
Changes:
Testing:
Please review the changes and provide feedback.