Hello Seba, please see my comments inline
On Fri, 24 Sept 2021 at 04:55, seba.wag...@gmail.com <seba.wag...@gmail.com> wrote: > Also just to reiterate: This is not to say "this API is bad" it is just > that over the course of the last 10 years REST has changed. As I tried to > explain in my previous email. > understood > > And also: I am happy to lead and spend my time on this activity around > introducing v2 and address the above issues. AS well as deprecating the > existing API endpoints and _over time_ remove them. Once consumers had > reasonable notice to migrate off to a new version. > Great! let's move forward :)) I believe the discussion can be splitted and continued on dev@ and/or in PR comments :) > > Thanks > Seb > > Sebastian Wagner > Director Arrakeen Solutions, OM-Hosting.com > http://arrakeen-solutions.co.nz/ > https://om-hosting.com - Cloud & Server Hosting for HTML5 > Video-Conferencing OpenMeetings > > <https://www.youracclaim.com/badges/da4e8828-743d-4968-af6f-49033f10d60a/public_url> > <https://www.youracclaim.com/badges/b7e709c6-aa87-4b02-9faf-099038475e36/public_url> > > > On Fri, 24 Sept 2021 at 09:20, seba.wag...@gmail.com < > seba.wag...@gmail.com> wrote: > >> *So far I don't see how the existence of SOAP complicates things :)* >> >> => See below list of what a potential v2 of the Rest API could/should >> fix. I don't see how this can be fixed while keeping SOAP and REST in the >> same class definition: >> >> *Security - 2.4. Never expose sensitive information on URLs* >> Reference: See https://restfulapi.net/security-essentials/ >> Quote: *Usernames, passwords, session tokens, and API keys should not >> appear in the URL, as this can be captured in web server logs, which makes >> them easily exploitable. *(aka == do not use GET parameters for >> sensitive information) >> Problematic OpenMeetings REST API: >> - UserService::login => Sends both username and password as GET / URL >> parameters >> - All methods (except a few) => Send security token as GET / URL >> parameter >> => Severity *High *=> Essentially no Client side JavaScript client >> should ever use our API. Anybody can use a simple DNS sniffer to read the >> blank password or get a god mode security token. Or for example you can >> read the security credentials by trailing the access log, since the full >> URL including GET parameters will be logged. >> > this one is weird I can log all other methods and detailed content as well And it is as simple as replacing @Get -> @POST && @QueryParam -> @FormParam URL params are easier to use and fit here well >> *Security - 2.3. Use Password Hash* >> Reference: See https://restfulapi.net/security-essentials/ >> Self explanatory. >> => Severity *Medium/High *I think similarly important but its not as >> High Severity as 2.4 >> > This one is unclear and NOT "Self explanatory." :))) sorry >> *Using appropriate HTTP response codes* >> See: https://restfulapi.net/http-status-codes/ >> We currently return HTTP 200 no matter if the call was successful or not >> (eg SID expired or wrong). We only return 500 in case there was a server >> error (actually that's is not quite true, I think there might be cases >> where application errors also throw 500) >> We should use appropriate HTTP codes for different kind of scenarios: >> Eg: >> Use 4xx errors for user errors: >> - Return 401 if login fails, 403 is SID expired or invalid >> - Return 400 if a BadRequest was sent (eg parameter validation fails) >> - Return 404 in case you request an item but none was found >> Use 5xx errors only for real server side errors, eg internal server error >> (eg database down) >> => Severity *Medium - *I think this would be better to be fixed, but >> compared to above 2.4 it's probably not quite as high >> > this one is partially implemented and to be fair I would say it should be of Severity "Very Low" >> *Appropriate usage of HTTP Methods GET just returns, doesn't action >> anything* >> See: https://restfulapi.net/http-methods/ >> We use a few examples where the HTTP Method is "GET" but it actually >> creates or actions something. >> For example: >> UserServier::login -> GET method. This should be a POST. Simply also >> because you should never expose user/pass as get parameters. >> RoomService::kick -> Currently "GET", but this is clearly doing >> something. It creates a request that kicks users. (And then returns >> something). But it is doing something. >> There are more examples for this. I think some methods have been made a >> GET method just because they have a single parameter. But that is not the >> criteria to use GET or POST. GET should never change any state or action >> something. >> => Severity *Medium - *I think this would be better to be fixed, but >> compared to above 2.4 it's probably not quite as high >> > OK but I don't see why to change All those REST rules and just *recommendations* IMO API should be easy to use, it should be clear this is why I point out parameter construction User can't guess it should be like this without asking mailing list This is the real problem, all above is cosmetics .... > >> *POST methods should create single object in post body * >> There is a good explanation what we currently do around "Form params", >> see: >> https://developer.mozilla.org/en-US/docs/Web/HTTP/Methods/POST#example >> This is probably more debatable but I would just suggest a single request >> body in JSON format and any other "meta" information as a potential GET >> parameter. >> Example: >> UserService::add >> - String SID => should be not a Get parameter, but probably a header >> parameter >> > SID can be moved to Header but it's not clear to me why it is bad to have it as is :( - User user => Post body, plain JSON >> - Boolean confirm => Meta data, simple GET Parameter. Won't be "saved" >> with the object, but just some additional information >> This also has the advantage that we simply can use Accept and >> Content-Type:application/json headers for all methods input and output. >> => Severity *Medium - *I think this would be better to be fixed, but >> compared to above 2.4 it's probably not quite as high >> >> I see no issues with having REST and SOAP together so far What Am I missing? > => I can't see how realistically we would be able to just change the >> current API with the above while its SOAP and REST in a single API. The >> differences are just too big. >> >> *I'm not sure if SOAP is being used in any integration so we can drop >>> SOAP support* >> >> => I would suggest to neither drop nor introduce breaking changes into >> current APIs. Neither SOAP nor REST. But rather deprecated and slowly phase >> out over time. Eg deprecated and then after 3 major releases drop. That >> gives reasonable notice to people to migrate using a new version of the >> API. Just because there isn't a reply to this email doesn't mean there is >> nobody using it. >> > OK let it be :) But we are a small project, so I guess we have more freedom to make changes :) > >> I would VOTE for a better API >>> Please step in and let's discuss how to make it better :) >> >> See above list. Those are the main things I can think of. And I think >> those would be good candidates to introduce a v2 rest only version of the >> API. And then -over time- deprecate the old SOAP/REST API and savely over >> 2-3 major releases then drop it. >> >> Thanks >> Seb >> >> Sebastian Wagner >> Director Arrakeen Solutions, OM-Hosting.com >> http://arrakeen-solutions.co.nz/ >> https://om-hosting.com - Cloud & Server Hosting for HTML5 >> Video-Conferencing OpenMeetings >> >> <https://www.youracclaim.com/badges/da4e8828-743d-4968-af6f-49033f10d60a/public_url> >> <https://www.youracclaim.com/badges/b7e709c6-aa87-4b02-9faf-099038475e36/public_url> >> >> >> On Thu, 23 Sept 2021 at 20:43, Maxim Solodovnik <solomax...@gmail.com> >> wrote: >> >>> >>> >>> On Thu, 23 Sept 2021 at 04:47, seba.wag...@gmail.com < >>> seba.wag...@gmail.com> wrote: >>> >>>> Hi all! >>>> >>>> I think we may try to solve too many things at the same time in this >>>> discussion. But also in our API. It just seems things are a bit too tightly >>>> coupled. >>>> >>>> For example: >>>> *We can't change/update REST methods, cause they affect SOAP calls. >>>> Currently we have a just a single class that does both* >>>> => It was easy when the API was simple. But then over time it actually >>>> became harder, cause we discovered that under the covers Soap and Rest >>>> representation is different. >>>> >>> >>> So far I don't see how the existence of SOAP complicates things :) >>> >>> >>> >>>> *We introduced REST 10years ago and the definitions of REST changed >>>> over time* >>>> => We created the OpenMeetings REST API over 10years(!!) ago. At that >>>> time the definition of REST was quite fluent. By now there is quite a >>>> different and more strict understanding of what Rest means. And how HTTP >>>> methods (GET/POST/PUT/DELETE), resource paths ( having nouns not verbs as >>>> resource paths, eg: /users/$identifier), using the right kind of >>>> request/response message structure or for example how >>>> authentication/security works. >>>> >>>> Things have changed. And I actually think by now it is taking more time >>>> and effort to try to get Soap/Rest into 1 class. Then separating them. As >>>> well as it's just maybe a long time since we had a major uplift. >>>> >>>> How about we do the following: >>>> >>>> 1) We keep the current SOAP/REST API structure as is >>>> => Minimise rework / No breaking changes >>>> => We accept some minor quirks around some *documentation only* >>>> annotations in order to document it in a way people can use it >>>> >>>> 2) For v7.0.0 or v8.0.0 we introduce a v2 of the Rest API >>>> But: >>>> => v2 is REST only. The existing/v1 API is the SOAP/REST API. And SOAP >>>> stays where it is. That way we have less work in trying to make 2 things >>>> into 1. >>>> >>> >>> I'm not sure if SOAP is being used in any integration >>> so we can drop SOAP support, but IMO this is another question :) >>> >>> >>>> => Soap and Rest can still use the same "adapter" class in order to >>>> achieve 'feature parity'. But we do _not_ attempt for example that method >>>> parameters need to match >>>> => Having a single v2 REST only interface makes it also easier to >>>> write integration tests. Cause you only need to test the REST interface. >>>> => There isn't really any issue with the SOAP interface. SOAP hasn't >>>> changed in the last 10years. There isn't any need to go for a v2 in the >>>> SOAP API >>>> >>>> 3) We agree _before_ adding a v2 what REST "guidelines" we adopt >>>> => Instead of arguing what kind of HTTP method, security headers, POST >>>> body parameters we adopt a guideline/standard document. And simply comply >>>> with that standard. >>>> => This will also make it a lot easier to: >>>> A) Integrate with it, cause people are used to the >>>> format/standard/guidelines and there is less discussion needed >>>> B) The tooling will be much easier, because all the code generators, >>>> documentation tools, CXF-RS, json-mappers we are using are referencing a >>>> similar or same guidelines/standard. So we not constantly need to customise >>>> things to fit into how the existing OpenMeetings API works >>>> An example of a REST guideline/standard that we could adopt is: OpenAPI >>>> 3.0.x (https://swagger.io/specification/) + Restful recommendations ( >>>> https://restfulapi.net/) >>>> >>>> @Maxim Solodovnik <solomax...@gmail.com> what do you think? I think >>>> more than 10years is enough for a single version of an API. I think by now >>>> it actually is _less_ work to have a new v2 Rest only version of the API >>>> then trying to somehow create a SOAP/REST API and try to enhance that into >>>> a "RESTful" way but not break it at the same time. >>>> >>> >>> I would VOTE for a better API >>> Please step in and let's discuss how to make it better :) >>> >>> >>>> Thanks, >>>> Seb >>>> >>>> Sebastian Wagner >>>> Director Arrakeen Solutions, OM-Hosting.com >>>> http://arrakeen-solutions.co.nz/ >>>> https://om-hosting.com - Cloud & Server Hosting for HTML5 >>>> Video-Conferencing OpenMeetings >>>> >>>> <https://www.youracclaim.com/badges/da4e8828-743d-4968-af6f-49033f10d60a/public_url> >>>> <https://www.youracclaim.com/badges/b7e709c6-aa87-4b02-9faf-099038475e36/public_url> >>>> >>>> >>>> On Thu, 23 Sept 2021 at 02:24, Daniel Baker < >>>> i...@collisiondetection.biz> wrote: >>>> >>>>> Can you not employ an extra programmer? >>>>> On 22/09/2021 13:24, Maxim Solodovnik wrote: >>>>> >>>>> >>>>> >>>>> On Wed, 22 Sept 2021 at 19:20, Ali Alhaidary < >>>>> ali.alhaid...@the5stars.org> wrote: >>>>> >>>>>> >>>>>> On 9/22/21 3:14 PM, Maxim Solodovnik wrote: >>>>>> >>>>>> I only can do manual testing here :( >>>>>> >>>>>> What is manual testing? >>>>>> >>>>> >>>>> I'm installing everything >>>>> setting up integration and do clicking :))) >>>>> >>>>> >>>>>> IMO these changes (if we will be able to do them) worth to be done >>>>>> >>>>>> what is IMO ? >>>>>> >>>>> >>>>> In My Opinion :) >>>>> >>>>>> Why I raise some old design issues: we can do changes now and let the >>>>>> API unchanged for another several years :))) >>>>>> >>>>>> What is several years ;-) >>>>>> >>>>> >>>>> Well I believe REST API was changed 2-3 times, so we are trying to >>>>> keep it stable >>>>> v1/v2 etc. approach can also be applied >>>>> the problem here: I don't have enough time to maintain more than one >>>>> version :(( >>>>> >>>>> >>>>>> On Wed, 22 Sept 2021 at 19:09, Ali Alhaidary < >>>>>> ali.alhaid...@the5stars.org> wrote: >>>>>> >>>>>>> The issue here is that, It is a lot of work, and, a lot of testing >>>>>>> that follows. We are not a direct API users, however, moodle plugin is. >>>>>>> Along the road, things could break in such change. So, if you see this >>>>>>> change is the the way forward, I am in with as usual a dedicated >>>>>>> production >>>>>>> server for selected teaches/students as long as the old work (mainly >>>>>>> recordings) is not lost, and, only one environment is used (as is now), >>>>>>> i.e. moodle plugin can handle all the communication. >>>>>>> >>>>>>> The issue is being discussed by only three people, how many others >>>>>>> are using these APIs ? How many apps are up and running on them now ? >>>>>>> looking at the moodle plugin downloads, >>>>>>> https://moodle.org/plugins/mod_openmeetings/stats there is a peak >>>>>>> during the past year, and I am sure the case is the same with other LMS >>>>>>> and >>>>>>> custom built apps, keeping in mind that OM can work exceptional good by >>>>>>> itself. >>>>>>> >>>>>>> Ali >>>>>>> >>>>>>> >>>>>>> On 9/22/21 2:16 PM, Maxim Solodovnik wrote: >>>>>>> >>>>>>> These changes are only being discussed >>>>>>> Nothing is broken, yet :)))) >>>>>>> we can @Deprecate these old methods and/or move it to some prefixed >>>>>>> URL >>>>>>> so API users will need to change base URL from >>>>>>> https://localhost:5443/openmeetings to >>>>>>> https://localhost:5443/openmeetings/v1 >>>>>>> >>>>>>> On Wed, 22 Sept 2021 at 13:14, seba.wag...@gmail.com < >>>>>>> seba.wag...@gmail.com> wrote: >>>>>>> >>>>>>>> @Ali Alhaidary <ali.alhaid...@the5stars.org> >>>>>>>> The other alternative to fix the issue AND make it backwards >>>>>>>> compatible would be to have a /v2 version of the API >>>>>>>> >>>>>>>> So all endpoints would be duplicated to have version /v2 of the API >>>>>>>> (with maybe some other fixes) >>>>>>>> and the current API stays the same. But would not receive any >>>>>>>> improvements anymore/deprecated. >>>>>>>> >>>>>>>> But that would be quite a bit of work. But yeah, that is what >>>>>>>> people do when they want to avoid breaking changes. Need to do >>>>>>>> versioning. >>>>>>>> >>>>>>>> Thanks >>>>>>>> Seb >>>>>>>> >>>>>>>> >>>>>>>> Sebastian Wagner >>>>>>>> Director Arrakeen Solutions, OM-Hosting.com >>>>>>>> http://arrakeen-solutions.co.nz/ >>>>>>>> https://om-hosting.com - Cloud & Server Hosting for HTML5 >>>>>>>> Video-Conferencing OpenMeetings >>>>>>>> >>>>>>>> <https://www.youracclaim.com/badges/da4e8828-743d-4968-af6f-49033f10d60a/public_url> >>>>>>>> <https://www.youracclaim.com/badges/b7e709c6-aa87-4b02-9faf-099038475e36/public_url> >>>>>>>> >>>>>>>> >>>>>>>> On Wed, 22 Sept 2021 at 18:10, Ali Alhaidary < >>>>>>>> ali.alhaid...@the5stars.org> wrote: >>>>>>>> >>>>>>>>> We are using OM in production with moodle front end, we can not >>>>>>>>> tolerate downtime neither with OM or its plugin (that needs fixing, >>>>>>>>> but >>>>>>>>> living with), and to tell you the truth, I do not see it as 'broken' >>>>>>>>> from >>>>>>>>> that angle. >>>>>>>>> >>>>>>>>> So my answer is B. >>>>>>>>> >>>>>>>>> Ali >>>>>>>>> On 9/22/21 2:10 AM, seba.wag...@gmail.com wrote: >>>>>>>>> >>>>>>>>> It is broken. The problem is the fix will be a breaking change >>>>>>>>> that will require 3rd party integration code to be fixed. Not a big >>>>>>>>> fix, >>>>>>>>> but a fix. Eg the Moodle Plugin requires some minor changes. >>>>>>>>> >>>>>>>>> The workaround is to write some additional wrapper code to make it >>>>>>>>> backwards compatible. Which is also a bit confusing. >>>>>>>>> >>>>>>>>> I also don't understand quite if you answer is pro or contra >>>>>>>>> changing the response. >>>>>>>>> >>>>>>>>> So is your statement: >>>>>>>>> A) Yes, lets fix it to align our JSON response with what the >>>>>>>>> schema/method signature says. We don't like wrapper objects. And I am >>>>>>>>> happy >>>>>>>>> that people have to change their integration code to use newer >>>>>>>>> versions of >>>>>>>>> OpenMeetings. >>>>>>>>> B) No, lets leave it like this for now and we do whatever other >>>>>>>>> additional code we need to write to workaround so that our >>>>>>>>> documentation >>>>>>>>> and schema matches what the actual API responses look like >>>>>>>>> >>>>>>>>> If you could please clarify if you are A, B. Or if you don't mind >>>>>>>>> either way/no strong opinion :) >>>>>>>>> >>>>>>>>> Thanks >>>>>>>>> Seb >>>>>>>>> >>>>>>>>> >>>>>>>>> Sebastian Wagner >>>>>>>>> Director Arrakeen Solutions, OM-Hosting.com >>>>>>>>> http://arrakeen-solutions.co.nz/ >>>>>>>>> https://om-hosting.com - Cloud & Server Hosting for HTML5 >>>>>>>>> Video-Conferencing OpenMeetings >>>>>>>>> >>>>>>>>> <https://www.youracclaim.com/badges/da4e8828-743d-4968-af6f-49033f10d60a/public_url> >>>>>>>>> <https://www.youracclaim.com/badges/b7e709c6-aa87-4b02-9faf-099038475e36/public_url> >>>>>>>>> >>>>>>>>> >>>>>>>>> On Wed, 22 Sept 2021 at 10:59, Ali Alhaidary < >>>>>>>>> ali.alhaid...@the5stars.org> wrote: >>>>>>>>> >>>>>>>>>> Hi, >>>>>>>>>> >>>>>>>>>> We have an old saying 'If it is not broken, do not fix it' ;-) >>>>>>>>>> >>>>>>>>>> Ali >>>>>>>>>> On 9/22/21 12:46 AM, seba.wag...@gmail.com wrote: >>>>>>>>>> >>>>>>>>>> Hi, >>>>>>>>>> >>>>>>>>>> as discussed in the comments section in >>>>>>>>>> https://github.com/apache/openmeetings/commit/4daf7c1f53738cd786dc976114cc5278b4f05f4f#comments >>>>>>>>>> >>>>>>>>>> >>>>>>>>>> we would like to propose a breaking change for the OpenMeetings >>>>>>>>>> Json/Rest API in v7.0.0 >>>>>>>>>> >>>>>>>>>> Problem: JSON response wrapping >>>>>>>>>> Currently CXF-RS is configured to wrap the JSON response into >>>>>>>>>> another object. >>>>>>>>>> >>>>>>>>>> Example: Method signature: public List<AppointmentDTO> range(...) >>>>>>>>>> { ... } (Example taken from >>>>>>>>>> https://github.com/apache/openmeetings/blob/master/openmeetings-webservice/src/main/java/org/apache/openmeetings/webservice/CalendarWebService.java#L111 >>>>>>>>>> ) >>>>>>>>>> >>>>>>>>>> OLD/CURRENT JSON Response: >>>>>>>>>> { >>>>>>>>>> "appointmentDTO": >>>>>>>>>> [ >>>>>>>>>> { >>>>>>>>>> itemXYZ: 123, ... >>>>>>>>>> } >>>>>>>>>> ] >>>>>>>>>> } >>>>>>>>>> >>>>>>>>>> >>>>>>>>>> Proposed NEW/UPDATED JSON Response: >>>>>>>>>> // no wrapping object around it, just return list >>>>>>>>>> [ >>>>>>>>>> { >>>>>>>>>> itemXYZ: 123, ... >>>>>>>>>> } >>>>>>>>>> ] >>>>>>>>>> >>>>>>>>>> Reasoning: The wrapping "{ "appointmentDTO": ... }" should be >>>>>>>>>> dropped from the json response body. "appointmentDTO" is generated >>>>>>>>>> but it >>>>>>>>>> is not in any schema definition or method signature. Cause there is >>>>>>>>>> nothing >>>>>>>>>> in the method signature that would tell anybody where " >>>>>>>>>> "appointmentDTO": >>>>>>>>>> [" is coming from. Other than by testing the API call and finding >>>>>>>>>> out by >>>>>>>>>> try and error. >>>>>>>>>> >>>>>>>>>> CXF-RS allows configuring our Web Service to NOT generate that >>>>>>>>>> wrapping element. And turn this behaviour off and just generate the >>>>>>>>>> list. >>>>>>>>>> See "dropRootName" in the CXF docs at: >>>>>>>>>> >>>>>>>>>> https://cxf.apache.org/docs/jax-rs-data-bindings.html#JAXRSDataBindings-WrappingandUnwrappingJSONsequences >>>>>>>>>> >>>>>>>>>> *This affects all methods returning a JSON response body (which >>>>>>>>>> is pretty much every API Method)* >>>>>>>>>> >>>>>>>>>> Please reply to this email if you have concerns, questions or >>>>>>>>>> objections. >>>>>>>>>> >>>>>>>>>> Thanks! >>>>>>>>>> Seb >>>>>>>>>> >>>>>>>>>> Sebastian Wagner >>>>>>>>>> Director Arrakeen Solutions, OM-Hosting.com >>>>>>>>>> http://arrakeen-solutions.co.nz/ >>>>>>>>>> https://om-hosting.com - Cloud & Server Hosting for HTML5 >>>>>>>>>> Video-Conferencing OpenMeetings >>>>>>>>>> >>>>>>>>>> <https://www.youracclaim.com/badges/da4e8828-743d-4968-af6f-49033f10d60a/public_url> >>>>>>>>>> <https://www.youracclaim.com/badges/b7e709c6-aa87-4b02-9faf-099038475e36/public_url> >>>>>>>>>> >>>>>>>>>> >>>>>>> >>>>>>> -- >>>>>>> Best regards, >>>>>>> Maxim >>>>>>> >>>>>>> >>>>>> >>>>>> -- >>>>>> Best regards, >>>>>> Maxim >>>>>> >>>>>> >>>>> >>>>> -- >>>>> Best regards, >>>>> Maxim >>>>> >>>>> >>> >>> -- >>> Best regards, >>> Maxim >>> >> -- Best regards, Maxim
