[SERVER-36137] Remove unnecessary AuthorizationManager::logOp() in OpObserver Created: 15/Jul/18  Updated: 06/Dec/22  Resolved: 12/Dec/18

Status: Closed
Project: Core Server
Component/s: Replication, Security
Affects Version/s: None
Fix Version/s: None

Type: Task Priority: Major - P3
Reporter: Siyuan Zhou Assignee: Backlog - Replication Team
Resolution: Duplicate Votes: 0
Labels: former-quick-wins
Remaining Estimate: Not Specified
Time Spent: Not Specified
Original Estimate: Not Specified

Issue Links:
Duplicate
is duplicated by SERVER-38556 Decide what to do with transaction re... Closed
Related
is related to SERVER-38556 Decide what to do with transaction re... Closed
is related to SERVER-38557 Make auth passthrough suites use user... Closed
Assigned Teams:
Replication
Participants:

 Description   

AuthorizationManager::logOp() is registered on all commands in OpObserver, but only drop, dropDatabase and renameCollection are interesting to RoleGraph.

The code also treats createIndexes on roles collection as an unsupported operation. We should remove these unnecessary AuthorizationManager::logOp() calls.

    if ((cmdName == "collMod" || cmdName == "emptycapped" || cmdName == "createIndexes") &&
        cmdObj.firstElement().str() != rolesCollectionNamespace.coll()) {
        // We don't care about these if they're not on the roles collection.
        return Status::OK();
    }
...
    //  No other commands expected.  Warn.
    return Status(ErrorCodes::OplogOperationUnsupported, "Unsupported oplog operation");



 Comments   
Comment by Spencer Jackson [ 12/Dec/18 ]

Any oplog command we add in the future could impact authorization's ability to synchronize its caches with the on-disk representation of data. That log warning is actually pretty serious, and indicates that authorization is falling back into a degraded mode. As such, we really need to update authorization whenever a new command is introduced. I've filed a ticket, SERVER-38557 which will make our test infrastructure more aggressive about identifying these issues. I believe that we should continue to push more data than is strictly needed into the AuthorizationManager, as that is a much less serious situation than pushing too little. When the OpObservers have been further split up, the AuthorizationOpObserver could be written to be selective.

I believe this ticket should be closed out in favour of SERVER-38557 and SERVER-38556.

Comment by Siyuan Zhou [ 12/Dec/18 ]

spencer.jackson, could you please confirm that the interesting commands for authorization only include drop, dropDatabase and renameCollection? Besides, authorization assumes it registers to all commands, which seems unnecessary. There might be some refactoring for auth we can do so that when we add new commands to replication, they won't confuse authorization logic.

tess.avitabile, can I nominate this ticket to Quick Wins or neweng, since it has caused confusions for several people.

Generated at Thu Feb 08 04:42:09 UTC 2024 using Jira 9.7.1#970001-sha1:2222b88b221c4928ef0de3161136cc90c8356a66.