Voting for resources should be limited to one per user

[aaron-junot]

Currently, you can upvote or downvote a resource over and over again. It should be limited to one vote per user. If the user has upvoted a resource and then downvotes the same resource, the upvote should clear out and be replaced by the downvote (and vice versa).

For the moment, users can be tracked by their API credentials, so all of this should be determined by the API key provided.

Acceptance criteria:

• The /upvote and /downvote endpoints should need authentication
• Votes should be restricted to one per API key
• Votes from a certain user should be able to be cleared out by sending an additional request to either of the vote endpoints. If it is the same vote endpoint, it should simply be cleared out. If it is the opposite vote endpoint, it should clear out the existing vote and update the vote.

[the-bose]

Hello, I would like to contribute to this

[aaron-junot]

@the-bose You kind of overshot Hacktoberfest. Do you still want to contribute to this? As you can see, it's a high priority and we'd like to get going on it

[chynh]

Hello,

I was working on this and stumbled upon an issue when modifying the test suite to reflect the changes that I made. As you can see, I modified so that it would take in API credentials.

tests/unit/test_routes.py

def test_update_votes(module_client, module_db, fake_algolia_save):
    client = module_client
    vote_direction = 'upvote'
    id = 1
    apikey = get_api_key(client)

    # Check voting on a valid resource
    data = client.get(f"api/v1/resources/{id}").json['data']
    response = client.put(
        f"/api/v1/resources/{id}/{vote_direction}",
        follow_redirects=True,
        headers={'x-apikey': apikey}
    )

    initial_votes = data.get(f"{vote_direction}s")

    assert (response.status_code == 200)
    assert (response.json['data'].get(f"{vote_direction}s") == initial_votes + 1)

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

def get_api_key(client):
    response = client.post('api/v1/apikey', json=dict(
        email="test@example.org",
        password="supersecurepassword"
    ))

    return response.json['data'].get('apikey')

Basically, test_update_votes would fail with the following error message:

tests/unit/test_routes.py:352: 
_ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _

client = <FlaskClient <Flask 'app'>>

    def get_api_key(client):
        response = client.post('api/v1/apikey', json=dict(
            email="test@example.org",
            password="supersecurepassword"
        ))
    
>       return response.json['data'].get('apikey')
E       AttributeError: 'NoneType' object has no attribute 'get'

tests/unit/test_routes.py:1407: AttributeError

However, it would pass if I modify get_api_key to accept my actual login credentials.

I am not sure how to debug it other than having two different get_api_key functions where one takes the test credentials and the other takes actual credentials.

[aaron-junot]

@chynh You need to mock the API call to the backend by passing in fake_auth_from_oc to the test function. Should look like this:

def test_update_votes(module_client, module_db, fake_algolia_save, fake_auth_from_oc):

That might fail linting for being too long of a line, but other than that it should solve your problem

[chynh]

Thanks for the pointer!

Now test_update_votes is passing but test_apikey_commit_error is failing for some reason after the change...

______________________________________________________________________________________________________ test_apikey_commit_error _______________________________________________________________________________________________________

module_client = <FlaskClient <Flask 'app'>>, module_db = <SQLAlchemy engine=sqlite:///:memory:>, fake_auth_from_oc = None, fake_commit_error = None

    def test_apikey_commit_error(
            module_client, module_db, fake_auth_from_oc, fake_commit_error):
        client = module_client
    
        response = client.post('api/v1/apikey', json=dict(
            email="test@example.org",
            password="supersecurepassword"
        ))
    
>       assert (response.status_code == 500)
E       assert 200 == 500
E        +  where 200 = <Response streamed [200 OK]>.status_code

tests/unit/test_routes.py:462: AssertionError

[aaron-junot]

What all did you change? Without knowing the full diff it's really hard to help you troubleshoot.

Might be worth pushing up your branch and then linking to it so I can see the diff on github

[chynh]

Here is the full diff:

master...chynh:limit-voting

[aaron-junot]

@chynh I pulled down your branch and I saw the same problem with test_apikey_commit_error. I think it would be easier to troubleshoot once we merge #266 so why don't we come back to this after that.

And just a note, your current implementation publically leaks everyone's API key that has voted on a given resource. I like the idea of a many-to-many relation of Users to Resources where each user can have one vote. Maybe we could make Vote it's own model. Definitely something to think about. But having a JSON string of everyone's API key and their vote direction (especially publicly viewable) is not something I'm going to approve.

[chynh]

@aaron-suarez Got it. I'll wait until we merge #266

I realize that I definitely have overlooked the security aspect of my implementation. I'll go ahead and modify it so that we no longer expose users' API keys.