step-up not triggered, 'this operation is forbidden'

[michielbdejong]

Split-out from #89 (comment)

[michielbdejong]

This is with commit 21ed0179b4b of the mfazones app, and it looks like neither SUNET/nextcloud-mfazones#10 nor SUNET/nextcloud-mfazones#11 covers this.

#10 provides the step-up when trying to add the MFA requirement (this can be seen in the screencast and also in our new sunet-custom test setup:

But it doesn't deal with inaccessible folders.
And #11 deals with triggering local verification if configured and coming through GSS (I haven't tested this myself yet btw, see #91)

[michielbdejong]

The added tab view is registered through https://github.com/nextcloud/server/blob/master/apps/files/js/filelist.js#L3884
I can probably do something with https://github.com/nextcloud/server/blob/master/apps/files/js/fileactions.js#L122 to add an 'MFA Zone' icon next to the 'share' icon for these inaccessible list entries.

[michielbdejong]

We're testing this with https://github.com/pondersource/nextcloud-mfa-awareness#setup-gsssh-testnet

[michielbdejong]

Steps to reproduce:

• follow the steps from the readme
• now run docker exec -it sunet-mdb1 mariadb -u sspuser -psspus3r -h sunet-ssp-mdb saml -e "update users set mfa_verified=0;"
• docker restart sunet-tester
• on localhost:5800 log in to http://sunet-nc2 as usr1 / pwd1 again
• in the MFA Checker app, the MFA verified checkbox should now not be checked
• go to Files, you'll see the 'asdf' folder as grey
• try to open

Expected

You will see a popup saying 'Enable MFA to enter this MFA Zone'

Actual

• you will see 'This operation is forbidden'

[michielbdejong]

Not sure how I did this earlier but I'm now seeing the files app is empty on http://sunet-nc2 over http but works OK on https://sunet-nc2 over https.

[yasharpm]

This is the code snippet that we need. But need to it over a loop with every MFA locked file name:

fileList.fileActions.registerAction({
        name: 'mfa',
        displayName: 'MFA protected',
        type: 1,
        mime: 'all',
        permissions: OC.PERMISSION_NONE,
        iconClass: 'icon-category-security',
        actionHandler: function(fileName, context) {
          OC.dialogs.info('ENABLE MFA to enter this MFA Zone', 'MFA Protected');
        },
        filename: 'asdf'
      });

The outcome looks like this:

And when clicks on the MFA button:

[michielbdejong]

@yasharpm any progress on the backend service that returns the MFA Zone filenames contained in a given folder?

[yasharpm]

@michielbdejong Just started to work on this. I'm gonna make sure that we can put our actions on specific files in the list first. Next step would be to retrieve the MFA file list from the backend. Because it might be that we already have the file tags on the list items.

[yasharpm]

This function is called on the file list with an array of the files for the current folder: FileList.prototype.setFiles. Luckily there is no pagination involved here so we got this part easy.

[yasharpm]

And in that array of files we have the file id which we can use to query the back-end.

[yasharpm]

My problem right now is to somehow intercept the call to this function to acquire the list of files and then return the control back to the original setFiles functions. Which I am having trouble to do so.

[yasharpm]

@michielbdejong I am blocked on the above.

const originalSetFiles = fileList.setFiles;

fileList.setFiles = function(filesArray) {
  console.log('FILES ARRAY', filesArray)
  originalSetFiles(filesArray)
}

Is the closest I have come to getting it working. But I am getting this error in js console logs:

jQuery.Deferred exception: this.$fileList is undefined setFiles@http://sunet-nc2/index.php/js/files/merged-index.js?v=b63f613c-0:5664:4
attach/fileList.setFiles@http://sunet-nc2/apps/mfazones/js/plugin.js?v=b63f613c-0:16:25
reloadCallback@http://sunet-nc2/index.php/js/files/merged-index.js?v=b63f613c-0:6433:9

My understanding is that the super function is getting called but it can not resolve this. I am not sure at all though.

[michielbdejong]

That particular problem can probably be solved by binding the setFiles function to the fileList object, using JavaScript function binding: https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Function/bind

IIUC the problem we're trying to solve here is to know how to set the filename filter in last week's approach.

What if we register the action for all icons, and then run some extra JavaScript to hide the icon for files that are not MFA Zones, and also to change the colour if the current user session is not MFA verified?

[michielbdejong]

As discussed, the second option would get messy when the file list changes and we don't notice. So let's go with the setFiles overwrite option

[yasharpm]

After a conversation with @michielbdejong we decided to add a plugin to the file dav that would add an additional field to the file properties which would tell us if the file is MFA protected and accessible or not.

Luckily the dav app allows installed apps to register their own plugins by design. Here is the code that refers to the apps.

I am now reading the source for the PluginManager to see what I need to do to make it possible.

[yasharpm]

Registered a plugin and I am receiving the propfind call and added a random property to files to see how it looks like on the other end. Next stop is that I have to modify the list of requested properties on the JS client side. This one seems easy enough.

[yasharpm]

I have pushed the changes so far to the issue_90 branch.

[michielbdejong]

I'm checking out https://github.com/pondersource/mfazones/tree/issue_90 now

[michielbdejong]

OK I finally got past #102 and am now able to fully reproduce #90 (comment) on my laptop again.

[michielbdejong]

I see it logging "No read permissions. This might be caused by files_accesscontrol, check your configured rules" server-side for "method":"PROPFIND","url":"/remote.php/dav/files/usr1/asdf" but don't see our own log messages appear yet. Will continue working on this tomorrow!

[michielbdejong]

SUNET/nextcloud-mfazones#13 (comment)

[michielbdejong]

Should whitelist our property in https://github.com/nextcloud/server/blob/master/core/src/files/client.js

[michielbdejong]

Should whitelist our property in https://github.com/nextcloud/server/blob/master/core/src/files/client.js

Need to make this change in a the nextcloud source, then build, then add a line to copy it into the dist/ folder in the container https://github.com/pondersource/dev-stock/blob/main/docker/dockerfiles/nextcloud-sunet.Dockerfile#L168

[michielbdejong]

Note to self: I downloaded https://download.nextcloud.com/.customers/server/26.0.7-153512ec/nextcloud-26.0.7-enterprise.zip
to /Users/michiel/gh/nextcloud/enterprise on my laptop for this

[michielbdejong]

In that folder, running:

nvm use 14
npm install
npm run build

But it says it's missing ./src. Will try to check out v26.0.7 in ~/gh/nextcloud/server, nvm use 16, npm install, npm run build, and take it from there.

[michielbdejong]

got the patch! https://github.com/pondersource/server/commit/4888e518eb09d8dfc35df3b21c17f511011d8ad7.patch

[michielbdejong]

https://github.com/pondersource/server/commits/white-list-mfa-zone-dav-attribute/

[michielbdejong]

new dev setup for this:

• take the main branch of nextcloud-mfa-awareness
• take the issue_90 branch of mfazones inside that
• cd servers; ./clean.sh ; ./setup-gss.sh testnet
• on localhost:5800 / sunet-nc2/index.php/login?direct=1 log in as Admin / !QAZ1qaz to activate the tag and the flow
• on localhost:5801 / sunet-nc2 log in as usr1 / pwd (SAML)
• mark the Documents folder as an MFA zone
• in the web console: document.getElementsByTagName('tr')[1].attributes

[michielbdejong]

I can see <nc:requires-mfa>ponder3source</nc:requires-mfa> in the PROPFIND response. Let's see if I can debug what the client does and whether our patch is correctly applied to it.

[michielbdejong]

it seems the requires-mfa property is present in the PROFIND response body but I'm not sure where the XML parsing is taking place, and whether there is still some filter there, or perhaps it's in a different place. maybe https://www.npmjs.com/package/davclient.js is filtering?

[michielbdejong]

It's very cumbersome to be adding debug statements into NC core JS files, then building patches for them, then building docker images with those patches, then setting up the dev env with those rebuilt images, running the experiment again, and then observing 1 debug value.

I now got a patch that doesn't apply cleanly. Maybe it's because I made changes to package-lock.json. Will either have to try to freshly generate a patch from v26.0.7 or find a better modus operandi.

[michielbdejong]

Now rebuilding v26.0.7, will re-apply 1ecbeda9ec4 when done.

[michielbdejong]

OK, got that working! After 5 weeks ... sorry for the delay! Now I just have to fix:

• setting the DAV property correctly in our DAV plugin
• displaying the step-up button based on the data attribute

[michielbdejong]

Hm looks like I have a typo somewhere, will dig into it:

[michielbdejong]

Got that working! Next: SUNET/nextcloud-mfazones@412d93b#diff-e2cc6ee02c4a8910bcc506b6b7fd45b3a2784ff76aa2ed205811f940bb0954a9R51

[michielbdejong]

Got past that by using a closure. Next error:
Call to undefined method OCA\\DAV\\Files\\FilesHome::getType()

[michielbdejong]

it works!
the MFA Zone file action is hidden through CSS if the data-mfa-required attribute on the embedding tr is false.
and I made the message different for whether you are MFA verified or not.
only problem is that it doesn't seem to work in the 'shared with you' tab, maybe because the file id is different there?
not sure if I can know the original file id from there! it's not going to work for remote shares anyway.

So maybe all this work went into creating a feature that is not even useful, except for people who had MFA before and disconfigured it. Anyway, for the file owner it works so let's ship it and discuss in the new year.