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.
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. 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. > > *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 > > *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 > > *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 > > *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 > - 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 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. > > 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 >> >
