alan-turing-institute / data-safe-haven

https://data-safe-haven.readthedocs.io
BSD 3-Clause "New" or "Revised" License
57 stars 15 forks source link

Fix IP address in list #2076

Closed jemrobinson closed 1 month ago

jemrobinson commented 1 month ago

:white_check_mark: Checklist

:vertical_traffic_light: Depends on

n/a

:arrow_heading_up: Summary

Fix logic about determining whether current IP address is allowed.

:closed_umbrella: Related issues

n/a

:microscope: Tests

Tested from an IP that should have been working but wasn't. Added an additional test to catch this.

github-actions[bot] commented 1 month ago

Coverage report

Click to see where and how coverage changed

FileStatementsMissingCoverageCoverage
(new stmts)
Lines missing
  data_safe_haven/commands
  context.py
  pulumi.py
  shm.py
  sre.py 59, 158
  users.py
  data_safe_haven/config
  config_sections.py
  context.py
  context_manager.py
  data_safe_haven/external/api
  azure_sdk.py
  graph_api.py
  data_safe_haven/external/interface
  azure_postgresql_database.py
  data_safe_haven/functions
  network.py
  data_safe_haven/infrastructure
  project_manager.py
  data_safe_haven/infrastructure/programs
  declarative_sre.py
  data_safe_haven/infrastructure/programs/sre
  monitoring.py
  networking.py
  remote_desktop.py
  workspaces.py
  data_safe_haven/serialisers
  yaml_serialisable_model.py
Project Total  

This report was generated by python-coverage-comment-action

jemrobinson commented 1 month ago

As the IP check is duplicated could we put that behaviour in a single function.

It is in a single function. That's what ip_address_in_list() is. The only other bit is the logic that does:

if not ip_address_in_list(my_list):
   raise Exception

which is slightly different in the two cases.

JimMadge commented 1 month ago

Ah, I didn't spot the difference. I think that is fine. Could do a func(verb="deploy") and func(verb="teardown") if we want to be very DRY.