search

ohmage / server

The ohmage server application.
37 stars 25 forks source link

survey_response calls ignore admin #860

Closed stevenolen closed 9 years ago

stevenolen commented 9 years ago

In particular, this block of code evaluates that the user is allowed to view the responses, but does not check if the user is an admin (where they should be allowed to view all responses)

https://github.com/ohmage/server/blob/master/src/org/ohmage/service/UserCampaignServices.java#L550-L598

@hongsudt , this is causing a few inconsistencies in @jeroenooms responsetool.

hongsudt commented 9 years ago

@stevenolen https://github.com/stevenolen which API call is this? One of the function? SurveyResponseFunction read request?

https://github.com/stevenolen

On Wed, Jun 10, 2015 at 2:12 PM, Steve Nolen notifications@github.com wrote:

In particular, this block of code evaluates that the user is allowed to view the responses, but does not check if the user is an admin (where they should be allowed to view all responses)

https://github.com/ohmage/server/blob/master/src/org/ohmage/service/UserCampaignServices.java#L550-L598

@hongsudt https://github.com/hongsudt , this is causing a few inconsistencies in @jeroenooms https://github.com/jeroenooms responsetool.

— Reply to this email directly or view it on GitHub https://github.com/ohmage/server/issues/860.

jeroen commented 9 years ago

Yes this SurveyResponseFunction read

hongsudt commented 9 years ago

@jeroenooms I just pushed the code. This should be fixed. Can you confirm?

jeroen commented 9 years ago

I think it works.

jeroen commented 9 years ago

So do I have to start implementing this rule on the client as well? For example how do I know if a response is editable? Is the rule still if the user is a supervisor in the campaign or the current user? Or do I have to check for admin as well?

hongsudt commented 9 years ago

You have to check for the admin as well..

On Thu, Jun 11, 2015 at 1:12 PM, Jeroen Ooms notifications@github.com wrote:

So do I have to start implementing this rule on the client as well? For example how do I know if a response is editable? Is the rule still if the user is a supervisor in the campaign or the current user? Or do I have to check for admin as well?

— Reply to this email directly or view it on GitHub https://github.com/ohmage/server/issues/860#issuecomment-111263194.

jeroen commented 9 years ago

How do i know if a user has admin?

stevenolen commented 9 years ago

user_info/read returns this

jeroen commented 9 years ago

Can we maybe include if the user is admin as a user_role in /campaign/read? Because apparently it effectively changes the permission that the user has over the campaign?

stevenolen commented 9 years ago

being an admin silently changes the data from a number of calls, though. I don't know if I agree that the user_role return should be different for an admin, since their user role isn't changing...they can still be of a particular set of roles in a campaign...they just get access to a superset of information because of their admin role..

hongsudt commented 9 years ago

I agree with steve. I am not sure whether it is appropriate to overload the user_campaign_role with admin.

On Thu, Jun 11, 2015 at 1:51 PM, Steve Nolen notifications@github.com wrote:

being an admin silently changes the data from a number of calls, though. I don't know if I agree that the user_role return should be different for an admin, since their user role isn't changing...they can still be of a particular set of roles in a campaign...they just get access to a superset of information because of their admin role..

— Reply to this email directly or view it on GitHub https://github.com/ohmage/server/issues/860#issuecomment-111274400.

jeroen commented 9 years ago

It's just tricky to puzzle together the ACL rules on the client side from the different api calls. I'm trying to find a way to make the server let the client know which permissions the user has.

hongsudt commented 9 years ago

I don't see an easy way to do this unless the server explicitly assign a state associated with each response. This will require a lot of change and i am not sure whether it can preserve backward compatibility.

On Thu, Jun 11, 2015 at 1:54 PM, Jeroen Ooms notifications@github.com wrote:

It's just tricky to puzzle together the ACL rules on the client side from the different api calls. I'm trying to find a way to make the server let the client know which permissions the user has.

— Reply to this email directly or view it on GitHub https://github.com/ohmage/server/issues/860#issuecomment-111274966.

jeroen commented 9 years ago

Its not just responses, it affects other things as well. For example we have a rule that a user is allowed to update the campaign (edit surveys) if the user is either author or supervisor. But I guess I have to check for admin there as well now?