Skip to content

fix possible leaking scope in Flow - #22359

Merged
nickvergessen merged 1 commit into
masterfrom
fix/noid/flow-leaking-scope
Aug 25, 2020
Merged

fix possible leaking scope in Flow#22359
nickvergessen merged 1 commit into
masterfrom
fix/noid/flow-leaking-scope

Conversation

@blizzz

Copy link
Copy Markdown
Member

How to reproduce:

  1. have two Notification operations set with two different entities: e.g. file change and webhook (https://github.com/nextcloud/flow_webhooks)
  2. trigger a webook (curl -i -X POST -H "Content-Type: application/json" -d '{"foo": "bar"}' https://my.nc.srv/ocs/v2.php/apps/flow_webhooks/api/v1/hook/1234567890, see personal config)
  3. realize a runtime exception was thrown, because file info was not set

The reason is that the getFlows() method does not take the event into account that is being currently processed.

This draft implements an API extension to IRuleMatcher to fix this, and also to backport it. Since it is not a component that is implemented by third parties, but initialized by ourselves. Existing methods are not touched. Thus it is safe to backport. Usual operation implementation do not need to worry about this as it dealt with in the Manager code. IComplexOperations might need to use that method, if it possible to react and configure different events. However, they are picked up by themselves (and they do event listening themselves), so even in these cases it is extremely unlikely that this needs to be used.

@blizzz

Copy link
Copy Markdown
MemberAuthor

/backport to stable19

@blizzz

Copy link
Copy Markdown
MemberAuthor

/backport to stable18

@rullzerrullzer left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

sure

@rullzer

Copy link
Copy Markdown
Member

php-cs says no ;)

@blizzz

blizzz commented Aug 24, 2020

Copy link
Copy Markdown
MemberAuthor

php-cs says no ;)

it always does the first few pushes

- a configured flow can be brought into consideration, despite its event
was not fired
- it could either run through
- or run into a RuntimeException and killing processing of valid flows
Signed-off-by: Arthur Schiwon <blizzz@arthur-schiwon.de>
@blizzz
blizzzforce-pushed the fix/noid/flow-leaking-scope branch from 6eb56fc to 28c0eeaCompareAugust 24, 2020 11:44
@ChristophWurstChristophWurst added 4. to release Ready to be released and/or waiting for tests to finish and removed 3. to review Waiting for reviews labels Aug 24, 2020
@faily-bot

faily-botBot commented Aug 24, 2020

Copy link
Copy Markdown

🤖 beep boop beep 🤖

Here are the logs for the failed build:

Status of 32247: failure

mysql5.6-php7.2

Show full log
There was 1 error:
1) Test\Files\Storage\Wrapper\PermissionsMaskTest::testScanNewFiles
Error: Call to a member function getPermissions() on boolean
/drone/src/tests/lib/Files/Storage/Wrapper/PermissionsMaskTest.php:117
--
There were 2 warnings:
1) Test\Files\ViewTest::testRenameFailDeleteTargetKeepSource
Trying to configure method "writeStream" which cannot be configured because it does not exist, has not been specified, is final, or is static
2) Test\Files\ViewTest::testCopyFailDeleteTargetKeepSource
Trying to configure method "writeStream" which cannot be configured because it does not exist, has not been specified, is final, or is static

acceptance-app-files

  • tests/acceptance/features/app-files.feature:262
Show full log
 Scenario: unmarking a file as favorite causes the file list to be sorted again # /drone/src/tests/acceptance/features/app-files.feature:262
Given I am logged in # LoginPageContext::iAmLoggedIn()
And I create a new folder named "A name alphabetically lower than welcome.txt" # FileListContext::iCreateANewFolderNamed()
And I see that "A name alphabetically lower than welcome.txt" precedes "welcome.txt" in the file list # FileListContext::iSeeThatPrecedesInTheFileList()
And I close the details view # FilesAppContext::iCloseTheDetailsView()
And I see that the details view is closed # FilesAppContext::iSeeThatTheDetailsViewIsClosed()
And I mark "welcome.txt" as favorite # FileListContext::iMarkAsFavorite()
And I see that "welcome.txt" is marked as favorite # FileListContext::iSeeThatIsMarkedAsFavorite()
And I see that "welcome.txt" precedes "A name alphabetically lower than welcome.txt" in the file list # FileListContext::iSeeThatPrecedesInTheFileList()
When I unmark "welcome.txt" as favorite # FileListContext::iUnmarkAsFavorite()
Then I see that "welcome.txt" is not marked as favorite # FileListContext::iSeeThatIsNotMarkedAsFavorite()
Not favorited state icon for file welcome.txt in file list could not be found after 100 seconds (NoSuchElementException)
And I see that "A name alphabetically lower than welcome.txt" precedes "welcome.txt" in the file list # FileListContext::iSeeThatPrecedesInTheFileList()

acceptance-header

  • tests/acceptance/features/header.feature:31
Show full log
 Scenario: users from other groups are not seen in the contacts menu when autocompletion is restricted within the same group # /drone/src/tests/acceptance/features/header.feature:31
Given I am logged in as the admin # LoginPageContext::iAmLoggedInAsTheAdmin()
And I visit the settings page # SettingsMenuContext::iVisitTheSettingsPage()
And I open the "Sharing" section of the "Administration" group # AppNavigationContext::iOpenTheSectionOf()
And I enable restricting username autocompletion to groups # SettingsContext::iEnableRestrictingUsernameAutocompletionToGroups()
And I see that username autocompletion is restricted to groups # SettingsContext::iSeeThatUsernameAutocompletionIsRestrictedToGroups()
When I open the Contacts menu # ContactsMenuContext::iOpenTheContactsMenu()
Then I see that the Contacts menu is shown # ContactsMenuContext::iSeeThatTheContactsMenuIsShown()
And I see that the contact "user0" in the Contacts menu is not shown # ContactsMenuContext::iSeeThatTheContactInTheContactsMenuIsNotShown()
Failed asserting that true is false.
And I see that the contact "admin" in the Contacts menu is not shown # ContactsMenuContext::iSeeThatTheContactInTheContactsMenuIsNotShown()

@nickvergessen
nickvergessen merged commit 0934aee into masterAug 25, 2020
@nickvergessen
nickvergessen deleted the fix/noid/flow-leaking-scope branch August 25, 2020 08:40
@rullzerrullzer mentioned this pull request Aug 25, 2020
19 tasks
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

4. to releaseReady to be released and/or waiting for tests to finishbugfeature: workflows

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants

@blizzz@rullzer@ChristophWurst@juliusknorr@nickvergessen