permissions: not working on not logged users #56
Conversation
|
|
||
| assert not permission_open.allows(identity_unknown) | ||
| assert not permission_read.allows(identity_unknown) | ||
| assert permission_not_logged.allows(identity_unknown) |
There was a problem hiding this comment.
Here, we are in the case of rule 1 in the description of this PR. So, anybody, including not logged users, should access to this action.
remileduc
changed the title
jirikuncar
changed the title
|
Is there a command to rerun Travis ? |
|
You should be able to re-run the travis build since you have write access. Or simply close/open the PR. However, this branch has conflicts that must be resolved. |
|
the tests will never pass on this branch as it is its purpose: to show a problem in the current invenio-access module. See it as a test case. This PR doesn't provide any solution to fix the issue. I can rebase though, I'll do it when I'll have a bit of time |
|
I've updated to the last version, it still crashes at the same point ;) |
* Uses argument instead of option for required parameters. (closes #56) Signed-off-by: Leonardo Rossi <leonardo.r@cern.ch>
|
New version, fixes the main problem with no users:
|
| @@ -61,7 +61,7 @@ def upgrade(): | |||
| sa.Column('exclude', sa.Boolean(name='exclude'), server_default='0', | |||
| nullable=False), | |||
| sa.Column('argument', sa.String(length=255), nullable=True), | |||
| sa.Column('user_id', sa.Integer(), nullable=True), | |||
| sa.Column('user_id', sa.Integer(), nullable=False), | |||
There was a problem hiding this comment.
We will need an alembic recipe for this change
There was a problem hiding this comment.
What would this recipe do? only change the "NOT NULL" or try to do some other smart things?
There was a problem hiding this comment.
only put it to "NOT NULL" so we have a predictable behavior. If set to NULL, we can have some issues, as described in the decsription
There was a problem hiding this comment.
ok no problem for B2SHARE then.
| @@ -119,14 +119,6 @@ def allow_action(action, argument): | |||
| """Allow action.""" | |||
|
|
|||
|
|
|||
| @allow_action.command('any') | |||
| def allow_any(): | |||
There was a problem hiding this comment.
@nharraud Will removal of these impact you on B2SHARE?
There was a problem hiding this comment.
I don't think so. We always assign permissions to specific users or have a permission factory which allow/forbids everybody.
remileduc
force-pushed
the
master
branch
2 times, most recently
from
6515412
to
4193ea0
Compare
- remove the possibility to create an ActionUsers without user doesn't make sens... We may have to implement special cases in the future like 'anyuser'...
- use the travis user instead of root to make the tests pass (maybe something has changed since travis is using Trusty as default distrib) - use precise distrib
I've added several tests for invenio-access, particularly for actions with arguments, and for not logged users.
Globally, we should have the following:
parameterized actions
Imagine you have 2 users:
user1anduser2, and 2 actions:action1andaction2.You have the following actions users table:
action1and all its children (action1 with any parameters) will be accessible by anybody because of the first rule.user1can access to theaction1, but it is useless because of the rule 1.user1can access toaction2.user2can access toaction2withp1parameter.To summarize,
user1can access everything.user2can onlyaction1andaction1with any parameters. He can also access toaction2but only with parameterp1Finally, a not-logged user will be able to access
action1, but that's all.The problem I actually have is the overriden rules explained in rules 1 and 2. Actually the test fails at line 131 (the only assert that fails), while this should pass.
See the comments for more details.