Skip to content

Commit 095953e

Browse files
committed
authenticate with user ids instead of usernames with the team api
Previously homu authenticated users by their usernames when interacting with the Team API. Unfortunately it's possible to change usernames on GitHub, and the previous usernames are free to be taken by different users, causing a possible big security issue. This commit authenticates users by their ID instead, preventing the problem.
1 parent abd0083 commit 095953e

File tree

3 files changed

+26
-13
lines changed

3 files changed

+26
-13
lines changed

homu/auth.py

Lines changed: 9 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -10,19 +10,19 @@ def fetch_rust_team(repo_label, level):
1010
try:
1111
resp = requests.get(url)
1212
resp.raise_for_status()
13-
return resp.json()["github_users"]
13+
return resp.json()["github_ids"]
1414
except requests.exceptions.RequestException as e:
1515
print("error while fetching " + url + ": " + str(e))
1616
return []
1717

1818

19-
def verify_level(username, repo_label, repo_cfg, state, toml_keys,
19+
def verify_level(username, user_id, repo_label, repo_cfg, state, toml_keys,
2020
rust_team_level):
2121
authorized = False
2222
if repo_cfg.get('auth_collaborators', False):
2323
authorized = state.get_repo().is_collaborator(username)
2424
if repo_cfg.get('rust_team', False):
25-
authorized = username in fetch_rust_team(repo_label, rust_team_level)
25+
authorized = user_id in fetch_rust_team(repo_label, rust_team_level)
2626
if not authorized:
2727
authorized = username.lower() == state.delegate.lower()
2828
for toml_key in toml_keys:
@@ -31,7 +31,8 @@ def verify_level(username, repo_label, repo_cfg, state, toml_keys,
3131
return authorized
3232

3333

34-
def verify(username, repo_label, repo_cfg, state, auth, realtime, my_username):
34+
def verify(username, user_id, repo_label, repo_cfg, state, auth, realtime,
35+
my_username):
3536
# The import is inside the function to prevent circular imports: main.py
3637
# requires auth.py and auth.py requires main.py
3738
from .main import AuthState
@@ -48,12 +49,13 @@ def verify(username, repo_label, repo_cfg, state, auth, realtime, my_username):
4849
authorized = False
4950
if auth == AuthState.REVIEWER:
5051
authorized = verify_level(
51-
username, repo_label, repo_cfg, state, ['reviewers'], 'review',
52+
username, user_id, repo_label, repo_cfg, state, ['reviewers'],
53+
'review',
5254
)
5355
elif auth == AuthState.TRY:
5456
authorized = verify_level(
55-
username, repo_label, repo_cfg, state, ['reviewers', 'try_users'],
56-
'try',
57+
username, user_id, repo_label, repo_cfg, state,
58+
['reviewers', 'try_users'], 'try',
5759
)
5860

5961
if authorized:

homu/main.py

Lines changed: 12 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -450,14 +450,16 @@ class LabelEvent(Enum):
450450
PORTAL_TURRET_IMAGE = "https://cloud.githubusercontent.com/assets/1617736/22222924/c07b2a1c-e16d-11e6-91b3-ac659550585c.png" # noqa
451451

452452

453-
def parse_commands(body, username, repo_label, repo_cfg, state, my_username,
454-
db, states, *, realtime=False, sha='', command_src=''):
453+
def parse_commands(body, username, user_id, repo_label, repo_cfg, state,
454+
my_username, db, states, *, realtime=False, sha='',
455+
command_src=''):
455456
global global_cfg
456457
state_changed = False
457458

458459
_reviewer_auth_verified = functools.partial(
459460
verify_auth,
460461
username,
462+
user_id,
461463
repo_label,
462464
repo_cfg,
463465
state,
@@ -468,6 +470,7 @@ def parse_commands(body, username, repo_label, repo_cfg, state, my_username,
468470
_try_auth_verified = functools.partial(
469471
verify_auth,
470472
username,
473+
user_id,
471474
repo_label,
472475
repo_cfg,
473476
state,
@@ -584,8 +587,9 @@ def parse_commands(body, username, repo_label, repo_cfg, state, my_username,
584587
# comment on the pull request if the user is not authorized), we
585588
# need to do the author check BEFORE the verify_auth check.
586589
if state.author != username:
587-
if not verify_auth(username, repo_label, repo_cfg, state,
588-
AuthState.REVIEWER, realtime, my_username):
590+
if not verify_auth(username, user_id, repo_label, repo_cfg,
591+
state, AuthState.REVIEWER, realtime,
592+
my_username):
589593
continue
590594

591595
state.approved_by = ''
@@ -594,7 +598,7 @@ def parse_commands(body, username, repo_label, repo_cfg, state, my_username,
594598
state.change_labels(LabelEvent.REJECTED)
595599

596600
elif command.action == 'prioritize':
597-
if not verify_auth(username, repo_label, repo_cfg, state,
601+
if not verify_auth(username, user_id, repo_label, repo_cfg, state,
598602
AuthState.TRY, realtime, my_username):
599603
continue
600604

@@ -611,7 +615,7 @@ def parse_commands(body, username, repo_label, repo_cfg, state, my_username,
611615
state.save()
612616

613617
elif command.action == 'delegate':
614-
if not verify_auth(username, repo_label, repo_cfg, state,
618+
if not verify_auth(username, user_id, repo_label, repo_cfg, state,
615619
AuthState.REVIEWER, realtime, my_username):
616620
continue
617621

@@ -1520,6 +1524,7 @@ def synchronize(repo_label, repo_cfg, logger, gh, states, repos, db, mergeable_q
15201524
parse_commands(
15211525
comment.body,
15221526
comment.user.login,
1527+
comment.user.id,
15231528
repo_label,
15241529
repo_cfg,
15251530
state,
@@ -1536,6 +1541,7 @@ def synchronize(repo_label, repo_cfg, logger, gh, states, repos, db, mergeable_q
15361541
parse_commands(
15371542
comment.body,
15381543
comment.user.login,
1544+
comment.user.id,
15391545
repo_label,
15401546
repo_cfg,
15411547
state,

homu/server.py

Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -378,6 +378,7 @@ def github():
378378
pull_num = info['pull_request']['number']
379379
body = info['comment']['body']
380380
username = info['sender']['login']
381+
user_id = info['sender']['id']
381382

382383
state = g.states[repo_label].get(pull_num)
383384
if state:
@@ -387,6 +388,7 @@ def github():
387388
if parse_commands(
388389
body,
389390
username,
391+
user_id,
390392
repo_label,
391393
repo_cfg,
392394
state,
@@ -435,6 +437,7 @@ def github():
435437
found = parse_commands(
436438
c.body,
437439
c.user.login,
440+
c.user.id,
438441
repo_label,
439442
repo_cfg,
440443
state,
@@ -546,6 +549,7 @@ def fail(err):
546549
elif event_type == 'issue_comment':
547550
body = info['comment']['body']
548551
username = info['comment']['user']['login']
552+
user_id = info['comment']['user']['id']
549553
pull_num = info['issue']['number']
550554

551555
state = g.states[repo_label].get(pull_num)
@@ -557,6 +561,7 @@ def fail(err):
557561
if parse_commands(
558562
body,
559563
username,
564+
user_id,
560565
repo_label,
561566
repo_cfg,
562567
state,

0 commit comments

Comments
 (0)