[coheigea]

Apply search authorization to persistent search notifications
Entries sent to a persistent search client were taken straight from the
operation context, bypassing the authentication and ACI checks applied
to a normal search, as well as the userPassword hiding. With changesOnly
set, no check at all was done on the client before registering it.

• PersistentSearchListener now reads every changed entry again through
  the client's own session, with a base level search, and only sends it
  if it is visible, with the attributes the client is allowed to read.
  userPassword is removed when the server hides passwords.
• As a deleted entry can't be read anymore, DirectoryListener gets a
  beforeEntryDeleted() callback, called by the EventInterceptor before
  the entry is deleted.
• A changesOnly persistent search now does a base level search through
  the client's session before the listener is registered, so that an
  unauthenticated client is rejected when anonymous access is disabled.
• The EventInterceptor executor queue is now unbounded: the listeners
  wait for the end of the operation which notifies them, and a rejected
  notification made that operation fail.

[elecharny]

Sorry for the delay, lot of $dayjob atm. Will review your PR soon!

[elecharny]

Not sure this is a good idea: persistent search can last for days, without any update operations being done.

[elecharny]

Not sure it's necessary. We can let the cursor being auto-closed without doing anything with it, teh idea is just to check that the user is authorized.

[elecharny]

The raised issue is really critical: we must absolutely check the authorization for each entry potentially returned to the client. Now, I'm not sure the proposed solution is ideal: it costs a supplemental search for each modified entry, something we want to avoid at all.
Ideally speaking, we should go through the interceptor chains up to the Authorization interceptor, but no farther. It would require the addition of a flag in the Operation context that would tell the Authz interceptor to just verify the resuqested authorization.

[elecharny]

It's even more complex than what I thought. Results are sent to the client when a modification is applied, through code like:

    public void modify( final ModifyOperationContext modifyContext ) throws LdapException
    {
        Entry oriEntry = modifyContext.getEntry();

        // modification was already done when this flag is turned on, move to sending the events
        if ( !modifyContext.isPushToEvtInterceptor() )
        {
            next( modifyContext );
        }

        List<RegistrationEntry> selecting = getSelectingRegistrations( modifyContext.getDn(), oriEntry );

        if ( selecting.isEmpty() )
        {
            return;
        }

        // Get the modified entry
        CoreSession session = modifyContext.getSession();
        LookupOperationContext lookupContext = new LookupOperationContext( session, modifyContext.getDn(),
            SchemaConstants.ALL_ATTRIBUTES_ARRAY );
        lookupContext.setPartition( modifyContext.getPartition() );
        lookupContext.setTransaction( modifyContext.getTransaction() );

        Entry alteredEntry = directoryService.getPartitionNexus().lookup( lookupContext );
        modifyContext.setAlteredEntry( alteredEntry );

        for ( final RegistrationEntry registration : selecting )
        {
            if ( EventType.isModify( registration.getCriteria().getEventMask() ) )
            {
                fire( modifyContext, EventType.MODIFY, registration.getListener() );
            }
        }
    }

As you can see, before passing into the listener (list code line) we first fetch the entry ("get the modified entry") which do a lookup using the context. I do think we then pass through the authorization interceptor.

To be verified... (and for all operations)

[coheigea]

OK thanks @elecharny . Should I close this Pr if you want to pursue a different way of fixing it?

[elecharny]

I still have to add some unit test for this one. I have little time atm, but I'll try to dedicate a part of it to this PR, so please, keep it open.

Thanks :-)