-
Notifications
You must be signed in to change notification settings - Fork 21
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
Add per-hunt access permissions #753
Conversation
google_api_lib/tasks.py
Outdated
|
||
UserModel = get_user_model() | ||
emails = get_file_user_emails.run(hunt.settings.google_drive_folder_id) | ||
print(f"Found emails {emails}") |
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.
Oops, delete these
I think I want to write some tests before trying to ship this but looking for feedback anyway |
@@ -22,6 +22,7 @@ redis = "^4.1.0" | |||
django-celery-beat = "^2.4.0" | |||
python-dateutil = "^2.8.2" | |||
django-cors-headers = "^4.3.1" | |||
django-guardian = "^2.4.0" |
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.
oh cool, didn't know something like this existed
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 remember researching it when @rgossiaux brought it up in #191
Should we be concerned that the codebase hasn't been touched in 2+ years? https://github.com/django-guardian/django-guardian
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.
On that note, is there a reason to use django-guardian
over the per-object permissions provided by DRF? https://www.django-rest-framework.org/api-guide/permissions/#object-level-permissions
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.
as far as I know it's the most popular/mature library for this in the django ecosystem. No commits in 2 years is probably a bit worrying but I'm just doing pretty basic core stuff with it so I'm personally not too bothered atm
DRF provides view methods for checking permissions (I'm using them in this PR already I think), but not a permissions backend itself. Still need some way of storing permissions in the DB--that's what django-guardian provides
@@ -131,11 +131,6 @@ Global Google Drive, Sheets, and API settings for automatic sheets creation: | |||
* `GOOGLE_API_PRIVATE_KEY` - the private key for the key you added, with newlines replaced with `\n` (should be the value of the `private_key` field in the downloaded JSON when you [created the key](https://cloud.google.com/iam/docs/creating-managing-service-account-keys#creating_service_account_keys); should look something like `-----BEGIN ... KEY-----\n...<long base64-encoded key>...\n-----END ... KEY-----\n`) | |||
* `GOOGLE_API_X509_CERT_URL` - the value of the `client_x509_cert_url` field in the downloaded JSON when you [created the key](https://cloud.google.com/iam/docs/creating-managing-service-account-keys#creating_service_account_keys) | |||
|
|||
Hunt-specific Google settings: | |||
|
|||
* `GOOGLE_DRIVE_HUNT_FOLDER_ID` - the id of your Google Drive folder, should be part of the URL (`https://drive.google.com/drive/folders/<folder_id>`). This environment variable is used for getting the list of allowed emails for OAuth. Please keep consistent with the value in HuntSettings. Deprecating this variable is tracked in #662. |
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.
are we able to delete the code in settings.py that read these environment variables? https://github.com/cardinalitypuzzles/cardboard/pull/753/files#diff-c003920b4e2c2658c5433b6599e1a2c0c21fd4abfeb45a7b32a8850b4a47f298L209
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.
oh we still use it as the default value in google_api_lib/tasks.py, probably should clean this up or make an issue to clean it up later.
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.
look great, woo!
bc140c6
to
35537f5
Compare
This change appears to have broken the hunt settings page by removing the submit button from the associated form |
After this change, anyone is allowed to log in to cardboard, but they can only see hunts that they have access to. For the moment, users gain access to a hunt via a button on the /edit page that syncs permissions from the Drive folder. Note that this only gives people access, it never removes access right now.