learningequality / kolibri

Kolibri Learning Platform: the offline app for universal education
https://learningequality.org/kolibri/
MIT License
779 stars 647 forks source link

Consolidate list remote users api #12321

Closed AlexVelezLl closed 1 month ago

AlexVelezLl commented 3 months ago

Summary

Consolidates the logic of listing remote users from the user_profile and setup_wizard plugins, and migrates them to core auth so that they are accessible by multiple plugins.

There were some differences between both endpoints, and the following decisions were made to merge them:

Reviewer guidance

Testing checklist

PR process

Reviewer checklist

github-actions[bot] commented 3 months ago

Build Artifacts

Asset type Download link
PEX file kolibri-0.17.0a0.dev0_git.88.g5cf101b9.pex
Windows Installer (EXE) kolibri-0.17.0a0.dev0+git.88.g5cf101b9-windows-setup-unsigned.exe
Debian Package kolibri_0.17.0a0.dev0+git.88.g5cf101b9-0ubuntu1_all.deb
Mac Installer (DMG) kolibri-0.17.0a0.dev0+git.88.g5cf101b9.dmg
Android Package (APK) kolibri-0.17.0a0.dev0+git.88.g5cf101b9-0.1.4-debug.apk
TAR file kolibri-0.17.0a0.dev0+git.88.g5cf101b9.tar.gz
WHL file kolibri-0.17.0a0.dev0+git.88.g5cf101b9-py2.py3-none-any.whl
pcenov commented 1 month ago

Hi @AlexVelezLl - I confirm that there are no issues when importing learners as an administrator in the setup wizard or when changing facilities for an "On my own" learner. However when I attempt to merge a learner that was created on the actual "On my own" device then when I try to migrate the learner either by using his password or by using an admin account I am getting permission denied 403 errors in the console and the learner cannot be migrated:

https://github.com/user-attachments/assets/ca56ec9f-968c-417b-809c-1cc73f6fa8c5

LogsDB.zip

AlexVelezLl commented 1 month ago

Hi @pcenov! I have solved the mentioned issues both when mergin the self user and with an admin:

Screenshot from 2024-08-08 15-42-38

@rtibbles I have updated the RemoteFacilityUserAuthenticatedViewset so if the queried user is not an admin we just return the user: https://github.com/learningequality/kolibri/pull/12321/commits/98f2f550c7f2f931fab16f636dde88b810a8d217.

Also, since this commit the IsSelf permission was not working ok because local_user_id was not in kwargs. I tried to solve this here, would love to hear if you have any comment about this :open_hands:.

rtibbles commented 1 month ago

I think rather than mess with the signature of the task function, it might be simpler just to update how the IsSelf references the args/kwargs?

class IsSelf(BasePermission):
    def user_can_run_job(self, user, job):
        return user.id == job.args[1] or user.id == job.kwargs.get("remote_user_pk", None)

    def user_can_read_job(self, user, job):
        return user.id == job.args[1] or user.id == job.kwargs.get("remote_user_pk", None)
pcenov commented 1 month ago

Thanks @AlexVelezLl - I confirm that everything is working correctly now!

AlexVelezLl commented 1 month ago

Thank you @pcenov! @rtibbles would you give it one last look, please? :open_hands:.