adlnet / ADL_LRS

ADL's Open Source Learning Record Store (LRS) is used to store learning data collected with the Experience API.
https://lrs.adlnet.gov
Apache License 2.0
309 stars 147 forks source link

Activity ID being constrained to only one user ? #285

Closed varunasingh closed 10 years ago

varunasingh commented 10 years ago

This might be an issue/doubt over some fundamentals. As far as I know, multiple users should be able to send statements and they should be able to paricipate in the same activity. Now, we know an Activity has to have its own unique identifier (Activity ID). In the statement is is the Object->id. Now, the way I see ADL_LRS Django project does activity is it stores every activity in the lrs_activity model table. Precisely in req_validate.py and method: server_validate_statement_object we have a check if the statement that is coming in is checked and its activity id is checked against the user who is sending it.

Specifically this code:

if activity.authoritative != '' and activity.authoritative != auth_name: err_msg = "This ActivityID already exists, and you do not have the correct authority to create or update it."

suggests that no two users can share or even do the same activity. I did test this by making two users in the ADL LRS and sent a statement. The second user to send the statement failed as the above error in code appeared.

Now, there are two assumptions that Im making which I think are correct.

  1. Every user in the ADL LRS represents a real world user and it is usuing this user credentials, that the user can send statements.
  2. Every user should be able to do the same activity. for example: Chapter 1 can be done by Bob and Mary.
  3. The user is authenticated on the http request side. When bob and mary's devices make statements to /xapi/statments/ their user is authorised by the basic authentication method that works in ADL_LRS Django.

I'd like to get some clarity on why this happens so. Even if this is supposed to happen, How are two distinct users supposed to a. authenticate themselves and b. be stored as users in the LRS.

ljwolford commented 10 years ago

A few lines above the line you are referencing it checks to see if the activity has a definition, if so then it grabs the canonical version of the activity being referenced by the ID. The system then checks who has the authority to change any activity information (i.e name or description which are in the activity definition) and if the user does not have permission, it throws the exception. Other users who do not have the authority to change can still use that particular activity, the information about it is just controlled by the activity creator.

varunasingh commented 10 years ago

This is how I am testing it.

  1. Clone the LRS project and run it (from the steps)
  2. On the browser if i go to /xapi/ I create two users: a. daftpunk b. blacksabbath
  3. I do a POST request to /xapi/statements with Basic authentication for 'daftpunk', Give the XAPI header: X-Experience-API-Version 1.0.1 Statement insertion: Success with return of Statement ID
  4. I do a POST request to /xapi/statements with Basic authentication blacksabbath/password and insert a similar statement (with different ID): Statement insertion failure.

Statement #1 to daftpunk: { "id": "26b8b80f-8a8f-4ab8-a8ef-ada2bebb2788", "timestamp": "2014-06-18T10:50:34.512Z", "actor": { "objectType": "Agent", "mbox": "mailto:blacksabbath@testdomain.com", "name": "Black Sabbath" }, "verb": { "id": "http://adlnet.gov/expapi/verbs/answered", "display": { "en-US": "answered" } }, "result": { "success": true, "response": "0_116" }, "object": { "id": "http://www.testdomain.com/tincan//0_110", "objectType": "Activity", "definition": { "type": "http://adlnet.gov/expapi/activities/cmi.interaction", "name": { "en-US": "What is 2+2?" }, "interactionType": "choice", "correctResponsesPattern": ["ans0_116"], "choices": [{ "id": "ans0_113", "description": { "en-US": "7" } }, { "id": "ans0_116", "description": { "en-US": "4" } }] } } } Return: 200 All OK Statement #2 to blacksabbath: { "id": "26b8b80f-8a8f-4ab8-a8ef-ada2bebb2777", "timestamp": "2014-06-18T10:50:34.512Z", "actor": { "objectType": "Agent", "mbox": "mailto:blacksabbath@testdomain.com", "name": "Black Sabbath" }, "verb": { "id": "http://adlnet.gov/expapi/verbs/answered", "display": { "en-US": "answered" } }, "result": { "success": true, "response": "0_116" }, "object": { "id": "http://www.testdomain.com/tincan//0_110", "objectType": "Activity", "definition": { "type": "http://adlnet.gov/expapi/activities/cmi.interaction", "name": { "en-US": "What is 2+2?" }, "interactionType": "choice", "correctResponsesPattern": ["ans0_116"], "choices": [{ "id": "ans0_113", "description": { "en-US": "7" } }, { "id": "ans0_116", "description": { "en-US": "4" } }] } } } Return: 403 FORBIDDEN This ActivityID already exists, and you do not have the correct authority to create or update it.

Is there something wrong in the statements' Object -> id ?

jmizgajski commented 10 years ago

have you solved it or at least confirmed it? this is a major bug if confirmed, a test for this exact scenario should be added, maybe @varunasingh would like to contribute it via PR?

ljwolford commented 10 years ago

please check out the closed PR #286 ....looking back it was a bug that we had handled on our production server but didn't pull the code into here. i added some tests similar to the ones above. the pull request also has some changes to the settings and setup so please read the readme before starting again. thank you for the bug find and if there is still something wrong please let us know.

varunasingh commented 10 years ago

@jmizgajski To my full knowledge, I have confirmed this. @ljwolford Is the pull going to merge with master ?

ljwolford commented 10 years ago

@varunasingh - the updated code is in master. please try it now and confirm it fixed the issue

varunasingh commented 10 years ago

Hi @ljwolford and @jmizgajski I got my hand on a fresh ubuntu server and I tried to replicate the server setup from the top. I then did the same steps in the accounts and submited the post json as mentioned in my test scenario above.

I see the change, however I get this error after posting the secod JSON: Enter valid JSON

I tried to debugg this and this is how the flow is going: First JSON in POST request body to /xapi/statements: -> lrs/views.py:statements(request) ->lrs/views/py:handle_request(request) ->req_validate.py:statements_post(req_dict): ->req_validate.py:server_validate_statement_object(stmt_object, auth) ->req_validate.py:server_validate_statement_object(): in the try clause: activity will not be present so there will be an exception and it will pass. However the server responds with just "" instead of saying "statement successfully put with id: "blah" as it used to. When i check the tables, the statements are put in successfully.

Seconf JSON in POST request body to /xapi/statements: -> lrs/views.py:statements(request) ->lrs/views/py:handle_request(request) ->req_validate.py:statements_post(req_dict): ->req_validate.py:server_validate_statement_object(stmt_object, auth) ->req_validate.py:server_validate_statement_object(): in the try clause: activity check FAILS but not because it does not exists, but for some other unknown reason. and it doesn't pass but just excepts to invlaid JSON elsewhere. I have no idea why it is getting stuck here.

Can any one of you replicate this ? Thanks.

varunasingh commented 10 years ago

Hi @ljwolford and @jmizgajski I figured out the root problem and applied a solution to this as you notice in my commit above. I forked the latest and made those changes. So the problem was that in models there wasn't a default defined for JSONFields and when we are calling self.save_activity_definition_to_db() within populate() in Activity Manager, we are setting default values to be : '' < This means that the corresponding JSONFields will have two double quote as stored in as field values: ""
eg: 2 | http://www.testdomain.com/tincan/Lesson001-1/random | Activity | {"en-US":"Motivational"} | {"en-US":"Motivational"} | http://adlnet.gov/expapi/activities/module | | | "" | "" | "" | "" | "" | "" | "" | testuser | t

When we insert another statement (or try to) with the same activity ID the code would try to get values or check with the Activity model, Django and python will throw an Exception: Invalid JSON. This exception is because the JSON in the Activity table is invalid ( the exception logs dont clearly state that, I had to digg in and debug the code). Hence, I got the error message.

So with the new solution applied which basically defaults empty fields to: {} and also at the model level: eg: jsonfield=JSONField(default={}, blank=true) the new table row has {} instead of the two double quotes and Django and python is happy that these mini JSONs are valid.

I also noticed another bug when if the statement gets successfullly inserted to the table they are stored in full_statement in the statement table but they can get stored as (for example): {/"type/": /"http://adlnet.gov/expapi/activities/module/", /"name/": {/"en-US'/" /"Intro3/"}... and so on.

Instead of returning the statements as is, I've serialised it:

in lrs/models.py of class Statement(models.Model): def object_return(self, lang=None, format='exact'): if format == 'exact':

returning JSON dump to avoid /" hurting in full_statement in Statement

        return json.dumps(self.full_statement)

instead of : return self.full_statement

This allowed me to see the full statement as it is when I clicked on the statement that we see in: xAPI/me/

Let me know what you guys thing and if this seems a suitable fix then feel free to pull from my fork.

jmizgajski commented 10 years ago

gj:) !

2014-08-24 17:23 GMT+02:00 varunasingh notifications@github.com:

Hi @ljwolford https://github.com/ljwolford and @jmizgajski https://github.com/jmizgajski I figured out the root problem and applied a solution to this as you notice in my commit above. I forked the latest and made those changes. So the problem was that in models there wasn't a default defined for JSONFields and when we are calling self.save_activity_definition_to_db() within populate() in Activity Manager, we are setting default values to be : '' < This means that the corresponding JSONFields will have two double quote as stored in as field values: ""

eg: 2 | http://www.testdomain.com/tincan/Lesson001-1/random | Activity | {"en-US":"Motivational"} | {"en-US":"Motivational"} | http://adlnet.gov/expapi/activities/module | | | "" | "" | "" | "" | "" | "" | "" | testuser | t

When we insert another statement (or try to) with the same activity ID the code would try to get values or check with the Activity model, Django and python will throw an Exception: Invalid JSON. This exception is because the JSON in the Activity table is invalid ( the exception logs dont clearly state that, I had to digg in and debug the code). Hence, I got the error message.

So with the new solution applied which basically defaults empty fields to: {} and also at the model level: eg: jsonfield=JSONField(default={}, blank=true) the new table row has {} instead of the two double quotes and Django and python is happy that these mini JSONs are valid.

I also noticed another bug when if the statement gets successfullly inserted to the table they are stored in full_statement in the statement table but they can get stored as (for example): {/"type/": /"http://adlnet.gov/expapi/activities/module/", /"name/": {/"en-US'/" /"Intro3/"}... and so on.

Instead of returning the statements as is, I've serialised it:

in lrs/models.py of class Statement(models.Model): def object_return(self, lang=None, format='exact'): if format == 'exact':

returning JSON dump to avoid /" hurting in full_statement in Statement

return json.dumps(self.full_statement)

instead of : return self.full_statement

This allowed me to see the full statement as it is when I clicked on the statement that we see in: xAPI/me/

Let me know what you guys thing and if this seems a suitable fix then feel free to pull from my fork.

— Reply to this email directly or view it on GitHub https://github.com/adlnet/ADL_LRS/issues/285#issuecomment-53196855.

varunasingh commented 10 years ago

Thank you. Let me know when this is in master so I can test it again.