✨(maildomain_access) add API endpoint to search users #561
Conversation
sdemagny
force-pushed
the
sdem/search_user_domain
branch
from
75b44d1 to
23eb00b
Compare
sdemagny
changed the title
sdemagny
changed the title
sdemagny
changed the title
sdemagny
force-pushed
the
sdem/search_user_domain
branch
2 times, most recently
from
68ae501 to
10e7537
Compare
|
Dans les get_abilities il y a manage_access qui me permettrait de limiter l'accès à ce endpoint. |
| @action(detail=False, url_path="users", methods=["get"]) | ||
| def get_available_users(self, request, domain_slug): | ||
| """API endpoint to search user to give them new access. | ||
| More filters and permission will be added soon. |
There was a problem hiding this comment.
This is a bit of a nitpick, but I feel that docstrings should be focused on the current state of the code, it's not really their job to say what is "coming soon".
| core_models.User.objects.all() | ||
| .order_by("-created_at") | ||
| # exclude inactive contacts | ||
| .filter(is_active=True) |
There was a problem hiding this comment.
This filter is not exercised by the test, I think ?
| More filters and permission will be added soon. | ||
| """ | ||
| queryset = ( | ||
| core_models.User.objects.all() |
There was a problem hiding this comment.
A note here that this raises a similar problem to issue #35 - if I'm authenticated it lets me see (effectively) all users.
From an API design perspective it's also a bit weird because this is an endpoint where we specify a domain, in order to see all users who do not have access to that domain (L199). It makes L79 quite misleading.
It would perhaps be more coherent to have an "exclude_by_domain_access" parameter on the /users/ endpoint ?
There was a problem hiding this comment.
(On the /users/ endpoint we already have a parameter - undocumented - to exclude users already on a team… I think for similar reasons; for consistency and for better clarity they should be named something like exclude_xxx)
sdemagny
force-pushed
the
sdem/search_user_domain
branch
3 times, most recently
from
3ffe70c to
c8b5673
Compare
sdemagny
force-pushed
the
sdem/search_user_domain
branch
3 times, most recently
from
74a54ae to
dac3cd3
Compare
| abilities = domain.get_abilities(request.user) | ||
| if not abilities["manage_accesses"]: | ||
| raise exceptions.PermissionDenied() |
There was a problem hiding this comment.
Should you consider adding a get_available_users to the abilities?
There was a problem hiding this comment.
User needs to list available users to manage access, so adding get_available_users to abilities would be a duplicate.
| if role == enums.MailDomainRoleChoices.VIEWER: | ||
| assert response.status_code == status.HTTP_403_FORBIDDEN | ||
| else: |
There was a problem hiding this comment.
I'm sorry, but this is a bad pattern in tests, there should not be logic like that here :/
| if role == enums.MailDomainRoleChoices.VIEWER: | ||
| assert response.status_code == status.HTTP_403_FORBIDDEN | ||
| else: |
There was a problem hiding this comment.
Same here, we don't want logic in tests
sdemagny
force-pushed
the
sdem/search_user_domain
branch
from
dac3cd3 to
f2900ef
Compare
Add new API endpoint to search for new users to whom we can assign new roles.
sdemagny
force-pushed
the
sdem/search_user_domain
branch
from
f2900ef to
5d84e22
Compare
closes #508