feat(notify-expire-priv):增加查询权限到期提醒#3186
Conversation
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 4654c535b1
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #3186 +/- ##
==========================================
+ Coverage 83.67% 83.87% +0.20%
==========================================
Files 140 140
Lines 23431 23628 +197
==========================================
+ Hits 19605 19819 +214
+ Misses 3826 3809 -17 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 6fa0f54fb4
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| admin_emails = list( | ||
| Users.objects.filter(is_superuser=True, email__isnull=False) | ||
| .exclude(email="") | ||
| .values_list("email", flat=True) |
There was a problem hiding this comment.
The repo treats users with sql.query_mgtpriv as query-permission managers (for example, the modify endpoint is gated by that permission), but this reminder builds the admin recipient list from is_superuser=True only. In deployments where resource-group/query admins are ordinary users with query_mgtpriv, expiring privileges will only CC superusers and the actual managers responsible for renewing/cleaning up those privileges will never be notified.
Useful? React with 👍 / 👎.
| if not user_email: | ||
| logger.warning(f"用户 {user_name} 未配置邮箱，跳过权限到期提醒") | ||
| continue |
There was a problem hiding this comment.
When an expiring privilege belongs to an account without an email address, this continue skips the entire reminder for that user's privileges, including the admin CC list that was already built above. For service/LDAP users or stale accounts without email, nobody is alerted before the permission expires even though the feature is meant to notify administrators as well as the permission holder.
Useful? React with 👍 / 👎.
| if not Schedule.objects.filter(name="查询权限到期提醒").exists(): | ||
| add_query_priv_expire_reminder_schedule() |
There was a problem hiding this comment.
Because AppConfig.ready() runs in every Django process, multiple web/qcluster workers starting against a database that does not yet have this schedule can all pass the exists() check before any one creates it, and then each calls schedule(). Since the schedule name is only used as data here, that race leaves duplicate daily reminders and users/admins receive the same expiry email multiple times; create the row atomically or enforce a unique schedule before scheduling.
Useful? React with 👍 / 👎.
1 participant
权限优化：当前的查询权限最长为一年，考虑增加定时任务，在权限到期前3天，给管理员和对应的权限人，以邮件形式，发送权限到期提醒。
#2417