Skip to content

Conversation

@ArtificialOwl
Copy link
Member

.wellknown/webfinger is used by ActivityPub to get local account. This would allow any app to catch requests using

./occ config:app:set --value path_to_phpfile core public_webfinger

Signed-off-by: Maxence Lange [email protected]

@MorrisJobke
Copy link
Member

Could you update the Nginx config as well? https://github.com/nextcloud/documentation/blob/master/admin_manual/installation/nginx.rst

And maybe a setup check for this would also be nice to detect that the config is missing or not working. ;)

@MorrisJobke
Copy link
Member

And maybe a setup check for this would also be nice to detect that the config is missing or not working. ;)

See

OC.SetupChecks.checkWellKnownUrl('/.well-known/caldav/', oc_defaults.docPlaceholderUrl, $('#postsetupchecks').data('check-wellknown') === 'true'),
and
checkWellKnownUrl: function(url, placeholderUrl, runCheck) {

@ArtificialOwl
Copy link
Member Author

@MorrisJobke : nextcloud/documentation#877

note that this is nginx configuration, I have no idea what I am doing there (I mean, even less idea than usual)

@ArtificialOwl
Copy link
Member Author

Check with @danxuliu on this tests and it seems that the javascript is executed each time an admin open the Overview page in the Settings

So, the .well-known/webfinger will returns 500 if no app registered the core/public_webfinger config value, then we're dependant on what is returned by the app (and if the app is still installed/enabled) (which is 200 in the case of Social)

@MorrisJobke
Copy link
Member

So, the .well-known/webfinger will returns 500 if no app registered the core/public_webfinger config value, then we're dependant on what is returned by the app (and if the app is still installed/enabled) (which is 200 in the case of Social)

But it should never return a 500, because that means "Internal server error". At least it should either return "404" for a not available source or "2xx"/"3xx". Obviously it should only be executed if there is something registered in the PHP side for this URL. I guess we can give this hint out to the JS part in the same way we return if those checks should be done at all - see #11411 for the code that does this. Or am I wrong?

cc @rullzer @nickvergessen

@rullzer
Copy link
Member

rullzer commented Sep 27, 2018

500 indeed seems odd...

@nickvergessen
Copy link
Member

Fixing

@nickvergessen
Copy link
Member

Done, ^

@danxuliu
Copy link
Member

I guess we can give this hint out to the JS part in the same way we return if those checks should be done at all - see #11411 for the code that does this. Or am I wrong?

Just for the record, as far as I know it is not possible to know whether /.well-known/webfinger/ returned a 404 because the redirection is not properly configured in the web server or because the service is not registered, so the documentation linked in the error message should address that (therefore probably a different section than https://docs.nextcloud.com/server/14/admin_manual/issues/general_troubleshooting.html#service-discovery should be used in the webfinger check).

@MorrisJobke
Copy link
Member

Just for the record, as far as I know it is not possible to know whether /.well-known/webfinger/ returned a 404 because the redirection is not properly configured in the web server or because the service is not registered, so the documentation linked in the error message should address that (therefore probably a different section than https://docs.nextcloud.com/server/14/admin_manual/issues/general_troubleshooting.html#service-discovery should be used in the webfinger check).

That's the reason why I said, that we need to check first if it is registered at all and only check then if it works. Otherwise a check would make no sense.

@danxuliu
Copy link
Member

That's the reason why I said, that we need to check first if it is registered at all and only check then if it works. Otherwise a check would make no sense.

Sorry, I misread you; you are totally right :-)

ArtificialOwl and others added 4 commits October 10, 2018 13:01
Signed-off-by: Maxence Lange <[email protected]>
The check is based on the HTTP status returned by the URL, and different
URLs may return different status (for example, DAV returns 207, while
a service like WebFinger would return 200), so the expected status needs
to be set depending on the URL.

Signed-off-by: Daniel Calviño Sánchez <[email protected]>
If the WebFinger service is not set in Nextcloud configuration no check
is performed.

Signed-off-by: Daniel Calviño Sánchez <[email protected]>
@danxuliu danxuliu force-pushed the wellknown-webfinger branch from 938e0e5 to 20a5ce2 Compare October 10, 2018 12:33
@danxuliu
Copy link
Member

I have rebased onto current master to get the latest fixes to checkWellKnownUrl and added a check for the well known URL of WebFinger that is only run when public_webfinger is set.

However, I have noticed that even if public_webfinger is set the server can still return an HTTP status code of 500 if the value set for public_webfinger is wrong (for example, socia/lib/webfinger.php, as in that case the socia app would not be found). In that case the error and the documentation would be misleading, as the redirection would work but the configuration for the service would be the actual problem.

@MorrisJobke
Copy link
Member

However, I have noticed that even if public_webfinger is set the server can still return an HTTP status code of 500 if the value set for public_webfinger is wrong (for example, socia/lib/webfinger.php, as in that case the socia app would not be found). In that case the error and the documentation would be misleading, as the redirection would work but the configuration for the service would be the actual problem.

Isn't this then a bug in the app? Also a 500 indicates a "server error", while a "40x" indicates a client error. We should show the "wrongly configured .well-known" for 400s and a "general problem" for the 500s. Or did I miss something here?

@ArtificialOwl
Copy link
Member Author

I think so, 404 if there is no configuration to catch the request on .well-known/webfinger, or if the configured app is disabled, or if the php file that should be called does not exists.

The app that catch the request should return a 200 (or 203 on fail ?)

@rullzer
Copy link
Member

rullzer commented Oct 10, 2018

So with our default nginx config this actually results in a 403 (a reqeust to <server>/.well-known/webfinger that is)

@danxuliu
Copy link
Member

@MorrisJobke

Isn't this then a bug in the app?

Yes, I thought that the core/public_webfinger configuration value had to be set manually, but it is automatically set by the app when needed, so if the value is wrong it would be a bug in the app.

Also a 500 indicates a "server error", while a "40x" indicates a client error. We should show the "wrongly configured .well-known" for 400s and a "general problem" for the 500s. Or did I miss something here?

No, that approach sounds good 👍

However, the misleading error message would come again if, for some reason, the redirection worked but the service returned a 40x error; as far as I know that would not happen in the URLs currently checked, so I think that it is not worth to worry about that scenario right now.

@daita

I think so, 404 if there is no configuration to catch the request on .well-known/webfinger, or if the configured app is disabled, or if the php file that should be called does not exists.

Note that if the configured app is disabled or if the php file that should be called does not exists the HTTP status is 500, not 404. Anyway in that case the error shown would be the right one with the approach mentioned by @MorrisJobke.

@rullzer
Copy link
Member

rullzer commented Oct 11, 2018

So. I had another idea here. But it is more of a general improvement.

<server>/.well-know/<service> if correctly configured returns a 301, if you don't follow the redirect. We could just check where the redirect is pointing to to validate if it is all setup correctly.

RewriteRule .* - [env=HTTP_AUTHORIZATION:%{HTTP:Authorization}]
RewriteRule ^\.well-known/host-meta /public.php?service=host-meta [QSA,L]
RewriteRule ^\.well-known/host-meta\.json /public.php?service=host-meta-json [QSA,L]
RewriteRule ^\.well-known/webfinger /public.php?service=webfinger [QSA,L]
Copy link
Member

Choose a reason for hiding this comment

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

So I looked into it. And I'm not a fan of this.
Mostly because it redirect to a route that does not follow the appframework route.

All our apps should handle routes via the appframework. This ensures a lot of safety checks and features.

For now I would vote to make it a 301. And just to direct it to the route of the app.

If this becomes an issue we can later always add some wellKnownManager somewhere where apps can register themselfs. But for now this seems a bit of overengineering.

Copy link
Member Author

Choose a reason for hiding this comment

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

Can we keep it like this at least for 15, just returns a 301 in case of no config, or config redirect to an non-existant app, or non-existant php file ?

Copy link
Member

@rullzer rullzer left a comment

Choose a reason for hiding this comment

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

I'm still not a huge fan of this but I guess for now it is somewhat good enough.

Copy link
Member

@skjnldsv skjnldsv left a comment

Choose a reason for hiding this comment

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

🐘

@MorrisJobke MorrisJobke merged commit df6e910 into master Oct 24, 2018
@MorrisJobke MorrisJobke deleted the wellknown-webfinger branch October 24, 2018 12:51
@MorrisJobke
Copy link
Member

Looks weird but unrelated:

PhantomJS 2.1.1 (Linux 0.0.0) OCA.Comments.CommentCollection resets page counted when calling reset FAILED
128s
1147
	Expected false to equal true.
128s
1148
	apps/files_versions/tests/js/versionmodelSpec.js:76:47
128s
1149
	core/vendor/jquery/dist/jquery.min.js:2:56437
128s
1150
	j@core/vendor/jquery/dist/jquery.min.js:2:27194
128s
1151
	fireWith@core/vendor/jquery/dist/jquery.min.js:2:54761
128s
1152
	core/vendor/jquery/dist/jquery.min.js:2:56856
128s
1153
	core/js/files/client.js:9:30735
128s
1154
	lib$es6$promise$$internal$$tryCatch@core/vendor/es6-promise/dist/es6-promise.js:331:24
128s
1155
	lib$es6$promise$$internal$$invokeCallback@core/vendor/es6-promise/dist/es6-promise.js:343:52
128s
1156
	lib$es6$promise$$internal$$publish@core/vendor/es6-promise/dist/es6-promise.js:314:52
128s
1157
	lib$es6$promise$asap$$flush@core/vendor/es6-promise/dist/es6-promise.js:125:17
128s
1158
	Expected false to equal true.
128s
1159
	apps/files_versions/tests/js/versionmodelSpec.js:77:43
128s
1160
	core/vendor/jquery/dist/jquery.min.js:2:56437
128s
1161
	j@core/vendor/jquery/dist/jquery.min.js:2:27194
128s
1162
	fireWith@core/vendor/jquery/dist/jquery.min.js:2:54761
128s
1163
	core/vendor/jquery/dist/jquery.min.js:2:56856
128s
1164
	core/js/files/client.js:9:30735
128s
1165
	lib$es6$promise$$internal$$tryCatch@core/vendor/es6-promise/dist/es6-promise.js:331:24
129s
1166
	lib$es6$promise$$internal$$invokeCallback@core/vendor/es6-promise/dist/es6-promise.js:343:52
129s
1167
	lib$es6$promise$$internal$$publish@core/vendor/es6-promise/dist/es6-promise.js:314:52
129s
1168
	lib$es6$promise$asap$$flush@core/vendor/es6-promise/dist/es6-promise.js:125:17

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

3. to review Waiting for reviews enhancement

Projects

None yet

Development

Successfully merging this pull request may close these issues.

7 participants