Skip to content
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

Problem with transfer permissions to new copied sheet #1507

Open
P-Arti-A opened this issue Aug 23, 2024 · 10 comments
Open

Problem with transfer permissions to new copied sheet #1507

P-Arti-A opened this issue Aug 23, 2024 · 10 comments
Assignees
Labels
Bug in progress Issue currently in progress by the assignee

Comments

@P-Arti-A
Copy link

Describe the bug
When using gspread.authorize(self.credentials).copy(file_id=file_id, title=title, copy_permissions=True, folder_id=folder_id) on enterprise shared drives, the error occurs: gspread.exceptions.APIError: APIError: [403]: FileOrganizer role is only allowed on folders.

To Reproduce
Steps to reproduce the behavior:

  1. Upload Google Sheets to a Shared Corporate Drive
  2. Give access to Google spreadsheet users, with the FileOrganizer role in the shared drive
  3. Make a copy of this Google spreadsheet with the parameter copy_permissions=True

Expected behavior
Set access roles in a new Google spreadsheet and continue running the script without error

Code example*

import gspread
from oauth2client.service_account import ServiceAccountCredentials

token = 'service_credentials.json'
url = ['https://spreadsheets.google.com/feeds',
          'https://www.googleapis.com/auth/drive']
credentials = ServiceAccountCredentials.from_json_keyfile_name(token, url)
client = gspread.authorize(self.credentials)
client.copy(file_id=file_id, 
                  title=title,
                  copy_permissions=True,
                  folder_id=folder_id)

Environment info:

  • Windows
  • Python 3.10.6 64-bit
  • gspread==6.1.2

Additional context
I was able to fix this issue manually by opening client.py on line 329 and adding the following code there:

if str(p["role"]).lower() == 'fileorganizer' or str(p["role"]).lower() == 'organizer':
    p["role"] = 'writer'

The error no longer appears

@lavigne958 lavigne958 self-assigned this Aug 23, 2024
@lavigne958 lavigne958 added Bug Need investigation This issue needs to be tested or investigated labels Aug 23, 2024
@wobYY
Copy link

wobYY commented Sep 23, 2024

I've done some debugging of this as the same is occurring in my case, so here are some of my findings so far.

I found that this is getting raised in the latest version due to the change in the copy method in client.py. I've tested with v5.12.4 and v6.1.2. The following part of the code is from 5.12.4, which doesn't raise the gspread.exceptions.APIError: APIError: [403]: FileOrganizer role is only allowed on folders.:

gspread/gspread/client.py

Lines 368 to 383 in fd9e0fe

if copy_permissions is True:
original = self.open_by_key(file_id)
permissions = original.list_permissions()
for p in permissions:
if p.get("deleted"):
continue
try:
new_spreadsheet.share(
value=p["emailAddress"],
perm_type=p["type"],
role=p["role"],
notify=False,
)
except Exception:
pass

I've added a print to the try-except block:
image

After running it I get:
image

So therefore, the gspread.exceptions.APIError: APIError: [403]: FileOrganizer role is only allowed on folders. is not raised as the Exception is being handled, unlike the new version where the same copy method when sharing doesn't catch any exceptions:

gspread/gspread/client.py

Lines 312 to 335 in abb8b10

if copy_permissions is True:
original = self.open_by_key(file_id)
permissions = original.list_permissions()
for p in permissions:
if p.get("deleted"):
continue
# In case of domain type the domain extract the domain
# In case of user/group extract the emailAddress
# Otherwise use None for type 'Anyone'
email_or_domain = ""
if str(p["type"]) == "domain":
email_or_domain = str(p["domain"])
elif str(p["type"]) in ("user", "group"):
email_or_domain = str(p["emailAddress"])
new_spreadsheet.share(
email_address=email_or_domain,
perm_type=str(p["type"]),
role=str(p["role"]),
notify=False,
)

Seems like this was the issue beforehand as well, but we just never noticed it because of the try-except 😅 I'll look into it a bit later again when I get more time to do so

@wobYY
Copy link

wobYY commented Sep 23, 2024

Again me... I think I found the source of the issue. The list_permissions() returns the folder permissions as well, i.e. if the user has the Manager permission for the folder in which the spreadsheet is located this will get returned in the return from list_permissions() as organizer and Content Manager as fileOrganizer. Here's the example of JSON from a user and a group that inherited the permissions through folder permission:

{
    "id": "01100....",
    "displayName": "some_name",
    "type": "user",
    "kind": "drive#permission",
    "permissionDetails":
    [
        {
            "permissionType": "member",
            "role": "organizer",
            "inherited": true
        }
    ],
    "photoLink": "photo_link_to_picture",
    "emailAddress": "[email protected]",
    "role": "organizer",
    "teamDrivePermissionDetails":
    [
        {
            "teamDrivePermissionType": "member",
            "role": "organizer",
            "inherited": true
        }
    ],
    "deleted": false
}
{
    "id": "0337188...",
    "displayName": "[email protected]",
    "type": "group",
    "kind": "drive#permission",
    "permissionDetails":
    [
        {
            "permissionType": "member",
            "role": "fileOrganizer",
            "inherited": true
        },
        {
            "permissionType": "file",
            "inheritedFrom": "",
            "role": "fileOrganizer",
            "inherited": true
        }
    ],
    "emailAddress": "[email protected]",
    "role": "fileOrganizer",
    "teamDrivePermissionDetails":
    [
        {
            "teamDrivePermissionType": "member",
            "role": "fileOrganizer",
            "inherited": true
        },
        {
            "teamDrivePermissionType": "file",
            "inheritedFrom": "",
            "role": "fileOrganizer",
            "inherited": true
        }
    ],
    "deleted": false
}

And then this is how the JSON is for user manually added as Editor/Commenter/Viewer:

{
    "id": "1314136....",
    "displayName": "some_name",
    "type": "user",
    "kind": "drive#permission",
    "permissionDetails":
    [
        {
            "permissionType": "file",
            "role": "writer",
            "inherited": false
        }
    ],
    "photoLink": "photo_link_to_picture",
    "emailAddress": "[email protected]",
    "role": "writer",
    "teamDrivePermissionDetails":
    [
        {
            "teamDrivePermissionType": "file",
            "role": "writer",
            "inherited": false
        }
    ],
    "deleted": false
}

The permissions are as follows:

  • Editor = writer
  • Commenter = commenter
  • Reader = reader

@wobYY
Copy link

wobYY commented Sep 23, 2024

I came up with a couple of suggestions to fix this,

Suggestion 1 (just look for three permissions that can be assigned through the UI):

        if copy_permissions is True:
            original = self.open_by_key(file_id)

            permissions = original.list_permissions()
            for p in permissions:
                # We ignore permissions that are applied to the folder and take
                # only the file permissions (writer, commenter, reader)
                if p.get("deleted") or str(p["role"]) not in ("writer", "commenter", "reader"):
                    continue

                # In case of domain type the domain extract the domain
                # In case of user/group extract the emailAddress
                # Otherwise use None for type 'Anyone'

Suggestion 2 (opposite of suggestion 1; check if the roles are folder roles, aka organizer and fileOrganizer):

        if copy_permissions is True:
            original = self.open_by_key(file_id)

            permissions = original.list_permissions()
            for p in permissions:
                # If the role is a folder role (due to it being inherited
                # from the parent folder), skip it.
                if p.get("deleted") or p.get("role") in ("organizer", "fileOrganizer"):
                    continue

                # In case of domain type the domain extract the domain
                # In case of user/group extract the emailAddress
                # Otherwise use None for type 'Anyone'

Suggestion 3 (check if file or member permission types are inherited):

        if copy_permissions is True:
            original = self.open_by_key(file_id)

            permissions = original.list_permissions()
            for p in permissions:
                if p.get("deleted"):
                    continue

                # If the permission is inherited from a parent folder, we
                # ignore it. Permission types we're looking for are 'file'
                # and 'member'.
                perm_details = {p_details.get("permissionType"): p_details.get("inherited") for p_details in p.get("permissionDetails")}
                if perm_details.get("file") or perm_details.get("member"):
                    continue

                # In case of domain type the domain extract the domain
                # In case of user/group extract the emailAddress
                # Otherwise use None for type 'Anyone'

@P-Arti-A
Copy link
Author

P-Arti-A commented Sep 23, 2024

@wobYY I have concerns that your suggested options may cause problems in other places. Examples 1 and 2 will skip users whose "role" does not match the condition. It seems to me that in this example, users will not be able to access the spreadsheet, although they had it before. In example 3, access will not be granted to "type" = "domain" (I look at the access in my team), because "domain" is not inherited in my case.
Also, I wanted to add that I have not observed a situation with dual access to folders and a spreadsheet in one ID
I hope this information will help to understand this bug

@wobYY
Copy link

wobYY commented Sep 23, 2024

@P-Arti-A could you send a JSON response for the "type" = "domain"? I could add a condition so that the "type": "user" is mandatory but, from a technical standpoint the spreadsheet can't have roles organizer and fileOrganizer for users so if we find the user with those, they should be skipped either way as it's impossible for a spreadsheet to have those permissions and the user will instead inherit the Editor role because they have higher permissions due to folder permissions (the organizer and fileOrganizer permissions). The only available permissions for spreadsheets are ("writer", "commenter", "reader")

@P-Arti-A
Copy link
Author

I have little knowledge about access in Google Drive. I didn't know that if the access level is "organizer" or "fileOrganizer", then the access to the spreadsheet is usually "Editor".
Yes, here is the JSON response from one of the spreadsheets:
{'id': '142809...', 'displayName': 'name', 'type': 'domain', 'kind': 'drive#permission', 'permissionDetails': [{'permissionType': 'file', 'role': 'writer', 'inherited': False}], 'role': 'writer', 'allowFileDiscovery': False, 'domain': 'name.com', 'teamDrivePermissionDetails': [{'teamDrivePermissionType': 'file', 'role': 'writer', 'inherited': False}]}

wobYY added a commit to wobYY/gspread that referenced this issue Sep 24, 2024
@lavigne958
Copy link
Collaborator

Hi everyone ! @P-Arti-A thank you for raising this issue. @wobYY I like your approach on solving this issue.

I will go for the option 1 partially. To me we should only copy permissions that we can actually set on the spreadsheet.

So I will take some time to run some tests and I will try to set permissions with different roles ("writer", "fileOrganizer",...) then all those which succeeded will be kept and we'll only allow those roles to be set.

To keep in mind here, whatever where the permissions are set on the original spreadsheet we are still blocked by what the API lets us set. And that is our limitations and it becomes the specs for that bug fix.

I might need someone to help me in testing these on corporate shared drives. I don't think I have access to one.

For reference here is the file permission documentation with all possible values for the role input: https://developers.google.com/drive/api/reference/rest/v3/permissions#Permission

Depending on the results either we can update the current PR or I can create a new one depending on if you want to contribute to the project or not. Just let me know 🙃

@wobYY
Copy link

wobYY commented Sep 25, 2024

Hey @lavigne958 I can provide a couple API responses for the shared drive. Although worth mentioning, I can only provide them for the users and groups. I'm fine with both updating the current PR or closing that one and creating a new one, fully up to you

@lavigne958
Copy link
Collaborator

thanks for the help provided, the API responses will be helpful, we can simply keep the currently open PR and just edit the code, this will add you as contributor of this project too !

I'll come back here once I had time to test different roles

@wobYY
Copy link

wobYY commented Sep 30, 2024

Here are the JSON objects for users and groups. I've just changed the id, displayName and emailAddress to placeholders to not share company information. The rest is raw, from the .list_permissions() method.

User JSON object:

[
    {
        "id": "131413",
        "displayName": "preset-username",
        "type": "user",
        "kind": "drive#permission",
        "permissionDetails": [
            {
                "permissionType": "file",
                "role": "writer",
                "inherited": false
            }
        ],
        "photoLink": "https://external-content.duckduckgo.com/iu/?u=https%3A%2F%2Fwww.shareicon.net%2Fdata%2F512x512%2F2016%2F07%2F10%2F119930_google_512x512.png&f=1&nofb=1&ipt=cdf36d48d2382b227bec2c6735f71833a4714a69b4f06109cab41990a184484d&ipo=images",
        "emailAddress": "[email protected]",
        "role": "writer",
        "teamDrivePermissionDetails": [
            {
                "teamDrivePermissionType": "file",
                "role": "writer",
                "inherited": false
            }
        ],
        "deleted": false
    },
    {
        "id": "131413",
        "displayName": "preset-username",
        "type": "user",
        "kind": "drive#permission",
        "permissionDetails": [
            {
                "permissionType": "file",
                "role": "commenter",
                "inherited": false
            }
        ],
        "photoLink": "https://external-content.duckduckgo.com/iu/?u=https%3A%2F%2Fwww.shareicon.net%2Fdata%2F512x512%2F2016%2F07%2F10%2F119930_google_512x512.png&f=1&nofb=1&ipt=cdf36d48d2382b227bec2c6735f71833a4714a69b4f06109cab41990a184484d&ipo=images",
        "emailAddress": "[email protected]",
        "role": "commenter",
        "teamDrivePermissionDetails": [
            {
                "teamDrivePermissionType": "file",
                "role": "commenter",
                "inherited": false
            }
        ],
        "deleted": false
    },
    {
        "id": "131413",
        "displayName": "preset-username",
        "type": "user",
        "kind": "drive#permission",
        "permissionDetails": [
            {
                "permissionType": "file",
                "role": "reader",
                "inherited": false
            }
        ],
        "photoLink": "https://external-content.duckduckgo.com/iu/?u=https%3A%2F%2Fwww.shareicon.net%2Fdata%2F512x512%2F2016%2F07%2F10%2F119930_google_512x512.png&f=1&nofb=1&ipt=cdf36d48d2382b227bec2c6735f71833a4714a69b4f06109cab41990a184484d&ipo=images",
        "emailAddress": "[email protected]",
        "role": "reader",
        "teamDrivePermissionDetails": [
            {
                "teamDrivePermissionType": "file",
                "role": "reader",
                "inherited": false
            }
        ],
        "deleted": false
    }
]

Groups JSON object:

[
    {
        "id": "0199",
        "displayName": "group-name",
        "type": "group",
        "kind": "drive#permission",
        "permissionDetails": [
            {
                "permissionType": "file",
                "role": "writer",
                "inherited": false
            }
        ],
        "emailAddress": "[email protected]",
        "role": "writer",
        "teamDrivePermissionDetails": [
            {
                "teamDrivePermissionType": "file",
                "role": "writer",
                "inherited": false
            }
        ],
        "deleted": false
    },
    {
        "id": "0199",
        "displayName": "group-name",
        "type": "group",
        "kind": "drive#permission",
        "permissionDetails": [
            {
                "permissionType": "file",
                "role": "commenter",
                "inherited": false
            }
        ],
        "emailAddress": "[email protected]",
        "role": "commenter",
        "teamDrivePermissionDetails": [
            {
                "teamDrivePermissionType": "file",
                "role": "commenter",
                "inherited": false
            }
        ],
        "deleted": false
    },
    {
        "id": "0199",
        "displayName": "group-name",
        "type": "group",
        "kind": "drive#permission",
        "permissionDetails": [
            {
                "permissionType": "file",
                "role": "reader",
                "inherited": false
            }
        ],
        "emailAddress": "[email protected]",
        "role": "reader",
        "teamDrivePermissionDetails": [
            {
                "teamDrivePermissionType": "file",
                "role": "reader",
                "inherited": false
            }
        ],
        "deleted": false
    }
]

@lavigne958 lavigne958 added in progress Issue currently in progress by the assignee and removed Need investigation This issue needs to be tested or investigated labels Oct 7, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Bug in progress Issue currently in progress by the assignee
Projects
None yet
Development

No branches or pull requests

3 participants