Skip to content

Commit 9471985

Browse files
committed
Better cookie handling, login_max_age setting, closes #80
1 parent 90d73c7 commit 9471985

3 files changed

Lines changed: 172 additions & 13 deletions

File tree

‎README.md‎

Lines changed: 17 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -45,7 +45,7 @@ Now you can start Datasette like this, passing in the secrets as environment var
4545

4646
Note that hard-coding secrets in `metadata.json` is a bad idea as they will be visible to anyone who can navigate to `/-/metadata`. Instead, we use Datasette's mechanism for [adding secret plugin configuration options](https://docs.datasette.io/en/stable/plugins.html#secret-configuration-values).
4747

48-
By default anonymous users will still be able to interact with Datasette. If you wish all users to have to sign in with a GitHub account first, add this to your ``metadata.json``:
48+
By default anonymous users will still be able to interact with Datasette. If you wish all users to have to sign in with a GitHub account first, add this to your Datasette configuration:
4949

5050
```json
5151
{
@@ -59,6 +59,21 @@ By default anonymous users will still be able to interact with Datasette. If you
5959
}
6060
}
6161
```
62+
63+
By default, signing in creates a cookie that stays valid for **30 days**. Set `login_max_age` to a positive integer number of seconds to change that login duration. For 24 hours, use 24 * 60 * 60 = 86400:
64+
65+
```json
66+
{
67+
"plugins": {
68+
"datasette-auth-github": {
69+
"client_id": {"$env": "GITHUB_CLIENT_ID"},
70+
"client_secret": {"$env": "GITHUB_CLIENT_SECRET"},
71+
"login_max_age": 86400
72+
}
73+
}
74+
}
75+
```
76+
6277
## The authenticated actor
6378

6479
Visit `/-/actor` when signed in to see the shape of the authenticated actor. It should look something like this:
@@ -159,7 +174,7 @@ If your organization is [arranged into teams](https://help.github.com/en/article
159174

160175
A user's organization and team memberships are checked once, when they first sign in. Those teams and organizations are then persisted in the user's signed `ds_actor` cookie.
161176

162-
This means that if a user is removed from an organization or team but still has a Datasette cookie, they will still be able to access that Datasette instance.
177+
This means that if a user is removed from an organization or team but still has a valid Datasette cookie, they will still be able to access that Datasette instance until the cookie expires or is invalidated. The default expiration is 30 days after sign-in.
163178

164179
You can remedy this by rotating the `DATASETTE_SECRET` environment variable any time you make changes to your GitHub organization members.
165180

‎datasette_auth_github/views.py‎

Lines changed: 22 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -1,17 +1,24 @@
11
from datasette.utils.asgi import Response
2+
from datasette.utils import baseconv
23
from urllib.parse import parse_qsl
34
from .utils import load_orgs_and_teams
45
import httpx
5-
6+
import time
67

78
DEPRECATED_KEYS = ("allow_users", "allow_orgs", "allow_teams")
9+
DEFAULT_LOGIN_MAX_AGE = 30 * 24 * 60 * 60
810

911

1012
def verify_config(config):
1113
config = config or {}
1214
for key in DEPRECATED_KEYS:
1315
assert key not in config, "{} is no longer a supported option".format(key)
1416
config.setdefault("host", "github.com")
17+
login_max_age = config.get("login_max_age", DEFAULT_LOGIN_MAX_AGE)
18+
if login_max_age is not None and (
19+
type(login_max_age) is not int or login_max_age <= 0
20+
):
21+
raise ValueError("login_max_age must be a positive integer or null")
1522

1623

1724
async def github_auth_start(datasette):
@@ -87,5 +94,18 @@ async def github_auth_callback(datasette, request, scope, receive, send):
8794

8895
# Set a signed cookie and redirect to homepage (respecting 'base_url' setting)
8996
response = Response.redirect(datasette.urls.path("/"))
90-
response.set_cookie("ds_actor", datasette.sign({"a": actor}, "actor"))
97+
login_max_age = config.get("login_max_age", DEFAULT_LOGIN_MAX_AGE)
98+
data = {"a": actor}
99+
if login_max_age is not None:
100+
# Enforce expiration server-side as well as in the browser.
101+
data["e"] = baseconv.base62.encode(int(time.time()) + login_max_age)
102+
response.set_cookie(
103+
"ds_actor",
104+
datasette.sign(data, "actor"),
105+
max_age=login_max_age,
106+
httponly=True,
107+
secure=request.scheme == "https",
108+
samesite="lax",
109+
path="/",
110+
)
91111
return response

‎tests/test_datasette_auth_github.py‎

Lines changed: 133 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -1,10 +1,22 @@
11
from datasette.app import Datasette
2+
from datasette.utils import baseconv
3+
from datasette_auth_github.views import verify_config
4+
from http.cookies import SimpleCookie
5+
import inspect
26
import pytest
37
import pytest_asyncio
48
import sqlite_utils
59
import re
610

711

12+
def datasette_with_config(files=None, config=None, **kwargs):
13+
# Datasette 1.0 moved plugin and permission configuration out of metadata.
14+
config_key = (
15+
"config" if "config" in inspect.signature(Datasette).parameters else "metadata"
16+
)
17+
return Datasette(files, **{config_key: config}, **kwargs)
18+
19+
820
@pytest.fixture
921
def non_mocked_hosts():
1022
return ["localhost"]
@@ -98,9 +110,9 @@ def mocked_github_api(httpx_mock):
98110
async def ds(tmpdir):
99111
filepath = str(tmpdir / "test.db")
100112
filepath2 = str(tmpdir / "demouser_org_only.db")
101-
ds = Datasette(
113+
ds = datasette_with_config(
102114
[filepath, filepath2],
103-
metadata={
115+
config={
104116
"plugins": {
105117
"datasette-auth-github": {
106118
"client_id": "x_client_id",
@@ -167,6 +179,115 @@ async def test_github_auth_callback(ds, mocked_github_api):
167179
assert "/" == response.headers["location"]
168180

169181

182+
@pytest.mark.asyncio
183+
@pytest.mark.parametrize(
184+
"scheme,force_https_urls,expected_secure",
185+
[("http", False, False), ("https", False, True), ("http", True, True)],
186+
ids=["http", "https", "https-proxy"],
187+
)
188+
@pytest.mark.parametrize(
189+
"cookie_config,expected_max_age",
190+
[({}, 2592000), ({"login_max_age": 3600}, 3600), ({"login_max_age": None}, None)],
191+
ids=["default", "custom", "session-only"],
192+
)
193+
async def test_callback_cookie_attributes(
194+
mocked_github_api,
195+
monkeypatch,
196+
scheme,
197+
force_https_urls,
198+
expected_secure,
199+
cookie_config,
200+
expected_max_age,
201+
):
202+
now = 1700000000
203+
monkeypatch.setattr("time.time", lambda: now)
204+
ds = datasette_with_config(
205+
config={
206+
"plugins": {
207+
"datasette-auth-github": {
208+
"client_id": "x_client_id",
209+
"client_secret": "x_client_secret",
210+
**cookie_config,
211+
}
212+
}
213+
},
214+
settings={"force_https_urls": force_https_urls},
215+
)
216+
response = await ds.client.request(
217+
"GET",
218+
f"{scheme}://localhost/-/github-auth-callback?code=github-code-here",
219+
follow_redirects=False,
220+
avoid_path_rewrites=True,
221+
)
222+
assert response.status_code == 302
223+
cookies = SimpleCookie()
224+
for header in response.headers.get_list("set-cookie"):
225+
cookies.load(header)
226+
cookie = cookies["ds_actor"]
227+
assert cookie["httponly"] is True
228+
assert bool(cookie["secure"]) is expected_secure
229+
assert cookie["samesite"].lower() == "lax"
230+
assert cookie["path"] == "/"
231+
assert cookie["domain"] == ""
232+
payload = ds.unsign(cookie.value, "actor")
233+
assert payload["a"]["id"] == "github:123"
234+
if expected_max_age is None:
235+
assert cookie["max-age"] == ""
236+
assert cookie["expires"] == ""
237+
assert "e" not in payload
238+
else:
239+
assert cookie["max-age"] == str(expected_max_age)
240+
assert int(baseconv.base62.decode(payload["e"])) == now + expected_max_age
241+
242+
243+
@pytest.mark.asyncio
244+
@pytest.mark.parametrize(
245+
"cookie_config,max_age",
246+
[({}, 2592000), ({"login_max_age": 3600}, 3600), ({"login_max_age": None}, None)],
247+
ids=["default", "custom", "session-only"],
248+
)
249+
async def test_callback_cookie_expiration(
250+
mocked_github_api, monkeypatch, cookie_config, max_age
251+
):
252+
now = 1700000000
253+
monkeypatch.setattr("time.time", lambda: now)
254+
ds = datasette_with_config(
255+
config={
256+
"plugins": {
257+
"datasette-auth-github": {
258+
"client_id": "x_client_id",
259+
"client_secret": "x_client_secret",
260+
**cookie_config,
261+
}
262+
}
263+
}
264+
)
265+
response = await ds.client.get(
266+
"/-/github-auth-callback?code=github-code-here", follow_redirects=False
267+
)
268+
# Always send the cookie, even after its browser expiry, to exercise
269+
# Datasette's verification of the signed expiration timestamp.
270+
headers = {"Cookie": "ds_actor={}".format(response.cookies["ds_actor"])}
271+
lifetime = max_age if max_age is not None else 2592000
272+
now += lifetime - 1
273+
response = await ds.client.get("/-/actor.json", headers=headers)
274+
assert response.json()["actor"]["id"] == "github:123"
275+
now += 2
276+
response = await ds.client.get("/-/actor.json", headers=headers)
277+
if max_age is None:
278+
assert response.json()["actor"]["id"] == "github:123"
279+
else:
280+
assert response.json()["actor"] is None
281+
282+
283+
@pytest.mark.parametrize("value", [True, False, 0, -1, "3600", 1.5])
284+
def test_invalid_login_max_age(value):
285+
with pytest.raises(
286+
ValueError, match="^login_max_age must be a positive integer or null$"
287+
):
288+
verify_config({"login_max_age": value})
289+
290+
170291
@pytest.mark.asyncio
171292
async def test_sign_in_with_github_button(ds):
172293
response = await ds.client.get("/")
@@ -197,11 +318,14 @@ async def test_database_access_permissions(
197318
)
198319
cookies = {"ds_actor": auth_response.cookies["ds_actor"]}
199320
databases = await ds.client.get("/.json", cookies=cookies)
200-
# This differs between Datasette <1.0 and >=1.0a20
201-
if "databases" in databases.json():
202-
assert set(databases.json()["databases"].keys()) == expected_databases
203-
else:
204-
assert set(databases.json().keys()) == expected_databases
321+
# Datasette versions expose databases as a top-level or nested dictionary,
322+
# or as a list of objects with a name field.
323+
data = databases.json()
324+
data = data.get("databases", data)
325+
names = (
326+
{database["name"] for database in data} if isinstance(data, list) else set(data)
327+
)
328+
assert names == expected_databases
205329

206330

207331
@pytest.mark.asyncio
@@ -235,9 +359,9 @@ async def test_github_enterprise_host(tmpdir, httpx_mock):
235359

236360
# Create Datasette instance with GitHub Enterprise configuration
237361
filepath = str(tmpdir / "test.db")
238-
ds = Datasette(
362+
ds = datasette_with_config(
239363
[filepath],
240-
metadata={
364+
config={
241365
"plugins": {
242366
"datasette-auth-github": {
243367
"client_id": "enterprise_client_id",

0 commit comments

Comments
 (0)