Skip to content

Feat/admin audit logging - #3076

Open
GhassenKefi wants to merge 4 commits into
masterfrom
feat/admin-audit-logging
Open

GhassenKefi wants to merge 4 commits into
masterfrom
feat/admin-audit-logging

Conversation

@GhassenKefi

Copy link
Copy Markdown
  • Resolves: #

Summary

TODO

  • ...

Checklist

AI (if applicable)

  • The content of this PR was partly or fully generated using AI

Creating, renaming and deleting a team, changing its configuration
flags (open, invite only, visible, federated, ...), setting or removing
a team setting, and creating or revoking its invitation link are now
written to the admin audit log.

Every change of a team goes through a federated item, which is applied
by its manage() method whether it comes from the web UI, the OCS API,
occ or another app through the CirclesManager. The entries are written
there, after the database change, by the new AuditLogService. The
EventService hooks named in the plan are not used: the "...ed" result
hooks only run for asynchronous items and only once every instance
answered, so renames, configuration and invitation changes never reach
them, and the deletion hook runs before the team is deleted.

A federated team is changed on every instance it is shared with, so
the entry is only written by the instance owning the team, not by the
instances receiving the change. Changes are often applied in a
background request without a user session, so the entries name the
user who did the change. Teams mirroring a Nextcloud group are not
logged, their changes come from the group, which is already audited.

The value of a team setting is never logged, as it can be the password
of the team shares, and neither is the invitation code. A change of
the description only is not logged.

The operations are collected in the new OCA\Circles\AuditLog\Operation
enum, with a short description of each for the documentation.

Assisted-by: ClaudeCode:claude-opus-5-5
Signed-off-by: Ghassen kefi <ghassen.kefi.dev@gmail.com>
Adding members to a team (one or several at once), inviting them to an
invite only team, removing them, changing their level (including
ownership transfers), users joining a team or requesting to join it,
and members leaving a team are now written to the admin audit log.

The entries are written by the federated items applying the change,
after the database change and only on the instance owning the team,
like the team changes. When the owner leaves, the member becoming the
new owner gets its own level change entry, and the team deletion when
the last member leaves is logged as well. A transfer of the ownership
also logs the previous owner becoming an admin.

Members are identified by their member ID, type and user, group or
team ID. Email addresses and contacts are only identified by their
member ID, the address is never logged. Membership changes of teams
mirroring a Nextcloud group are not logged, as the group changes are
already audited, which also keeps the group sync out of the log.

Assisted-by: ClaudeCode:claude-opus-5-5
Signed-off-by: Ghassen kefi <ghassen.kefi.dev@gmail.com>
Adding a remote instance for federated teams, changing the identity or
the type of a known one, and removing one are now written to the admin
audit log, as are allowing a remote instance to add members to teams
managed by SCIM and revoking that again.

Remote instances are added by occ circles:remote and by the sync of
the trusted servers, which now both store them through the new
RemoteStreamService::saveRemoteInstance(), and occ circles:remote
--delete removes them through RemoteStreamService::removeRemoteInstance(),
so each change is logged in one place. Deleting an unknown instance is
not logged. Refreshing the cached data or the address of a known
instance does not change its trust and is not logged.

The SCIM instances are a list in the app config only changed by occ
circles:scim:add-remote-instance and circles:scim:remove-remote-instance,
so these commands log the change themselves.

Assisted-by: ClaudeCode:claude-opus-5-5
Signed-off-by: Ghassen kefi <ghassen.kefi.dev@gmail.com>
Changing an admin setting of Teams through the admin settings page
(enabling federated teams, the frontal address of the instance, the
default team folder quota and the team folder quotas per group) is now
written to the admin audit log with the key and the new value. None of
these values is secret. Rejected values are not logged.

Assisted-by: ClaudeCode:claude-opus-5-5
Signed-off-by: Ghassen kefi <ghassen.kefi.dev@gmail.com>
self::TeamSettingUpdated => 'A setting of a team, for example the password of its shares, was set or removed',
self::InvitationCreated => 'An invitation link to join a team was created',
self::InvitationRevoked => 'The invitation link to join a team was revoked',
self::MemberAdded => 'A user, group, team, email address or contact was added to a team, or a request to join it was accepted',

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I think we could keep it more generic here, as types of members might change during time.

Suggested change
self::MemberAdded => 'A user, group, team, email address or contact was added to a team, or a request to join it was accepted',
self::MemberAdded => 'A member was added to a team, or a request to join it was accepted',

public function description(): string {
return match ($this) {
self::TeamCreated => 'A team was created',
self::TeamDeleted => 'A team was deleted, directly or because its last member left it',

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I think this could be more generic

Suggested change
self::TeamDeleted => 'A team was deleted, directly or because its last member left it',
self::TeamDeleted => 'A team was deleted',

self::RemoteInstanceUpdated => 'The identity or the type of a known remote instance for federated teams was changed',
self::RemoteInstanceRemoved => 'A remote instance for federated teams was removed',
self::ScimInstanceAdded => 'A remote instance was allowed to add members to teams managed by SCIM',
self::ScimInstanceRemoved => 'A remote instance was no longer allowed to add members to teams managed by SCIM, removing the members it added',

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Suggested change
self::ScimInstanceRemoved => 'A remote instance was no longer allowed to add members to teams managed by SCIM, removing the members it added',
self::ScimInstanceRemoved => 'A remote instance was no longer allowed to add members to teams managed by SCIM',

Comment thread lib/Db/RemoteRequest.php
Comment on lines +245 to +249
public function deleteInstance(string $host): int {
$qb = $this->getRemoteDeleteSql();
$qb->limitToInstance($host);
$qb->executeStatement();

return $qb->executeStatement();

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

change unrelated to this PR purpose

Suggested change
public function deleteInstance(string $host): int {
$qb = $this->getRemoteDeleteSql();
$qb->limitToInstance($host);
$qb->executeStatement();
return $qb->executeStatement();
public function deleteInstance(string $host) {
$qb = $this->getRemoteDeleteSql();
$qb->limitToInstance($host);
$qb->executeStatement();

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants