Support for redirect actions

[PVince81]

Have the ability to specify a URL for action buttons.

I believe there is already a URL there so maybe just a flag to tell the handler that the URL is not ajax but really a page that needs to be opened.

Then adjust the JS notification handler to work as such.

Would also need adjusting in clients. We could also embed those in emails.

@jvillafanez @michaelstingl thoughts ?

[michaelstingl]

@PVince81 context? (maybe ping me for ☕️ the next days…)

[PVince81]

Context is here https://github.com/owncloud/password_policy/issues/18 but I can explain it to you.

In general notifications can have action buttons. So far the design said they are "ajax" actions, basically some curl request to send to a URL with a given method. They were not designed to open the URL in the browser, just do ajax/XHR.

The proposed enhancement is to also add a flag/mechanism to use the action buttons as a "open in browser" action. This would make it possible to have more than a single link. Also from the UX perspective, seeing a button would encourage action, even if the button would link to the same link as the notification.

[PVince81]

@ogoffart please have a look. I heard desktop clients already display the buttons, so if we add this feature the clients would need adjusting as well.

I'm not sure yet about how to represent it in the action JSON. To be discussed.

[ogoffart]

If you just change the meaning of the "link" property, this will break the current client (the client would just do a request to that url, but not open a browser). If you leave the link property empty, that would make current client to have an useless button. If you change the "type" of the link to anything but GET PUT POST or DELETE, the button would also have no effect.

There could be an "actions2" list in which actions could have a type which is "REDIRECT" or something like that which would mean to open the browser.

Note that this would by default not be autenticated. If we want to authenticate, we'd need to interoperate with something like discussed in https://github.com/owncloud/client/issues/633

[PVince81]

Another idea is to have the result of the URL request to be a 302 redirect. The drawback is that you cannot distinguish between a legit redirect from the web server (due to env) from one returned by OC. That is unless we use a custom header like "Content-Location".

[PVince81]

@jvillafanez let's spend 0.5md to examine the different proposals and let them be reviewed by the client teams.

[ogoffart]

I don't particularly like the idea of a 302 redirect that would mean opening the browser instead. First, that would still not work with older sync client. And it looks like a hack since this is not really what HTTP redirect is supposed to mean.

[PVince81]

We need a solution that:

• doesn't break on older clients or at least doesn't make it worse than now
• can be implemented in future client versions

[ogoffart]

From one of my previous comments:

There could be an "actions2" list in which actions could have a type which is "REDIRECT" or something like that which would mean to open the browser.

This would fit both criteria: old client would ignore the actions2 list. And new client can make it work. (I'm talking about the desktop client. I don't know about mobiles ones)

The old items in the old action could contains a hidden='true' action which would mean that the new client don't show this old action, in case we still want to show an action that does something only on the old clients.

[jvillafanez]

It makes more sense to me that this redirect is associated to the specific action. An "accept" action could hit the target url to perform the action and the redirect to an specific page, while a "reject" action would just hit the target url.

I'll try to enrich the action with a list of "postActions" or something similar. That should be compatible. The only drawback is that, if we just want to redirect the user to a page, we'll need to provide a dummy action.

From a client perspective, if we have a json for an action such as

{
"label": "My Label",
"link": "http://server/ownCloud/api/endpoint",
"type": "POST",
"primary": false
}

we could enrich with

{
"label": "My Label",
"link": "http://server/ownCloud/api/endpoint",
"type": "POST",
"primary": false,
"postActions": [
  {"action": "redirect", "url": "http://another.server/owncloud"},
  {"action": "reload"}
]
}

I'm not sure if we need a list of "post actions" or just one is enough. So far, I think we only need those for the webUI. For emails, we're ignoring the actions so there is nothing to do. I don't know if the desktop or mobile clients would need something special.

I still need to figure out how to implement this, but for the communication to client it should be good enough.

[PVince81]

@jvillafanez interesting.

If we had a standard format for the ajax response of the actions, we could introduce a new response type "postActions" which tells the client what to do. The advantage here is that you have the ability to return different redirect URLs depending on the outcome of the action.

What you proposed wouldn't allow for such flexibility.

[jvillafanez]

If we had a standard format for the ajax response of the actions, we could introduce a new response type "postActions" which tells the client what to do. The advantage here is that you have the ability to return different redirect URLs depending on the outcome of the action.

We can't provide an standard response because the actions are completely different among each other. The actions are supposed to be API calls, each one with its own meaning, restrictions and expectations.

The only way is to provide a page or frontend between the client and the server, and make sure all the actions go through that. The frontent will take care of calling the action and wrap the response. Obviously, the complexity of the solution increases by a lot. We still need to figure out how we can authenticate the frontend against the server.

In any case, we're still bound by the unmodifiable response of the action.

I don't really see any advantage in this approach. Both the redirection and the reload are actions that the client needs to take, and it's the client the one deciding. Note that the web UI uses ajax to call the action, which means that any redirection or reload will be performed by ajax and not the web. So, in the end, the wrapper will still need to wrap the call and send additional information for the client to decide, which is mostly (https://github.com/owncloud/notifications/issues/212#issuecomment-408413447)

[PVince81]

@jvillafanez while we cannot provide a standard response format for different actions, we can still specify a standard response for a redirection then. For example if an action handler wants to tell the clients to redirect, return something like {'redirectTo': 'http://...'}. The likeliness that there's already an existing action in the wild that looks like this is extremely low.

[jvillafanez]

We'll need to mark the action somehow, maybe with something like:

{
"label": "My Label",
"link": "http://server/ownCloud/api/endpoint",
"type": "POST",
"primary": false,
"redirectToInResponse": true
}

This would indicate the clients to look for a "redirectTo" in the response to redirect there.

The expectation is that the action is executed, and then the client redirects to the first "redirectTo" link that appears in the successful response. Error responses won't have this behaviour, so the redirection should be ignored by the client if an error response is returned (or maybe not, I'm not sure). The redirection will be handled by each client (for the web UI, the notifications app)

This is probably the best compromise we have. The question is if we might need more flexibility, such as full page reload or something similar.

In any case, the behaviour feels weird

[PVince81]

In any case, the behaviour feels weird

can you elaborate what exactly feels weird ?

The way I see it is that the action itself is giving a hint to the clients what should happen next. Maybe we need the word "next" somewhere ?

[jvillafanez]

• Just thinking about the action, it's weird to try to redirect with a HTTP 200 with a "redirectTo" in the response when you should be returning the usual 302. In any case, redirections in a REST environment doesn't look good.
• Having a valid action, it's weird to somehow include a "redirectTo" when you're expecting a valid object
  • Maybe as part of the meta field of the OCS response, but this can't be done without modifying the OCS class.
• We also need to mark that the client needs to check the response for something specific, when most of the time the client shouldn't care about the response. They should just check if the action is correctly done or not.
• We also need to define how the client should behave in case of errors. If a 40x or 50x HTTP code is returned, should the client follow the redirection (if available)? Should it follow the redirection just with a specific HTTP code?.

I'm still checking options, but an OCSRedirectResult might fit in what we want to do.

[jvillafanez]

Checking other approach similar to https://github.com/owncloud/notifications/issues/212#issuecomment-408796796

{
"label": "My Label",
"link": "http://server/ownCloud/api/endpoint",
"type": "GET",
"primary": false,
"browserDriven": true
}

where "browserDriven" implies that the request should be made by the browser and not via ajax. I think we can make sure that this flag is active only for GET requests (for the rest it would be missing). For the clients (desktop in particular) would mean that they should open that url in a browser instead of making an API call. It would still be fine if the browser still ask for authentication to access to the url.

[DeepDiver1975]

Just to keep it somewhere - as proposed on chat: introduce a new type: OPEN

[jvillafanez]

I'll discard the last 2 solutions proposed: if we redirect directly to the target we won't be able to mark the notification as processed without making weird things (probably including some js code, listen to the action event and hit another url to mark the notification as processed before the redirection happens; looks bad because we always want to discard the notification, so this extra js code will be a requirement in all the apps that wants a redirect action).

Now https://github.com/owncloud/notifications/issues/212#issuecomment-408796796 makes more sense because we hit a custom url that will mark the notification as processed, and also provide the redirection in the response.

[jvillafanez]

Improving a bit https://github.com/owncloud/notifications/issues/212#issuecomment-408796796 :

{
"label": "My Label",
"link": "http://server/ownCloud/api/endpoint",
"type": "POST",
"primary": false,
"redirect": true
}

Clients will make the request to the target link. No changes with what we have now.

The "redirect" in the action might not be present:

• If it isn't present, no changes; just make the request
• If it's present, make the request, and redirect / open the browser in the location specified in the response, in the "redirectTo" key (to be properly specified, it might change a bit in the PR)
  • If the "redirectTo" key is missing in the response, just ignore and do nothing
  • ~It's expected the target app (password_policy for example) wire things properly. This means that the "redirectTo" should be unambiguous in the response.~
    • ~In case the app needs more "redirectTo" keys, the client should follow the first outermost "redirectTo" key (usually the first appearing)~
    • ~If the app doesn't want a redirection, it shouldn't specify the "redirect" in the notification~
  • The response must be an OCS response, and the "redirectTo" key must be in ocs.data.redirectTo:

    <?xml version="1.0"?>
    <ocs>
     <meta>
      <status>ok</status>
      <statuscode>200</statuscode>
      <message/>
      <totalitems></totalitems>
      <itemsperpage></itemsperpage>
     </meta>
     <data>
      <redirectTo>http://10.0.2.8:7080/settings/personal?sectionid=general</redirectTo>
     </data>
    </ocs>

Expected list of changes:

• (core) New get / set methods for the redirect in the OCP\Notification\IAction and in the implementation
• (notifications) Changes around OCA\Notifications\Handler to store and retrieve the new action attribute
• (notifications) Changes in "notifications/js/app.js" to handle properly this scenario
• (apps) Generate the notification properly if they want to use this feature, otherwise no changes.

Possible compatibility problems:

• Get / set methods for the redirect might not exists in previous versions:
  • This affects to the notifications app (to check for the method in order to store and retrieve the information) as well as each other app (if they want to use this new feature). Note that apps that won't use this feature should be unaffected.
• Apps needs to rely on specific core and notifications versions:
  • for core, same as the previous point: get / set might not be available
  • the notification app might not redirect if the version isn't the right one

[PVince81]

@jvillafanez there's one part that isn't clear to me yet is why the action spec JSON needs to "warn" about a potential redirect in the response for the URL above by adding redirect:true.

Regardless of that value a client would send a curl-like request and act on the response, depending whether it contains redirectTo or not. So clients could simply ignore that flag. Or is there any scenario where that flag is absolutely necessary.

[jvillafanez]

The response could have a "redirectTo" that is part of the normal response and isn't intended to be used for this. The flag is there so both the app and the client are aware. In case of multiple "redirectTo", the resolution should be as specified above, so both the app knows what is the link that the client is expected to follow.

[jvillafanez]

Should we use OCS, or just JSON? (https://github.com/owncloud/notifications/issues/212#issuecomment-409208423)

[jvillafanez]

For a kind of tech preview, you'll need https://github.com/owncloud/core/pull/32212 and https://github.com/owncloud/notifications/pull/218 merged. Then you can merge https://github.com/owncloud/password_policy/pull/78 for a practical example with the password_policy app.

Note that there is still some work to do in the PR (mainly unittests), but the code should work. In addition, https://github.com/owncloud/password_policy/pull/78 isn't scheduled for now, so no further progress is expected there. Consider that PR as a way to check how things should look like.

[PVince81]

@jvillafanez so as I understand, now https://github.com/owncloud/notifications/issues/212#issuecomment-409208423 is up to date with your latest idea about the API spec ?

would be good to get feedback from client devs on this @michaelstingl @ogoffart

[jvillafanez]

Yes. If there is any case that was left behind, feel free to point it out.

[PVince81]

Should we use OCS, or just JSON

for existing known actions, are they using OCS or JSON ? let's keep whatever pattern already exists

[ogoffart]

Why do we need the "redirect": true in the notification info? We wan just rely on the RedirectTo from the reply. However the old clients will still show the button.

[PVince81]

I don't think there is any good solution to prevent old clients to show the button, apart from maybe a user-agent check and some hacks...

[jvillafanez]

for existing known actions, are they using OCS or JSON ? let's keep whatever pattern already exists

This is only for this new feature: the endpoint of these new actions must return an OCS response. Existing actions won't have this "redirect" flag, so we don't need to check the response. We don't really care if it's JSON or OCS.

Why do we need the "redirect": true in the notification info? We wan just rely on the RedirectTo from the reply.

There could be valid OCS responses that include a "redirectTo" in the same place. Having the mark brings 2 main benefits:

• If the "redirect" isn't in the notification info, you don't need to spend time parsing the response because normally you won't be interested in it.
• In case there is an OCS response that has a "redirectTo" in the same place but the "redirect" mark isn't in the notification info, the client is expected to treat the "redirectTo" as a part of the normal response, so it shouldn't redirect

[PVince81]

If the "redirect" isn't in the notification info, you don't need to spend time parsing the response because normally you won't be interested in it.

This is likely an unnecessary optimization as it doesn't bring much benefit. We're not talking about parsing that would take several minutes. Besides, the flag itself could cause more confusion when reading the spec.

It is likely that clients use the same code path anyway for parsing responses, and only then add an if statement. So unless there is a clear requirement from clients for this I'd vote for removing it.

[jvillafanez]

It's more like the app generating the notification explictly says that it wants the client to redirect to the url in the response. Otherwise the client assumes that the app wants a redirection when it might not want that.

Besides, the flag itself could cause more confusion when reading the spec.

It might be badly explained. We can improve it. However, without the flag, there is nowhere stating that the client should follow the redirection. Adding the flag feels like a new feature, without the flag feels like a bug in the client

[PVince81]

here it seems you are anticipating implementation details of clients which might not be necessary at all.

clients do not need to know about redirection before running the action with a curl call. I can't think of scenario where the flag is mandatory or would break something if it isn't there

also I could think of scenarios where the action fails despite anticipated redirect, and in the failure case an error is returned without redirect URL as opening the browser would not be necessary in case of error. So the fact whether a redirection needs to occur depends on the outcome of the action.

[jvillafanez]

Ok, adjusted the PRs. https://github.com/owncloud/notifications/issues/212#issuecomment-408796796 is obsolete. Spec is included in the core's PR. As discussed, clients should redirect if the ocs.data.redirectTo key is present, and it should be redirected to the link specified in that key. No further checks are needed.

Question: in case of error, should the client redirect to the link in the response if it's present, or should just ignore it and do nothing?

[PVince81]

Question: in case of error, should the client redirect to the link in the response if it's present, or should just ignore it and do nothing?

In case of error, if the server decides to specify a "redirectTo" value in the error JSON, the client should still follow it.

A possible use case could be: "something failed, let's open the web browser and let the user/admin check some settings".

[PVince81]

Spec is included in the core's PR

@jvillafanez link?

[PVince81]

@jvillafanez please repost the updated spec here.

If client devs and @DeepDiver1975 can agree, we can finish the implementation then.

[jvillafanez]

https://github.com/owncloud/core/pull/32212/files

     * Whenever a client want to execute this particular action, it should
     * call the link set here with the same request type. Clients are
     * expected to make a request to that endpoint, but not to open or
     * redirect a browser there. The request might need to be authenticated
     * somehow (via cookies, basic auth, oAuth token, etc)
     *
     * Clients are not expected to take further action UNLESS the response
     * of the action is an OCS response with a "redirectTo" key in the data
     * (ocs.data.redirectTo = <another-link>). In this case, clients are
     * expected to open a browser or redirect it to the link specified
     * in the "redirectTo" link
     *

That's for the setLink($link, $requestType); in the action.

In addition, if we want to also the redirect behaviour also for errors (https://github.com/owncloud/notifications/issues/212#issuecomment-410612351) I'll need to adjust the PR for the notifications app properly because right now it doesn't redirect on error.

[ogoffart]

That would work (just that other client would not redirect)

Note that if the redirected url is suppose to be authenticated, the user will probably have to enter the password again. (Unless https://github.com/owncloud/core/issues/5453 is done)

[PVince81]

To clarify: we don't have a solution that would work or hide the button for older clients and are ok with this limitation.

[PVince81]

Do we also have agreement from the Mobile side ? @michaelstingl

[michaelstingl]

@jvillafanez maybe you could check with @davigonz , @javiergonzper and @jesmrec ?