Produits Entr'ouvert » Authentic 2

• I would use the base URL /api/roles/
• I would use django-rest-framework
• I would use the uuid of the role as the first path element (/api/roles/<role.uuid>/)
• I would use the uuid of the user as the second path element (/api/roles/<role.uuid>/members/<user.uuid>/)
• I would implement that using the POST verb with an empty content

For the first version access control should be based on HTTP authentication, like the register API, by checking permission to modify the role, user.has_perm('a2_rbac.change_role', obj=role).

Je n'utiliserai pas de Serializer, ça sert surtout s'il y a un payload dans un POST ou un PUT, là on se base entièrement sur l'URL.

Tu peux simplement résoudre user_uuid et role_uuid dans la méthode dispatch:

def dispatch(..., role_uuid, user_uuid):
    self.role = get_object_or_404(Role, uuid=role_uuid)
    self.user = get_object_or_404(User, uuid=user_uuid)
    return super(....
...

Je ne renverrai pas un 404 sur un POST qui réussit parce que l'objet existe déjà, soit un 400 soit un 200. Si quelqu'un a un avis plus éclairé.

Par contre sur un DELETE d'un object qui n'existe pas, je renverrai un 404.

The "remove user from role" test should fail currently as the user is not part of the role initially; it should returns a 404 status code. The passing test should first add the user to the role then remove it.

I would create a role from scratch instead of using the first found role. I would attach this role to the entity by default to test role 'manager of roles' of this entity.

• a super-user as currently,
• a user having the global role 'manager of roles' defined by Role.objects.get_admin_role(ContentType.objects.get_for_model(Role), None, None).
• a user having the role 'manager of roles' on the default entity defined by Role.objects.get_admin_role(ContentType.objects.get_for_model(Role), None, None, ou=get_default_ou())
• a user having the managing role of the new role new_role.get_admin_role(),

In the end you should have 3 (created, delete failure, delete success) * 4 (kinds of users) + 4 (forbidden with anonymous, forbidden with logged having no permission, role not found, user not found) = 16 tests.

Sorry to not have reviewed your patch earlier. Do not hesitate to ping me again next time.

Le test de permission doit être fait sur l'objet rôle précis par sur sa classe:

   request.user.has_perm(perm, obj=Role)

ici Role devrait être un self.role à cette occasion je déplacerai tout le code de dispatch() dans initial() on en a besoin là de toute façon et ça fait une méthode en moins.

Ce serait bien de trouver les conditions qui font qu'on reçoit "Vous n\'avez pas la permission d\'effectuer cette action." ou bien "User not allowed to change role".

Apparemment il n'y a que le superuser qui arrive à utiliser l'API, il y a peut-être un souci... (cetainement lié à la remarque plus haut sur la façon de faire le check de permission).

Un seul patch please.

Tu n'as pas pris en compte ma demande de déplacer le code dans dispatch() vers initial(), user.is_superuser or user.has_perm(...) est redondant car has_perm fait aussi un test sur is_superuser.

Si tous les tests passent, pousse donc.