Users.update_user_by_id does not handle `enterprise=None` correctly

Djones4822 commented 6 days ago

[X ] I have checked that the [SDK documentation][sdk-docs] doesn't solve my issue.
[X ] I have checked that the [API documentation][api-docs] doesn't solve my issue.
[X ] I have searched the [Box Developer Forums][dev-forums] and my issue isn't already reported (or if it has been reported, I have attached a link to it, for reference).
[ X] I have searched [Issues in this repo][github-repo] and my issue isn't already reported.

Description of the Issue

Executing the function client.users.update_user_by_id with enterprise=None does not clear the enterprise for the user. The documentation indicates that the value is Optional[str] and so None is being filtered out.

The issue seems to arise from the serialize method which filters out values that are None through the to_dict method on the BaseObject class.

I would expect that setting the enterprise to None would clear the field and remove a user from the managed user pool.

Steps to Reproduce

Add an external user, promote them to managed user, then with an authenticated client, run client.users.update_user_by_id(f'{USER_ID}', enterprise=None), alternatively, try client.users.update_user_by_id(f'{USER_ID}', enterprise='null') - would expect the latter to work based on the documentation, but it returns a 403 privilege error.

Recommendation

To elegantly add this functionality, I would consider changing the method to use kwarg collection instead of explicitly defining all the fields, with good documentation this allows for flexibility so that you don't need to filter out body elements that are set to None automatically. However, alternatively you can simply add a handler for enterprise and allow the value null like is stated in the documentation. For example:

    def update_user_by_id(...):
        [...]
        json_body: SerializedData = serialize(request_body)
        if request_body['enterprise'] == 'null':
            json_body['enterprise'] = None
        response: FetchResponse = fetch(
            ''.join(
                [
                    self.network_session.base_urls.base_url,
                    '/2.0/users/',
                    to_string(user_id),
                ]
            ),
            FetchOptions(
                method='PUT',
                params=query_params_map,
                headers=headers_map,
                data=json_body,                                # replace with the new json_body instead of serializing here
                content_type='application/json',
                response_format='json',
                auth=self.auth,
                network_session=self.network_session,
            ),
        )
        return deserialize(response.data, UserFull)

I have a fork with that change in place if you'd accept the PR.

Djones4822 commented 6 days ago

In case it's of any value, I have a forum post on the topic here: https://forum.box.com/t/python-updating-user-enterprise-not-working-permission-error/2503

Djones4822 commented 5 days ago

I did a quick manual scan to see if any other fields might have this issue.

In PUT User there is enterprise and nofitication_email in PUT Restore file version there is trashed_at In PUT Update file there is collections, locked, shared_link, and shared_link.password In PUT Update folder there is collections, folder_upload_email, and shared_link.password In PUT Update group membership there is configurable_permissions In PUT Update retention policy there is disposition_action and status

I stopped there... I can run an automated crawl for this later if needed.

This problem is pervasive. Unless I'm missing something and there already is a solution for this...

arjankowski commented 4 days ago

Hi @Djones4822,

Thank you so much for identifying the bug and pointing out additional places where it occurs. Indeed, we have an issue in the generated SDK where we don't handle fields with the nullable: true attribute correctly from the open api specification. (https://swagger.io/docs/specification/data-models/data-types/#null). Instead, None values are being omitted.

We have created a ticket for this and will address it soon. As soon as we fix it, we will inform you in this thread.

Best regards, Artur

box / box-python-sdk-gen