Skip to content

Commit

Permalink
(PC-32153)[API] chore: remove feature flag WIP_ENABLE_NEW_HASHING_ALG…
Browse files Browse the repository at this point in the history
…ORITHM
  • Loading branch information
tconte-pass committed Feb 12, 2025
1 parent 80135da commit 63c50fe
Show file tree
Hide file tree
Showing 11 changed files with 22 additions and 97 deletions.
2 changes: 1 addition & 1 deletion api/.env.development
Original file line number Diff line number Diff line change
Expand Up @@ -117,7 +117,7 @@ TYPEFORM_IMPORT_CHUNK_SIZE=100
TZ=UTC
UBBLE_MOCK_API_URL=https://mock-ubble.ehp.passculture.team/api
UBBLE_MOCK_CONFIG_URL=https://mock-ubble.ehp.passculture.team
USE_FAST_AND_INSECURE_PASSWORD_HASHING_ALGORITHM=1
USE_FAST_AND_INSECURE_PASSWORD_HASHING_ALGORITHM=0
VIRUSTOTAL_BACKEND=pcapi.connectors.virustotal.LoggerBackend
WEBAPP_V2_REDIRECT_URL=http://localhost:5173
WEBAPP_V2_URL=http://localhost:5173
Expand Down
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 @@ -814,10 +814,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 @@ -829,14 +826,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 @@ -370,11 +369,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
7 changes: 3 additions & 4 deletions api/src/pcapi/core/offerers/models.py
Original file line number Diff line number Diff line change
Expand Up @@ -1084,10 +1084,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 @@ -112,7 +112,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_ENABLE_OFFER_ADDRESS_COLLECTIVE = "Activer l'association des offres collectives à des adresses."
WIP_SUGGESTED_SUBCATEGORIES = "Activer les sous-catégories suggérées par IA lors de la création d'offre"
Expand Down Expand Up @@ -210,7 +209,6 @@ def nameKey(self) -> str:
FeatureToggle.WIP_ENABLE_OFFER_ADDRESS_COLLECTIVE,
FeatureToggle.WIP_ENABLE_NEW_COLLECTIVE_OFFERS_AND_BOOKINGS_STRUCTURE,
FeatureToggle.WIP_ENABLE_NEW_FINANCE_WORKFLOW,
FeatureToggle.WIP_ENABLE_NEW_HASHING_ALGORITHM,
FeatureToggle.WIP_ENABLE_PRO_DIDACTIC_ONBOARDING,
FeatureToggle.WIP_ENABLE_PRO_DIDACTIC_ONBOARDING_AB_TEST,
FeatureToggle.WIP_ENABLE_REMINDER_MARKETING_MAIL_METADATA_DISPLAY,
Expand Down
8 changes: 5 additions & 3 deletions api/tests/core/offerers/test_api.py
Original file line number Diff line number Diff line change
Expand Up @@ -529,16 +529,18 @@ def test_generate_and_save_api_key(self):
assert found_api_key.offerer == offerer

def test_get_provider_from_api_key(self):
value = "a very secret legacy key"
value = "development_prefix_a very secret key"
offerer = offerers_factories.OffererFactory()
provider = providers_factories.ProviderFactory(localClass=None, name="RiotRecords")
providers_factories.OffererProviderFactory(offerer=offerer, provider=provider)
offerers_factories.ApiKeyFactory(
offerer=offerer, provider=provider, prefix="development_a very s", secret="ecret legacy key"
offerer=offerer, provider=provider, prefix="development_prefix", secret="a very secret key"
)

with assert_num_queries(1):
found_api_key = offerers_api.find_api_key(value)
assert found_api_key.provider == provider

assert found_api_key.provider == provider

def test_legacy_api_key(self):
value = "a very secret legacy key"
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -17,7 +17,6 @@ class GetEventsTest(PublicAPIVenueEndpointHelper):
endpoint_method = "get"

num_queries_with_error = 1 # select api_key, offerer and provider
num_queries_with_error += 1 # select features
num_queries_with_error += 1 # check provider EXISTS
num_queries = num_queries_with_error + 1 # select offers

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -15,7 +15,6 @@ class GetProductsTest(PublicAPIVenueEndpointHelper):
endpoint_method = "get"

num_queries_400 = 1 # select api_key, offerer and provider
num_queries_400 += 1 # select features
num_queries_404 = num_queries_400 + 1 # check venue_provider exists
num_queries_success = num_queries_404 + 1 # select offer

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -385,15 +385,14 @@ def test_update_name_and_description(self, client):
new_name = product_offer.name + " updated"
new_desc = product_offer.description + " updated"

# 1. get api key
# 2. check FF
# 3. get offer and related data
# 4. select oa
# 5. update offer
# 6. reload provider
# 7. reload offer and related data (before serialization)
# 8. check venue offerer address
with assert_num_queries(8):
expected_num_queries = 1 # get api key
expected_num_queries += 1 # get offer and related data
expected_num_queries += 1 # select oa
expected_num_queries += 1 # update offer
expected_num_queries += 1 # reload provider
expected_num_queries += 1 # reload offer and related data (before serialization)
expected_num_queries += 1 # check venue offerer address
with assert_num_queries(expected_num_queries):
response = client.with_explicit_token(offerers_factories.DEFAULT_CLEAR_API_KEY).patch(
"/public/offers/v1/products",
json={"offerId": offer_id, "name": new_name, "description": new_desc},
Expand Down
1 change: 0 additions & 1 deletion api/tests/utils/crypto_test.py
Original file line number Diff line number Diff line change
Expand Up @@ -27,7 +27,6 @@ def test_no_password_leak(self):
assert "user@AZERTY123" not in str(exception), str(exception)


@pytest.mark.settings(USE_FAST_AND_INSECURE_PASSWORD_HASHING_ALGORITHM=False)
class ProdEnvironmentPasswordHasherTest:
def test_hash_password_uses_bcrypt(self):
hashed = crypto.hash_password("secret")
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 @@ -162,40 +162,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])
@pytest.mark.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,
settings,
):
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()

@pytest.mark.settings(
USE_FAST_AND_INSECURE_PASSWORD_HASHING_ALGORITHM=False,
)
@pytest.mark.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 @@ -215,7 +184,6 @@ def test_old_api_key_update(self, db_session):
@pytest.mark.settings(
USE_FAST_AND_INSECURE_PASSWORD_HASHING_ALGORITHM=False,
)
@pytest.mark.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 @@ -234,7 +202,6 @@ def test_deprecated_hash(self, mock_check_public_api_key, mock_check_password, h
@pytest.mark.settings(
USE_FAST_AND_INSECURE_PASSWORD_HASHING_ALGORITHM=True,
)
@pytest.mark.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 @@ -248,27 +215,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()

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

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)

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 63c50fe

Please sign in to comment.