Restrict Hackbot triggers to authorized user with editbugs permissions - #6444
Restrict Hackbot triggers to authorized user with editbugs permissions#6444ayoubdiourin7 wants to merge 4 commits into
Conversation
| matches.append( | ||
| HackbotMention( | ||
| raw=raw, | ||
| author_phid=transaction.get("authorPHID", ""), |
There was a problem hiding this comment.
I do not think there is a comment transaction without authorPHID, if that is a possibility, we should skip the transaction altogether.
| ) | ||
| return None | ||
|
|
||
| authorized_members = await client.get_project_members(EDITBUGS_GROUP_PHID) |
There was a problem hiding this comment.
We might want to implement TTL cache, and re-fetch if a user is not exist in the list. So we optimize for the happy path. We could do it now or file a follow up issue and we do it later.
| comments: list[str] = [] | ||
| for mention in mentions: | ||
| if mention.author_phid in authorized_members: | ||
| comments.append(mention.raw) | ||
| else: | ||
| log.warning( | ||
| "Ignoring %s mention from non-editbugs user %s on %s", | ||
| webhook.mention_token, | ||
| mention.author_phid or "<missing PHID>", | ||
| object_phid, | ||
| ) | ||
| if not comments: | ||
| return None |
There was a problem hiding this comment.
I would move this before No %s mention found in triggering, so we have only one check for the comments.
28a08b1 to
1f5820a
Compare
| data = result.get("data") or [] | ||
| if not data: | ||
| return frozenset() | ||
| members = data[0].get("attachments", {}).get("members", {}).get("members", []) |
There was a problem hiding this comment.
This pattern can easily introduce silent bugs. I would prefer failing load, so we could fix it. We have other places where we do this, but I would like to reactivate that to remove them, not introduce more.
If it a value that I expect to always be there, I would access it directly, without git(). If it could be missed, I would log that if it something that would need attention.
| EDITBUGS_GROUP_PHID = "PHID-PROJ-njo5uuqyyq3oijbkhy55" | ||
| _MEMBERSHIP_CACHE_TTL_SECONDS = 60 | ||
| _MISSING_MEMBER_REFRESH_COOLDOWN_SECONDS = 30 | ||
|
|
||
| _editbugs_members_cache: TTLCache[str, frozenset[str]] = TTLCache( | ||
| maxsize=1, | ||
| ttl=_MEMBERSHIP_CACHE_TTL_SECONDS, | ||
| ) | ||
| _editbugs_members_lock = asyncio.Lock() | ||
| _last_editbugs_members_refresh = 0.0 |
There was a problem hiding this comment.
This can grow to be a bit ugly. I would encapsulate this in a class that exposes a simple method to check if the user is authorized or not.
2 participants
Changes
@hackbotcomment.Resolves #6423