Project

General

Profile

Bug #8234

webservice to add/remove users to/from roles

Added by Frédéric Péters about 4 years ago. Updated about 2 years ago.

Status:
Fermé
Priority:
Normal
Assignee:
Josué Kouka
Category:
-
Target version:
Start date:
10 Sep 2015
Due date:
22 Nov 2015
% Done:

0%

Patch proposed:
Yes
Planning:
No

Description

To be used by wcs "add/remove role" workflow actions.

fake.patch View (3.96 KB) Josué Kouka, 15 Oct 2015 02:56 PM

0001-webservice-add-remove-user-from-role-8234.patch View (7.6 KB) Josué Kouka, 16 Oct 2015 02:16 PM

0001-roles-add-remeve-member-api-8234.patch View (7.79 KB) Josué Kouka, 30 Nov 2015 11:39 AM

roles-add-remeve-member-api-8234.patch View (12.6 KB) Josué Kouka, 01 Dec 2015 10:03 AM

0002-improving-role-api-tests-8234.patch View (4.79 KB) Josué Kouka, 01 Dec 2015 10:12 AM

0001-roles-add-remeve-member-api-8234.patch View (7.79 KB) Josué Kouka, 01 Dec 2015 10:12 AM

0001-roles-api-add_remove-members-8234.patch View (1.96 KB) Josué Kouka, 01 Dec 2015 12:54 PM


Related issues

Related to Publik - Project management #8006: Supprimer la création de rôles dans w.c.s. Solution déployée 03 Aug 2015 05 Jan 2016
Duplicated by Authentic 2 - Bug #8599: API to add/remove user roles Fermé 11 Oct 2015

Associated revisions

Revision 313e17e4 (diff)
Added by Josué Kouka about 4 years ago

roles add/remeve member api #8234

Revision 0b1e833c (diff)
Added by Josué Kouka about 4 years ago

improving role api tests #8234

Revision f9f510c3 (diff)
Added by Josué Kouka about 4 years ago

roles-api-add_remove-members-#8234

History

#1 Updated by Frédéric Péters about 4 years ago

#2 Updated by Benjamin Dauvergne about 4 years ago

  • Duplicated by Bug #8599: API to add/remove user roles added

#3 Updated by Benjamin Dauvergne about 4 years ago

  • Assignee set to Josué Kouka

#4 Updated by Benjamin Dauvergne about 4 years ago

To give some insight on implementing this ticket:
  • 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).

#5 Updated by Josué Kouka about 4 years ago

Fake patch submission.

#6 Updated by Benjamin Dauvergne about 4 years ago

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.

#7 Updated by Josué Kouka about 4 years ago

#8 Updated by Benjamin Dauvergne about 4 years ago

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.

Also like the registration API tests I would make tests with different kind of users:
  • 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.

#9 Updated by Benjamin Dauvergne about 4 years ago

  • Target version set to 2.2.0

#10 Updated by Benjamin Dauvergne about 4 years ago

  • Status changed from Nouveau to En cours

#11 Updated by Josué Kouka about 4 years ago

  • Due date set to 20 Sep 2015

#12 Updated by Josué Kouka about 4 years ago

  • Due date changed from 20 Sep 2015 to 22 Nov 2015

#13 Updated by Josué Kouka about 4 years ago

#14 Updated by Benjamin Dauvergne about 4 years ago

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).

#15 Updated by Josué Kouka about 4 years ago

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).

L'erreur était du au fait que dans mon RolesAPI qui avait en permissions HasUserAddPermissions. J'ai enlevé la permission et tout semble OK.

#17 Updated by Benjamin Dauvergne about 4 years ago

Un seul patch please.

#18 Updated by Benjamin Dauvergne about 4 years ago

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.

#19 Updated by Josué Kouka about 4 years ago

Désolé. C'est fait

#20 Updated by Benjamin Dauvergne about 4 years ago

Si tous les tests passent, pousse donc.

#21 Updated by Josué Kouka about 4 years ago

  • Status changed from En cours to Résolu (à déployer)

#22 Updated by Benjamin Dauvergne almost 4 years ago

  • Status changed from Résolu (à déployer) to Solution déployée

#23 Updated by Benjamin Dauvergne about 2 years ago

  • Status changed from Solution déployée to Fermé

Also available in: Atom PDF