From f272a0a128918abfa2f88292c68c022f858625b7 Mon Sep 17 00:00:00 2001 From: Cheick Keita Date: Mon, 21 Jun 2021 11:04:25 -0700 Subject: [PATCH 1/7] Fix issue when deserializing GithubIssueTemplate with hidden secret Updated unit tests to check the expected data type for SecretData --- src/api-service/tests/test_secrets.py | 127 ++++++++++++++++++++------ src/pytypes/onefuzztypes/models.py | 19 +++- src/pytypes/tests/test_models.py | 2 +- 3 files changed, 116 insertions(+), 32 deletions(-) diff --git a/src/api-service/tests/test_secrets.py b/src/api-service/tests/test_secrets.py index 89dae42bd4..5f30373685 100644 --- a/src/api-service/tests/test_secrets.py +++ b/src/api-service/tests/test_secrets.py @@ -14,6 +14,9 @@ JobTemplateNotification, ) from onefuzztypes.models import ( + ADOTemplate, + GithubAuth, + GithubIssueTemplate, JobConfig, Notification, NotificationConfig, @@ -27,13 +30,14 @@ from __app__.onefuzzlib.orm import hide_secrets +def hider(secret_data: SecretData) -> SecretData: + if not isinstance(secret_data.secret, SecretAddress): + secret_data.secret = SecretAddress(url="blah blah") + return secret_data + + class TestSecret(unittest.TestCase): def test_hide(self) -> None: - def hider(secret_data: SecretData) -> SecretData: - if not isinstance(secret_data.secret, SecretAddress): - secret_data.secret = SecretAddress(url="blah blah") - return secret_data - notification = Notification( container=Container("data"), config=TeamsTemplate(url=SecretData(secret="http://test")), @@ -47,11 +51,6 @@ def hider(secret_data: SecretData) -> SecretData: self.fail(f"Invalid config type {type(notification.config)}") def test_hide_nested_list(self) -> None: - def hider(secret_data: SecretData) -> SecretData: - if not isinstance(secret_data.secret, SecretAddress): - secret_data.secret = SecretAddress(url="blah blah") - return secret_data - job_template_index = JobTemplateIndex( name="test", template=JobTemplate( @@ -100,12 +99,37 @@ def test_roundtrip_github_issue(self) -> None: f"{current_path}" + "/../../../contrib/onefuzz-job-github-actions/github-issues.json" ) as json_file: - b = json.load(json_file) - b["container"] = "testing" - c = NotificationCreate.parse_obj(b) - d = c.json() - e = json.loads(d) - NotificationCreate.parse_obj(e) + notification_dict = json.load(json_file) + notification_dict["container"] = "testing" + notification1 = NotificationCreate.parse_obj(notification_dict) + self.assertIsInstance( + notification1.config, GithubIssueTemplate, "invalid config type" + ) + self.assertIsInstance( + notification1.config.auth.secret, GithubAuth, "Invalid secret type" + ) + + notification2 = NotificationCreate.parse_obj( + json.loads(notification1.json()) + ) + self.assertIsInstance( + notification2.config, GithubIssueTemplate, "invalid config type" + ) + self.assertIsInstance( + notification2.config.auth.secret, GithubAuth, "Invalid secret type" + ) + + hide_secrets(notification2, hider) + + notification3 = NotificationCreate.parse_obj( + json.loads(notification2.json()) + ) + self.assertIsInstance( + notification3.config, GithubIssueTemplate, "invalid config type" + ) + self.assertIsInstance( + notification2.config.auth.secret, SecretAddress, "Invalid secret type" + ) def test_roundtrip_team_issue(self) -> None: a = """ @@ -115,11 +139,33 @@ def test_roundtrip_team_issue(self) -> None: } """ # noqa - b = json.loads(a) - c = NotificationCreate.parse_obj(b) - d = c.json() - e = json.loads(d) - NotificationCreate.parse_obj(e) + notification_dict = json.loads(a) + notification_dict["container"] = "testing" + notification1 = NotificationCreate.parse_obj(notification_dict) + self.assertIsInstance( + notification1.config, TeamsTemplate, "invalid config type" + ) + self.assertIsInstance( + notification1.config.url.secret, str, "Invalid secret type" + ) + + notification2 = NotificationCreate.parse_obj(json.loads(notification1.json())) + self.assertIsInstance( + notification2.config, TeamsTemplate, "invalid config type" + ) + self.assertIsInstance( + notification2.config.url.secret, str, "Invalid secret type" + ) + + hide_secrets(notification2, hider) + + notification3 = NotificationCreate.parse_obj(json.loads(notification2.json())) + self.assertIsInstance( + notification3.config, TeamsTemplate, "invalid config type" + ) + self.assertIsInstance( + notification3.config.url.secret, SecretAddress, "Invalid secret type" + ) def test_roundtrip_ado(self) -> None: current_path = pathlib.Path(__file__).parent.absolute() @@ -127,9 +173,36 @@ def test_roundtrip_ado(self) -> None: f"{current_path}" + "/../../../contrib/onefuzz-job-azure-devops-pipeline/ado-work-items.json" # noqa ) as json_file: - b = json.load(json_file) - b["container"] = "testing" - c = NotificationCreate.parse_obj(b) - d = c.json() - e = json.loads(d) - NotificationCreate.parse_obj(e) + notification_dict = json.load(json_file) + notification_dict["container"] = "testing" + notification1 = NotificationCreate.parse_obj(notification_dict) + self.assertIsInstance( + notification1.config, ADOTemplate, "invalid config type" + ) + self.assertIsInstance( + notification1.config.auth_token.secret, str, "Invalid secret type" + ) + + notification2 = NotificationCreate.parse_obj( + json.loads(notification1.json()) + ) + self.assertIsInstance( + notification2.config, ADOTemplate, "invalid config type" + ) + self.assertIsInstance( + notification2.config.auth_token.secret, str, "Invalid secret type" + ) + + hide_secrets(notification2, hider) + + notification3 = NotificationCreate.parse_obj( + json.loads(notification2.json()) + ) + self.assertIsInstance( + notification3.config, ADOTemplate, "invalid config type" + ) + self.assertIsInstance( + notification3.config.auth_token.secret, + SecretAddress, + "Invalid secret type", + ) diff --git a/src/pytypes/onefuzztypes/models.py b/src/pytypes/onefuzztypes/models.py index f333d82be7..69599f0585 100644 --- a/src/pytypes/onefuzztypes/models.py +++ b/src/pytypes/onefuzztypes/models.py @@ -511,15 +511,26 @@ class GithubIssueTemplate(BaseModel): # validator needed for backward compatibility @validator("auth", pre=True, always=True) def validate_auth(cls, v: Any) -> SecretData: + def try_parse_GithubAuth(x) -> Optional[GithubAuth]: + try: + return GithubAuth.parse_obj(x) + except Exception: + None + if isinstance(v, GithubAuth): return SecretData(secret=v) elif isinstance(v, SecretData): return v elif isinstance(v, dict): - try: - return SecretData(GithubAuth.parse_obj(v)) - except Exception: - return SecretData(GithubAuth.parse_obj(v["secret"])) + githubAuth = try_parse_GithubAuth(v) + if githubAuth: + return SecretData(secret=githubAuth) + + githubAuth = try_parse_GithubAuth(v["secret"]) + if githubAuth: + return SecretData(secret=githubAuth) + + return SecretData(secret=v["secret"]) else: raise TypeError(f"invalid datatype {type(v)}") diff --git a/src/pytypes/tests/test_models.py b/src/pytypes/tests/test_models.py index f5e4b1d71d..c18458bb56 100755 --- a/src/pytypes/tests/test_models.py +++ b/src/pytypes/tests/test_models.py @@ -6,8 +6,8 @@ import unittest from onefuzztypes.models import Scaleset, SecretData, TeamsTemplate -from onefuzztypes.requests import NotificationCreate from onefuzztypes.primitives import PoolName, Region +from onefuzztypes.requests import NotificationCreate from pydantic import ValidationError From 48e771c39ce7581d489fda828eb831fd31321910 Mon Sep 17 00:00:00 2001 From: Cheick Keita Date: Mon, 21 Jun 2021 13:08:55 -0700 Subject: [PATCH 2/7] add type anotation --- src/pytypes/onefuzztypes/models.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/pytypes/onefuzztypes/models.py b/src/pytypes/onefuzztypes/models.py index 69599f0585..ecd9a718bd 100644 --- a/src/pytypes/onefuzztypes/models.py +++ b/src/pytypes/onefuzztypes/models.py @@ -511,11 +511,11 @@ class GithubIssueTemplate(BaseModel): # validator needed for backward compatibility @validator("auth", pre=True, always=True) def validate_auth(cls, v: Any) -> SecretData: - def try_parse_GithubAuth(x) -> Optional[GithubAuth]: + def try_parse_GithubAuth(x: dict) -> Optional[GithubAuth]: try: return GithubAuth.parse_obj(x) except Exception: - None + return None if isinstance(v, GithubAuth): return SecretData(secret=v) From 6bea451074d3a13cff90b0d13b493733523dbbbd Mon Sep 17 00:00:00 2001 From: Cheick Keita Date: Mon, 21 Jun 2021 13:39:20 -0700 Subject: [PATCH 3/7] fix mypy issues --- src/api-service/tests/test_secrets.py | 28 ++++++++++++++++++--------- 1 file changed, 19 insertions(+), 9 deletions(-) diff --git a/src/api-service/tests/test_secrets.py b/src/api-service/tests/test_secrets.py index 5f30373685..56a3876805 100644 --- a/src/api-service/tests/test_secrets.py +++ b/src/api-service/tests/test_secrets.py @@ -6,6 +6,7 @@ import json import pathlib import unittest +from typing import cast from onefuzztypes.enums import OS, ContainerType from onefuzztypes.job_templates import ( @@ -105,8 +106,9 @@ def test_roundtrip_github_issue(self) -> None: self.assertIsInstance( notification1.config, GithubIssueTemplate, "invalid config type" ) + notification1_config = cast(GithubIssueTemplate, notification1.config) self.assertIsInstance( - notification1.config.auth.secret, GithubAuth, "Invalid secret type" + notification1_config.auth.secret, GithubAuth, "Invalid secret type" ) notification2 = NotificationCreate.parse_obj( @@ -115,8 +117,9 @@ def test_roundtrip_github_issue(self) -> None: self.assertIsInstance( notification2.config, GithubIssueTemplate, "invalid config type" ) + notification2_config = cast(GithubIssueTemplate, notification2.config) self.assertIsInstance( - notification2.config.auth.secret, GithubAuth, "Invalid secret type" + notification2_config.auth.secret, GithubAuth, "Invalid secret type" ) hide_secrets(notification2, hider) @@ -127,8 +130,9 @@ def test_roundtrip_github_issue(self) -> None: self.assertIsInstance( notification3.config, GithubIssueTemplate, "invalid config type" ) + notification3_config = cast(GithubIssueTemplate, notification3.config) self.assertIsInstance( - notification2.config.auth.secret, SecretAddress, "Invalid secret type" + notification3_config.auth.secret, SecretAddress, "Invalid secret type" ) def test_roundtrip_team_issue(self) -> None: @@ -145,16 +149,18 @@ def test_roundtrip_team_issue(self) -> None: self.assertIsInstance( notification1.config, TeamsTemplate, "invalid config type" ) + notification1_config = cast(TeamsTemplate, notification1.config) self.assertIsInstance( - notification1.config.url.secret, str, "Invalid secret type" + notification1_config.url.secret, str, "Invalid secret type" ) notification2 = NotificationCreate.parse_obj(json.loads(notification1.json())) self.assertIsInstance( notification2.config, TeamsTemplate, "invalid config type" ) + notification2_config = cast(TeamsTemplate, notification2.config) self.assertIsInstance( - notification2.config.url.secret, str, "Invalid secret type" + notification2_config.url.secret, str, "Invalid secret type" ) hide_secrets(notification2, hider) @@ -163,8 +169,9 @@ def test_roundtrip_team_issue(self) -> None: self.assertIsInstance( notification3.config, TeamsTemplate, "invalid config type" ) + notification3_config = cast(TeamsTemplate, notification3.config) self.assertIsInstance( - notification3.config.url.secret, SecretAddress, "Invalid secret type" + notification3_config.url.secret, SecretAddress, "Invalid secret type" ) def test_roundtrip_ado(self) -> None: @@ -179,8 +186,9 @@ def test_roundtrip_ado(self) -> None: self.assertIsInstance( notification1.config, ADOTemplate, "invalid config type" ) + notification1_config = cast(ADOTemplate, notification1.config) self.assertIsInstance( - notification1.config.auth_token.secret, str, "Invalid secret type" + notification1_config.auth_token.secret, str, "Invalid secret type" ) notification2 = NotificationCreate.parse_obj( @@ -189,8 +197,9 @@ def test_roundtrip_ado(self) -> None: self.assertIsInstance( notification2.config, ADOTemplate, "invalid config type" ) + notification2_config = cast(ADOTemplate, notification2.config) self.assertIsInstance( - notification2.config.auth_token.secret, str, "Invalid secret type" + notification2_config.auth_token.secret, str, "Invalid secret type" ) hide_secrets(notification2, hider) @@ -201,8 +210,9 @@ def test_roundtrip_ado(self) -> None: self.assertIsInstance( notification3.config, ADOTemplate, "invalid config type" ) + notification3_config = cast(ADOTemplate, notification3.config) self.assertIsInstance( - notification3.config.auth_token.secret, + notification3_config.auth_token.secret, SecretAddress, "Invalid secret type", ) From f3c49c89ef7e1879cea703de17aa4cd71852aa8c Mon Sep 17 00:00:00 2001 From: Cheick Keita Date: Tue, 22 Jun 2021 08:34:49 -0700 Subject: [PATCH 4/7] address PR comments --- src/api-service/tests/test_secrets.py | 66 +++++++++------------------ 1 file changed, 21 insertions(+), 45 deletions(-) diff --git a/src/api-service/tests/test_secrets.py b/src/api-service/tests/test_secrets.py index 56a3876805..a57199b60a 100644 --- a/src/api-service/tests/test_secrets.py +++ b/src/api-service/tests/test_secrets.py @@ -103,23 +103,19 @@ def test_roundtrip_github_issue(self) -> None: notification_dict = json.load(json_file) notification_dict["container"] = "testing" notification1 = NotificationCreate.parse_obj(notification_dict) + + assert isinstance(notification1.config, GithubIssueTemplate) self.assertIsInstance( - notification1.config, GithubIssueTemplate, "invalid config type" - ) - notification1_config = cast(GithubIssueTemplate, notification1.config) - self.assertIsInstance( - notification1_config.auth.secret, GithubAuth, "Invalid secret type" + notification1.config.auth.secret, GithubAuth, "Invalid secret type" ) notification2 = NotificationCreate.parse_obj( json.loads(notification1.json()) ) + + assert isinstance(notification2.config, GithubIssueTemplate) self.assertIsInstance( - notification2.config, GithubIssueTemplate, "invalid config type" - ) - notification2_config = cast(GithubIssueTemplate, notification2.config) - self.assertIsInstance( - notification2_config.auth.secret, GithubAuth, "Invalid secret type" + notification2.config.auth.secret, GithubAuth, "Invalid secret type" ) hide_secrets(notification2, hider) @@ -127,12 +123,9 @@ def test_roundtrip_github_issue(self) -> None: notification3 = NotificationCreate.parse_obj( json.loads(notification2.json()) ) + assert isinstance(notification2.config, GithubIssueTemplate) self.assertIsInstance( - notification3.config, GithubIssueTemplate, "invalid config type" - ) - notification3_config = cast(GithubIssueTemplate, notification3.config) - self.assertIsInstance( - notification3_config.auth.secret, SecretAddress, "Invalid secret type" + notification2.config.auth.secret, SecretAddress, "Invalid secret type" ) def test_roundtrip_team_issue(self) -> None: @@ -146,32 +139,24 @@ def test_roundtrip_team_issue(self) -> None: notification_dict = json.loads(a) notification_dict["container"] = "testing" notification1 = NotificationCreate.parse_obj(notification_dict) + + assert isinstance(notification1.config, TeamsTemplate) self.assertIsInstance( - notification1.config, TeamsTemplate, "invalid config type" - ) - notification1_config = cast(TeamsTemplate, notification1.config) - self.assertIsInstance( - notification1_config.url.secret, str, "Invalid secret type" + notification1.config.url.secret, str, "Invalid secret type" ) notification2 = NotificationCreate.parse_obj(json.loads(notification1.json())) + assert isinstance(notification2.config, TeamsTemplate) self.assertIsInstance( - notification2.config, TeamsTemplate, "invalid config type" - ) - notification2_config = cast(TeamsTemplate, notification2.config) - self.assertIsInstance( - notification2_config.url.secret, str, "Invalid secret type" + notification2.config.url.secret, str, "Invalid secret type" ) hide_secrets(notification2, hider) notification3 = NotificationCreate.parse_obj(json.loads(notification2.json())) + assert isinstance(notification3.config, TeamsTemplate) self.assertIsInstance( - notification3.config, TeamsTemplate, "invalid config type" - ) - notification3_config = cast(TeamsTemplate, notification3.config) - self.assertIsInstance( - notification3_config.url.secret, SecretAddress, "Invalid secret type" + notification3.config.url.secret, SecretAddress, "Invalid secret type" ) def test_roundtrip_ado(self) -> None: @@ -183,23 +168,17 @@ def test_roundtrip_ado(self) -> None: notification_dict = json.load(json_file) notification_dict["container"] = "testing" notification1 = NotificationCreate.parse_obj(notification_dict) + assert isinstance(notification1.config, ADOTemplate) self.assertIsInstance( - notification1.config, ADOTemplate, "invalid config type" - ) - notification1_config = cast(ADOTemplate, notification1.config) - self.assertIsInstance( - notification1_config.auth_token.secret, str, "Invalid secret type" + notification1.config.auth_token.secret, str, "Invalid secret type" ) notification2 = NotificationCreate.parse_obj( json.loads(notification1.json()) ) + assert isinstance(notification2.config, ADOTemplate) self.assertIsInstance( - notification2.config, ADOTemplate, "invalid config type" - ) - notification2_config = cast(ADOTemplate, notification2.config) - self.assertIsInstance( - notification2_config.auth_token.secret, str, "Invalid secret type" + notification2.config.auth_token.secret, str, "Invalid secret type" ) hide_secrets(notification2, hider) @@ -207,12 +186,9 @@ def test_roundtrip_ado(self) -> None: notification3 = NotificationCreate.parse_obj( json.loads(notification2.json()) ) + assert isinstance(notification3.config, ADOTemplate) self.assertIsInstance( - notification3.config, ADOTemplate, "invalid config type" - ) - notification3_config = cast(ADOTemplate, notification3.config) - self.assertIsInstance( - notification3_config.auth_token.secret, + notification3.config.auth_token.secret, SecretAddress, "Invalid secret type", ) From 6f9ffe19ad88e88f1a606cb899dd0c51538fbb6d Mon Sep 17 00:00:00 2001 From: Cheick Keita Date: Tue, 22 Jun 2021 09:10:56 -0700 Subject: [PATCH 5/7] flake fix --- src/api-service/tests/test_secrets.py | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/src/api-service/tests/test_secrets.py b/src/api-service/tests/test_secrets.py index a57199b60a..cd3512e383 100644 --- a/src/api-service/tests/test_secrets.py +++ b/src/api-service/tests/test_secrets.py @@ -6,7 +6,6 @@ import json import pathlib import unittest -from typing import cast from onefuzztypes.enums import OS, ContainerType from onefuzztypes.job_templates import ( @@ -125,7 +124,7 @@ def test_roundtrip_github_issue(self) -> None: ) assert isinstance(notification2.config, GithubIssueTemplate) self.assertIsInstance( - notification2.config.auth.secret, SecretAddress, "Invalid secret type" + notification3.config.auth.secret, SecretAddress, "Invalid secret type" ) def test_roundtrip_team_issue(self) -> None: From 334aee8b52a4a348d2d5f3644c56066d8ab126ac Mon Sep 17 00:00:00 2001 From: Cheick Keita Date: Tue, 22 Jun 2021 10:26:58 -0700 Subject: [PATCH 6/7] mypy fix --- src/api-service/tests/test_secrets.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/api-service/tests/test_secrets.py b/src/api-service/tests/test_secrets.py index cd3512e383..487e2537da 100644 --- a/src/api-service/tests/test_secrets.py +++ b/src/api-service/tests/test_secrets.py @@ -124,7 +124,7 @@ def test_roundtrip_github_issue(self) -> None: ) assert isinstance(notification2.config, GithubIssueTemplate) self.assertIsInstance( - notification3.config.auth.secret, SecretAddress, "Invalid secret type" + notification2.config.auth.secret, SecretAddress, "Invalid secret type" ) def test_roundtrip_team_issue(self) -> None: From 03d05437ef1e81a69250419e8d843b1fb33265ca Mon Sep 17 00:00:00 2001 From: Cheick Keita Date: Tue, 22 Jun 2021 10:54:50 -0700 Subject: [PATCH 7/7] fix mypy --- src/api-service/tests/test_secrets.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/api-service/tests/test_secrets.py b/src/api-service/tests/test_secrets.py index 487e2537da..e484cf9ee1 100644 --- a/src/api-service/tests/test_secrets.py +++ b/src/api-service/tests/test_secrets.py @@ -122,9 +122,9 @@ def test_roundtrip_github_issue(self) -> None: notification3 = NotificationCreate.parse_obj( json.loads(notification2.json()) ) - assert isinstance(notification2.config, GithubIssueTemplate) + assert isinstance(notification3.config, GithubIssueTemplate) self.assertIsInstance( - notification2.config.auth.secret, SecretAddress, "Invalid secret type" + notification3.config.auth.secret, SecretAddress, "Invalid secret type" ) def test_roundtrip_team_issue(self) -> None: