mORMot Open Source

DigDiver

Member

Registered: 2013-04-29

Posts: 137

Check user access rights in the function TSQLRestServer.EngineBatchSen

I need to check if the current logged user has sufficient rights to perform BatchSend.

Though the function TSQLRestServer.EngineBatchSend calls RecordCanBeUpdated, it's possible to check the user access rights in the overload function RecordCanBeUpdated,
but in that case the checking will be called fo each inserted record which will slow down the process of adding records considerably.

function TGroupServer.RecordCanBeUpdated(Table: TSQLRecordClass; ID: integer;
  Action: TSQLEvent; ErrorMsg: PRawUTF8): boolean;
var
 U    : TAuthUser;
 intU : TAuthUser;
 G    : TAuthGroup;
 f    : TServiceRunningContext;
begin
 if (Table <> TEmails) and (Table <> TExclusionList)  then exit;

 f :=  CurrentServiceContext;
 if Assigned(f.Request) then begin

 U := Self.SessionGetUser(f.Request.Session) as TAuthUser;
 if Assigned(U) then
 try
  intU := TAuthUser.CreateAndFillPrepare(self, 'LogonName=?', [U.LogonName]);
  try
   intU.FillOne;
   g := TAuthGroup.CreateAndFillPrepare(self, 'ID=?', [intU.GroupRights.ID]);
   try
    g.FillOne;
    if ((Action = seUpdate) and ((g.Ident = 'Guest') or not (contact_edit   in G.AccessInfo))) or
       ((Action = seAdd)    and ((g.Ident = 'Guest') or not (contact_add    in G.AccessInfo))) or
       ((Action = seDelete) and ((g.Ident = 'Guest') or not (contact_delete in G.AccessInfo)))  then
     begin
      ErrorMsg^ := StringToUTF8(sNoRights);
      Result    := False;
     end;
   finally
    g.Free;
   end;
  finally
   intU.Free;
  end;
 finally
  U.Free;
 end;
 end;

end;

I slightly modified the function TSQLRestServer.EngineBatchSend in order the checking of the user access rights is called one time before the start of BatchSend

function TSQLRestServer.EngineBatchSend(Table: TSQLRecordClass;
  const Data: RawUTF8; var Results: TIntegerDynArray): integer;
...
begin
  Sent := pointer(Data);
  if Sent=nil then
    raise EORMBatchException.CreateUTF8('%.EngineBatchSend(%,"")',[self,Table]);
  if Table<>nil then begin
    // unserialize expected sequence array as '{"Table":["cmd",values,...]}'
    while not (Sent^ in ['{',#0]) do inc(Sent);
    if Sent^<>'{' then
      raise EORMBatchException.CreateUTF8('%.EngineBatchSend: Missing {',[self]);
    inc(Sent);
    TableName := GetJSONPropName(Sent);
    if (TableName='') or (Sent=nil) then
      raise EORMBatchException.CreateUTF8('%.EngineBatchSend: Wrong "Table":"%"',
        [self,TableName]);

 // NEW CODE ===========================================

  if not BatchCanPerformed(Table,@ErrMsg)  then
    raise EORMBatchException.CreateUTF8('%.EngineBatchSend impossible: %',
     [self,ErrMsg]);

// END NEW CODE ========================================

  end; //
  

  What do you think about this modification?

ab

Administrator

From: France

Registered: 2010-06-21

Posts: 14,662

Website

Re: Check user access rights in the function TSQLRestServer.EngineBatchSen

First of all, your TGroupServer.RecordCanBeUpdated() function is a bit broken.
If the table is not one the 2 you expect, it returns immediately - leaving result in an unknown state: true? false? - it will depend on the garbage on the CPU stack at this moment.

What is this "BatchCanPerformed()" method?

In all cases, your modification will only work if there is a Table specified at Batch, which may not be the case.
The test is done later one, in the process loop.
RecordCanBeUpdated() is already called, per ID, in the loop.

IMHO if you want such level of application-level security, you should just disallow BATCH process from the client side, and use instead a "Repository" interface-based service with proper access policy, depending on your business logic.

What we may do is add some new attribute for TSQLAllowRemoteExecute, and explicitly check the TSQLAccessRights for the current logged user.

DigDiver

Member

Registered: 2013-04-29

Posts: 137

Re: Check user access rights in the function TSQLRestServer.EngineBatchSen

What we may do is add some new attribute for TSQLAllowRemoteExecute, and explicitly check the TSQLAccessRights for the current logged user.

I think this will be the best solution.

ab

Administrator

From: France

Registered: 2010-06-21

Posts: 14,662

Website

Re: Check user access rights in the function TSQLRestServer.EngineBatchSen

OK.
We would need to add those new security attributes, for BATCH process.

Could you please add a feature request ticket, with a link to this forum thread?

DigDiver

Member

Registered: 2013-04-29

Posts: 137

Re: Check user access rights in the function TSQLRestServer.EngineBatchSen

OK. http://synopse.info/fossil/tktview/27cf … 08c333205f

ab

Administrator

From: France

Registered: 2010-06-21

Posts: 14,662

Website

Re: Check user access rights in the function TSQLRestServer.EngineBatchSen

We have fixed the BATCH process to check for the TSQLAccessRights of the current logged user just like other CRUD methods.
This was in fact a major potential security breach.

See http://synopse.info/fossil/info/77c4011676