Skip to content

Commit

Permalink
(PC-33673)[API] feat: remove unused WIP_ENABLE_NEW_HASHING_ALGORITHM
Browse files Browse the repository at this point in the history
The feature is not always set, no need to keep it as a FF.
  • Loading branch information
jbaudet-pass committed Jan 7, 2025
1 parent ea4f6a3 commit cc0d8fc
Show file tree
Hide file tree
Showing 5 changed files with 8 additions and 82 deletions.
16 changes: 4 additions & 12 deletions api/src/pcapi/core/offerers/api.py
Original file line number Diff line number Diff line change
Expand Up @@ -810,10 +810,7 @@ def generate_and_save_api_key(offerer_id: int) -> str:
def generate_offerer_api_key(offerer_id: int) -> tuple[models.ApiKey, str]:
clear_secret = secrets.token_hex(32)
prefix = _generate_api_key_prefix()
if feature.FeatureToggle.WIP_ENABLE_NEW_HASHING_ALGORITHM.is_active():
key = models.ApiKey(offererId=offerer_id, prefix=prefix, secret=crypto.hash_public_api_key(clear_secret))
else:
key = models.ApiKey(offererId=offerer_id, prefix=prefix, secret=crypto.hash_password(clear_secret))
key = models.ApiKey(offererId=offerer_id, prefix=prefix, secret=crypto.hash_public_api_key(clear_secret))

return key, f"{prefix}{API_KEY_SEPARATOR}{clear_secret}"

Expand All @@ -825,14 +822,9 @@ def generate_provider_api_key(provider: providers_models.Provider) -> tuple[mode

clear_secret = secrets.token_hex(32)
prefix = _generate_api_key_prefix()
if feature.FeatureToggle.WIP_ENABLE_NEW_HASHING_ALGORITHM.is_active():
key = models.ApiKey(
offerer=offerer, provider=provider, prefix=prefix, secret=crypto.hash_public_api_key(clear_secret)
)
else:
key = models.ApiKey(
offerer=offerer, provider=provider, prefix=prefix, secret=crypto.hash_password(clear_secret)
)
key = models.ApiKey(
offerer=offerer, provider=provider, prefix=prefix, secret=crypto.hash_public_api_key(clear_secret)
)

return key, f"{prefix}{API_KEY_SEPARATOR}{clear_secret}"

Expand Down
7 changes: 1 addition & 6 deletions api/src/pcapi/core/offerers/factories.py
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,6 @@
from pcapi.core.factories import BaseFactory
import pcapi.core.geography.factories as geography_factories
import pcapi.core.users.factories as users_factories
from pcapi.models import feature
from pcapi.models.validation_status_mixin import ValidationStatus
from pcapi.utils import crypto
from pcapi.utils import siren as siren_utils
Expand Down Expand Up @@ -364,11 +363,7 @@ def _create(
*args: typing.Any,
**kwargs: typing.Any,
) -> models.ApiKey:
if feature.FeatureToggle.WIP_ENABLE_NEW_HASHING_ALGORITHM.is_active():
kwargs["secret"] = crypto.hash_public_api_key(kwargs.get("secret", DEFAULT_SECRET))
else:
kwargs["secret"] = crypto.hash_password(kwargs.get("secret", DEFAULT_SECRET))

kwargs["secret"] = crypto.hash_public_api_key(kwargs.get("secret", DEFAULT_SECRET))
return super()._create(model_class, *args, **kwargs)


Expand Down
8 changes: 3 additions & 5 deletions api/src/pcapi/core/offerers/models.py
Original file line number Diff line number Diff line change
Expand Up @@ -57,7 +57,6 @@
from pcapi.models import db
from pcapi.models.accessibility_mixin import AccessibilityMixin
from pcapi.models.deactivable_mixin import DeactivableMixin
from pcapi.models.feature import FeatureToggle
from pcapi.models.has_address_mixin import HasAddressMixin
from pcapi.models.has_thumb_mixin import HasThumbMixin
from pcapi.models.pc_object import PcObject
Expand Down Expand Up @@ -1071,10 +1070,9 @@ def check_secret(self, clear_text: str) -> bool:
if self.secret.decode("utf-8").startswith("$sha3_512$"):
return crypto.check_public_api_key(clear_text, self.secret)
if crypto.check_password(clear_text, self.secret):
if FeatureToggle.WIP_ENABLE_NEW_HASHING_ALGORITHM.is_active():
self.secret = crypto.hash_public_api_key(clear_text)
db.session.commit()
logger.info("Switched hash of API key from bcrypt to SHA3-512", extra={"key_id": self.id})
self.secret = crypto.hash_public_api_key(clear_text)
db.session.commit()
logger.info("Switched hash of API key from bcrypt to SHA3-512", extra={"key_id": self.id})
return True
return False

Expand Down
2 changes: 0 additions & 2 deletions api/src/pcapi/models/feature.py
Original file line number Diff line number Diff line change
Expand Up @@ -110,7 +110,6 @@ class FeatureToggle(enum.Enum):
WIP_ENABLE_NATIONAL_PROGRAM_NEW_RULES_PUBLIC_API = (
"Activer les nouvelles règles de création et d'édition d'offres collecrives pour l'API publique (collective)"
)
WIP_ENABLE_NEW_HASHING_ALGORITHM = "Activer le nouveau système de hachage des clés publiques d'API"
WIP_ENABLE_OFFER_ADDRESS = "Activer l'association des offres à des adresses."
WIP_SUGGESTED_SUBCATEGORIES = "Activer les sous-catégories suggérées par IA lors de la création d'offre"
WIP_EAN_CREATION = "Activer la création d'offre par EAN"
Expand Down Expand Up @@ -205,7 +204,6 @@ def nameKey(self) -> str:
FeatureToggle.WIP_ENABLE_CLICKHOUSE_IN_BO,
FeatureToggle.WIP_ENABLE_NEW_COLLECTIVE_OFFERS_AND_BOOKINGS_STRUCTURE,
FeatureToggle.WIP_ENABLE_NEW_FINANCE_WORKFLOW,
FeatureToggle.WIP_ENABLE_NEW_HASHING_ALGORITHM,
FeatureToggle.WIP_ENABLE_OFFER_ADDRESS,
FeatureToggle.WIP_ENABLE_PRO_DIDACTIC_ONBOARDING,
FeatureToggle.WIP_ENABLE_REMINDER_MARKETING_MAIL_METADATA_DISPLAY,
Expand Down
57 changes: 0 additions & 57 deletions api/tests/validation/routes/users_authentifications_test.py
Original file line number Diff line number Diff line change
Expand Up @@ -164,40 +164,9 @@ def test_require_active_provider(self, provider_is_active, expected_response_sta

assert response.status_code == expected_response_status

@pytest.mark.parametrize("use_fast_and_insecure_password_hashing_algorithm", [True, False])
@override_features(WIP_ENABLE_NEW_HASHING_ALGORITHM=False)
@patch("pcapi.utils.crypto.check_public_api_key")
@patch("pcapi.utils.crypto.hash_public_api_key")
def test_ff_for_prod_and_testing_env(
self,
hash_public_api_key_function,
check_public_api_key_function,
use_fast_and_insecure_password_hashing_algorithm,
):
with override_settings(
USE_FAST_AND_INSECURE_PASSWORD_HASHING_ALGORITHM=use_fast_and_insecure_password_hashing_algorithm
):
api_key = offerers_factories.ApiKeyFactory()
old_secret = api_key.secret
if use_fast_and_insecure_password_hashing_algorithm:
# md5 hashing
assert not old_secret.decode("utf-8").startswith("$2b$")
assert not old_secret.decode("utf-8").startswith("$sha3_512$")
else:
assert old_secret.decode("utf-8").startswith("$2b$") # testing that the secret is hashed with bcrypt
app = self._make_app()
client = TestClient(app.test_client())
headers = {"Authorization": "Bearer development_prefix_clearSecret"}
response = client.get("/test", headers=headers)
assert response.status_code == 200
assert api_key.secret.decode("utf-8") == old_secret.decode("utf-8")
check_public_api_key_function.assert_not_called()
hash_public_api_key_function.assert_not_called()

@override_settings(
USE_FAST_AND_INSECURE_PASSWORD_HASHING_ALGORITHM=False,
)
@override_features(WIP_ENABLE_NEW_HASHING_ALGORITHM=True)
def test_old_api_key_update(self, db_session):
"""Test that old api key is updated to the new hashing algorithm"""
api_key = offerers_factories.ApiKeyFactory()
Expand All @@ -217,7 +186,6 @@ def test_old_api_key_update(self, db_session):
@override_settings(
USE_FAST_AND_INSECURE_PASSWORD_HASHING_ALGORITHM=False,
)
@override_features(WIP_ENABLE_NEW_HASHING_ALGORITHM=True)
@pytest.mark.parametrize("hashcode", ["$2a$12$hashed_password", "$2x$12$hashed_password", "$2y$12$hashed_password"])
@patch("pcapi.utils.crypto.check_password")
@patch("pcapi.utils.crypto.check_public_api_key")
Expand All @@ -236,7 +204,6 @@ def test_deprecated_hash(self, mock_check_public_api_key, mock_check_password, h
@override_settings(
USE_FAST_AND_INSECURE_PASSWORD_HASHING_ALGORITHM=True,
)
@override_features(WIP_ENABLE_NEW_HASHING_ALGORITHM=True)
def test_unsecure_old_api_key_update(self, db_session):
"""Test that old md5 api key isn't updated to the new hashing algorithm"""
api_key = offerers_factories.ApiKeyFactory()
Expand All @@ -250,27 +217,3 @@ def test_unsecure_old_api_key_update(self, db_session):
assert not api_key.secret.decode("utf-8").startswith("$sha3_512$")
assert check_public_api_key("clearSecret", api_key.secret)
db_session.flush()

@override_settings(
USE_FAST_AND_INSECURE_PASSWORD_HASHING_ALGORITHM=False,
)
def test_switching_ff(self, db_session):
"""Test that the hashing algorithm is updated when the feature flag is switched"""
app = self._make_app()
client = TestClient(app.test_client())
with override_features(WIP_ENABLE_NEW_HASHING_ALGORITHM=False):
api_key = offerers_factories.ApiKeyFactory()
assert api_key.secret.decode("utf-8").startswith("$2b$")

with override_features(WIP_ENABLE_NEW_HASHING_ALGORITHM=True):
headers = {"Authorization": "Bearer development_prefix_clearSecret"}
response = client.get("/test", headers=headers)
assert response.status_code == 200
db_session.refresh(api_key)
assert api_key.secret.decode("utf-8").startswith("$sha3_512$")
assert check_public_api_key("clearSecret", api_key.secret)

with override_features(WIP_ENABLE_NEW_HASHING_ALGORITHM=False):
response = client.get("/test", headers=headers)
assert response.status_code == 200
assert check_public_api_key("clearSecret", api_key.secret)

0 comments on commit cc0d8fc

Please sign in to comment.