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

Compabilitiy with NC15 (was: NC13) and documentation about mimetype mapping for Keeweb #81

Closed
wants to merge 10 commits into from

Conversation

arnowelzel
Copy link
Collaborator

@arnowelzel arnowelzel commented Jul 7, 2018

As discussed in #67 this is the description how to add the mimetype mapping for Keeweb to work properly. I tested this in my own Nextcloud 13 15 and it works fine (of course with an updated appinfo/info.xml to indicate Keeweb as compatible to NC 13 15).

@mr-bolle
Copy link

mr-bolle commented Jul 8, 2018

Amazing, i follow your suggestion, and with the mimetype mapping the app works with Nextcloud 13.
Could you adjust your pull request, and change the appinfo/info.xml to
<nextcloud min-version="11" max-version="13" />

many thanks for your Solution

@arnowelzel arnowelzel changed the title Documentation how to add the required mimetype mapping Compabilitiy with NC13 and documentation about mimetype mapping for Keeweb Jul 8, 2018
Copy link

@FloThinksPi FloThinksPi left a comment

Choose a reason for hiding this comment

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

Best solution so far to get this back working.
Works for me and is better than waiting for NC cahnges to include mime type selection by plugins.

@jhass
Copy link
Owner

jhass commented Jul 17, 2018

Hey, first of all thank you!

If we do this we should also drop the code at https://github.com/jhass/nextcloud-keeweb/blob/master/keeweb/appinfo/app.php#L61-L66

However a big pain point I still see here, and I wonder how the mentioned plugins do that, how is this communicated when installing a plugin from the appstore?

@jhass
Copy link
Owner

jhass commented Jul 17, 2018

Also https://docs.nextcloud.com/server/13/admin_manual/configuration_mimetypes/index.html suggests to just create the file with the additional mimetypes and some steps afterwards.

@arnowelzel
Copy link
Collaborator Author

@jhass Interesting - according to https://github.com/jhass/nextcloud-keeweb/blob/master/keeweb/appinfo/app.php#L61-L66 it looks like there is an API call in Nextcloud to register your own MIME type. But it does not work, or does it?

@jhass
Copy link
Owner

jhass commented Jul 17, 2018

No, it's not thoroughly affecting everything, it fails short on influencing the detector.

@arnowelzel
Copy link
Collaborator Author

@jhass About the problem "how to communicate the needed changes to make the app work": I also use Ownpad in my Nextcloud installation and this links to the documentation within the store description. The link "user documentation" is a direct link to the description in Github: https://github.com/otetard/ownpad/blob/master/README.md#mimetype-detection

image

@arnowelzel
Copy link
Collaborator Author

@reiner-text I created a separate issue (#82) for this, as this not only affects the documentatio but the code as well.

keeweb/appinfo/info.xml Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
@arnowelzel arnowelzel changed the title Compabilitiy with NC13 and documentation about mimetype mapping for Keeweb Compabilitiy with NC15 and documentation about mimetype mapping for Keeweb Jan 15, 2019
@arnowelzel arnowelzel changed the title Compabilitiy with NC15 and documentation about mimetype mapping for Keeweb Compabilitiy with ~~NC13~~ NC15 and documentation about mimetype mapping for Keeweb Jan 15, 2019
@arnowelzel arnowelzel changed the title Compabilitiy with ~~NC13~~ NC15 and documentation about mimetype mapping for Keeweb Compabilitiy with NC15 (was: NC13) and documentation about mimetype mapping for Keeweb Jan 15, 2019
Updated explanation how to add the custom MIME type for Keeweb.
@jhass
Copy link
Owner

jhass commented Jan 16, 2019

Thank you, I'll take a look at this later this week, probably Friday.

@jhass
Copy link
Owner

jhass commented Jan 16, 2019

I really don't know the rules of properly making up a mimetype and it's certainly not my place to care for an IANA registration. Anyway, this discussion is pretty out of scope of this PR.

@jhass
Copy link
Owner

jhass commented Jan 16, 2019

The readme explains how to register the type the application expects. Changing this type is a entirely unrelated change.

@arnowelzel
Copy link
Collaborator Author

arnowelzel commented Jan 16, 2019

Let's do one thing at a time.

  1. Merge the changes in README to explain how to add the mime type in Nextcloud and the CSS changes from Bump max-version to 14 and fix css width #85 - so the app will work as it is
  2. AFTER this change, resolve Change media type to application/x-keepass #82 by changing the type from x-application/kdbx to the correct value application/x-kdbx (in the app and the README - I can provide a speparate pull request, if needed)

@arnowelzel arnowelzel mentioned this pull request Jan 16, 2019
@e-alfred
Copy link

@jhass Maybe my comment on how another app handles mimetypes/file extensions could be of interest:

#67 (comment)

@jhass
Copy link
Owner

jhass commented Jan 18, 2019

Unfortunately I wasn't able to get Keeweb loading in Nextcloud 15. While the iframe-source URL shows and loads keeweb properly, it fails to embed, or rather show, within the iframe for some reason. There's no error message in the browser console, the JS seems to run, but the DOM inside the iframe doesn't update so the screen stays blank. I'm out of ideas for now. I pushed my WIP to https://github.com/jhass/nextcloud-keeweb/tree/nc15-wip

@flower1024 @myxor since you guys seems to have invested most into getting this running on NC14, did that actually ever work out for you? Are your forks working for NC15 for you? None of your changes seemed to help for me unfortunately :/

@eloo
Copy link

eloo commented Jan 19, 2019

@jhass
not sure this is related to your problems. but as mentioned in #89 there is an issue with the csp config related to unsafe-eval.
i'm running nc-keeweb from the appstore with the changes from the nc14 pr under nc15.0.2 after i've adapted the unsafe-eval allowed property.

@jhass
Copy link
Owner

jhass commented Jan 19, 2019

What exactly did you change where? If you check my branch I did a clean very open CSP policy

@eloo
Copy link

eloo commented Jan 20, 2019

@jhass
i've reverted the change of
nextcloud/server@5b61ef9#diff-ea7fde28f6b4be25c841ed420fef5cdf

 	protected $evalScriptAllowed = false;

and set it to true...

maybe you have already fixed this in your master? but with the last release of your plugin the revert is neccessary

@jhass
Copy link
Owner

jhass commented Jan 22, 2019

Well, modifying the server code is certainly not a solution for the plugin. Tbh I thought my new CSP policy would have about the same effect though, obviously I missed something :(

It's just weird that I get zero CSP violations logged in any browsers console.

@eloo
Copy link

eloo commented Jan 22, 2019

@jhass maybe we are misunderstooding each other..
i've not tested your last recent changes.. im using the normal app from the store with the small changes for fixing css and version...
if you're already covering the csp in your last recent changes maybe the problem is already fixed.

@arnowelzel
Copy link
Collaborator Author

arnowelzel commented Jan 27, 2019

It is not neccessary to modify code in the core. You can just add this to the CSP:

		$csp->allowEvalScript(true);

But for me this does not change anything. Keeweeb doesn't load if it is newer than 1.5 :-(

@andmor-
Copy link

andmor- commented Feb 9, 2019

To make it compatible with NC15.0.4 and Keeweeb 1.7.6, undo this commit for keeweb source and add this to the CSP:

$csp->addAllowedChildSrcDomain("blob:");

I'm not sure I you would prefer to do this. I created PR #90 using sed but perhaps a patch would be more appropriate

@jhass
Copy link
Owner

jhass commented Feb 10, 2019

Woah, good catch!

I'll work on this tomorrow :)

@jhass
Copy link
Owner

jhass commented Feb 11, 2019

Merged as 26d5926

Thank you!

@jhass jhass closed this Feb 11, 2019
@andmor-
Copy link

andmor- commented Feb 11, 2019

Much nicer solution! Also nice that Keeweb will implement an option for the iframe issue 👍
Thank you for your work on this

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

9 participants