vassili/seitime-frappe
1
0
Fork 0
Code Issues Pull requests Projects Releases Packages Wiki Activity Actions 1

fix: Missing bucket name check (#9546)

Browse source 
* fix: missing bucket name check when user make changes on s3 backup setting

Problem: If the user changed the backup limit, then press save. The validate function will give an error message that the bucket name already exists. It would be inconvenient for the user to use a different bucket name to save any changes.

So I implemented a flag ''bucket_name_exist'' to indicate if the bucket name exists, if not, then go to the flow of trying to create a bucket.

* fix: extra line removed

* fix: Use head_bucket Boto3 API 

1. Head_bucket will return 200 Ok if the bucket exists and you have permission to access it.
2. Error code 403 Forbidden, Error code 404 Not Found.
3. Use bucket_name_exist to check if need to create bucket.

Thanks @Mangesh-Khairnar suggestions.
Reference: 
1. https://boto3.amazonaws.com/v1/documentation/api/latest/reference/services/s3.html#S3.Client.head_bucket
2. https://boto3.amazonaws.com/v1/documentation/api/latest/guide/migrations3.html#accessing-a-bucket

Further improvements:
1. Use 'GET' Requestion to check if the Access Key ID and Secret Access Key is valid (Because AWS tier has 20,000 GET requests while only 2,000 LIST requests)
2. Edge case on Error prompts when head_bucket returns an error code other than '403' or '404'.

* fix: add an extra line to align with original code style

* Update frappe/integrations/doctype/s3_backup_settings/s3_backup_settings.py

Co-Authored-By: Chinmay Pai <chinmaydpai@gmail.com>

* fix: remove the flag and redundant exception

When we got a 404 error, we can just create the bucket as 1. The bucket name does not exist in the current bucket. 2. we have permission to access the bucket.

* Update frappe/integrations/doctype/s3_backup_settings/s3_backup_settings.py

Co-Authored-By: Himanshu <himanshuwarekar@yahoo.com>

* Update frappe/integrations/doctype/s3_backup_settings/s3_backup_settings.py

Co-Authored-By: Chinmay Pai <chinmaydpai@gmail.com>

* fix: missing handling the error code '400' - 'Bad Request'

* fix: applied with DeepSource analysis

1. Lines too long
2. Doc Lines too long
3. Expected 2 blank lines between class and method
4. Unused variable removed

Co-authored-by: Chinmay Pai <chinmaydpai@gmail.com>
Co-authored-by: Himanshu <himanshuwarekar@yahoo.com>
This commit is contained in:
Andy Zhu 2020-03-13 20:36:36 +13:00 • committed by GitHub
parent 1e1cc1b6c0
commit fcdb923d2e
No known key found for this signature in database
GPG key ID: 4AEE18F83AFDEB23
1 changed files with 10 additions and 4 deletions
Show stats Download patch file Download diff file Expand all files Collapse all files

14
frappe/integrations/doctype/s3_backup_settings/s3_backup_settings.py
View file

@ -35,10 +35,16 @@ class S3BackupSettings(Document):
frappe.throw(_("Invalid Access Key ID or Secret Access Key."))
try:
conn.create_bucket(Bucket=bucket_lower, CreateBucketConfiguration={
'LocationConstraint': self.region})
except ClientError:
frappe.throw(_("Unable to create bucket: {0}. Change it to a more unique name.").format(bucket_lower))
# Head_bucket returns a 200 OK if the bucket exists and have access to it.
conn.head_bucket(Bucket=bucket_lower)
except ClientError as e:
error_code = e.response['Error']['Code']
if error_code == '403':
frappe.throw(_("Do not have permission to access {0} bucket.").format(bucket_lower))
else: # '400'-Bad request or '404'-Not Found return
# try to create bucket
conn.create_bucket(Bucket=bucket_lower, CreateBucketConfiguration={
'LocationConstraint': self.region})
@frappe.whitelist()