From 10306eba8a17113566588306121909ed63b1cf7e Mon Sep 17 00:00:00 2001 From: msramalho <19508417+msramalho@users.noreply.github.com> Date: Thu, 31 Oct 2024 14:52:14 +0000 Subject: [PATCH] refactors user-groups setup and auth logic --- README.md | 5 +- src/.env.alembic | 5 + src/core/config.py | 2 +- src/core/events.py | 8 +- src/db/crud.py | 144 ++++++++++++------ src/db/models.py | 5 + ...a012ec405b8_add_columns_to_groups_table.py | 42 +++++ src/shared/settings.py | 1 + src/tests/db/test_crud.py | 30 +++- src/tests/user-groups.test.yaml | 36 ++++- 10 files changed, 216 insertions(+), 62 deletions(-) create mode 100644 src/.env.alembic create mode 100644 src/migrations/versions/fa012ec405b8_add_columns_to_groups_table.py diff --git a/README.md b/README.md index 102fd2d..5cd1ee6 100644 --- a/README.md +++ b/README.md @@ -46,8 +46,9 @@ orchestrators: check https://alembic.sqlalchemy.org/en/latest/tutorial.html#the-migration-environment * create migrations with `alembic revision -m "create account table"` -* migrate to most recent with `alembic upgrade head` -* downgrade with `alembic downgrade -1` +* if running in the normal pipenv environment use `PIPENV_DOTENV_LOCATION=.env.alembic pipenv run` followed by: + * migrate to most recent with `alembic upgrade head` + * downgrade with `alembic downgrade -1` ## Release Update `main.py:VERSION`. diff --git a/src/.env.alembic b/src/.env.alembic new file mode 100644 index 0000000..a53af2a --- /dev/null +++ b/src/.env.alembic @@ -0,0 +1,5 @@ +CHROME_APP_IDS='["1234567890"]' +ALLOWED_ORIGINS='["allowed"]' +BLOCKED_EMAILS='[]' +DATABASE_PATH="sqlite:///./auto-archiver.db" +API_BEARER_TOKEN=THIS_API_TOKEN_SHOULD_NEVER_BE_USED \ No newline at end of file diff --git a/src/core/config.py b/src/core/config.py index d3cba7a..dcfd135 100644 --- a/src/core/config.py +++ b/src/core/config.py @@ -1,4 +1,4 @@ -VERSION = "0.7.2" +VERSION = "0.8.0" API_DESCRIPTION = """ #### API for the Auto-Archiver project, a tool to archive web pages and Google Sheets. diff --git a/src/core/events.py b/src/core/events.py index c5cde2c..e9bbfbc 100644 --- a/src/core/events.py +++ b/src/core/events.py @@ -23,8 +23,9 @@ async def lifespan(app: FastAPI): # disabling uvicorn logger since we use loguru in logging_middleware logging.getLogger("uvicorn.access").disabled = True asyncio.create_task(redis_subscribe_worker_exceptions(get_settings().REDIS_EXCEPTIONS_CHANNEL, get_settings().CELERY_BROKER_URL)) - asyncio.create_task(refresh_user_groups()) asyncio.create_task(repeat_measure_regular_metrics()) + with get_db() as db: + crud.upsert_user_groups(db) yield # separates startup from shutdown instructions @@ -34,11 +35,6 @@ async def lifespan(app: FastAPI): # CRON JOBS -@repeat_every(seconds=60 * 60) # 1 hour -async def refresh_user_groups(): - with get_db() as db: - crud.upsert_user_groups(db) - @repeat_every(seconds=get_settings().REPEAT_COUNT_METRICS_SECONDS) async def repeat_measure_regular_metrics(): diff --git a/src/db/crud.py b/src/db/crud.py index 4d03318..7f9c67f 100644 --- a/src/db/crud.py +++ b/src/db/crud.py @@ -1,3 +1,4 @@ +from collections import defaultdict from functools import cache from sqlalchemy.orm import Session, load_only from sqlalchemy import Column, or_, func @@ -9,15 +10,15 @@ from shared.settings import get_settings from . import models, schemas import yaml -DOMAIN_GROUPS = {} -DOMAIN_GROUPS_LOADED = False DATABASE_QUERY_LIMIT = get_settings().DATABASE_QUERY_LIMIT # --------------- TASK = Archive -def get_limit(user_limit:int): + +def get_limit(user_limit: int): return max(1, min(user_limit, DATABASE_QUERY_LIMIT)) + def get_archive(db: Session, id: str, email: str): email = email.lower() query = base_query(db).filter(models.Archive.id == id) @@ -76,9 +77,11 @@ def count_archives(db: Session): def count_archive_urls(db: Session): return db.query(func.count(models.ArchiveUrl.url)).scalar() + def count_users(db: Session): return db.query(func.count(models.User.email)).scalar() + def count_by_user_since(db: Session, seconds_delta: int = 15): time_threshold = datetime.now() - timedelta(seconds=seconds_delta) return db.query(models.Archive.author_id, func.count().label('total'))\ @@ -89,7 +92,7 @@ def count_by_user_since(db: Session, seconds_delta: int = 15): def base_query(db: Session): - #NOTE: load_only is for optimization and not obfuscation, use .with_entities() if needed + # NOTE: load_only is for optimization and not obfuscation, use .with_entities() if needed return db.query(models.Archive)\ .filter(models.Archive.deleted == False)\ .options(load_only(models.Archive.id, models.Archive.created_at, models.Archive.url, models.Archive.result)) @@ -106,14 +109,17 @@ def create_tag(db: Session, tag: str): db.refresh(db_tag) return db_tag + def is_active_user(db: Session, email: str) -> bool: email = email.lower() - if "@" not in email: return False - global DOMAIN_GROUPS, DOMAIN_GROUPS_LOADED - if not DOMAIN_GROUPS_LOADED: upsert_user_groups(db) + if not email or not len(email) or "@" not in email: return False domain = email.split('@')[1] - if domain in DOMAIN_GROUPS: return True - return len(email) and db.query(models.User).filter(models.User.email == email, models.User.is_active == True).first() is not None + + explicitly_active = db.query(models.User).filter(models.User.email == email, models.User.is_active == True).first() is not None + if explicitly_active: return True + + return db.query(models.Group).filter(models.Group.domains.contains(domain)).first() is not None + def is_user_in_group(db: Session, group_name: str, email: str) -> models.Group: if email == ALLOW_ANY_EMAIL: return True @@ -121,16 +127,23 @@ def is_user_in_group(db: Session, group_name: str, email: str) -> models.Group: def get_user_groups(db: Session, email: str): + """ + given an email retrieves the user groups from the DB and then the email-domain groups from a global variable, the email does not need to belong to an existing user. User does not need to be active. + """ + if not email or not len(email) or "@" not in email: return [] email = email.lower() - if "@" not in email: return [] - global DOMAIN_GROUPS, DOMAIN_GROUPS_LOADED - if not DOMAIN_GROUPS_LOADED: upsert_user_groups(db) - # given an email retrieves the user groups from the DB and then the email-domain groups from a global variable - groups = db.query(models.association_table_user_groups).filter_by(user_id=email).with_entities(Column("group_id")).all() - user_level_groups = [g[0] for g in groups] - domain_level_groups = DOMAIN_GROUPS.get(email.split('@')[1], []) - logger.success(f"EMAIL {email} has {user_level_groups=} and {domain_level_groups=}") - return list(set(user_level_groups) | set(domain_level_groups)) + + # get user groups + user_groups = db.query(models.association_table_user_groups).filter_by(user_id=email).with_entities(Column("group_id")).all() + user_level_groups = [g[0] for g in user_groups] + + # get domain groups + domain = email.split('@')[1] + domain_level_groups = db.query(models.Group.id).filter(models.Group.domains.contains(domain)).with_entities(Column("id")).all() + domain_level_groups = [g[0] for g in domain_level_groups] + + # combine and return + return list(set(user_level_groups + domain_level_groups)) # --------------- INIT User-Groups @@ -147,19 +160,36 @@ def create_or_get_user(db: Session, author_id: str, is_active: bool = models.Use return db_user -@cache -def create_or_get_group(db: Session, group_name: str) -> models.Group: +def upsert_group(db: Session, group_name: str, description: str, orchestrator: str, orchestrator_sheet: str, permissions: dict, domains: list) -> models.Group: db_group = db.query(models.Group).filter(models.Group.id == group_name).first() if db_group is None: - db_group = models.Group(id=group_name) + db_group = models.Group(id=group_name, description=description, orchestrator=orchestrator, orchestrator_sheet=orchestrator_sheet, permissions=permissions, domains=domains) db.add(db_group) - db.commit() - db.refresh(db_group) + else: + db_group.description = description + db_group.orchestrator = orchestrator + db_group.orchestrator_sheet = orchestrator_sheet + db_group.permissions = permissions + db_group.domains = domains + db.commit() + db.refresh(db_group) return db_group +def upsert_user(db: Session, email: str, active: bool): + db_user = db.query(models.User).filter(models.User.email == email).first() + if db_user is None: + db_user = models.User(email=email, is_active=active) + db.add(db_user) + else: + db_user.is_active = active + db.commit() + return db_user + + def upsert_user_groups(db: Session): - global DOMAIN_GROUPS, DOMAIN_GROUPS_LOADED + def display_email_pii(email: str): + return f"'{email[0:3]}...@{email.split('@')[1]}'" """ reads the user_groups yaml file and inserts any new users, groups, along with new participation of users in groups @@ -174,32 +204,56 @@ def upsert_user_groups(db: Session): except Exception as e: logger.error(f"could not open user groups filename {filename}: {e}") raise e - # updating domain->groups access - DOMAIN_GROUPS = user_groups_yaml.get("domains", {}) - # upserting in DB - user_groups = user_groups_yaml.get("users", {}) - logger.debug(f"Found {len(user_groups)} users.") + # delete all user-groups relationships db.query(models.association_table_user_groups).delete() # set all users to inactive db.query(models.User).update({models.User.is_active: False}) - for user_email, groups in user_groups.items(): - user_email = user_email.lower() - assert '@' in user_email, f'Invalid user email {user_email}' - logger.info(f"email='{user_email[0:3]}...{user_email[-8:]}', {groups=}") - db_user = db.query(models.User).filter(models.User.email == user_email).first() - if db_user is None: - db_user = models.User(email=user_email, is_active=True) - db.add(db_user) - else: - db_user.is_active = True - if not groups: continue # avoid hanging in for x in None: - for group in groups: - db_group = create_or_get_group(db, group) - db_group.users.append(db_user) + + # create a map of group_id -> domains and another of domain -> groups + group_domains = defaultdict(set) + domain_groups = defaultdict(list) + for domain, explicit_groups in user_groups_yaml.get("domains", {}).items(): + domain_groups[domain] = list(set(explicit_groups)) + for group in explicit_groups: + group_domains[group].add(domain) + + # upsert groups and save a map of groupid -> dbobject + for group_id, g in user_groups_yaml.get("groups", {}).items(): + upsert_group(db, group_id, g["description"], g["orchestrator"], g["orchestrator_sheet"], g["permissions"], list(group_domains.get(group_id, []))) + db_groups: dict[str, models.Group] = {g.id: g for g in db.query(models.Group).all()} + + # integrity checks + for group_in_domains in group_domains: + if group_in_domains not in db_groups: + logger.error(f"[CONFIG] Group '{group_in_domains}' does not exist in the database: domains setting will not work.") + if group_in_domains not in user_groups_yaml.get("groups", {}): + logger.error(f"[CONFIG] Group '{group_in_domains}' does not exist in the config file: domains setting will not work.") + + # reinsert users in their EXPLICITLY DEFINED groups + # domain groups are check live, as there may be new users that are not explicitly registered but belong to a domain + for email, explicit_groups in user_groups_yaml.get("users", {}).items(): + explicit_groups = explicit_groups or [] + email = email.lower().strip() + if '@' not in email: + logger.error(f'[CONFIG] Invalid user email {email}, skipping.') + continue + + logger.info(f"{display_email_pii(email)} => {explicit_groups}") + + # upsert active user + db_user = upsert_user(db, email, active=True) + + # connect users to groups + for group_id in explicit_groups: + if group_id not in db_groups: + logger.error(f"[CONFIG] Group {group_id} does not exist in config file, skipping for email={display_email_pii(email)}.") + continue + db_groups[group_id].users.append(db_user) db.commit() count_user_groups = db.query(models.association_table_user_groups).count() - logger.success(f"Completed refresh, now: {count_user_groups} user-groups relationships.") - DOMAIN_GROUPS_LOADED = True + count_groups = db.query(func.count(models.Group.id)).scalar() + + logger.success(f"[CONFIG] DONE: [users={count_users(db)}, groups={count_groups}, explicit user groups={count_user_groups}].") diff --git a/src/db/models.py b/src/db/models.py index 2718687..193adba 100644 --- a/src/db/models.py +++ b/src/db/models.py @@ -74,6 +74,11 @@ class Group(Base): __tablename__ = "groups" id = Column(String, primary_key=True, index=True) + description = Column(String, default=None) + orchestrator = Column(String, default=None) + orchestrator_sheet = Column(String, default=None) + permissions = Column(JSON, default=None) + domains = Column(JSON, default=[]) archives = relationship("Archive", back_populates="group") users = relationship("User", back_populates="groups", secondary=association_table_user_groups) \ No newline at end of file diff --git a/src/migrations/versions/fa012ec405b8_add_columns_to_groups_table.py b/src/migrations/versions/fa012ec405b8_add_columns_to_groups_table.py new file mode 100644 index 0000000..da77d41 --- /dev/null +++ b/src/migrations/versions/fa012ec405b8_add_columns_to_groups_table.py @@ -0,0 +1,42 @@ +"""add columns to groups table + +Revision ID: fa012ec405b8 +Revises: 93a611e4c066 +Create Date: 2024-10-31 09:36:50.360710 + +""" +from alembic import op +import sqlalchemy as sa +from sqlalchemy.engine.reflection import Inspector + + +# revision identifiers, used by Alembic. +revision = 'fa012ec405b8' +down_revision = '93a611e4c066' +branch_labels = None +depends_on = None + + +def upgrade() -> None: + conn = op.get_bind() + inspector = Inspector.from_engine(conn) + columns = [col['name'] for col in inspector.get_columns('groups')] + + if 'description' not in columns: + op.add_column('groups', sa.Column('description', sa.String(), nullable=True)) + if 'orchestrator' not in columns: + op.add_column('groups', sa.Column('orchestrator', sa.String(), nullable=True)) + if 'orchestrator_sheet' not in columns: + op.add_column('groups', sa.Column('orchestrator_sheet', sa.String(), nullable=True)) + if 'permissions' not in columns: + op.add_column('groups', sa.Column('permissions', sa.JSON(), nullable=True)) + if 'domains' not in columns: + op.add_column('groups', sa.Column('domains', sa.JSON(), nullable=True)) + + +def downgrade() -> None: + op.drop_column('groups', 'description') + op.drop_column('groups', 'orchestrator') + op.drop_column('groups', 'orchestrator_sheet') + op.drop_column('groups', 'permissions') + op.drop_column('groups', 'domains') diff --git a/src/shared/settings.py b/src/shared/settings.py index 56b9557..8fa5ae5 100644 --- a/src/shared/settings.py +++ b/src/shared/settings.py @@ -30,6 +30,7 @@ class Settings(BaseSettings): API_BEARER_TOKEN: Annotated[str, Len(min_length=20)] ALLOWED_ORIGINS: Annotated[set[str], Len(min_length=1)] CHROME_APP_IDS: Annotated[set[Annotated[str, Len(min_length=10)]], Len(min_length=1)] + #TODO: deprecate blocklist BLOCKED_EMAILS: Annotated[Set[str], Len(min_length=0)] = set() @lru_cache diff --git a/src/tests/db/test_crud.py b/src/tests/db/test_crud.py index f293cf8..6f838df 100644 --- a/src/tests/db/test_crud.py +++ b/src/tests/db/test_crud.py @@ -300,8 +300,11 @@ def test_is_active_user(test_data, db_session): assert crud.is_active_user(db_session, "") == False assert crud.is_active_user(db_session, "example.com") == False assert crud.is_active_user(db_session, "unknown@example.com") == True + assert crud.is_active_user(db_session, "ANYONE@example.com") == True + assert crud.is_active_user(db_session, "ANYONE@birdy.com") == True assert crud.is_active_user(db_session, "rick@example.com") == True assert crud.is_active_user(db_session, "RICK@example.com") == True + assert crud.is_active_user(db_session, "summer@herself.com") == True assert crud.is_active_user(db_session, "rick@not-in-groups.com") == False @@ -317,6 +320,7 @@ def test_is_user_in_group(test_data, db_session): ("rick@example.com", "spaceship", True), ("rick@example.com", "SPACESHIP", False), ("RICK@example.com", "interdimensional", True), + ("rick@example.com", "animated-characters", True), ("rick@example.com", "the-jerrys-club", False), ("morty@example.com", "spaceship", True), @@ -325,17 +329,22 @@ def test_is_user_in_group(test_data, db_session): ("jerry@example.com", "spaceship", False), ("jerry@example.com", "interdimensional", False), - ("jerry@example.com", "the-jerrys-club", True), + ("jerry@example.com", "the-jerrys-club", False), # group not in 'groups' ("rick@example.com", "animated-characters", True), ("morty@example.com", "animated-characters", True), ("jerry@example.com", "animated-characters", True), + ("ANYONE@example.com", "animated-characters", True), + ("ANYONE@birdy.com", "animated-characters", True), + + ("summer@herself.com", "animated-characters", False), ("rick@example.com", "", False), ("", "spaceship", False), ("BADEMAILexample.com", "spaceship", False), ] for email, group, expected in test_pairs: + print(f"{email} in {group} == {expected}") assert crud.is_user_in_group(db_session, group, email) == expected @@ -362,22 +371,29 @@ def test_create_or_get_user(test_data, db_session): assert db_session.query(models.User).count() == 6 -def test_get_group(test_data, db_session): +def test_upsert_group(test_data, db_session): from db import crud assert db_session.query(models.Group).count() == 3 - assert (g1 := crud.create_or_get_group(db_session, "spaceship")) is not None + repeatable_params = ["desc 1", "orch.yaml", "sheet.yaml", {"read": ["all"]}, ["example.com"]] + + assert (g1 := crud.upsert_group(db_session, "spaceship", *repeatable_params)) is not None assert g1.id == "spaceship" + assert g1.description == "desc 1" + assert g1.orchestrator == "orch.yaml" + assert g1.orchestrator_sheet == "sheet.yaml" + assert g1.permissions == {"read": ["all"]} + assert g1.domains == ["example.com"] assert len(g1.users) == 2 assert [u.email for u in g1.users] == ["rick@example.com", "morty@example.com"] - assert (g2 := crud.create_or_get_group(db_session, "the-jerrys-club")) is not None - assert g2.id == "the-jerrys-club" + assert (g2 := crud.upsert_group(db_session, "interdimensional", *repeatable_params)) is not None + assert g2.id == "interdimensional" assert len(g2.users) == 1 - assert g2.users[0].email == "jerry@example.com" + assert [u.email for u in g2.users] == ["rick@example.com"] - assert (g3 := crud.create_or_get_group(db_session, "this-is-a-new-group")) is not None + assert (g3 := crud.upsert_group(db_session, "this-is-a-new-group", *repeatable_params)) is not None assert g3.id == "this-is-a-new-group" assert len(g3.users) == 0 diff --git a/src/tests/user-groups.test.yaml b/src/tests/user-groups.test.yaml index 545b38c..612f09d 100644 --- a/src/tests/user-groups.test.yaml +++ b/src/tests/user-groups.test.yaml @@ -7,13 +7,47 @@ users: - spaceship jerry@example.com: - the-jerrys-club - birdman@example.com: + summer@herself.com: + badyemail.com: domains: example.com: - animated-characters + birdy.com: + - animated-characters + - this-does-not-exist + orchestrators: spaceship: tests/orchestration.test.yaml interdimensional: tests/orchestration.test.yaml default: tests/orchestration.test.yaml + +groups: + spaceship: + description: "The spaceship crew" + orchestrator: tests/orchestration.test.yaml + orchestrator_sheet: tests/orchestration.test.yaml + permissions: + read: ["all"] + active_sheets: -1 + monthly_urls: all + monthly_mbs: all + interdimensional: + description: "Interdimensional travelers" + orchestrator: tests/orchestration.test.yaml + orchestrator_sheet: tests/orchestration.test.yaml + permissions: + read: ["interdimensional", "animated-characters"] + active_sheets: 5 + monthly_urls: 1000 + monthly_mbs: 1000 + animated-characters: + description: "Animated characters" + orchestrator: tests/orchestration.test.yaml + orchestrator_sheet: tests/orchestration.test.yaml + permissions: + read: ["animated-characters"] + active_sheets: -1 + monthly_urls: all + monthly_mbs: all \ No newline at end of file