-
Notifications
You must be signed in to change notification settings - Fork 9
[WIP] Emit audit message for non-bot user modifications of managed roles #138
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
[WIP] Emit audit message for non-bot user modifications of managed roles #138
Conversation
- Implement BeforeWriteStorageObjectsHook and BeforeDeleteStorageObjectsHook to enforce StorageObjects intent (or IsGlobalOperator) and block unauthorized write/delete requests by returning nil. - Clarify AfterReadStorageObjectsHook comment to specify retry when zero objects were returned. - Small TODO placement fix in BeforeListMatchesHook. - Comment out registration of the BeforeWriteStorageObjects guard in registerAPIGuards.
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com> Signed-off-by: Andrew Bates <a@sprock.io>
Co-authored-by: copilot-swe-agent[bot] <198982749+Copilot@users.noreply.github.com> Co-authored-by: Andrew Bates <a@sprock.io>
* Initial plan * Add nakama-debug to .gitignore Co-authored-by: thesprockee <962164+thesprockee@users.noreply.github.com> --------- Co-authored-by: copilot-swe-agent[bot] <198982749+Copilot@users.noreply.github.com> Co-authored-by: thesprockee <962164+thesprockee@users.noreply.github.com> Co-authored-by: Andrew Bates <a@sprock.io>
…tion (#124) * Initial plan * Extract common storage authorization logic into helper function Co-authored-by: thesprockee <962164+thesprockee@users.noreply.github.com> --------- Co-authored-by: copilot-swe-agent[bot] <198982749+Copilot@users.noreply.github.com> Co-authored-by: thesprockee <962164+thesprockee@users.noreply.github.com> Co-authored-by: Andrew Bates <a@sprock.io>
…ing (#127) * Initial plan * Add novalidation tag support for game server registration Co-authored-by: thesprockee <962164+thesprockee@users.noreply.github.com> * Add documentation for novalidation tag Co-authored-by: thesprockee <962164+thesprockee@users.noreply.github.com> * Use constant for novalidation tag to avoid magic string Co-authored-by: thesprockee <962164+thesprockee@users.noreply.github.com> * Make audit message sending async and clarify documentation Co-authored-by: thesprockee <962164+thesprockee@users.noreply.github.com> --------- Co-authored-by: copilot-swe-agent[bot] <198982749+Copilot@users.noreply.github.com> Co-authored-by: thesprockee <962164+thesprockee@users.noreply.github.com> Co-authored-by: Andrew Bates <a@sprock.io>
…ions (#128) * Initial plan * Extract common authorization logic into helper function Co-authored-by: thesprockee <962164+thesprockee@users.noreply.github.com> --------- Signed-off-by: Andrew Bates <a@sprock.io> Co-authored-by: copilot-swe-agent[bot] <198982749+Copilot@users.noreply.github.com> Co-authored-by: thesprockee <962164+thesprockee@users.noreply.github.com> Co-authored-by: Andrew Bates <a@sprock.io>
…130) * Initial plan * Add nakama-debug to .gitignore Co-authored-by: thesprockee <962164+thesprockee@users.noreply.github.com> * Conditionally show moderator Discord ID in suspension embeds Co-authored-by: thesprockee <962164+thesprockee@users.noreply.github.com> * Add tests for conditional enforcer ID display in suspension embeds Co-authored-by: thesprockee <962164+thesprockee@users.noreply.github.com> --------- Co-authored-by: copilot-swe-agent[bot] <198982749+Copilot@users.noreply.github.com> Co-authored-by: thesprockee <962164+thesprockee@users.noreply.github.com> Co-authored-by: Andrew Bates <a@sprock.io>
* Initial plan * Initial analysis - Identify error message improvement needed Co-authored-by: thesprockee <962164+thesprockee@users.noreply.github.com> * Update error messages to include internal IP, external IP, and port Co-authored-by: thesprockee <962164+thesprockee@users.noreply.github.com> --------- Co-authored-by: copilot-swe-agent[bot] <198982749+Copilot@users.noreply.github.com> Co-authored-by: thesprockee <962164+thesprockee@users.noreply.github.com> Co-authored-by: Andrew Bates <a@sprock.io>
Use the loop variable (gID) instead of the outer groupID when calling createSuspensionDetailsEmbedField so each embed field references the correct guild instead of always using the current group.
* Initial plan * Initial plan for fixing Past Display Names embed bug Co-authored-by: thesprockee <962164+thesprockee@users.noreply.github.com> * Fix Past Display Names embed bug and add missing ServerEventsPublish method - Fixed timestamp assignment bug in createPastDisplayNameEmbed where variable 'e' was being assigned instead of 'ts' - Added comprehensive test coverage for createPastDisplayNameEmbed function - Fixed Satori build issue by implementing missing ServerEventsPublish method Co-authored-by: thesprockee <962164+thesprockee@users.noreply.github.com> --------- Co-authored-by: copilot-swe-agent[bot] <198982749+Copilot@users.noreply.github.com> Co-authored-by: thesprockee <962164+thesprockee@users.noreply.github.com> Co-authored-by: Andrew Bates <a@sprock.io>
* Added IGN Override lock toggle and interaction for user metadata * Update server/evr_discord_appbot_handlers.go Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com> Signed-off-by: Andrew Bates <a@sprock.io> * Update server/evr_discord_appbot_igp.go Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com> Signed-off-by: Andrew Bates <a@sprock.io> * Update server/evr_discord_appbot.go Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com> Signed-off-by: Andrew Bates <a@sprock.io> --------- Signed-off-by: Andrew Bates <a@sprock.io> Co-authored-by: Andrew Bates <a@sprock.io> Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com> Signed-off-by: Andrew Bates <a@sprock.io>
Co-authored-by: thesprockee <962164+thesprockee@users.noreply.github.com>
Co-authored-by: thesprockee <962164+thesprockee@users.noreply.github.com>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
This PR adds monitoring and audit logging for Discord role modifications. When a managed role in a tracked guild is updated by a non-bot user, the system now sends an audit message to the guild's audit channel with details about who made the change and what was modified.
Key changes:
- Added a new
handleGuildRoleUpdateevent handler in the DiscordIntegrator - Integrated the handler into the Start() method to listen for GuildRoleUpdate events
- Created a comprehensive test file documenting expected behavior (all tests currently skipped)
Reviewed Changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 4 comments.
| File | Description |
|---|---|
| server/evr_discord_integrator.go | Adds handler registration in Start() method and implements handleGuildRoleUpdate function to audit managed role changes |
| server/evr_discord_integrator_roleupdate_test.go | Adds skipped documentation tests describing the expected behavior of role update handling |
| func TestHandleGuildRoleUpdate_NotTrackedGuild(t *testing.T) { | ||
| // This test would require mocking the DiscordIntegrator and its dependencies | ||
| // Since the codebase doesn't have an established mocking pattern for this component, | ||
| // we're documenting the expected behavior: | ||
| // | ||
| // Given: A GuildRoleUpdate event for a guild that is not in our system | ||
| // When: handleGuildRoleUpdate is called | ||
| // Then: The function should return nil without sending any audit messages | ||
| // | ||
| // This is handled by the groupID check on line 910-914 in evr_discord_integrator.go | ||
| t.Skip("Test requires extensive mocking infrastructure") | ||
| } |
Copilot
AI
Nov 6, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
All test functions in this file are skipped with no actual test implementation. Per the custom guideline 1000000, only EVR unit tests should be included. While documentation tests can be useful, these skipped tests provide no test coverage. Consider either implementing actual unit tests using table-driven test patterns (similar to evr_enforcement_journal_test.go and evr_discord_appbot_test.go), or removing these skipped documentation tests entirely since they don't provide executable coverage.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Removed in b9de6f8. The skipped documentation tests have been deleted since they provide no executable coverage. Actual unit tests for this feature would require extensive mocking infrastructure that doesn't currently exist in the codebase.
server/evr_discord_integrator.go
Outdated
| roleType := "unknown" | ||
| switch e.Role.ID { | ||
| case guildGroup.RoleMap.AccountLinked: | ||
| roleType = "linked" | ||
| case guildGroup.RoleMap.Member: | ||
| roleType = "member" | ||
| case guildGroup.RoleMap.Enforcer: | ||
| roleType = "enforcer" | ||
| case guildGroup.RoleMap.Auditor: | ||
| roleType = "auditor" | ||
| case guildGroup.RoleMap.ServerHost: | ||
| roleType = "server_host" | ||
| case guildGroup.RoleMap.Allocator: | ||
| roleType = "allocator" | ||
| case guildGroup.RoleMap.Suspended: | ||
| roleType = "suspended" | ||
| case guildGroup.RoleMap.APIAccess: | ||
| roleType = "api_access" | ||
| case guildGroup.RoleMap.AccountAgeBypass: | ||
| roleType = "account_age_bypass" | ||
| case guildGroup.RoleMap.VPNBypass: | ||
| roleType = "vpn_bypass" | ||
| case guildGroup.RoleMap.UsernameOnly: | ||
| roleType = "username_only" | ||
| } |
Copilot
AI
Nov 6, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[nitpick] This large switch statement to determine roleType could be refactored into a helper method on GuildGroupRoles (similar to AsSlice() and AsSet()). This would improve maintainability and keep role-related logic centralized in the GuildGroupRoles type. Consider adding a method like GetRoleType(roleID string) string to the GuildGroupRoles struct.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Refactored in b9de6f8. Added GetRoleType() helper method to GuildGroupRoles, centralizing role-related logic as suggested.
Co-authored-by: thesprockee <962164+thesprockee@users.noreply.github.com>
…ion-again Signed-off-by: Andrew Bates <a@sprock.io>
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com> Signed-off-by: Andrew Bates <a@sprock.io>
|
try again. |
…e logging - Extract roleType determination into GetRoleType helper method on GuildGroupRoles - Remove duplicate error logging (let caller handle it) - Remove skipped documentation tests that provide no executable coverage Co-authored-by: thesprockee <962164+thesprockee@users.noreply.github.com>
Could you clarify what you'd like me to try again? The PR currently implements:
What specific aspect would you like me to revisit or change? |
Implementation Details:
Original prompt
💬 We'd love your input! Share your thoughts on Copilot coding agent in our 2 minute survey.