contrib: CERN<->group linking #7
Conversation
| db.session.add(user) | ||
| current_user.reload() | ||
| # update user object | ||
| if user: |
There was a problem hiding this comment.
is it possible that we don't get a user from https://github.com/inveniosoftware/invenio-oauthclient/pull/7/files#diff-b49dd27458efb60f2de74978e6d3eecfR211?
If the user is empty we should probably better logout the user. WDYT?
There was a problem hiding this comment.
CC @ludmilamarian what do you think?
There was a problem hiding this comment.
I'm thinking that the cern client (as well as the other clients) should only be a thin layer that handles specific client data, and all the logic and making sure that the user/token/etc. are well created and made available to the application should be taken care of in the oauthclient handlers - which I think is already the case. account_setup function should not even be called if anything is wrong with creating/fetching the user.
| db.session.add(candidate) | ||
| modified = True | ||
| except ImportError: | ||
| # no invenio-groups installed |
There was a problem hiding this comment.
Can you register a warning in this case, so that it's clear for the admin why groups are not updated?
I'm not sure how we will use the invenio-groups module - I don't think we are using the groups now, but it's good to have it in the logs if anything is expected, but it's not installed.
One more thing: how do you remove people from groups in this case?
There was a problem hiding this comment.
@ludmilamarian was there any configuration option before for synchronizing Invenio groups with CERN LDAP groups?
There was a problem hiding this comment.
This is what I was discussing with @crepererum IRL just now - we are not using Invenio groups as far as I know, but we are relying on the groups that we receive each time via SSO and are stored in the user_info / current_user object. So we don't really need any synchro.
crepererum
force-pushed
the
feature/cern_groups
branch
from
d67a47c
to
59115a5
Compare
* BETTER Links user to existing managed groups if invenio-groups is installed. Signed-off-by: Marco Neumann <marco@crepererum.net>
crepererum
force-pushed
the
feature/cern_groups
branch
from
59115a5
to
256a008
Compare
| modified = False | ||
|
|
||
| # try to add/remove groups | ||
| try: |
There was a problem hiding this comment.
Wouldn't it be better to complement this via a signal?
There was a problem hiding this comment.
It is there to decide if we do a current_user.reload(). Ideally, our invenio.ext.login.legacy_user module should know do this on its own.
|
Since #19 has been merged, one can use new signals to update groups. |
Signed-off-by: Marco Neumann marco@crepererum.net