inveniosoftware / invenio-accounts-rest Goto Github PK

@nharraud commented on Fri Sep 02 2016

As for the other modules a REST API will be needed here.

The main features would be:

• searching/listing users
• getting a user's information
• updating a user's information
• searching roles
• creating roles
• assigning roles to users or unassigning them.

It would also need a shortcut URI for the currently authenticated user.

There is however one question. Do we want this in invenio-accounts or do we create an API for both invenio-accounts and invenio-userprofiles at the same time? Not being able to search users by name would be a strange limitation.

@inveniosoftware/triagers

@nharraud commented on Fri Sep 02 2016

Needed for #137

@lnielsen commented on Fri Sep 02 2016

would need careful though about oauth2 scopes as well

@nharraud commented on Mon Sep 05 2016

Regarding B2Share, we will need a search/suggest of users and roles based on their names. I see at least one use case where this would be useful: admin interface.

This would imply that we need to have a combined REST API for invenio-accounts and invenio-userprofile.
The cons of this approach is that other people might want to use a custom profile module.

@lnielsen commented on Mon Sep 05 2016

It's useful to have an endpoint for the current authenticated user as well similar to GitHub:
https://developer.github.com/v3/users/#get-the-authenticated-user.

Also we might be forward looking an allow support for multiple email addresses:
https://developer.github.com/v3/users/emails/

Regarding Accounts/UserProfiles:
This might be taken care of by serialisers and loaders, since usually the profile can be accessed from the user model. Also we can launch an Invenio-Accounts-REST which depends on both accounts and user profiles.

@PaulinaLach commented on Mon Sep 05 2016

Here is my suggestion of list of the endpoints:

• List all users - GET /users
• Search user - GET /users?q=sth
• Get info about the user - GET /users/<id>
• Update user’s info - PATCH /users/<id>
• Create user - POST /users
• Search roles - GET /roles?q=sth
• Create role - POST /roles
• Delete role - DELETE /roles/<role_id>
• Assign role - PUT /users/<user_id>/roles/<role_id>
• Divest role - DELETE /users/<user_id>/roles/<role_id>

Is it ok for everyone? Or do you have any suggestions?

@jirikuncar commented on Mon Sep 05 2016

It's needed also for Invenio-Circulation (inveniosoftware/invenio-circulation#38 cc @mvesper).

Can we create Invenio-Accounts-REST?

Problem:
The serializer status_code_serializer returns a JSON response containing the status code. It is used in multiple place in views.py with a 204 HTTP code. This code says that there is no returned content, thus no JSON data should be sent.

Problem:
The permission factories for assigning and unassigning roles are given one tuple containing both user who will have the role assigned to and the role itself. In order to be consistent with other Invenio REST APIs the parameters should be given as separate keywords.

The following cookiecutter change:

inveniosoftware/cookiecutter-invenio-module#98

should be propagated to this Invenio module.

Namely, in run-tests.sh, the sphinx for doctests is invoked after pytest run:

$ tail -3 ./\{\{\ cookiecutter.project_shortname\ \}\}/run-tests.sh
sphinx-build -qnNW docs docs/_build/html && python setup.py test && sphinx-build -qnNW -b doctest docs docs/_build/doctest

This sometimes led to problems on Travis CI with the second sphinx-build run due
to "disappearing" dependencies after the example application was tested.

A solution that worked for invenio-marc21 (see
inveniosoftware/invenio-marc21#49 (comment))
and that was integrated in cookiecutter (see
inveniosoftware/cookiecutter-invenio-module#98) was to
run doctest execution in pytest, removing the second sphinx-build invocation.

This both solved Travis CI build failures and simplified test suite execution.

Note that this change may necessitate to amend the code tests etc so that things
would be executed with the Flask application context (see
inveniosoftware/invenio-marc21@09e98fc).

Problem:
ACCOUNTS_REST_ACCOUNT_SERIALIZERS is used for both UserListResource and UserAccountResource . Because of that it is not possible to override the serializers as the one provided would be used for both accounts search which returns a list of result, and single account requests.

Problem:
db.session.commit() is not called after roles are assigned or unassigned. The tests don't detect this bug as the check is ran in the same application context.

Module does not seem to need I18N hence Babel related code should be removed.

Problem:
role name can be patched but not the description.

The library jsonpatch is only included in the tests, not in the installation requirements. In some cases, it will be installed by other modules that has it as a dependency. However, as is my case of a "UI" only app (No records backend, since I connect to the API via the react-searchkit) it fails due to this missing dependency.

Problem:
View's functions checking permissions are calling with current_app.app_context() unnecessary. This reinitializes flask.g used for permissions.

Problem:
There is currently no configuration variable allowing overlays to override the roles serializers.

Problem:
The views RolesListResource and RoleResource do not use any loader. Thus:

• there is no check that the input is valid.
• it is not possible for overlays to override the loading.