diff --git a/.env.example b/.env.example index 9952ccf..1e15a55 100644 --- a/.env.example +++ b/.env.example @@ -6,6 +6,8 @@ GITHUB_APP_CLIENT_ID="" GITHUB_APP_CLIENT_SECRET="" # Token for background jobs (a personal access token with read access to the repos) GITHUB_TOKEN="" +GITHUB_ORG="" +GITHUB_TEAM="" # Database config DB_NAME= DB_USER= diff --git a/README.md b/README.md index 215ed63..dd883fc 100644 --- a/README.md +++ b/README.md @@ -4,6 +4,12 @@ A deployment orchestrator for GitHub-hosted projects. drawing +## Login + +Users log in with GitHub, and only active members of one GitHub team are let in. Set the team with `GITHUB_ORG` and `GITHUB_TEAM` (the team's slug from its URL, not its display name). If either is missing, nobody can log in. + +Membership is checked once at login, so someone removed from the team keeps access until their session ends. + ## Database setup & migrations `make create_db && make upgrade_db` should be all that's needed to get started. Those two commands will create the database if it does not exist and run the migrations. diff --git a/helm/makeitso/templates/deployment.yaml b/helm/makeitso/templates/deployment.yaml index ef2e9b1..bf20e6a 100644 --- a/helm/makeitso/templates/deployment.yaml +++ b/helm/makeitso/templates/deployment.yaml @@ -58,6 +58,10 @@ spec: secretKeyRef: name: {{ .Values.app.githubAppSecretName | quote }} key: ghWebhookSecret + - name: GITHUB_ORG + value: {{ .Values.app.githubOrg | quote }} + - name: GITHUB_TEAM + value: {{ .Values.app.githubTeam | quote }} readinessProbe: httpGet: path: /healthz diff --git a/helm/makeitso/values.yaml b/helm/makeitso/values.yaml index dfde01e..7bc953b 100644 --- a/helm/makeitso/values.yaml +++ b/helm/makeitso/values.yaml @@ -6,6 +6,8 @@ app: env: prod githubAppSecretName: makeitso-github-app-secret + githubOrg: bcgov + githubTeam: cas-developers # Runs deploys deployWorker: diff --git a/src/makeitso/auth/routes.py b/src/makeitso/auth/routes.py index e7e4f94..00f4477 100644 --- a/src/makeitso/auth/routes.py +++ b/src/makeitso/auth/routes.py @@ -1,5 +1,5 @@ from authlib.integrations.base_client import OAuthError -from flask import Blueprint, current_app, redirect, render_template, session, url_for +from flask import Blueprint, current_app, flash, redirect, render_template, session, url_for from makeitso.extensions import oauth @@ -24,11 +24,33 @@ def authorize(): response.raise_for_status() user_info = response.json() + + org = current_app.config["GITHUB_ORG"] + team = current_app.config["GITHUB_TEAM"] + if not _is_team_member(org, team, user_info["login"]): + session.clear() + flash(f"You need to be a member of the {org}/{team} GitHub team to log in", "danger") + return redirect("/") + session["user"] = user_info return redirect("/") +def _is_team_member(org: str | None, team: str | None, username: str) -> bool: + # Fail closed if the team isn't configured + if not org or not team: + return False + + response = oauth.github.get(f"orgs/{org}/teams/{team}/memberships/{username}") + # 404 means not a member, or the token can't see the team + if response.status_code != 200: + return False + + # Pending invites don't count + return response.json().get("state") == "active" + + @bp.route("/login") def login(): scheme = "https" if current_app.config["SESSION_COOKIE_SECURE"] else "http" diff --git a/src/makeitso/config.py b/src/makeitso/config.py index 557d900..f82b4c3 100644 --- a/src/makeitso/config.py +++ b/src/makeitso/config.py @@ -30,6 +30,9 @@ class Config: # Used by background jobs, which have no logged-in user GITHUB_TOKEN = os.environ.get("GITHUB_TOKEN") GITHUB_WEBHOOK_SECRET = os.environ.get("GITHUB_WEBHOOK_SECRET") + # Only members of this team can log in + GITHUB_ORG = os.environ.get("GITHUB_ORG") + GITHUB_TEAM = os.environ.get("GITHUB_TEAM") # Where deploys check out code; Helm mounts /workspace DEPLOY_WORKSPACE = os.environ.get("DEPLOY_WORKSPACE", "/tmp/makeitso-workspace") diff --git a/src/makeitso/extensions.py b/src/makeitso/extensions.py index 13526ef..5277305 100644 --- a/src/makeitso/extensions.py +++ b/src/makeitso/extensions.py @@ -22,5 +22,6 @@ authorize_url="https://github.com/login/oauth/authorize", authorize_params=None, api_base_url="https://api.github.com/", - client_kwargs={"scope": "openid email profile"}, + # read:org is needed to check team membership at login + client_kwargs={"scope": "read:user read:org"}, ) diff --git a/src/makeitso/static/css/theme.css b/src/makeitso/static/css/theme.css index 6a1da55..f261e2f 100644 --- a/src/makeitso/static/css/theme.css +++ b/src/makeitso/static/css/theme.css @@ -14,7 +14,9 @@ font-display: swap; } -:root { +/* Also on [data-bs-theme=light], where Bootstrap resets its colours to the defaults */ +:root, +[data-bs-theme="light"] { --bs-body-font-family: "BC Sans", "Noto Sans", system-ui, -apple-system, "Segoe UI", sans-serif; --bc-primary-blue: #003366; --bc-bg-blue: #38598a; diff --git a/src/makeitso/templates/base.html b/src/makeitso/templates/base.html index 9260928..c441e51 100644 --- a/src/makeitso/templates/base.html +++ b/src/makeitso/templates/base.html @@ -25,13 +25,23 @@ {% block navbar_actions %}{% endblock %} {% if session.user %} {{ render_icon("activity") }} Jobs -
- User Avatar - {{ session.user.login }}@github -
- - -
+
+ +
+
+ +
+ {% if session.user.name %}
{{ session.user.name }}
{% endif %} + @{{ session.user.login }} +
+
+
+ + +
+
{# The landing page has its own big login button #} {% elif request.endpoint != "main.index" %} diff --git a/tests/auth/test_routes.py b/tests/auth/test_routes.py new file mode 100644 index 0000000..87903fd --- /dev/null +++ b/tests/auth/test_routes.py @@ -0,0 +1,108 @@ +from unittest.mock import MagicMock, patch +from urllib.parse import parse_qs, urlparse + +import pytest +from authlib.integrations.base_client import OAuthError + +USER = {"login": "octocat", "avatar_url": ""} + + +def _response(status_code, json=None): + response = MagicMock(status_code=status_code) + response.json.return_value = json or {} + return response + + +class TestLogin: + def test_asks_github_for_org_access(self, client): + response = client.get("/login") + + assert response.status_code == 302 + location = urlparse(response.location) + assert location.netloc == "github.com" + assert parse_qs(location.query)["scope"] == ["read:user read:org"] + + +class TestLogout: + def test_clears_session(self, client, login): + login() + + response = client.post("/logout") + + assert response.status_code == 302 + assert response.location == "/" + with client.session_transaction() as session: + assert "user" not in session + assert "token" not in session + + +class TestAuthorize: + @pytest.fixture(autouse=True) + def team(self, app, monkeypatch): + monkeypatch.setitem(app.config, "GITHUB_ORG", "bcgov") + monkeypatch.setitem(app.config, "GITHUB_TEAM", "makeitso-users") + + # Fakes the GitHub OAuth client: GET /user, then the team membership call + @pytest.fixture + def github(self): + with patch("makeitso.auth.routes.oauth") as oauth: + oauth.github.authorize_access_token.return_value = {"access_token": "t"} + yield oauth.github + + @staticmethod + def _authorize(client, github, membership): + github.get.side_effect = [_response(200, USER), membership] + return client.get("/authorize") + + def test_member_logs_in(self, client, github): + response = self._authorize(client, github, _response(200, {"state": "active"})) + + assert response.status_code == 302 + assert response.location == "/" + github.get.assert_called_with("orgs/bcgov/teams/makeitso-users/memberships/octocat") + with client.session_transaction() as session: + assert session["user"] == USER + + @pytest.mark.parametrize( + "membership", + [_response(404), _response(200, {"state": "pending"})], + ids=["not-a-member", "pending-invite"], + ) + def test_non_member_is_sent_back_with_a_message(self, client, github, membership): + response = self._authorize(client, github, membership) + + assert response.status_code == 302 + assert response.location == "/" + with client.session_transaction() as session: + assert "user" not in session + assert "token" not in session + + def test_non_member_sees_message_on_landing_page(self, client, github): + github.get.side_effect = [_response(200, USER), _response(404)] + + response = client.get("/authorize", follow_redirects=True) + + assert response.request.path == "/" + assert b"member of the bcgov/makeitso-users GitHub team" in response.data + assert b"Log in with GitHub" in response.data + + def test_oauth_error_shows_error_page(self, client, github): + github.authorize_access_token.side_effect = OAuthError(description="bad code") + + response = client.get("/authorize") + + assert b"bad code" in response.data + with client.session_transaction() as session: + assert "user" not in session + + def test_denied_when_team_not_configured(self, app, client, github, monkeypatch): + monkeypatch.setitem(app.config, "GITHUB_TEAM", None) + github.get.return_value = _response(200, USER) + + response = client.get("/authorize") + + assert response.status_code == 302 + with client.session_transaction() as session: + assert "user" not in session + # Never asks GitHub about membership + assert github.get.call_count == 1 diff --git a/tests/main/test_routes.py b/tests/main/test_routes.py index 6b72cb7..48a7eaf 100644 --- a/tests/main/test_routes.py +++ b/tests/main/test_routes.py @@ -1,6 +1,31 @@ from tests.factories import StackFactory +class TestNavbar: + def test_logged_in_shows_account_menu(self, client): + with client.session_transaction() as session: + session["user"] = { + "login": "octocat", + "name": "Mona Lisa", + "avatar_url": "https://avatars.example/octocat", + "html_url": "https://github.com/octocat", + } + + response = client.get("/") + + assert b'src="https://avatars.example/octocat"' in response.data + assert b"Mona Lisa" in response.data + assert b'href="https://github.com/octocat"' in response.data + assert b"@octocat" in response.data + assert b'action="/logout"' in response.data + + def test_logged_out_has_no_account_menu(self, client): + response = client.get("/") + + assert b"Account menu" not in response.data + assert b'action="/logout"' not in response.data + + class TestHealthz: def test_ok(self, client): response = client.get("/healthz")