[GitHub] [openmeetings] sebawagner merged pull request #166: OPENMEETINGS-2692 Update review comments on parsing path element.

2021-10-20 Thread GitBox 


sebawagner merged pull request #166:
URL: https://github.com/apache/openmeetings/pull/166


   


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: dev-unsubscr...@openmeetings.apache.org

For queries about this service, please contact Infrastructure at:
us...@infra.apache.org

[GitHub] [openmeetings] sebawagner commented on a change in pull request #166: OPENMEETINGS-2692 Update review comments on parsing path element.

2021-10-20 Thread GitBox 


sebawagner commented on a change in pull request #166:
URL: https://github.com/apache/openmeetings/pull/166#discussion_r732528533



##
File path: 
openmeetings-util/src/main/java/org/apache/openmeetings/util/OpenmeetingsVariables.java
##
@@ -249,11 +256,12 @@ public static String getBaseUrl() {
}
 
public static String getWebappPath() {
-   String webappPath = baseUrl;
-   if (webappPath.endsWith("/")) {
-   webappPath = webappPath.substring(0, 
webappPath.length() - 1);
+   try {

Review comment:
   done. Seems to work fine.




-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: dev-unsubscr...@openmeetings.apache.org

For queries about this service, please contact Infrastructure at:
us...@infra.apache.org

Jenkins build is back to normal : OpenMeetings ยป openmeetings #56

2021-10-20 Thread Apache Jenkins Server 
See

Re: [VOTE] Apache OpenMeetings 6.2.0 based on Release Candidate 2

2021-10-20 Thread Maxim Solodovnik 
On Wed, 20 Oct 2021 at 13:58, seba.wag...@gmail.com 
wrote:

> >
> > How would you "guess" the correct HTTPS port?
> >
> I think you don't need to know.
>
> I think you can simply specify the "redirect" port in the "connector"
>
> https://serverfault.com/questions/758817/whats-the-redirect-port-for-in-tomcat
>
>
It is already specified:
https://github.com/apache/openmeetings/blob/master/openmeetings-server/src/main/assembly/conf/server.xml#L57


> And then the client can directly request HTTPS on the HTTP port.
>
> And since the connector can't handle HTTPS (but has the redirect port
> specified) it will automatically redirect the user to the HTTPS version of
> the website.
>
> 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, 20 Oct 2021 at 19:40, Maxim Solodovnik 
> wrote:
>
> > On Wed, 20 Oct 2021 at 12:13, seba.wag...@gmail.com <
> seba.wag...@gmail.com
> > >
> > wrote:
> >
> > > >
> > > > I believe all these points are non-blockers due to
> > > > almost everything below is something new
> > > > 6.2.0 intended to be bug-fix release (fixing serious issue at iPad )
> > >
> > > Ok. Lets just vote and do that in 7.0.0
> > >
> > > I would say this is something user can *guess* to do :)
> > > > it will provide much more space
> > >
> > >  Lets do landscape rotating user prompt in v7.0.0 (or 6.3.0)
> > >
> > > This might be moved to network-testing-tool
> > > > I doubt there is ready to use browser list we can use
> > > > And there is definitely no browser+version list :(((
> > >
> > > Lets do in v7.0.0 but I think this is really necessary. We don't need a
> > > "exclusive" list. E.g. we can just check for Safari for a start where
> we
> > > know v15.x is a minimum.
> > > Also I'm thinking this "prompt" can be non blocking == Meaning if you
> > have
> > > an old browser you can still use it, you just need to click away the
> > > prompt.
> > >
> > > Well
> > > > OM is currently shipped with self-signed-HTTPS ready to use
> > > > And It prompts user HTTPS is required
> > > > And all documentation states HTTPS is required
> > >
> > >
> > > > It is bad idea to drop HTTP just because it is the "right way" to set
> > up
> > > > front-end proxy
> > > > (Like it currently set-up on all demo servers)
> > >
> > > I'm not suggesting the "drop" HTTP.
> > > I am suggesting to change the "default" redirect when you enter
> > > http://myhost:port/ and redirect not only to /openmeetings but to
> https
> > > And potentially when you arrive at "BasePage.html" to make sure it
> > redirect
> > > you to "HTTPS" in case you are not.
> > > That is it.
> > >
> >
> > I'm afraid this is hardly possible :((
> > How would you "guess" the correct HTTPS port?
> > for ex. it is 5443 by default, 443 on demo, 8443 on demo-next 
> >
> >
> > > That will not have an impact on any HTTPS/proxy. You can still run
> NGINX
> > or
> > > Apache mod_proxy to terminate SSL and have OpenMeetings run over
> HTTP/Non
> > > SSL.
> > > That will still work fine. All of the above are client side redirects
> > > (meaning if the client detects they are on http they redirect to
> https).
> > >
> > > Anyway, let me start voting on 6.2.0. And then we can address the
> above.
> > >
> > > 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, 20 Oct 2021 at 05:55, Maxim Solodovnik 
> > > wrote:
> > >
> > > > On Sun, 17 Oct 2021 at 09:40, seba.wag...@gmail.com <
> > > seba.wag...@gmail.com
> > > > >
> > > > wrote:
> > > >
> > > > > There might be a few relatively small but impactful things to add
> to
> > > the
> > > > > release:
> > > > >
> > > > >
> > > > I believe all these points are non-blockers due to
> > > > almost everything below is something new
> > > > 6.2.0 intended to be bug-fix release (fixing serious issue at iPad )
> > > >
> > > >
> > > > > *Prompt mobile browsers to use landscape mode *
> > > > > I would add this only inside the conference room, not outside.
> > Outside
> > > of
> > > > > the conference room it's actually okay to be in portrait mode.
> > > > > But inside the conference room portrait mode really just doesn't
> > work.
> > > > With
> > > > > the viewport that I've added you can finally click on some of the
> > icons
> > > > > inside the conference room, but

Re: [VOTE] Apache OpenMeetings 6.2.0 based on Release Candidate 2

2021-10-20 Thread seba.wag...@gmail.com 
>
> How would you "guess" the correct HTTPS port?
>
I think you don't need to know.

I think you can simply specify the "redirect" port in the "connector"
https://serverfault.com/questions/758817/whats-the-redirect-port-for-in-tomcat

And then the client can directly request HTTPS on the HTTP port.

And since the connector can't handle HTTPS (but has the redirect port
specified) it will automatically redirect the user to the HTTPS version of
the website.

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




On Wed, 20 Oct 2021 at 19:40, Maxim Solodovnik  wrote:

> On Wed, 20 Oct 2021 at 12:13, seba.wag...@gmail.com  >
> wrote:
>
> > >
> > > I believe all these points are non-blockers due to
> > > almost everything below is something new
> > > 6.2.0 intended to be bug-fix release (fixing serious issue at iPad )
> >
> > Ok. Lets just vote and do that in 7.0.0
> >
> > I would say this is something user can *guess* to do :)
> > > it will provide much more space
> >
> >  Lets do landscape rotating user prompt in v7.0.0 (or 6.3.0)
> >
> > This might be moved to network-testing-tool
> > > I doubt there is ready to use browser list we can use
> > > And there is definitely no browser+version list :(((
> >
> > Lets do in v7.0.0 but I think this is really necessary. We don't need a
> > "exclusive" list. E.g. we can just check for Safari for a start where we
> > know v15.x is a minimum.
> > Also I'm thinking this "prompt" can be non blocking == Meaning if you
> have
> > an old browser you can still use it, you just need to click away the
> > prompt.
> >
> > Well
> > > OM is currently shipped with self-signed-HTTPS ready to use
> > > And It prompts user HTTPS is required
> > > And all documentation states HTTPS is required
> >
> >
> > > It is bad idea to drop HTTP just because it is the "right way" to set
> up
> > > front-end proxy
> > > (Like it currently set-up on all demo servers)
> >
> > I'm not suggesting the "drop" HTTP.
> > I am suggesting to change the "default" redirect when you enter
> > http://myhost:port/ and redirect not only to /openmeetings but to https
> > And potentially when you arrive at "BasePage.html" to make sure it
> redirect
> > you to "HTTPS" in case you are not.
> > That is it.
> >
>
> I'm afraid this is hardly possible :((
> How would you "guess" the correct HTTPS port?
> for ex. it is 5443 by default, 443 on demo, 8443 on demo-next 
>
>
> > That will not have an impact on any HTTPS/proxy. You can still run NGINX
> or
> > Apache mod_proxy to terminate SSL and have OpenMeetings run over HTTP/Non
> > SSL.
> > That will still work fine. All of the above are client side redirects
> > (meaning if the client detects they are on http they redirect to https).
> >
> > Anyway, let me start voting on 6.2.0. And then we can address the above.
> >
> > 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, 20 Oct 2021 at 05:55, Maxim Solodovnik 
> > wrote:
> >
> > > On Sun, 17 Oct 2021 at 09:40, seba.wag...@gmail.com <
> > seba.wag...@gmail.com
> > > >
> > > wrote:
> > >
> > > > There might be a few relatively small but impactful things to add to
> > the
> > > > release:
> > > >
> > > >
> > > I believe all these points are non-blockers due to
> > > almost everything below is something new
> > > 6.2.0 intended to be bug-fix release (fixing serious issue at iPad )
> > >
> > >
> > > > *Prompt mobile browsers to use landscape mode *
> > > > I would add this only inside the conference room, not outside.
> Outside
> > of
> > > > the conference room it's actually okay to be in portrait mode.
> > > > But inside the conference room portrait mode really just doesn't
> work.
> > > With
> > > > the viewport that I've added you can finally click on some of the
> icons
> > > > inside the conference room, but it's still un-usable. It just doesn't
> > > have
> > > > enough space on the screen in portrait mode.
> > > > So adding a simple and small prompt for users to suggest to rotate to
> > > > landscape mode will be good. And maybe only add that inside the
> > > conference
> > > > room as outside is actually okay.
> > > >
> > >
> > > I would say this is something user can *guess* to do :)
> > > it will provide much more space
> > >
> > >
> > > >
> > > > *Check for minimum browser versions, especially on mobile devices*
> > >

Re: [VOTE] Apache OpenMeetings 6.2.0 based on Release Candidate 2

2021-10-20 Thread Maxim Solodovnik 
On Wed, 20 Oct 2021 at 12:13, seba.wag...@gmail.com 
wrote:

> >
> > I believe all these points are non-blockers due to
> > almost everything below is something new
> > 6.2.0 intended to be bug-fix release (fixing serious issue at iPad )
>
> Ok. Lets just vote and do that in 7.0.0
>
> I would say this is something user can *guess* to do :)
> > it will provide much more space
>
>  Lets do landscape rotating user prompt in v7.0.0 (or 6.3.0)
>
> This might be moved to network-testing-tool
> > I doubt there is ready to use browser list we can use
> > And there is definitely no browser+version list :(((
>
> Lets do in v7.0.0 but I think this is really necessary. We don't need a
> "exclusive" list. E.g. we can just check for Safari for a start where we
> know v15.x is a minimum.
> Also I'm thinking this "prompt" can be non blocking == Meaning if you have
> an old browser you can still use it, you just need to click away the
> prompt.
>
> Well
> > OM is currently shipped with self-signed-HTTPS ready to use
> > And It prompts user HTTPS is required
> > And all documentation states HTTPS is required
>
>
> > It is bad idea to drop HTTP just because it is the "right way" to set up
> > front-end proxy
> > (Like it currently set-up on all demo servers)
>
> I'm not suggesting the "drop" HTTP.
> I am suggesting to change the "default" redirect when you enter
> http://myhost:port/ and redirect not only to /openmeetings but to https
> And potentially when you arrive at "BasePage.html" to make sure it redirect
> you to "HTTPS" in case you are not.
> That is it.
>

I'm afraid this is hardly possible :((
How would you "guess" the correct HTTPS port?
for ex. it is 5443 by default, 443 on demo, 8443 on demo-next 


> That will not have an impact on any HTTPS/proxy. You can still run NGINX or
> Apache mod_proxy to terminate SSL and have OpenMeetings run over HTTP/Non
> SSL.
> That will still work fine. All of the above are client side redirects
> (meaning if the client detects they are on http they redirect to https).
>
> Anyway, let me start voting on 6.2.0. And then we can address the above.
>
> 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, 20 Oct 2021 at 05:55, Maxim Solodovnik 
> wrote:
>
> > On Sun, 17 Oct 2021 at 09:40, seba.wag...@gmail.com <
> seba.wag...@gmail.com
> > >
> > wrote:
> >
> > > There might be a few relatively small but impactful things to add to
> the
> > > release:
> > >
> > >
> > I believe all these points are non-blockers due to
> > almost everything below is something new
> > 6.2.0 intended to be bug-fix release (fixing serious issue at iPad )
> >
> >
> > > *Prompt mobile browsers to use landscape mode *
> > > I would add this only inside the conference room, not outside. Outside
> of
> > > the conference room it's actually okay to be in portrait mode.
> > > But inside the conference room portrait mode really just doesn't work.
> > With
> > > the viewport that I've added you can finally click on some of the icons
> > > inside the conference room, but it's still un-usable. It just doesn't
> > have
> > > enough space on the screen in portrait mode.
> > > So adding a simple and small prompt for users to suggest to rotate to
> > > landscape mode will be good. And maybe only add that inside the
> > conference
> > > room as outside is actually okay.
> > >
> >
> > I would say this is something user can *guess* to do :)
> > it will provide much more space
> >
> >
> > >
> > > *Check for minimum browser versions, especially on mobile devices*
> > > e.g. Safari we know is broken for webRTC if Safari version is smaller
> > then
> > > v15.x. I'm sure other browsers have similar minimum versions.
> > > It will just otherwise lead to a lot of frustration or users give up on
> > > OpenMeetings cause they don't understand that their problem is with
> their
> > > browser. Not OpenMeetings. And in most scenarios this does not indicate
> > > anything to users. It just silently breaks.
> > >
> > >
> > This might be moved to network-testing-tool
> > I doubt there is ready to use browser list we can use
> > And there is definitely no browser+version list :(((
> >
> >
> >
> > > *Update initial check to redirect all users to HTTPS*
> > > OpenMeetings really does not work without SSL/HTTPS.
> > > Most of the HTML5/webRTC APIs require HTTPS. But also e.g. the
> > > "Notification" API requires HTTPS (see
> > > https://developer.mozilla.org/en-US/docs/Web/API/notification). So
> users
> > > can not  watch a video conference. Nor publish mic/cam. And that is not
> > > just "inside" the conference room. It looks increasingly difficult to
> > > safeguard every possible

[GitHub] [openmeetings] solomax commented on a change in pull request #166: OPENMEETINGS-2692 Update review comments on parsing path element.

2021-10-20 Thread GitBox 


solomax commented on a change in pull request #166:
URL: https://github.com/apache/openmeetings/pull/166#discussion_r732451204



##
File path: 
openmeetings-util/src/main/java/org/apache/openmeetings/util/OpenmeetingsVariables.java
##
@@ -249,11 +256,12 @@ public static String getBaseUrl() {
}
 
public static String getWebappPath() {
-   String webappPath = baseUrl;
-   if (webappPath.endsWith("/")) {
-   webappPath = webappPath.substring(0, 
webappPath.length() - 1);
+   try {

Review comment:
   I have created the method to avoid code duplication :)
   
   your current code is: `return URI.create(DEFAULT_BASE_URL);`
   
   OK :)
   you are right, I was too fast and not use compiler one more time :((
   This code will work as expected:
   
   ```
   private static URI getWebappPath(url) {
   return URI.create(URI.create(url + "/").normalize().getPath());
   }
   
   public static URI getWebappPath() {
   try {
   return getWebappPath(baseUrl);
   } catch (Exception e) {
   return getWebappPath(DEFAULT_BASE_URL);
   }
   }
   ```




-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: dev-unsubscr...@openmeetings.apache.org

For queries about this service, please contact Infrastructure at:
us...@infra.apache.org

[GitHub] [openmeetings] sebawagner commented on a change in pull request #166: OPENMEETINGS-2692 Update review comments on parsing path element.

2021-10-20 Thread GitBox 


sebawagner commented on a change in pull request #166:
URL: https://github.com/apache/openmeetings/pull/166#discussion_r732447851



##
File path: 
openmeetings-util/src/main/java/org/apache/openmeetings/util/OpenmeetingsVariables.java
##
@@ -249,11 +256,12 @@ public static String getBaseUrl() {
}
 
public static String getWebappPath() {
-   String webappPath = baseUrl;
-   if (webappPath.endsWith("/")) {
-   webappPath = webappPath.substring(0, 
webappPath.length() - 1);
+   try {

Review comment:
   ```
   } catch (URISyntaxException e) {
   return getWebappPath(DEFAULT_BASE_URL);
   }
   ```
   Will not compile. It will throw an exception in the catch block. Which will 
just not compile. I also don't see why you need to split this into 2 methods.
   
   > your current code will return https://localhost:5443/openmeetings
   
   No it won't. It says ".getPath()" so it won't. DEFAULT_BASE_URL is actually 
the same it will use by default. 
   
   There is no different between our alternatives. Except your alternative 
splits it into 2 methods. For no obvious reason. 
   




-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: dev-unsubscr...@openmeetings.apache.org

For queries about this service, please contact Infrastructure at:
us...@infra.apache.org

[GitHub] [openmeetings] sebawagner commented on a change in pull request #166: OPENMEETINGS-2692 Update review comments on parsing path element.

2021-10-20 Thread GitBox 


sebawagner commented on a change in pull request #166:
URL: https://github.com/apache/openmeetings/pull/166#discussion_r732447851



##
File path: 
openmeetings-util/src/main/java/org/apache/openmeetings/util/OpenmeetingsVariables.java
##
@@ -249,11 +256,12 @@ public static String getBaseUrl() {
}
 
public static String getWebappPath() {
-   String webappPath = baseUrl;
-   if (webappPath.endsWith("/")) {
-   webappPath = webappPath.substring(0, 
webappPath.length() - 1);
+   try {

Review comment:
   ```
   } catch (URISyntaxException e) {
   return getWebappPath(DEFAULT_BASE_URL);
   }
   ```
   Will not compile. It will throw an exception in the catch block. Which will 
just not compile. I also don't see why you need to split this into 2 methods.
   
   > your current code will return https://localhost:5443/openmeetings
   
   No it won't. It says ".getPath()" so it won't. DEFAULT_BASE_URL is actually 
the same it will test with by default. 
   
   There is no different between our alternatives. Except your alternative 
splits it into 2 methods. For no obvious reason. 
   




-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: dev-unsubscr...@openmeetings.apache.org

For queries about this service, please contact Infrastructure at:
us...@infra.apache.org

[GitHub] [openmeetings] sebawagner commented on a change in pull request #166: OPENMEETINGS-2692 Update review comments on parsing path element.

2021-10-20 Thread GitBox 


sebawagner commented on a change in pull request #166:
URL: https://github.com/apache/openmeetings/pull/166#discussion_r732447851



##
File path: 
openmeetings-util/src/main/java/org/apache/openmeetings/util/OpenmeetingsVariables.java
##
@@ -249,11 +256,12 @@ public static String getBaseUrl() {
}
 
public static String getWebappPath() {
-   String webappPath = baseUrl;
-   if (webappPath.endsWith("/")) {
-   webappPath = webappPath.substring(0, 
webappPath.length() - 1);
+   try {

Review comment:
   ```
   } catch (URISyntaxException e) {
   return getWebappPath(DEFAULT_BASE_URL);
   }
   ```
   Will not compile. It will throw an exception in the catch block. Which will 
just not compile. I also don't see why you need to split this into 2 methods.
   
   > your current code will return https://localhost:5443/openmeetings
   
   No it won't. It says ".getPath()" so it won't. I was testing it.
   
   There is no different between our alternatives. Except your alternative 
splits it into 2 methods. For no obvious reason. 
   




-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: dev-unsubscr...@openmeetings.apache.org

For queries about this service, please contact Infrastructure at:
us...@infra.apache.org

[GitHub] [openmeetings] sebawagner commented on a change in pull request #166: OPENMEETINGS-2692 Update review comments on parsing path element.

2021-10-20 Thread GitBox 


sebawagner commented on a change in pull request #166:
URL: https://github.com/apache/openmeetings/pull/166#discussion_r732447851



##
File path: 
openmeetings-util/src/main/java/org/apache/openmeetings/util/OpenmeetingsVariables.java
##
@@ -249,11 +256,12 @@ public static String getBaseUrl() {
}
 
public static String getWebappPath() {
-   String webappPath = baseUrl;
-   if (webappPath.endsWith("/")) {
-   webappPath = webappPath.substring(0, 
webappPath.length() - 1);
+   try {

Review comment:
   ```
   } catch (URISyntaxException e) {
   return getWebappPath(DEFAULT_BASE_URL);
   }
   ```
   Will not compile. It will throw an exception in the catch block. Which will 
just not compile. I also don't see why you need to split this into 2 methods.
   
   > your current code will return https://localhost:5443/openmeetings
   No it won't. It says ".getPath()" so it won't. I was testing it.
   
   There is no different between our alternatives. Except your alternative 
splits it into 2 methods. For no obvious reason. 
   




-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: dev-unsubscr...@openmeetings.apache.org

For queries about this service, please contact Infrastructure at:
us...@infra.apache.org

[GitHub] [openmeetings] solomax commented on a change in pull request #166: OPENMEETINGS-2692 Update review comments on parsing path element.

2021-10-20 Thread GitBox 


solomax commented on a change in pull request #166:
URL: https://github.com/apache/openmeetings/pull/166#discussion_r732439198



##
File path: 
openmeetings-util/src/main/java/org/apache/openmeetings/util/OpenmeetingsVariables.java
##
@@ -249,11 +256,12 @@ public static String getBaseUrl() {
}
 
public static String getWebappPath() {
-   String webappPath = baseUrl;
-   if (webappPath.endsWith("/")) {
-   webappPath = webappPath.substring(0, 
webappPath.length() - 1);
+   try {

Review comment:
   My code was sort-of "pseudo code"
   I wrote it without compiler :))
   
   it should look as follows:
   ```
   private static URI getWebappPath(url) throws URISyntaxException {
   return new URI(new URI(url + "/").normalize().getPath());
   }
   public static URI getWebappPath() {
   try {
   return getWebappPath(baseUrl);
   } catch (URISyntaxException e) {
   return getWebappPath(DEFAULT_BASE_URL);
   }
   }
   ```
   
   The idea is `new URI` can `throw URISyntaxException` while parsing
   While subsequent `URI.create` should be safe
   
   I dropped `URI.create` to be 100% bulletproof
   
   now we do same actions on current `baseUrl` OR `DEFAULT_BASE_URL` in case of 
error
   
   your current code will return `https://localhost:5443/openmeetings`
   which is not intended 




-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: dev-unsubscr...@openmeetings.apache.org

For queries about this service, please contact Infrastructure at:
us...@infra.apache.org