From 7f1e3dc2733a22bb60f3d20b265be12f3012da59 Mon Sep 17 00:00:00 2001 From: Aman Dalmia Date: Sat, 19 Jun 2021 09:08:27 +0530 Subject: [PATCH 1/8] NEW: otp test for new user --- users/tests.py | 14 ++++++++++++++ 1 file changed, 14 insertions(+) diff --git a/users/tests.py b/users/tests.py index e5f0bc0f..671a3f9f 100644 --- a/users/tests.py +++ b/users/tests.py @@ -45,6 +45,20 @@ def test_valid_otp_should_pass(self): ) self.assertEqual(response.status_code, status.HTTP_200_OK) + def test_valid_otp_should_pass_new_user(self): + # new user + new_user_mobile = "+919988776654" + + # request otp + self.client.post(reverse("request-otp"), {"mobile": new_user_mobile}) + + # verify valid otp + otp = OneTimePassword.objects.filter(mobile=new_user_mobile).first() + response = self.client.post( + reverse("verify-otp"), {"mobile": new_user_mobile, "otp": otp.otp} + ) + self.assertEqual(response.status_code, status.HTTP_200_OK) + class UserMetaTestCase(BaseTestCase): def setUp(self): From fd259c704664a1d288b70919767c987653df0297 Mon Sep 17 00:00:00 2001 From: Aman Dalmia Date: Sat, 19 Jun 2021 09:18:41 +0530 Subject: [PATCH 2/8] NEW: added .pep8speaks config file --- .pep8speaks.yml | 27 +++++++++++++++++++++++++++ 1 file changed, 27 insertions(+) create mode 100644 .pep8speaks.yml diff --git a/.pep8speaks.yml b/.pep8speaks.yml new file mode 100644 index 00000000..7fa1b69c --- /dev/null +++ b/.pep8speaks.yml @@ -0,0 +1,27 @@ +scanner: + diff_only: True # If False, the entire file touched by the Pull Request is scanned for errors. If True, only the diff is scanned. + linter: pycodestyle # Other option is flake8 + +pycodestyle: # Same as scanner.linter value. Other option is flake8 + max-line-length: 100 # Default is 79 in PEP 8 + # ignore: # Errors and warnings to ignore + # - W504 # line break after binary operator + # - E402 # module level import not at top of file + # - E731 # do not assign a lambda expression, use a def + # - C406 # Unnecessary list literal - rewrite as a dict literal. + # - E741 # ambiguous variable name + +no_blank_comment: True # If True, no comment is made on PR without any errors. +descending_issues_order: False # If True, PEP 8 issues in message will be displayed in descending order of line numbers in the file + +message: # Customize the comment made by the bot + opened: # Messages when a new PR is submitted + header: "Hello @{name}! Thanks for opening this PR. We checked the lines you've touched for [PEP 8](https://www.python.org/dev/peps/pep-0008) issues, and found:" + # The keyword {name} is converted into the author's username + footer: + "Do see the [Hitchhiker's guide to code style](https://goo.gl/hqbW4r)" + # The messages can be written as they would over GitHub + updated: # Messages when new commits are added to the PR + header: "Hello @{name}! Thanks for updating this PR. We checked the lines you've touched for [PEP 8](https://www.python.org/dev/peps/pep-0008) issues, and found:" + footer: "" # Why to comment the link to the style guide everytime? :) + no_errors: "There are currently no PEP 8 issues detected in this Pull Request. Cheers! :beers: " From 80710b0b934f2466022b713e1d8841a8384d3f8e Mon Sep 17 00:00:00 2001 From: Aman Dalmia Date: Sat, 19 Jun 2021 10:15:53 +0530 Subject: [PATCH 3/8] NEW: user config test --- users/tests.py | 69 ++++++++++++++++++++++++++++++++++++++++++++------ 1 file changed, 61 insertions(+), 8 deletions(-) diff --git a/users/tests.py b/users/tests.py index 671a3f9f..ce550015 100644 --- a/users/tests.py +++ b/users/tests.py @@ -1,3 +1,4 @@ +import json from rest_framework import status from django.urls import reverse from users.models import OneTimePassword, User, Role @@ -12,36 +13,35 @@ def setUp(self): super().setUp() # unset client credentials token so that the subsequent API calls goes as guest self.client.credentials() - self.user_mobile = "+919876543210" def test_guest_can_request_for_otp(self): response = self.client.post( - reverse("request-otp"), {"mobile": self.user_mobile} + reverse("request-otp"), {"mobile": self.user.mobile} ) self.assertEqual(response.status_code, status.HTTP_200_OK) - otp_exists = OneTimePassword.objects.filter(mobile=self.user_mobile).exists() + otp_exists = OneTimePassword.objects.filter(mobile=self.user.mobile).exists() self.assertTrue(otp_exists) def test_invalid_otp_should_fail(self): # request otp - self.client.post(reverse("request-otp"), {"mobile": self.user_mobile}) + self.client.post(reverse("request-otp"), {"mobile": self.user.mobile}) # invalid otp otp = "000000" response = self.client.post( - reverse("verify-otp"), {"mobile": self.user_mobile, "otp": otp} + reverse("verify-otp"), {"mobile": self.user.mobile, "otp": otp} ) self.assertEqual(response.status_code, status.HTTP_401_UNAUTHORIZED) def test_valid_otp_should_pass(self): # request otp - self.client.post(reverse("request-otp"), {"mobile": self.user_mobile}) + self.client.post(reverse("request-otp"), {"mobile": self.user.mobile}) # verify valid otp - otp = OneTimePassword.objects.filter(mobile=self.user_mobile).first() + otp = OneTimePassword.objects.filter(mobile=self.user.mobile).first() response = self.client.post( - reverse("verify-otp"), {"mobile": self.user_mobile, "otp": otp.otp} + reverse("verify-otp"), {"mobile": self.user.mobile, "otp": otp.otp} ) self.assertEqual(response.status_code, status.HTTP_200_OK) @@ -60,6 +60,59 @@ def test_valid_otp_should_pass_new_user(self): self.assertEqual(response.status_code, status.HTTP_200_OK) +class UserTestCase(BaseTestCase): + def test_get_config(self): + config = {"test": True} + # set config + self.user.config = config + self.user.save() + + # get config + response = self.client.get(f"/api/v1/users/{self.user.id}/config/") + + self.assertEqual(response.status_code, status.HTTP_200_OK) + self.assertEqual(response.json(), config) + + def test_update_config_no_config_provided(self): + # update config + response = self.client.patch(f"/api/v1/users/{self.user.id}/config/", {}) + self.assertEqual(response.status_code, status.HTTP_400_BAD_REQUEST) + + def test_update_config_extra_params(self): + # update config + response = self.client.patch( + f"/api/v1/users/{self.user.id}/config/", + {"config": {}, "extra_param": True}, + format="json", + ) + self.assertEqual(response.status_code, status.HTTP_400_BAD_REQUEST) + self.assertEqual( + json.loads(response.content)["detail"], + "extra keys apart from config are not allowed", + ) + + def test_update_config(self): + # update config + config = {"test": True} + response = self.client.patch( + f"/api/v1/users/{self.user.id}/config/", + {"config": config}, + format="json", + ) + self.assertEqual(response.status_code, status.HTTP_200_OK) + + # get the config to see if it is updated + response = self.client.get(f"/api/v1/users/{self.user.id}/config/") + + self.assertEqual( + response.json(), + config, + ) + + # one user should not be able to get config of another user + # one user should not be able to update config of another user + + class UserMetaTestCase(BaseTestCase): def setUp(self): super().setUp() From e151d963fbba51a51e01f67003d3b7d9b3eb7acf Mon Sep 17 00:00:00 2001 From: Aman Dalmia Date: Sat, 19 Jun 2021 10:20:55 +0530 Subject: [PATCH 4/8] NEW: missing config tests --- users/tests.py | 15 +++++++++++++-- 1 file changed, 13 insertions(+), 2 deletions(-) diff --git a/users/tests.py b/users/tests.py index ce550015..1208712a 100644 --- a/users/tests.py +++ b/users/tests.py @@ -73,6 +73,11 @@ def test_get_config(self): self.assertEqual(response.status_code, status.HTTP_200_OK) self.assertEqual(response.json(), config) + def test_user_cannot_get_other_user_config(self): + # get config + response = self.client.get(f"/api/v1/users/{self.user_2.id}/config/") + self.assertEqual(response.status_code, status.HTTP_403_FORBIDDEN) + def test_update_config_no_config_provided(self): # update config response = self.client.patch(f"/api/v1/users/{self.user.id}/config/", {}) @@ -109,8 +114,14 @@ def test_update_config(self): config, ) - # one user should not be able to get config of another user - # one user should not be able to update config of another user + def test_user_cannot_update_other_user_config(self): + # get config + response = self.client.patch( + f"/api/v1/users/{self.user_2.id}/config/", + {"config": {"test": True}}, + format="json", + ) + self.assertEqual(response.status_code, status.HTTP_403_FORBIDDEN) class UserMetaTestCase(BaseTestCase): From 5935ed328d89c3309ac75658ad719c104033924c Mon Sep 17 00:00:00 2001 From: Aman Dalmia Date: Sat, 19 Jun 2021 12:36:22 +0530 Subject: [PATCH 5/8] NEW: test listing org users --- users/tests.py | 73 +++++++++++++++++++++++++++++++++++++++++--------- 1 file changed, 60 insertions(+), 13 deletions(-) diff --git a/users/tests.py b/users/tests.py index 1208712a..3695c15f 100644 --- a/users/tests.py +++ b/users/tests.py @@ -1,7 +1,7 @@ import json from rest_framework import status from django.urls import reverse -from users.models import OneTimePassword, User, Role +from users.models import OneTimePassword, Role, OrganizationUser from plio.tests import BaseTestCase from organizations.models import Organization @@ -145,21 +145,70 @@ def test_for_role(self): class OrganizationUserTestCase(BaseTestCase): def setUp(self): super().setUp() - # set up an organization - self.organization = Organization.objects.create(name="Org 1", shortcode="org-1") - # set up a user that's supposed to be in the organization - self.organization_user = User.objects.create(mobile="+919988776655") + + # create another organization + self.organization_2 = Organization.objects.create( + name="Org 2", shortcode="org-2" + ) + + # seed some organization users + self.org_user_1 = OrganizationUser.objects.create( + organization=self.organization, user=self.user, role=self.org_view_role + ) + + self.org_user_2 = OrganizationUser.objects.create( + organization=self.organization_2, user=self.user, role=self.org_view_role + ) + + def test_superuser_can_list_all_organization_users(self): + # make the current user as superuser + self.user.is_superuser = True + self.user.save() + + # get organization users + response = self.client.get(reverse("organization-users-list")) + self.assertEqual(response.status_code, status.HTTP_200_OK) + self.assertEqual(len(response.json()), 2) + self.assertEqual(response.json()[0]["user"], self.user.id) + self.assertEqual(response.json()[1]["user"], self.user.id) + self.assertEqual(response.json()[0]["organization"], self.organization.id) + self.assertEqual(response.json()[1]["organization"], self.organization_2.id) + + def test_normal_user_cannot_list_organization_users(self): + # get organization users + response = self.client.get(reverse("organization-users-list")) + self.assertEqual(response.status_code, status.HTTP_200_OK) + self.assertEqual(len(response.json()), 0) + + def test_org_admin_can_list_only_their_organization_users(self): + # make user 2 org-admin for org 1 + org_admin_role = Role.objects.filter(name="org-admin").first() + OrganizationUser.objects.create( + organization=self.organization, user=self.user_2, role=org_admin_role + ) + + # change user + self.client.credentials( + HTTP_AUTHORIZATION="Bearer " + self.access_token_2.token, + ) + + # get organization users + response = self.client.get(reverse("organization-users-list")) + self.assertEqual(response.status_code, status.HTTP_200_OK) + self.assertEqual(len(response.json()), 2) + self.assertEqual(response.json()[0]["user"], self.user.id) + self.assertEqual(response.json()[0]["organization"], self.organization.id) + self.assertEqual(response.json()[1]["user"], self.user_2.id) + self.assertEqual(response.json()[1]["organization"], self.organization.id) def test_normal_user_cannot_create_organization_user(self): - # get org-view role - role_org_view = Role.objects.filter(name="org-view").first() # add organization_user to the organization response = self.client.post( reverse("organization-users-list"), { - "user": self.organization_user.id, + "user": self.user.id, "organization": self.organization.id, - "role": role_org_view.id, + "role": self.org_view_role.id, }, ) self.assertEqual(response.status_code, status.HTTP_403_FORBIDDEN) @@ -169,15 +218,13 @@ def test_superuser_can_create_organization_user(self): self.user.is_superuser = True self.user.save() - # get org-view role - role_org_view = Role.objects.filter(name="org-view").first() # add organization_user to the organization response = self.client.post( reverse("organization-users-list"), { - "user": self.organization_user.id, + "user": self.user.id, "organization": self.organization.id, - "role": role_org_view.id, + "role": self.org_view_role.id, }, ) self.assertEqual(response.status_code, status.HTTP_201_CREATED) From 1a59f0ed08dce24bb786d31642d282d3d5f5902a Mon Sep 17 00:00:00 2001 From: Aman Dalmia Date: Sat, 19 Jun 2021 13:03:50 +0530 Subject: [PATCH 6/8] NEW: get-by-access-token --- plio/urls.py | 2 +- users/tests.py | 15 +++++++++++++++ users/views.py | 6 ++++++ 3 files changed, 22 insertions(+), 1 deletion(-) diff --git a/plio/urls.py b/plio/urls.py index fa0f4e58..d18159b0 100644 --- a/plio/urls.py +++ b/plio/urls.py @@ -87,7 +87,7 @@ # API routes path("api/v1/otp/request/", request_otp, name="request-otp"), path("api/v1/otp/verify/", verify_otp, name="verify-otp"), - path("api/v1/users/token/", get_by_access_token), + path("api/v1/users/token/", get_by_access_token, name="get-by-access-token"), path("api/v1/", include(api_router.urls)), path("api-auth/", include("rest_framework.urls", namespace="rest_framework")), path("auth/cubejs-token/", retrieve_analytics_app_access_token), diff --git a/users/tests.py b/users/tests.py index 3695c15f..1ec945a8 100644 --- a/users/tests.py +++ b/users/tests.py @@ -123,6 +123,21 @@ def test_user_cannot_update_other_user_config(self): ) self.assertEqual(response.status_code, status.HTTP_403_FORBIDDEN) + def test_get_by_access_token_access_token_not_passed(self): + response = self.client.get(reverse("get-by-access-token")) + self.assertEqual(response.status_code, status.HTTP_400_BAD_REQUEST) + + def test_get_by_access_token_access_token_invalid(self): + response = self.client.get(reverse("get-by-access-token"), {"token": "1234"}) + self.assertEqual(response.status_code, status.HTTP_404_NOT_FOUND) + + def test_get_by_access_token_valid_user(self): + response = self.client.get( + reverse("get-by-access-token"), {"token": self.access_token.token} + ) + self.assertEqual(response.status_code, status.HTTP_200_OK) + self.assertEqual(response.json()["id"], self.user.id) + class UserMetaTestCase(BaseTestCase): def setUp(self): diff --git a/users/views.py b/users/views.py index ef08bb23..783dd6b1 100644 --- a/users/views.py +++ b/users/views.py @@ -196,6 +196,12 @@ def verify_otp(request): @api_view(["GET"]) def get_by_access_token(request): + if "token" not in request.query_params: + return Response( + {"detail": "token not provided"}, + status=status.HTTP_400_BAD_REQUEST, + ) + token = request.query_params["token"] access_token = AccessToken.objects.filter(token=token).first() if access_token: From 6f584ec481a5f991bae5f9acca6e27249ea5bbf6 Mon Sep 17 00:00:00 2001 From: Aman Dalmia Date: Sat, 19 Jun 2021 13:19:35 +0530 Subject: [PATCH 7/8] NEW: added test for analytics app token fetching --- .github/workflows/ci.yml | 12 ++++++++---- docs/DEPLOYMENT.md | 6 ++++++ plio/urls.py | 6 +++++- users/tests.py | 5 +++++ 4 files changed, 24 insertions(+), 5 deletions(-) diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index e81b15ad..af220f32 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -10,9 +10,9 @@ jobs: name: Pre-commit runs-on: ubuntu-latest steps: - - uses: actions/checkout@v2 - - uses: actions/setup-python@v2 - - uses: pre-commit/action@v2.0.0 + - uses: actions/checkout@v2 + - uses: actions/setup-python@v2 + - uses: pre-commit/action@v2.0.0 test: name: Test cases @@ -58,11 +58,15 @@ jobs: DB_NAME: github_actions_testing DB_USER: postgres DB_PASSWORD: postgres - SECRET_KEY: wpurj&oym6m@kcp(m&z(q-g0bo-r*+!f_&j(94di8j&_j4m%2s # random secret key + SECRET_KEY: wpurj&oym6m@kcp(m&z(q-g0bo-r*+!f_&j(94di8j&_j4m%2s # random secret key AWS_ACCESS_KEY_ID: ${{ secrets.AWS_ACCESS_KEY_ID }} AWS_SECRET_ACCESS_KEY: ${{ secrets.AWS_SECRET_ACCESS_KEY }} AWS_REGION: ${{ secrets.AWS_REGION }} AWS_STORAGE_BUCKET_NAME: ${{ secrets.AWS_STORAGE_BUCKET_NAME }} + ANALYTICS_IDP_TYPE: ${{ secrets.ANALYTICS_IDP_TYPE }} + ANALYTICS_IDP_TOKEN_URL: ${{ secrets.ANALYTICS_IDP_TOKEN_URL }} + ANALYTICS_IDP_CLIENT_ID: ${{ secrets.ANALYTICS_IDP_CLIENT_ID }} + ANALYTICS_IDP_CLIENT_SECRET: ${{ secrets.ANALYTICS_IDP_CLIENT_SECRET }} REDIS_HOSTNAME: 127.0.0.1 REDIS_PORT: 6379 # command to run tests and generate coverage metrics diff --git a/docs/DEPLOYMENT.md b/docs/DEPLOYMENT.md index f0c7ed9b..efe2b4f6 100644 --- a/docs/DEPLOYMENT.md +++ b/docs/DEPLOYMENT.md @@ -200,6 +200,12 @@ Follow the steps below to set up the staging environment on AWS. - AWS_ACCESS_KEY_ID - AWS_SECRET_ACCESS_KEY - AWS_REGION + - AWS_STORAGE_BUCKET_NAME + - ANALYTICS_IDP_TYPE + - ANALYTICS_IDP_TOKEN_URL + - ANALYTICS_IDP_CLIENT_ID + - ANALYTICS_IDP_CLIENT_SECRET + - ANALYTICS_IDP_AUDIENCE (optional) 14. We are using Github Actions to trigger deployments. You can find the workflow defined in `.github/workflows/deploy_to_ecs_staging.yml`. It defines a target branch such that a deployment is initiated whenever a change is pushed to the target branch. diff --git a/plio/urls.py b/plio/urls.py index d18159b0..76a713b9 100644 --- a/plio/urls.py +++ b/plio/urls.py @@ -90,7 +90,11 @@ path("api/v1/users/token/", get_by_access_token, name="get-by-access-token"), path("api/v1/", include(api_router.urls)), path("api-auth/", include("rest_framework.urls", namespace="rest_framework")), - path("auth/cubejs-token/", retrieve_analytics_app_access_token), + path( + "auth/cubejs-token/", + retrieve_analytics_app_access_token, + name="get-analytics-token", + ), url(r"^auth/", include("rest_framework_social_oauth2.urls")), url( r"^api/v1/docs/$", diff --git a/users/tests.py b/users/tests.py index 1ec945a8..fecbb915 100644 --- a/users/tests.py +++ b/users/tests.py @@ -138,6 +138,11 @@ def test_get_by_access_token_valid_user(self): self.assertEqual(response.status_code, status.HTTP_200_OK) self.assertEqual(response.json()["id"], self.user.id) + def test_get_analytics_app_access_token(self): + response = self.client.post(reverse("get-analytics-token")) + self.assertEqual(response.status_code, status.HTTP_200_OK) + self.assertIn("access_token", response.json()) + class UserMetaTestCase(BaseTestCase): def setUp(self): From 6057e305c642cdcaa347f24fe3cfbf2f0eea76d2 Mon Sep 17 00:00:00 2001 From: Aman Dalmia Date: Sat, 19 Jun 2021 14:56:49 +0530 Subject: [PATCH 8/8] FIX: PR feedback --- users/tests.py | 12 +++++++----- 1 file changed, 7 insertions(+), 5 deletions(-) diff --git a/users/tests.py b/users/tests.py index fecbb915..2d7c8707 100644 --- a/users/tests.py +++ b/users/tests.py @@ -79,11 +79,13 @@ def test_user_cannot_get_other_user_config(self): self.assertEqual(response.status_code, status.HTTP_403_FORBIDDEN) def test_update_config_no_config_provided(self): + """Updating config without passing the config to use should fail""" # update config response = self.client.patch(f"/api/v1/users/{self.user.id}/config/", {}) self.assertEqual(response.status_code, status.HTTP_400_BAD_REQUEST) def test_update_config_extra_params(self): + """Passing params other than config while updating config should fail""" # update config response = self.client.patch( f"/api/v1/users/{self.user.id}/config/", @@ -180,7 +182,7 @@ def setUp(self): organization=self.organization_2, user=self.user, role=self.org_view_role ) - def test_superuser_can_list_all_organization_users(self): + def test_superuser_can_list_all_org_users(self): # make the current user as superuser self.user.is_superuser = True self.user.save() @@ -194,13 +196,13 @@ def test_superuser_can_list_all_organization_users(self): self.assertEqual(response.json()[0]["organization"], self.organization.id) self.assertEqual(response.json()[1]["organization"], self.organization_2.id) - def test_normal_user_cannot_list_organization_users(self): + def test_normal_user_only_sees_empty_list_of_org_users(self): # get organization users response = self.client.get(reverse("organization-users-list")) self.assertEqual(response.status_code, status.HTTP_200_OK) self.assertEqual(len(response.json()), 0) - def test_org_admin_can_list_only_their_organization_users(self): + def test_org_admin_can_list_only_their_org_users(self): # make user 2 org-admin for org 1 org_admin_role = Role.objects.filter(name="org-admin").first() OrganizationUser.objects.create( @@ -221,7 +223,7 @@ def test_org_admin_can_list_only_their_organization_users(self): self.assertEqual(response.json()[1]["user"], self.user_2.id) self.assertEqual(response.json()[1]["organization"], self.organization.id) - def test_normal_user_cannot_create_organization_user(self): + def test_normal_user_cannot_create_org_user(self): # add organization_user to the organization response = self.client.post( reverse("organization-users-list"), @@ -233,7 +235,7 @@ def test_normal_user_cannot_create_organization_user(self): ) self.assertEqual(response.status_code, status.HTTP_403_FORBIDDEN) - def test_superuser_can_create_organization_user(self): + def test_superuser_can_create_org_user(self): # make the current user as superuser self.user.is_superuser = True self.user.save()