15five / django-scim2

A SCIM 2.0 Service Provider Implementation (for Django)
http://django-scim2.readthedocs.io/
Other
76 stars 27 forks source link

admin's scim_id was changed. #87

Closed daisfukuda closed 2 years ago

daisfukuda commented 2 years ago

I built the environment as follows.

Doing some tests with curl, I was able to get information of ServiceProviderConfig, ResourceTypes, and Schemas with the GET method. (I also confirmed that errors occur when there is no bearer token.)

However, when I tried to create a User with POST, scim_id was duplicated. So I overwrite scim_id in models.py to comment-out unique=True (change to unique=False), I was able to create a user.

I soon found out why this happened.

Attempting to get the user with scim_id=21 with GET /scim/v2/Users/21 returned an error that multiple users were detected. And looking at admin's scim_id, it changed to 21.

I changed admin's scim_id to 1 and tried creating another user again, and admin's scim_id is changed to 22 which is the same as the new user.

Considering the timing, I think that there is a problem with save (L368) of PostView Class in views.py below: https://github.com/15five/django-scim2/blob/master/src/django_scim/views.py#L368

I overwrite "save" method by referring to "demo/app/adapters.py".


def save(self):
    is_new_user = self.is_new_user
    try:
        with transaction.atomic():
            super().save()
            if is_new_user:
                # Set SCIM ID to be equal to database ID. Because users are uniquely identified with this value
                # its critical that changes to this line are well considered before executed.
                self.obj.__class__.objects.update(scim_id=str(self.obj.id))
            logger.info(f'User saved. User id {self.obj.id}')
    except Exception as e:
        # raise self.reformat_exception(e)
        raise RuntimeError(e.args) from e

I think that there is a problem with the below URL.

https://github.com/15five/django-scim2/blob/master/src/django_scim/models.py#L150-L153

Or is there a problem with the save overriding or the method of issuing Bearer Tokens?

daisfukuda commented 2 years ago

I found all user's scim_id is changed to the same one...

logston commented 2 years ago

Thanks for the bug report. Pardon the long delay on this. My time to support this library is very sparse.

Did you create a custom SCIM User Adapter? If so, did you override the .save() method? Was it override to the snippet you posted above? If not, can you post it in full?

The current implementation is working well in production so I'm hesitant to say its an issue with the URL or anything related to bearer tokens.

daisfukuda commented 2 years ago

Thank you very much for providing this wonderful library and for taking time out of your busy schedule to reply to this matter.

As you indicated, I create a custom SCIM User Adapter and override .save() method to the snippet I posted aboe, even though not necessary.

By removing the custom .save method the issue was resolved.

I am very sorry to take your time for my lack of understanding...

By the way, I wonder if demo/app/models.py L7

from django_scim.models import AbstractSCIMGroup, AbstractSCIMUser

should be replaced to

from django_scim.models import AbstractSCIMGroupMixin, AbstractSCIMUserMixin

This is the only level I can propose, but if you don't mind, I will raise as pull request. If it is not necessary, there is no need to reply. I will close this issue on August 27th (I set the reminder.)

logston commented 2 years ago

Great! Glad you got it working!

And now worries about the issue. Your question will hopefully help others with the same issue in the future.

By the way, I wonder if demo/app/models.py L7 from django_scim.models import AbstractSCIMGroup, AbstractSCIMUser should be replaced to from django_scim.models import AbstractSCIMGroupMixin, AbstractSCIMUserMixin

Ah yes, it looks like the demo project is well out of date. If you'd like to issue a PR and test it works, that would be very welcome.