Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Antivirus update #111

Merged
merged 4 commits into from
Jul 8, 2016
Merged

Antivirus update #111

merged 4 commits into from
Jul 8, 2016

Conversation

VicDeo
Copy link
Member

@VicDeo VicDeo commented May 4, 2016

  • Where possible: Use ===, remove deprecated calls
  • Fix saving rules at admin page
  • Fix broken tests

@VicDeo VicDeo added this to the 9.1-current milestone May 4, 2016
@mention-bot
Copy link

By analyzing the blame information on this pull request, we identified @artem-sidorenko, @fsbruva and @DeepDiver1975 to be potential reviewers

@VicDeo VicDeo force-pushed the antivirus-update branch 2 times, most recently from 17f982c to a0b6395 Compare June 9, 2016 17:34
@VicDeo VicDeo changed the title [WIP] Antivirus update Antivirus update Jun 9, 2016
@VicDeo VicDeo force-pushed the antivirus-update branch 5 times, most recently from 662e460 to 6714a60 Compare June 9, 2016 19:26
@VicDeo
Copy link
Member Author

VicDeo commented Jun 29, 2016

@icewind1991 may I ask you to review this PR?

$qb->expr()->like('fc.path', $qb->expr()->literal('files/%'))
)
->andWhere(
$qb->expr()->neq('fc.size', '?1')
Copy link
Contributor

Choose a reason for hiding this comment

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

'?1' should be 0?

@icewind1991
Copy link
Contributor

Looks good otherwise

@PVince81 PVince81 modified the milestones: 9.2, 9.1 Jul 6, 2016
@PVince81
Copy link
Contributor

PVince81 commented Jul 6, 2016

@VicDeo from the IRC discussion it seems that this fix is critical to make antivirus work again with OC >= 9.0 ?

@PVince81
Copy link
Contributor

PVince81 commented Jul 6, 2016

@VicDeo how to test ? Is this only about background scanning, not upload time scanning ?

CC @owncloud/qa

@PVince81
Copy link
Contributor

PVince81 commented Jul 6, 2016

Will need backport to 9.0 and 9.1

CC @DeepDiver1975 @dragotin

@VicDeo
Copy link
Member Author

VicDeo commented Jul 6, 2016

@PVince81 it doesn't fix backround job itself, but a couple of other issues (see the first message)
But the background job is changed with this PR so it's better to merge it first

Fix saving rules at admin page

  1. modify rule in advanced section at the admin page
  2. click save
  3. refresh page

Expected: changes are saved
Actual: they are not

Also it moves adding a cron task to the install script instead of adding it any time app.php is executed.

@SergioBertolinSG
Copy link
Contributor

Doesn't work for me. 👎

The rule is not saved.

Also if the save button is required also for rules, it should be duplicated or moved to the bottom of the antivirus section. Otherwise it is a bad UX.

BTW what is the difference in rules between "Infected" and "Clean" ?

@VicDeo
Copy link
Member Author

VicDeo commented Jul 7, 2016

@SergioBertolinSG

save button is required also for rules

It is not. It saves non-advanced section.
As soon as rule was added, it is saved by clicking checkmark at the left. The same is true for changing a rule

BTW what is the difference in rules between "Infected" and "Clean" ?

If av binary/socket/etc response contains something that matches "Infected" rule, the file is considered to be infected.
As soon all infected rules are tested, the response is checked against "Clean" rules.

In no rules matched the file status is unknown.

@VicDeo VicDeo closed this Jul 7, 2016
@VicDeo VicDeo reopened this Jul 7, 2016
@SergioBertolinSG
Copy link
Contributor

SergioBertolinSG commented Jul 8, 2016

OK, I didn't understand the interface. I suggest an icon change, something like ✅ for me doesn't look like a save button. Filed here in a different issue #115

👍

$qb->expr()->eq('fc.storage', 'ss.numeric_id'),
$qb->expr()->orX(
$qb->expr()->like('ss.id', $qb->expr()->literal('local::%')),
$qb->expr()->like('ss.id', $qb->expr()->literal('home::%'))
Copy link
Contributor

Choose a reason for hiding this comment

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

note: this won't work for md5 storage ids https://github.com/owncloud/core/wiki/Storage-IDs#random-numberstring
Will need separate fixing.

The way how repair steps do is iterate over all users and get their home storages... yes, cumbersome.

Copy link
Contributor

Choose a reason for hiding this comment

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

Ah, and starting with OC 9.0 it will be possible to found the home mount from the oc_mounts table

@PVince81
Copy link
Contributor

PVince81 commented Jul 8, 2016

Code looks good 👍

Needs backport ?

@PVince81 PVince81 merged commit db5d43d into master Jul 8, 2016
@PVince81 PVince81 deleted the antivirus-update branch July 8, 2016 07:59
@VicDeo
Copy link
Member Author

VicDeo commented Jul 8, 2016

@PVince81 yes, otherwise rules in advanced section are not changed/deleted

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.

None yet

5 participants