You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@brooklyn.apache.org by bostko <gi...@git.apache.org> on 2016/08/03 20:31:12 UTC
[GitHub] brooklyn-ui pull request #30: BROOKLYN-323: Improve logout script
GitHub user bostko opened a pull request:
https://github.com/apache/brooklyn-ui/pull/30
BROOKLYN-323: Improve logout script
- fix logout for Internet Explorer
- use get call to logout
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/bostko/brooklyn-ui master
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/brooklyn-ui/pull/30.patch
To close this pull request, make a commit to your master/trunk branch
with (at least) the following in the commit message:
This closes #30
----
commit 3c87b66ce49c6bfec9e022bd4718a5261df080fb
Author: Valentin Aitken <bo...@gmail.com>
Date: 2016-08-03T19:58:45Z
Improve logout script
- fix logout for Internet Explorer
- use get call to logout
----
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] brooklyn-ui pull request #30: BROOKLYN-323: Improve logout script
Posted by bostko <gi...@git.apache.org>.
Github user bostko commented on a diff in the pull request:
https://github.com/apache/brooklyn-ui/pull/30#discussion_r76041379
--- Diff: src/main/webapp/assets/js/util/brooklyn-utils.js ---
@@ -175,17 +175,16 @@ define([
};
Util.logout = function logout() {
- $.ajax({
- type: "POST",
- dataType: "text",
- url: "/v1/logout",
- success: function() {
- window.location.replace("/");
- },
- failure: function() {
- window.location.replace("/");
- }
- });
+ var ua = window.navigator.userAgent;
+ if (ua.indexOf("MSIE ") > 0 || ua.indexOf(" Edge/") > 0 || ua.indexOf(" Trident/") > 0) {
--- End diff --
Theoretically yes but in reality it is always like `* \(Windows ... (MSIE|Edge)`.
However changing it to `>=`.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] brooklyn-ui pull request #30: BROOKLYN-323: Improve logout script
Posted by m4rkmckenna <gi...@git.apache.org>.
Github user m4rkmckenna commented on a diff in the pull request:
https://github.com/apache/brooklyn-ui/pull/30#discussion_r76034844
--- Diff: src/main/webapp/assets/js/util/brooklyn-utils.js ---
@@ -175,17 +175,16 @@ define([
};
Util.logout = function logout() {
- $.ajax({
- type: "POST",
- dataType: "text",
- url: "/v1/logout",
- success: function() {
- window.location.replace("/");
- },
- failure: function() {
- window.location.replace("/");
- }
- });
+ var ua = window.navigator.userAgent;
+ if (ua.indexOf("MSIE ") > 0 || ua.indexOf(" Edge/") > 0 || ua.indexOf(" Trident/") > 0) {
--- End diff --
Should this be `>=`?
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] brooklyn-ui pull request #30: BROOKLYN-323: Improve logout script
Posted by neykov <gi...@git.apache.org>.
Github user neykov commented on a diff in the pull request:
https://github.com/apache/brooklyn-ui/pull/30#discussion_r73879185
--- Diff: src/main/webapp/assets/js/util/brooklyn-utils.js ---
@@ -175,17 +175,22 @@ define([
};
Util.logout = function logout() {
- $.ajax({
- type: "POST",
- dataType: "text",
- url: "/v1/logout",
- success: function() {
- window.location.replace("/");
- },
- failure: function() {
- window.location.replace("/");
- }
- });
+ var ua = window.navigator.userAgent;
+ if (ua.indexOf("MSIE ") > 0 || ua.indexOf(" Edge/") > 0 || ua.indexOf(" Trident/") > 0) {
+ $.ajax({
+ async: false,
--- End diff --
I'd avoid this at all costs. If the server is stalled or unreachable for a short while the whole UI will freeze. See other comment for doing away with this call altogether.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] brooklyn-ui issue #30: BROOKLYN-323: Improve logout script
Posted by aledsage <gi...@git.apache.org>.
Github user aledsage commented on the issue:
https://github.com/apache/brooklyn-ui/pull/30
@m4rkmckenna @sjcorbett can you please review and merge?
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] brooklyn-ui issue #30: BROOKLYN-323: Improve logout script
Posted by m4rkmckenna <gi...@git.apache.org>.
Github user m4rkmckenna commented on the issue:
https://github.com/apache/brooklyn-ui/pull/30
LGTM
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] brooklyn-ui issue #30: BROOKLYN-323: Improve logout script
Posted by nakomis <gi...@git.apache.org>.
Github user nakomis commented on the issue:
https://github.com/apache/brooklyn-ui/pull/30
Tested alongside https://github.com/apache/brooklyn-server/pull/288 and this resolves the issue on Windows 10 on both IE and Edge. Behaviour on Chrome on OS X remains unchanged
LGTM
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] brooklyn-ui pull request #30: BROOKLYN-323: Improve logout script
Posted by neykov <gi...@git.apache.org>.
Github user neykov commented on a diff in the pull request:
https://github.com/apache/brooklyn-ui/pull/30#discussion_r73882175
--- Diff: src/main/webapp/assets/js/util/brooklyn-utils.js ---
@@ -175,17 +175,22 @@ define([
};
Util.logout = function logout() {
- $.ajax({
- type: "POST",
- dataType: "text",
- url: "/v1/logout",
- success: function() {
- window.location.replace("/");
- },
- failure: function() {
- window.location.replace("/");
- }
- });
+ var ua = window.navigator.userAgent;
+ if (ua.indexOf("MSIE ") > 0 || ua.indexOf(" Edge/") > 0 || ua.indexOf(" Trident/") > 0) {
+ $.ajax({
+ async: false,
+ type: "GET",
+ dataType: "text",
+ url: "/v1/logout"
+ });
+ document.execCommand('ClearAuthenticationCache', 'false');
--- End diff --
To avoid the (potentially brittle) user agent check - it will fail but it doesn't matter when in a try-catch block. And when it works it will do its thing.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] brooklyn-ui pull request #30: BROOKLYN-323: Improve logout script
Posted by neykov <gi...@git.apache.org>.
Github user neykov commented on a diff in the pull request:
https://github.com/apache/brooklyn-ui/pull/30#discussion_r73878672
--- Diff: src/main/webapp/assets/js/util/brooklyn-utils.js ---
@@ -175,17 +175,22 @@ define([
};
Util.logout = function logout() {
- $.ajax({
- type: "POST",
- dataType: "text",
- url: "/v1/logout",
- success: function() {
- window.location.replace("/");
- },
- failure: function() {
- window.location.replace("/");
- }
- });
+ var ua = window.navigator.userAgent;
+ if (ua.indexOf("MSIE ") > 0 || ua.indexOf(" Edge/") > 0 || ua.indexOf(" Trident/") > 0) {
+ $.ajax({
+ async: false,
+ type: "GET",
+ dataType: "text",
+ url: "/v1/logout"
+ });
+ document.execCommand('ClearAuthenticationCache', 'false');
--- End diff --
Instead of doing a user agent check can we call `ClearAuthenticationCache` in a try-catch and use the code below for all browsers?
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] brooklyn-ui issue #30: BROOKLYN-323: Improve logout script
Posted by neykov <gi...@git.apache.org>.
Github user neykov commented on the issue:
https://github.com/apache/brooklyn-ui/pull/30
Looks good. To add some background on the unusual logout implementation - the server-side implementation will work correctly only if changing user names after logout. If the user tries to login with the same username it will keep prompting him for password. More details in the server code.
This is papering over the problem until we implement form based auth.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] brooklyn-ui pull request #30: BROOKLYN-323: Improve logout script
Posted by neykov <gi...@git.apache.org>.
Github user neykov commented on a diff in the pull request:
https://github.com/apache/brooklyn-ui/pull/30#discussion_r75841018
--- Diff: src/main/webapp/assets/js/util/brooklyn-utils.js ---
@@ -175,17 +175,23 @@ define([
};
Util.logout = function logout() {
- $.ajax({
- type: "POST",
- dataType: "text",
- url: "/v1/logout",
- success: function() {
- window.location.replace("/");
- },
- failure: function() {
- window.location.replace("/");
- }
- });
+ var ua = window.navigator.userAgent;
+ if (ua.indexOf("MSIE ") > 0 || ua.indexOf(" Edge/") > 0 || ua.indexOf(" Trident/") > 0) {
+ $.ajax({
+ type: "GET",
--- End diff --
`logout` is a `POST` operation.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] brooklyn-ui pull request #30: BROOKLYN-323: Improve logout script
Posted by bostko <gi...@git.apache.org>.
Github user bostko commented on a diff in the pull request:
https://github.com/apache/brooklyn-ui/pull/30#discussion_r73879162
--- Diff: src/main/webapp/assets/js/util/brooklyn-utils.js ---
@@ -175,17 +175,22 @@ define([
};
Util.logout = function logout() {
- $.ajax({
- type: "POST",
- dataType: "text",
- url: "/v1/logout",
- success: function() {
- window.location.replace("/");
- },
- failure: function() {
- window.location.replace("/");
- }
- });
+ var ua = window.navigator.userAgent;
+ if (ua.indexOf("MSIE ") > 0 || ua.indexOf(" Edge/") > 0 || ua.indexOf(" Trident/") > 0) {
+ $.ajax({
+ async: false,
+ type: "GET",
+ dataType: "text",
+ url: "/v1/logout"
+ });
+ document.execCommand('ClearAuthenticationCache', 'false');
--- End diff --
The command ClearAuthenticationCache it's compatible only for IE. What's the point in trying it for other browsers?
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] brooklyn-ui pull request #30: BROOKLYN-323: Improve logout script
Posted by asfgit <gi...@git.apache.org>.
Github user asfgit closed the pull request at:
https://github.com/apache/brooklyn-ui/pull/30
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] brooklyn-ui issue #30: BROOKLYN-323: Improve logout script
Posted by aledsage <gi...@git.apache.org>.
Github user aledsage commented on the issue:
https://github.com/apache/brooklyn-ui/pull/30
jenkins test failure is (presumably) unrelated: `java.io.IOException: No space left on device`.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---