Skip to content

Add gRPC methods for admin panel#10

Merged
binaryoverload merged 41 commits into
masterfrom
feat/admin-grpc
Jan 14, 2026
Merged

Add gRPC methods for admin panel#10
binaryoverload merged 41 commits into
masterfrom
feat/admin-grpc

Conversation

@mrjvs
Copy link
Copy Markdown
Collaborator

@mrjvs mrjvs commented Oct 24, 2025

Changes:

  • Added methods for devices based on what the admin panel can currently do:
    • For these I assumed that device_id is a unique ID for the device
    • ListDevices
    • GetDevice
    • UpdateDevice
  • Added methods for NEX accounts based on what the admin panel can currently do:
    • ListNEXAccounts
    • GetNEXAccount
    • UpdateNEXAccount
  • Added methods for servers based on what the admin panel can currently do:
    • ListServers
    • CreateServer
    • GetServer
    • UpdateServer
    • DeleteServer
  • Added methods for Audit logs:
    • Audit logs can have comments, this is to add extra context behind an action. This has been discussed with headmods and chosen as the best approach
    • ListAuditLogs - it has a bunch of filters so mods can find what they need + we can display relevant logs on a resource specific page
    • ListAuditLogComment - The only list method that doesn't have pagination, I figured we wouldn't get enough comments to warrant it
    • CreateAuditLogComment

Some notes about the implementation:

  • The idea is to remove database access from the admin panel and have it be purely done through gRPC
  • Audit logs will be created automatically when an action is performed using the new gRPC methods.
  • All listing methods have pagination utils, as the quantities are too much to list all in one RPC call
  • All fields on an update method except the ID are optional, as you need to be able to update only one field and not replace all contents. (Think of race conditions with people editing the same field)

Todos before it can be marked as ready for review:

  • Add request/response types for PNID actions
    • What should be done with deleteAccount?
  • Go through permissions and add permissions where needed for new gRPC methods.
    • The goal is to make the admin panel safely accessible by mods without risking security

@github-actions
Copy link
Copy Markdown
Contributor

github-actions Bot commented Oct 24, 2025

The latest Buf updates on your PR. Results from workflow Validate Pull Request / check_buf (pull_request).

BuildFormatLintBreakingUpdated (UTC)
✅ passed⏩ skipped✅ passed✅ passedJan 10, 2026, 5:36 PM

@jonbarrow
Copy link
Copy Markdown
Member

Why are mod audit logs expected to be in the account server? That doesn't make a ton of sense to me. These are audit logs for moderator actions done on the admin panel, that feels like it would be much better suited to be stored in a database the admin panel maintains itself (it's fine for the admin panel to use a database for itself, the idea with removing the database connections was to remove it's direct connections to other services databases), and the admin panel would just update it's audit logs before/after the gRPC calls. I don't see a reason to keep these logs in the account server?

Keeping the audit logs in the admin panel itself will also help keep everything centralized, rather than having logs spread across multiple services. For example another future goal of the admin panel is to move Miiverse moderation to it which will also need audit logs for it's admin panel actions

@mrjvs
Copy link
Copy Markdown
Collaborator Author

mrjvs commented Oct 24, 2025

Moving audit logs to the admin panel would mean that it wouldn't be possible for any other services to manage audit logs. (Nextjs is a frontend, it can't host a grpc server)

Moving juxt mod to admin panel will not be possible. It was added to juxt web so mods don't have to go to a separate spreadsheet just to remove a post or ban someone (it resulted in no moderation being done, it was too much effort). Moving it to admin panel would reintroduce the problem.

@jonbarrow
Copy link
Copy Markdown
Member

Would would any other service besides the admin panel need to manage audit logs? I don't see the logic here, nor do I see what the frontend has to do with this? I am aware that Next is a frontend, but that doesn't really change what I said?

Currently, for example, the admin panel does this for updating a PNID:

await PNID.updateOne({ pid: pid }, { $set: body });

What I've said is that this would instead look more like:

await AuditLog.create({ whatever }});
await grpcClientV2.modifyAccount(whatever); // however this would look with the new protobufs

The admin panel already maintains database connections, I don't see why it isn't possible to run a database along side the admin panel and connect to it the same way it already connects to the account server database, to manage its own audit logs or why any service other than the admin panel itself needs to know about admin panel audit logs, this feels like an odd mixing of responsibilities?

Also no one is suggesting moving back to a spreadsheet for Juxtaposition moderation. I don't doubt that the old system resulted in a lack of moderation, but I do somewhat doubt that the crux of the issue was "opening another tab" and not "having to manually maintain a spreadsheet", that seems pretty extreme to me (especially since there's virtually no friction there outside of just a new tab being opened, since being logged into one service keeps you logged in to all of them)

I am, of course, willing to hear the mods out on the Juxtaposition issue, but I do still think that at the minimum keeping the audit logs for admin panel actions on the admin panel makes more sense than trying to mix that into the responsibilities of the account server?

To be 100% clear I am genuinely asking for clarifications here in case there's something I've missed, and am giving my view point on things

@mrjvs
Copy link
Copy Markdown
Collaborator Author

mrjvs commented Oct 24, 2025

My gut tells me that there will be more things that need to be logged, not just admin panel actions. Like deleting a juxt post or stripe tier changes.

To me it makes sense to keep the the audit log close to the resource it's auditing. A pnid ban has a audit log in the same db. I would expect the same from juxt posts. Just have a separate listAuditLogs for juxt.

That said, I don't really have the free time to come to a middleground.

Could you take over on the account server changes and this pr?


As for juxt mod, it's best if you talk to the moderation team there on how it will best be implemented.

@jonbarrow
Copy link
Copy Markdown
Member

My gut tells me that there will be more things that need to be logged, not just admin panel actions. Like deleting a juxt post or stripe tier changes.

I agree that more things will need to be logged, that's why I was trying to have a dialogue about it to clarify the intentions and make sure we're using the best solution. To clarify, was the intent to store things like Juxtaposition information in this same audit log database on the account server? I ask because that was the feeling I got, and why I was bringing it up, and because Juxtaposition was brought up in the same sentence as Stripe here. I'm just asking so we're on the same page, and because later in your reply seems to imply that this isn't actually the case.

To me it makes sense to keep the the audit log close to the resource it's auditing. A pnid ban has a audit log in the same db. I would expect the same from juxt posts. Just have a separate listAuditLogs for juxt.

I also agree this makes sense. It seemed to me like the idea was to just dump all the audit stuff onto the account server, which is why I had concerns about the mixing of responsibilities.

My logic was to keep the audit logs close to the tool that performed the action, but keeping the logs close to the data being affected is also fine logic that I'm happy with.

Could you take over on the account server changes and this pr?

Sure.

@mrjvs
Copy link
Copy Markdown
Collaborator Author

mrjvs commented Oct 25, 2025

To clarify, was the intent to store things like Juxtaposition information in this same audit log database on the account server?

The intent was to store the audit log next to the resource it's auditing. And since admin panel only manages accound atm, it's only implemented for account.

Could you take over on the account server changes and this pr?

Sure.

Thanks 🙂

this field can be used to add additional metadata to an audit log
tracks the comments parent log ID, and comment ID, separately in case we ever need to target a specific audit log comment, such as for edits to a comments content
making this an enum rather than a string gives us type safety/hints in the implementation side of things
@jonbarrow
Copy link
Copy Markdown
Member

I did a few touch ups so far. Namely I swapped action_type to an enum so we can get type hints/safety in the implementation, and added a metadata_json string field to audit logs to store action-specific metadata (for example, tracking specific changes made to a server config, or tracking how long an account was banned for, etc.)

@DaniElectra I want to get you in on this as well for input on the design. There's a couple things I'm not to sure what the use is atm (like target_resource_id and target_resource_type?), so wanted your opinion here. I'm also thinking that adding dedicated Ban methods for both NEX and network accounts might be good UX? And it would help us further separate permissions (atm mods would need the ability to change access levels in general, which would mean they could also change someone to being above a standard user, but we have a dedicated "can ban" permission that should be used there instead)

@jonbarrow
Copy link
Copy Markdown
Member

@DaniElectra poke

@DaniElectra
Copy link
Copy Markdown
Member

There's a couple things I'm not to sure what the use is atm (like target_resource_id and target_resource_type?)

I assume those would be for adding context about what information is being changed specifically (kinda like metadata_json?). Not fully certain about it though.

I'm also thinking that adding dedicated Ban methods for both NEX and network accounts might be good UX?

I fully agree that we want to handle bans separately, that way we could also implement temporary / game-specific bans. I'm thinking on the following:

  • Bans model
    • id
    • ban_type (PID or console)
    • target_id (PID or console identifier, either serial number or device ID whichever we want to use)
    • source_pid
    • start_date
    • end_date (if applicable)
    • ban_scope (all, NEX, service)
    • ban_scope_id (for specifying the game server ID or the client ID where the ban applies)
    • comments

@jonbarrow
Copy link
Copy Markdown
Member

I assume those would be for adding context about what information is being changed specifically (kinda like metadata_json?). Not fully certain about it though.

In that case they seem to be a little redundant when paired with metadata_json since that can just be encoded there. I'll probably remove that then

I'm thinking on the following

That looks good to me!

Copy link
Copy Markdown
Member

@DaniElectra DaniElectra left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looking pretty good! Only a couple of comments

Comment thread protobufs/account/v2/list_devices_rpc.proto
Comment thread protobufs/account/v2/update_pnid_rpc.proto Outdated
Copy link
Copy Markdown
Member

@DaniElectra DaniElectra left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Outside of this, after resolving conflicts LGTM

Comment thread protobufs/account/v2/account_service.proto Outdated
jonbarrow and others added 2 commits December 30, 2025 19:52
Using "remove" can imply deleting the entry, "lift" is more clear

Co-authored-by: Daniel López Guimaraes <112760654+DaniElectra@users.noreply.github.com>
@DaniElectra
Copy link
Copy Markdown
Member

Hm, it's complaining about renaming one of the RPCs as _rpc. Do we want to keep the rename or revert it?

If we're keeping the rename we also have to rename exchange_token_for_user_data.proto

@jonbarrow
Copy link
Copy Markdown
Member

Hm, it's complaining about renaming one of the RPCs as _rpc. Do we want to keep the rename or revert it?

If we're keeping the rename we also have to rename exchange_token_for_user_data.proto

Using _rpc as a suffix is a pretty common standard I've seen to visually separate RPCs from other messages (though in fairness the Google style guide does not use this), which we also use in other protocols in this repo already (though there's a few cases where this didn't happen, like with BOSS, which should be updated at some point). So for consistency I would prefer to keep it

It looks like we can get around this by disabling FILE_NO_DELETE in the buf.yaml. Maybe we should temporarily disable this check, merge this PR, then update the other files that need to be renamed, and then add back in the check?

@DaniElectra
Copy link
Copy Markdown
Member

I'd say we just force merge it so that we don't have to mess with the current config

I'll approve after the other RPC is renamed

@jonbarrow jonbarrow requested a review from DaniElectra January 1, 2026 18:44
Comment thread protobufs/account/v2/account_service.proto Outdated
Comment thread protobufs/account/v2/account_service.proto Outdated
Comment thread protobufs/account/v2/account_service.proto Outdated
@jonbarrow jonbarrow requested a review from DaniElectra January 4, 2026 18:26
Comment thread protobufs/account/v2/validate_independent_service_token_rpc.proto Outdated
@jonbarrow jonbarrow requested a review from DaniElectra January 5, 2026 17:29
Copy link
Copy Markdown
Member

@DaniElectra DaniElectra left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM! Only a reminder to remove the breaking changes validation temporary hack after this

@binaryoverload binaryoverload merged commit 8fdcc38 into master Jan 14, 2026
2 checks passed
@binaryoverload binaryoverload deleted the feat/admin-grpc branch January 14, 2026 22:12
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants