Rewrite/refactor delagent and fix #273 #646

maxhbr · 2016-03-01T09:14:03Z

This PR

adds permission handling for directories
refactors delagent
repairs permission checking

this will fix #273
see #602 for the discussion on the previous PR concerned with this issue

This PR should not be merged yet.

maxhbr · 2016-03-01T09:20:40Z

@shaheemazmalmmd: even with the debugger i was not able to find the difference in

PGresult * PQexecCheck(const char *desc, char *SQL, char *file, const int line)
{
  PGresult *result;
  [...]
  result = PQexec(db_conn, SQL);
  if (fo_checkPQcommand(db_conn, result, SQL, file, line))
  {
    exit(-1);
  }
  return result;
}
[...]
#if 0
  result = PQexecCheck(NULL, SQL, __FILE__, __LINE__);
#else
  result = PQexec(db_conn, SQL);
  if (fo_checkPQresult(db_conn, result, SQL, __FILE__, __LINE__))
  {
    exit(-1);
  }
#endif
[...]

I would suggest to remove the #if 0 case.

~~Btw.: I would rebase this PR if this is OK for you?~~ DONE

maxhbr · 2016-03-01T13:15:09Z

@shaheemazmalmmd: I have already rebased it but I can revert it if you have problems with that.

shaheemazmalmmd · 2016-03-02T04:58:41Z

no, good that you rebased it. i missed it yesterday.

maxhbr · 2016-03-02T08:26:41Z

I think that we have to have a look at the tests for delagent. I think at least the functional agent tests are outdated and also the unit tests should be extended.

~~There is currently also a logic problem in the code where uploads contained in a Folder get deleted, even without write permissions. I will try to fix this.~~ Fixed

maxhbr · 2016-03-09T10:11:32Z

Problem: fixed with next commit

./delagent --user rUser --password --rPassword -f, where rUser is a user with read permissions, lists all uploads and ignores permissions

~~On the other side does ./delagent [...] -u only list uploads where one has read permissions.~~ UPDATE: ./delagent [...] -u only lists uploads, where user is mentioned as user_fk.

Possible solution

~~Use logic from UploadPermissionDao.php~~ Was done with the GetUploadPerm(..) function.

maxhbr · 2016-03-09T15:11:16Z

@shaheemazmalmmd: I have manually tested that PR and it looks OK. Do you have any ideas what could be missing? Otherwise I would ask @steffen-weber or @wuan to review this PR. After that I might squash the commits and remove that silly #if 0 thing.

BUT: I am still not able to run the functional tests. The used scripts

src/testing/db/createTestDB.php
src/testing/lib/libTestDB.php and
src/testing/db/gtdbcreate.sh

seam to be outdated. I think their functionality should be replaced by the php classes

Fossology\Lib\Test\TestPgDb together with
Fossology\Lib\Test\TestInstaller.

But in this environment one has no deployed www and thus cant use the upload functionality to set the DB up.

coveralls · 2016-03-21T14:41:46Z

Coverage decreased (-0.0002%) to 13.363% when pulling 9d846a2 on siemens:dev/rewriteDelagentPermissions into 09ed2a4 on fossology:master.

any user who is admin or owner can delete the folder, not all users

@shaheemazmalmmd

* pass the variables `user_id` and `user_perm` to all functions where they are needed * extract the permission checks into three functions (for uploads, folders, licenses) * start adding the permission checks into the correct places (not done yet, not tested yet) * move `admin_folder_delete.php` to `delagent/ui` * general rewriting of the delagent partly based on work by @shaheemazmalmmd

maxhbr · 2016-03-22T10:31:58Z

Hello,

we need someone to review this PR. Maybe @wuan or @steffen-weber? Or maybe even @fogninid since this is essentially c-code?

When someone volunteers, I will support him actively and give an overview on the changes.

@shaheemazmalmmd

... and more refactoring by @shaheemazmalmmd: style(delagent): Alignment of code removed tabs and added spaces

* TODO: add tests!

* in `src/delagent/ui/admin-upload-delete.php` was no check for permissions, this enabled eveyone to delete every file with the correct POST-request

coveralls · 2016-03-22T10:56:10Z

Coverage decreased (-0.01%) to 13.36% when pulling eb1856a on siemens:dev/rewriteDelagentPermissions into cc73013 on fossology:master.

coveralls · 2016-03-22T11:22:59Z

Coverage decreased (-0.01%) to 13.363% when pulling 639221c on siemens:dev/rewriteDelagentPermissions into cc73013 on fossology:master.

maxhbr · 2016-03-22T15:49:54Z

Oh sorry, instead of @fogninid I wanted to write @joshovi.

joshovi · 2016-03-23T17:39:01Z

src/delagent/agent/delagent.c



+/***********************************************
+ Usage():


I would prefer all function names and variable names to be start with lower case. It should be uniform at least.

joshovi · 2016-03-23T17:45:02Z

src/delagent/agent/delagent.h

+int check_permission_folder(long folder_id, int user_id, int user_perm);
+int check_permission_license(long license_id, int user_perm);
+
+/* functions that list things */


does the captital letter at the start signify anything?

No, that is just the style the code was written...

coveralls · 2016-03-23T20:42:27Z

Coverage decreased (-0.01%) to 13.36% when pulling bdc88f0 on siemens:dev/rewriteDelagentPermissions into cc73013 on fossology:master.

maxhbr · 2016-03-23T20:49:10Z

I will refactor the returncodes tomorrow

coveralls · 2016-03-23T21:08:20Z

Coverage decreased (-0.003%) to 13.371% when pulling 0e7c179 on siemens:dev/rewriteDelagentPermissions into cc73013 on fossology:master.

joshovi · 2016-03-29T18:12:44Z

src/delagent/agent/delagent.c

-      if (ReadParameter(Parm) < 0)
-        exit(-1);
+      returnedCode = listUploads(user_id, user_perm);
+    }


if returnedCode != 0 you might want to leave the program here before you set it again if listFolder is true

You are right. Its fixed now

joshovi · 2016-03-29T18:24:53Z

src/delagent/agent/delagent.c

+
+      writeMessageAfterDelete("upload", delUpload, user_name, returnedCode);
+    }
+    if (delFolder)


same here, or are delUpload and delFolder mutually exclusive?

The function writeMessageAfterDelete keeps track of that and exits if the returned code is not 0

maxhbr · 2016-03-29T18:57:19Z

@joshovi: your last mentioned issues are solved with the commit 4958b92. When merging this should be squashed into 1676498

- rewritten /refactored functions: - `DeleteUpload` - `DeleteLicense` - `UnlinkContent` - and more - rewrite naming of `TempTable` - remove all unnecessary `memset` calls - make return codes more consistend - remove dead code - die on permission failure - this change was made to show delagent jobs, which have had permission problems, as failed agents (i.e. red).

* The function `SHA1(...)` could generate strings containing `\000` which did not work with the following call of `strlen(...)` * This is solved, since one knows that a SHA1 hash as hex is 40 characters long (20 chars for the raw value) * The old test, whether the creation of the sha1 failed, can not work since the first char could also be `\000` ==> removed

@joshovi

as suggested by @joshovi

`rc=0` should always be the "good" case `rc>0` is a permission problem `rc<0` is a database connection problem, which has to be propagated out

maxhbr added enhancement needs code review labels Mar 1, 2016

maxhbr force-pushed the dev/rewriteDelagentPermissions branch from 5d77b16 to b4645c4 Compare March 1, 2016 13:14

maxhbr changed the title ~~Rewrite/refactor delagent~~ Rewrite/refactor delagent and fix #273 Mar 2, 2016

maxhbr force-pushed the dev/rewriteDelagentPermissions branch 3 times, most recently from fd6355d to 9d846a2 Compare March 21, 2016 14:12

shaheemazmalmmd and others added 2 commits March 22, 2016 11:23

feature(ui): Added userid to folder table

c8f0837

any user who is admin or owner can delete the folder, not all users

maxhbr force-pushed the dev/rewriteDelagentPermissions branch from 9d846a2 to eb1856a Compare March 22, 2016 10:26

maxhbr and others added 5 commits March 22, 2016 11:36

refactor(delagent): split ListFolders-function

cb7c917

... and more refactoring by @shaheemazmalmmd: style(delagent): Alignment of code removed tabs and added spaces

fix(testcases): insert query fixed as part of phpunit tests

30c1fda

refactor(delagent): refactoring and fixing of double PQclear(...)

0722666

refactor(libfossagent): rewrite GetUploadPerm

f617dc6

* TODO: add tests!

fix(ui): check for perms bevore deleting upload

f9e5c1c

* in `src/delagent/ui/admin-upload-delete.php` was no check for permissions, this enabled eveyone to delete every file with the correct POST-request

maxhbr force-pushed the dev/rewriteDelagentPermissions branch from eb1856a to 639221c Compare March 22, 2016 10:39

joshovi reviewed Mar 23, 2016
View reviewed changes

maxhbr force-pushed the dev/rewriteDelagentPermissions branch from 1cd77fe to bf14882 Compare March 24, 2016 11:17

joshovi reviewed Mar 29, 2016
View reviewed changes

maxhbr force-pushed the dev/rewriteDelagentPermissions branch from bf14882 to 579b71b Compare March 29, 2016 18:17

joshovi reviewed Mar 29, 2016
View reviewed changes

maxhbr added 5 commits March 29, 2016 21:27

refactor(delagent): refactor function names

f43713f

as suggested by @joshovi

refactor(delagent): rewrite SQL query

117d7cb

refactor(delagent): refactor return codes

cc17579

`rc=0` should always be the "good" case `rc>0` is a permission problem `rc<0` is a database connection problem, which has to be propagated out

maxhbr force-pushed the dev/rewriteDelagentPermissions branch from 4958b92 to cc17579 Compare March 29, 2016 19:29

joshovi removed the needs code review label Mar 29, 2016

joshovi merged commit 8e630b2 into fossology:master Mar 29, 2016

shaheemazmalmmd mentioned this pull request Apr 6, 2016

error when run ./delagent -T #368

Closed

mcjaeger mentioned this pull request Mar 13, 2017

Any normal user can remove anyone's uploads using delagent from the command line #186

Closed



		/***********************************************
		Usage():

Rewrite/refactor delagent and fix #273 #646

Rewrite/refactor delagent and fix #273 #646

Uh oh!

Conversation

maxhbr commented Mar 1, 2016

Uh oh!

maxhbr commented Mar 1, 2016

Uh oh!

maxhbr commented Mar 1, 2016

Uh oh!

shaheemazmalmmd commented Mar 2, 2016

Uh oh!

maxhbr commented Mar 2, 2016

Uh oh!

maxhbr commented Mar 9, 2016

Problem: fixed with next commit

Possible solution

Uh oh!

maxhbr commented Mar 9, 2016

Uh oh!

coveralls commented Mar 21, 2016

Uh oh!

maxhbr commented Mar 22, 2016

Uh oh!

coveralls commented Mar 22, 2016

Uh oh!

coveralls commented Mar 22, 2016

Uh oh!

maxhbr commented Mar 22, 2016

Uh oh!

joshovi Mar 23, 2016

Choose a reason for hiding this comment

Uh oh!

joshovi Mar 23, 2016

Choose a reason for hiding this comment

Uh oh!

maxhbr Mar 23, 2016

Choose a reason for hiding this comment

Uh oh!

coveralls commented Mar 23, 2016

Uh oh!

maxhbr commented Mar 23, 2016

Uh oh!

coveralls commented Mar 23, 2016

Uh oh!

joshovi Mar 29, 2016

Choose a reason for hiding this comment

Uh oh!

maxhbr Mar 29, 2016

Choose a reason for hiding this comment

Uh oh!

joshovi Mar 29, 2016

Choose a reason for hiding this comment

Uh oh!

maxhbr Mar 29, 2016

Choose a reason for hiding this comment

Uh oh!

maxhbr commented Mar 29, 2016

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants