Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

access: usedeposit action addition #2906

Closed
wants to merge 1 commit into from

Conversation

jstypka
Copy link
Contributor

@jstypka jstypka commented Mar 20, 2015

Signed-off-by: Jan Stypka jan.stypka@cern.ch

cc @jmartinm @jalavik

@@ -558,7 +558,8 @@
('claimpaper_change_others_data', 'Change data of any person ID', '', 'no'),
('runbibtasklet', 'run BibTaskLet', '', 'no'),
('cfgbibsched', 'configure BibSched', '', 'no'),
('runinfomanager', 'run Info Space Manager', '', 'no')
('runinfomanager', 'run Info Space Manager', '', 'no'),
('usedeposit', 'use deposit type', 'type', 'yes'),
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@@ -236,6 +246,10 @@ def save(deposition_type=None, uuid=None, draft_id=None):

deposition = Deposition.get(uuid, current_user, type=deposition_type)

dep_type = deposition_type if deposition_type else Deposition.get_type(uuid)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

def deposition_type_from_request():
    """Return deposition from parsed view arguments."""
    return request.view_args.get(
        'deposition_type', None
    ) or (
        Deposition.get_type(request.view_args.get('uuid')) 
        if request.view_args.get('uuid', None) else None
    )

@jirikuncar jirikuncar added this to the v2.1 milestone Mar 20, 2015
@jirikuncar
Copy link
Member

@jstypka can I ask you to first improve PEP8/PEP257 code style of the files you are going to edit? Thank you in advance

@jstypka
Copy link
Contributor Author

jstypka commented Mar 20, 2015

@jirikuncar I've improved style of my code and the files in general. Hope that helps.

@jstypka
Copy link
Contributor Author

jstypka commented Mar 20, 2015

@jirikuncar I can add it, but shouldn't the roles be specific for a particular Invenio instance?
E.g.
('articledeposituser', 'usedeposit', {'type' : 'article'}),
would imply that there should be an article deposition type, which is not always the case.

@jirikuncar
Copy link
Member

Just use this: ('deposituser', 'usedeposit', {}), for Invenio and you can modify it in INSPIRE overlay similarly to https://github.com/inveniosoftware/invenio-demosite/blob/master/invenio_demosite/modules/access/config.py.

@jstypka jstypka force-pushed the rfc-2724 branch 2 times, most recently from 4de138b to e38ac49 Compare March 20, 2015 12:50
@jstypka
Copy link
Contributor Author

jstypka commented Mar 20, 2015

@jirikuncar There you go!

@jirikuncar
Copy link
Member

@jstypka it's shaping well. There is one last not addressed point in #2724 (comment).

@@ -521,8 +553,29 @@ def __unicode__(self):
""" Return a name for this class """
return self.get_identifier()

@classmethod
def all_authorized(cls, current_user):
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

def all_authorized(cls, user_info=None):
    user_info = user_info or current_user
    ...

@jstypka jstypka force-pushed the rfc-2724 branch 3 times, most recently from f8c21b5 to 3c8e030 Compare March 26, 2015 13:20
@jstypka
Copy link
Contributor Author

jstypka commented Mar 26, 2015

@jirikuncar I finally decided to go with your initial suggestion for the dict creation. The problem is that the acc_find_possible_actions_user function returns an empty list for the SUPERADMIN role, which would complicate the situation.

I also made an attempt to implement the cache - please have a look at it, because I'm not sure about some stuff.

for collection, auth in zip(deposit_type_cacher.cache, auths):
if auth[0] == 0:
ret.append(collection)
return ret
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you keep the new line character here(\n) (not new empty line \n\n or none \0). Thanks

@jstypka jstypka force-pushed the rfc-2724 branch 2 times, most recently from 029e9d7 to e881964 Compare March 26, 2015 14:50
@jstypka
Copy link
Contributor Author

jstypka commented Mar 30, 2015

@jirikuncar so how does it look? Any other remarks?

@jirikuncar
Copy link
Member

Can you also mention in commit message that one need to run: webaccessadmin -u admin -c -a -D.

cc @kaplun can you confirm that the command arguments work well? Thanks

@jstypka
Copy link
Contributor Author

jstypka commented Apr 7, 2015

@jirikuncar done, is it good to go?

@jirikuncar
Copy link
Member

@jstypka have you talked with @kaplun about the webaccessadmin? I'm not sure about the arguments. Otherwise it looks ok.

PS: Please keep the PR rebased on top of latest master. Thanks

@kaplun
Copy link
Member

kaplun commented Apr 7, 2015

@jstypka, @jirikuncar, indeed it's correct. We can skip the -D since this is for the demosite and there are no demo authorization added in @jstypka's PR.

@jirikuncar
Copy link
Member

@kaplun thanks! Please consider adding yourself there as reviewer. @jstypka please rebase the PR.

@jirikuncar
Copy link
Member

ping @jstypka (#2906 (comment))

@jstypka
Copy link
Contributor Author

jstypka commented Apr 7, 2015

@jirikuncar after resolving rebase conflicts I'm testing the application again. Please have a look at:
#2906 (comment)
I think there might be a bug in the batch_args usage.

user_info,
'usedeposit',
batch_args=True,
type=deposit_type_cacher.cache
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

type=list(deposit_type_cacher.cache)

@jirikuncar
Copy link
Member

@jstypka please note that there is a failing test

======================================================================
FAIL: test_registration (invenio.modules.deposit.testsuite.test_type_simplerecord.SimpleRecordTest)
----------------------------------------------------------------------
Traceback (most recent call last):
  File "/home/travis/build/inveniosoftware/invenio/invenio/modules/deposit/testsuite/test_type_simplerecord.py", line 102, in test_registration
    assert "Tests" in res.data
AssertionError

----------------------------------------------------------------------

@jirikuncar
Copy link
Member

@jstypka you have also left out (addresses #2724) or (closes #2724) in commit message.

@jstypka jstypka force-pushed the rfc-2724 branch 2 times, most recently from 28ccf4b to 87b26d2 Compare April 8, 2015 15:43
@jstypka
Copy link
Contributor Author

jstypka commented Apr 9, 2015

@jirikuncar I think I pushed all the changes that we discussed. Is it fine?

@jirikuncar
Copy link
Member

@jstypka the tests are still failing 😞

@jirikuncar
Copy link
Member

@jstypka do you need any help with the failing test? Please let us know if you are working on it. Thanks

@jstypka
Copy link
Contributor Author

jstypka commented Apr 10, 2015

@jirikuncar Yes, I am currently trying to figure out why it fails. If I get stuck I'll let you know.

@jirikuncar
Copy link
Member

@jstypka I offered my help so please tell me where is the problem or deliver working solution. Thank you!

@jstypka
Copy link
Contributor Author

jstypka commented Apr 14, 2015

@jirikuncar I sent you an e-mail few days ago. Basically the cache at the beginning seems to be filled properly, however the UserInfo object that is checked during the test seems to be a blank one - all lists empty, all fields False etc.

@jirikuncar
Copy link
Member

@jstypka please use my updated commit from https://github.com/jirikuncar/invenio/tree/2906-fix. Moreover we need to protect also deposit REST API - most importantly https://github.com/inveniosoftware/invenio/blob/master/invenio/modules/deposit/restful.py#L250.

* NEW Adds usedeposit action which enables per user access
  restrictions for different deposit types (closes inveniosoftware#2724).

* Requires running `webaccessadmin -u admin -c -a -D` command.

Signed-off-by: Jan Stypka <jan.stypka@cern.ch>
@jirikuncar
Copy link
Member

Is anybody looking into this? (cc @kaplun @jmartinm @jalavik)

@jirikuncar
Copy link
Member

I have fixed the issue with tests (see https://github.com/jirikuncar/invenio/tree/2906-fix) caused by caches in user info object.

@jstypka add permission checking to REST API.

cc @kaplun @jmartinm @jalavik @tiborsimko

@jirikuncar
Copy link
Member

Superseded by #3172

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

RFC deposit: filter depositions based on user roles
4 participants