diff --git a/swh/web/api/views/origin_save.py b/swh/web/api/views/origin_save.py --- a/swh/web/api/views/origin_save.py +++ b/swh/web/api/views/origin_save.py @@ -87,7 +87,9 @@ bypass_pending_review = request.user.is_authenticated and request.user.has_perm( SWH_AMBASSADOR_PERMISSION ) - sor = create_save_origin_request(visit_type, origin_url, bypass_pending_review) + sor = create_save_origin_request( + visit_type, origin_url, bypass_pending_review, user_id=request.user.id + ) del sor["id"] else: sor = get_save_origin_requests(visit_type, origin_url) diff --git a/swh/web/common/migrations/0010_saveoriginrequest_user_id.py b/swh/web/common/migrations/0010_saveoriginrequest_user_id.py new file mode 100644 --- /dev/null +++ b/swh/web/common/migrations/0010_saveoriginrequest_user_id.py @@ -0,0 +1,22 @@ +# Copyright (C) 2021 The Software Heritage developers +# See the AUTHORS file at the top-level directory of this distribution +# License: GNU General Public License version 3, or any later version +# See top-level LICENSE file for more information +# Generated by Django 2.2.20 on 2021-05-03 14:16 + +from django.db import migrations, models + + +class Migration(migrations.Migration): + + dependencies = [ + ("swh_web_common", "0009_saveoriginrequest_visit_status"), + ] + + operations = [ + migrations.AddField( + model_name="saveoriginrequest", + name="user_id", + field=models.CharField(max_length=200, null=True), + ), + ] diff --git a/swh/web/common/models.py b/swh/web/common/models.py --- a/swh/web/common/models.py +++ b/swh/web/common/models.py @@ -100,6 +100,9 @@ loading_task_status = models.TextField( choices=SAVE_TASK_STATUS, default=SAVE_TASK_NOT_CREATED ) + # user integer ids computed from keycloak subs are too large + # to be stored in SQLite so we store them as strings + user_id = models.CharField(max_length=200, null=True) class Meta: app_label = "swh_web_common" diff --git a/swh/web/common/origin_save.py b/swh/web/common/origin_save.py --- a/swh/web/common/origin_save.py +++ b/swh/web/common/origin_save.py @@ -338,7 +338,10 @@ def create_save_origin_request( - visit_type: str, origin_url: str, bypass_pending_review: bool = False + visit_type: str, + origin_url: str, + bypass_pending_review: bool = False, + user_id: Optional[int] = None, ) -> SaveOriginRequestInfo: """ Create a loading task to save a software origin into the archive. @@ -445,6 +448,7 @@ origin_url=origin_url, status=save_request_status, loading_task_id=task["id"], + user_id=str(user_id) if user_id else None, ) # save request must be manually reviewed for acceptation elif save_request_status == SAVE_REQUEST_PENDING: @@ -457,13 +461,19 @@ # if not add it to the database except ObjectDoesNotExist: sor = SaveOriginRequest.objects.create( - visit_type=visit_type, origin_url=origin_url, status=save_request_status + visit_type=visit_type, + origin_url=origin_url, + status=save_request_status, + user_id=str(user_id) if user_id else None, ) # origin can not be saved as its url is blacklisted, # log the request to the database anyway else: sor = SaveOriginRequest.objects.create( - visit_type=visit_type, origin_url=origin_url, status=save_request_status + visit_type=visit_type, + origin_url=origin_url, + status=save_request_status, + user_id=str(user_id) if user_id else None, ) if save_request_status == SAVE_REQUEST_REJECTED: diff --git a/swh/web/tests/api/views/test_origin_save.py b/swh/web/tests/api/views/test_origin_save.py --- a/swh/web/tests/api/views/test_origin_save.py +++ b/swh/web/tests/api/views/test_origin_save.py @@ -443,7 +443,7 @@ def test_create_save_request_pending_review_anonymous_user( - api_client, origin_to_review, requests_mock + api_client, origin_to_review ): url = reverse( @@ -460,10 +460,10 @@ def test_create_save_request_accepted_ambassador_user( - api_client, origin_to_review, requests_mock, keycloak_oidc, mocker + api_client, origin_to_review, keycloak_oidc, mocker ): - keycloak_oidc.realm_permissions += [SWH_AMBASSADOR_PERMISSION] + keycloak_oidc.realm_permissions = [SWH_AMBASSADOR_PERMISSION] oidc_profile = keycloak_oidc.login() api_client.credentials(HTTP_AUTHORIZATION=f"Bearer {oidc_profile['refresh_token']}") @@ -476,3 +476,36 @@ ) assert SaveAuthorizedOrigin.objects.get(url=origin_to_review) + + +def test_create_save_request_anonymous_user_no_user_id(api_client): + origin_url = "https://some.git.hosters/user/repo" + url = reverse( + "api-1-save-origin", url_args={"visit_type": "git", "origin_url": origin_url}, + ) + + check_api_post_responses(api_client, url, status_code=200) + + sor = SaveOriginRequest.objects.get(origin_url=origin_url) + + assert sor.user_id is None + + +def test_create_save_request_authenticated_user_id( + api_client, origin_to_review, keycloak_oidc, mocker +): + oidc_profile = keycloak_oidc.login() + api_client.credentials(HTTP_AUTHORIZATION=f"Bearer {oidc_profile['refresh_token']}") + + origin_url = "https://some.git.hosters/user/repo2" + url = reverse( + "api-1-save-origin", url_args={"visit_type": "git", "origin_url": origin_url}, + ) + + response = check_api_post_response(api_client, url, status_code=200) + + assert response.wsgi_request.user.id is not None + + user_id = str(response.wsgi_request.user.id) + sor = SaveOriginRequest.objects.get(user_id=user_id) + assert sor.user_id == user_id diff --git a/swh/web/tests/conftest.py b/swh/web/tests/conftest.py --- a/swh/web/tests/conftest.py +++ b/swh/web/tests/conftest.py @@ -123,13 +123,13 @@ # Fixture to get test client from Django REST Framework -@pytest.fixture(scope="module") +@pytest.fixture def api_client(): return APIClient() # Fixture to get API request factory from Django REST Framework -@pytest.fixture(scope="module") +@pytest.fixture def api_request_factory(): return APIRequestFactory() diff --git a/swh/web/tests/test_migrations.py b/swh/web/tests/test_migrations.py --- a/swh/web/tests/test_migrations.py +++ b/swh/web/tests/test_migrations.py @@ -7,17 +7,32 @@ MIGRATION_0008 = "0008_save-code-now_indexes_20210106_1327" MIGRATION_0009 = "0009_saveoriginrequest_visit_status" +MIGRATION_0010 = "0010_saveoriginrequest_user_id" def test_migrations_09_add_visit_status_to_sor_model(migrator): """Ensures the migration adds the visit_status field to SaveOriginRequest table""" old_state = migrator.apply_initial_migration((APP_NAME, MIGRATION_0008),) - old_webapp = old_state.apps.get_model(APP_NAME, "SaveOriginRequest") + old_model = old_state.apps.get_model(APP_NAME, "SaveOriginRequest") - assert hasattr(old_webapp, "visit_status") is False + assert hasattr(old_model, "visit_status") is False new_state = migrator.apply_tested_migration((APP_NAME, MIGRATION_0009)) - new_webapp = new_state.apps.get_model(APP_NAME, "SaveOriginRequest") + new_model = new_state.apps.get_model(APP_NAME, "SaveOriginRequest") - assert hasattr(new_webapp, "visit_status") is True + assert hasattr(new_model, "visit_status") is True + + +def test_migrations_10_add_user_id_to_sor_model(migrator): + """Ensures the migration adds the user_id field to SaveOriginRequest table""" + + old_state = migrator.apply_initial_migration((APP_NAME, MIGRATION_0009),) + old_model = old_state.apps.get_model(APP_NAME, "SaveOriginRequest") + + assert hasattr(old_model, "user_id") is False + + new_state = migrator.apply_tested_migration((APP_NAME, MIGRATION_0010)) + new_model = new_state.apps.get_model(APP_NAME, "SaveOriginRequest") + + assert hasattr(new_model, "user_id") is True