You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@brooklyn.apache.org by GitBox <gi...@apache.org> on 2021/06/03 14:55:07 UTC
[GitHub] [brooklyn-ui] duncangrant opened a new pull request #211: Login page fixes - WIP - please do not merge
duncangrant opened a new pull request #211:
URL: https://github.com/apache/brooklyn-ui/pull/211
Alert on failed login attempt
Redirect to home if already logged in
Fix issue where after logout and then clicking the return button it may
show the popup instead of redirecting to the login page.
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [brooklyn-ui] algairim commented on a change in pull request #211: Login page fixes - WIP - please do not merge
Posted by GitBox <gi...@apache.org>.
algairim commented on a change in pull request #211:
URL: https://github.com/apache/brooklyn-ui/pull/211#discussion_r645370904
##########
File path: ui-modules/utils/server-status/server-status.js
##########
@@ -65,6 +65,9 @@ export function BrServerStatusDirective() {
}else if(response.status === 404) {
state = BrServerStatusModalController.STATES.NO_CONNECTION;
}else if(response.status === 401 || response.status === 403 ) {
+ if( response.headers("LOGIN_PAGE")) {
Review comment:
Frankly, I do not know this bit, look at the `$location` service docs https://docs.angularjs.org/guide/$location
##########
File path: ui-modules/utils/server-status/server-status.js
##########
@@ -65,6 +65,9 @@ export function BrServerStatusDirective() {
}else if(response.status === 404) {
state = BrServerStatusModalController.STATES.NO_CONNECTION;
}else if(response.status === 401 || response.status === 403 ) {
+ if( response.headers("LOGIN_PAGE")) {
+ $window.location.href = '/' + response.headers("LOGIN_PAGE");
Review comment:
Frankly, I do not know this bit, look at the `$location` service docs https://docs.angularjs.org/guide/$location
##########
File path: ui-modules/login/app/views/main/main.controller.js
##########
@@ -47,18 +47,28 @@ export function loginStateController($scope, $http, $window, brBrandInfo) {
$scope.$emit(HIDE_INTERSTITIAL_SPINNER_EVENT);
$scope.getBrandedText = brBrandInfo.getBrandedText;
+ let loginController = this;
+ loginController.error = {};
+
+ var testAuthReq = {
+ method: 'HEAD',
+ url: '/v1/server/up/extended'
+ }
+
+ // If the user is already logged in then redirect to home
+ $http(testAuthReq).then( () => {$window.location.href = '/';} );
+
$scope.login = (user)=> {
var req = {
- method: 'GET',
- url: '/',
+ method: 'HEAD',
+ url: '/v1/server/up/extended',
headers: {
'Authorization': 'Basic ' + btoa(user.name +":" + user.password)
}
}
// ui registry metadata
$http(req)
- .then(function () {
- $window.location.href = '/';
- });
+ .then(() => { $window.location.href = '/'; },
+ () => { $window.alert("Login Failed") ; loginController.error.loginFailed = "Login Failed" });
Review comment:
We do not use alert windows. I would think about modal for it.
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [brooklyn-ui] duncangrant commented on a change in pull request #211: Login page fixes
Posted by GitBox <gi...@apache.org>.
duncangrant commented on a change in pull request #211:
URL: https://github.com/apache/brooklyn-ui/pull/211#discussion_r646723222
##########
File path: ui-modules/utils/server-status/server-status.js
##########
@@ -33,13 +33,15 @@ angular.module(MODULE_NAME, [uibModal])
export default MODULE_NAME;
+const LOGIN_PAGE_HEADER = "X_BROOKLYN_LOGIN_PAGE";
Review comment:
It is only returned if there is a login page to redirect to
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [brooklyn-ui] duncangrant commented on a change in pull request #211: Login page fixes - WIP - please do not merge
Posted by GitBox <gi...@apache.org>.
duncangrant commented on a change in pull request #211:
URL: https://github.com/apache/brooklyn-ui/pull/211#discussion_r644865960
##########
File path: ui-modules/login/app/views/main/main.controller.js
##########
@@ -47,18 +47,28 @@ export function loginStateController($scope, $http, $window, brBrandInfo) {
$scope.$emit(HIDE_INTERSTITIAL_SPINNER_EVENT);
$scope.getBrandedText = brBrandInfo.getBrandedText;
+ let loginController = this;
+ loginController.error = {};
+
+ var testAuthReq = {
+ method: 'HEAD',
+ url: '/v1/server/up/extended'
+ }
+
+ // If the user is already logged in then redirect to home
+ $http(testAuthReq).then( () => {$window.location.href = '/';} );
Review comment:
I think that there is a more angularjs way to redirect but I'm not sure I understand what it is.
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [brooklyn-ui] duncangrant commented on a change in pull request #211: Login page fixes - WIP - please do not merge
Posted by GitBox <gi...@apache.org>.
duncangrant commented on a change in pull request #211:
URL: https://github.com/apache/brooklyn-ui/pull/211#discussion_r644866891
##########
File path: ui-modules/login/app/views/main/main.controller.js
##########
@@ -47,18 +47,28 @@ export function loginStateController($scope, $http, $window, brBrandInfo) {
$scope.$emit(HIDE_INTERSTITIAL_SPINNER_EVENT);
$scope.getBrandedText = brBrandInfo.getBrandedText;
+ let loginController = this;
+ loginController.error = {};
+
+ var testAuthReq = {
+ method: 'HEAD',
+ url: '/v1/server/up/extended'
+ }
+
+ // If the user is already logged in then redirect to home
+ $http(testAuthReq).then( () => {$window.location.href = '/';} );
+
$scope.login = (user)=> {
var req = {
- method: 'GET',
- url: '/',
+ method: 'HEAD',
+ url: '/v1/server/up/extended',
headers: {
'Authorization': 'Basic ' + btoa(user.name +":" + user.password)
}
}
// ui registry metadata
$http(req)
- .then(function () {
- $window.location.href = '/';
- });
+ .then(() => { $window.location.href = '/'; },
+ () => { $window.alert("Login Failed") ; loginController.error.loginFailed = "Login Failed" });
Review comment:
Alerting works here but feels old / odd and would be better inline but I can't see how to get the loginFailed error to appear on. the form,
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [brooklyn-ui] duncangrant commented on a change in pull request #211: Login page fixes - WIP - please do not merge
Posted by GitBox <gi...@apache.org>.
duncangrant commented on a change in pull request #211:
URL: https://github.com/apache/brooklyn-ui/pull/211#discussion_r644865960
##########
File path: ui-modules/login/app/views/main/main.controller.js
##########
@@ -47,18 +47,28 @@ export function loginStateController($scope, $http, $window, brBrandInfo) {
$scope.$emit(HIDE_INTERSTITIAL_SPINNER_EVENT);
$scope.getBrandedText = brBrandInfo.getBrandedText;
+ let loginController = this;
+ loginController.error = {};
+
+ var testAuthReq = {
+ method: 'HEAD',
+ url: '/v1/server/up/extended'
+ }
+
+ // If the user is already logged in then redirect to home
+ $http(testAuthReq).then( () => {$window.location.href = '/';} );
Review comment:
I think that there is a more angularjs way to redirect but I'm not sure I understand what it is.
##########
File path: ui-modules/login/app/views/main/main.controller.js
##########
@@ -47,18 +47,28 @@ export function loginStateController($scope, $http, $window, brBrandInfo) {
$scope.$emit(HIDE_INTERSTITIAL_SPINNER_EVENT);
$scope.getBrandedText = brBrandInfo.getBrandedText;
+ let loginController = this;
+ loginController.error = {};
+
+ var testAuthReq = {
+ method: 'HEAD',
+ url: '/v1/server/up/extended'
+ }
+
+ // If the user is already logged in then redirect to home
+ $http(testAuthReq).then( () => {$window.location.href = '/';} );
+
$scope.login = (user)=> {
var req = {
- method: 'GET',
- url: '/',
+ method: 'HEAD',
+ url: '/v1/server/up/extended',
headers: {
'Authorization': 'Basic ' + btoa(user.name +":" + user.password)
}
}
// ui registry metadata
$http(req)
- .then(function () {
- $window.location.href = '/';
- });
+ .then(() => { $window.location.href = '/'; },
+ () => { $window.alert("Login Failed") ; loginController.error.loginFailed = "Login Failed" });
Review comment:
Alerting works here but feels old / odd and would be better inline but I can't see how to get the loginFailed error to appear on. the form,
##########
File path: ui-modules/login/app/views/main/main.template.html
##########
@@ -20,6 +20,9 @@
<div class="row">
<div class="col-md-4 col-md-offset-4 col-sm-12">
<form>
+ <p ng-if="error['loginFailed']">
Review comment:
This is a test
##########
File path: ui-modules/utils/server-status/server-status.js
##########
@@ -65,6 +65,9 @@ export function BrServerStatusDirective() {
}else if(response.status === 404) {
state = BrServerStatusModalController.STATES.NO_CONNECTION;
}else if(response.status === 401 || response.status === 403 ) {
+ if( response.headers("LOGIN_PAGE")) {
+ $window.location.href = '/' + response.headers("LOGIN_PAGE");
Review comment:
As before I'm not sure this is the angularjs way
##########
File path: ui-modules/utils/server-status/server-status.js
##########
@@ -65,6 +65,9 @@ export function BrServerStatusDirective() {
}else if(response.status === 404) {
state = BrServerStatusModalController.STATES.NO_CONNECTION;
}else if(response.status === 401 || response.status === 403 ) {
+ if( response.headers("LOGIN_PAGE")) {
Review comment:
Is it really bad practice to invent response headers?
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [brooklyn-ui] duncangrant commented on a change in pull request #211: Login page fixes - WIP - please do not merge
Posted by GitBox <gi...@apache.org>.
duncangrant commented on a change in pull request #211:
URL: https://github.com/apache/brooklyn-ui/pull/211#discussion_r644867469
##########
File path: ui-modules/utils/server-status/server-status.js
##########
@@ -65,6 +65,9 @@ export function BrServerStatusDirective() {
}else if(response.status === 404) {
state = BrServerStatusModalController.STATES.NO_CONNECTION;
}else if(response.status === 401 || response.status === 403 ) {
+ if( response.headers("LOGIN_PAGE")) {
+ $window.location.href = '/' + response.headers("LOGIN_PAGE");
Review comment:
As before I'm not sure this is the angularjs way
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [brooklyn-ui] algairim commented on a change in pull request #211: Login page fixes - WIP - please do not merge
Posted by GitBox <gi...@apache.org>.
algairim commented on a change in pull request #211:
URL: https://github.com/apache/brooklyn-ui/pull/211#discussion_r645430605
##########
File path: ui-modules/utils/server-status/server-status.js
##########
@@ -65,6 +65,9 @@ export function BrServerStatusDirective() {
}else if(response.status === 404) {
state = BrServerStatusModalController.STATES.NO_CONNECTION;
}else if(response.status === 401 || response.status === 403 ) {
+ if( response.headers("LOGIN_PAGE")) {
+ $window.location.href = '/' + response.headers("LOGIN_PAGE");
Review comment:
My vote is to redirect it from the backend.
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [brooklyn-ui] tbouron commented on a change in pull request #211: Login page fixes
Posted by GitBox <gi...@apache.org>.
tbouron commented on a change in pull request #211:
URL: https://github.com/apache/brooklyn-ui/pull/211#discussion_r646720600
##########
File path: ui-modules/utils/server-status/server-status.js
##########
@@ -33,13 +33,15 @@ angular.module(MODULE_NAME, [uibModal])
export default MODULE_NAME;
+const LOGIN_PAGE_HEADER = "X_BROOKLYN_LOGIN_PAGE";
Review comment:
@duncangrant Is that always returned from the API? I'm asking because it is used to build up the link to redirect to
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [brooklyn-ui] duncangrant commented on a change in pull request #211: Login page fixes - WIP - please do not merge
Posted by GitBox <gi...@apache.org>.
duncangrant commented on a change in pull request #211:
URL: https://github.com/apache/brooklyn-ui/pull/211#discussion_r644867106
##########
File path: ui-modules/login/app/views/main/main.template.html
##########
@@ -20,6 +20,9 @@
<div class="row">
<div class="col-md-4 col-md-offset-4 col-sm-12">
<form>
+ <p ng-if="error['loginFailed']">
Review comment:
This is a test
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [brooklyn-ui] tbouron commented on a change in pull request #211: Login page fixes
Posted by GitBox <gi...@apache.org>.
tbouron commented on a change in pull request #211:
URL: https://github.com/apache/brooklyn-ui/pull/211#discussion_r646726444
##########
File path: ui-modules/utils/server-status/server-status.js
##########
@@ -33,13 +33,15 @@ angular.module(MODULE_NAME, [uibModal])
export default MODULE_NAME;
+const LOGIN_PAGE_HEADER = "X_BROOKLYN_LOGIN_PAGE";
Review comment:
@duncangrant So if the header is not set, it will redirect to the home page. Is that the expected behaviour?
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [brooklyn-ui] duncangrant commented on a change in pull request #211: Login page fixes - WIP - please do not merge
Posted by GitBox <gi...@apache.org>.
duncangrant commented on a change in pull request #211:
URL: https://github.com/apache/brooklyn-ui/pull/211#discussion_r644867814
##########
File path: ui-modules/utils/server-status/server-status.js
##########
@@ -65,6 +65,9 @@ export function BrServerStatusDirective() {
}else if(response.status === 404) {
state = BrServerStatusModalController.STATES.NO_CONNECTION;
}else if(response.status === 401 || response.status === 403 ) {
+ if( response.headers("LOGIN_PAGE")) {
Review comment:
Is it really bad practice to invent response headers?
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [brooklyn-ui] tbouron commented on a change in pull request #211: Login page fixes - WIP - please do not merge
Posted by GitBox <gi...@apache.org>.
tbouron commented on a change in pull request #211:
URL: https://github.com/apache/brooklyn-ui/pull/211#discussion_r645467670
##########
File path: ui-modules/login/app/views/main/main.controller.js
##########
@@ -47,18 +47,28 @@ export function loginStateController($scope, $http, $window, brBrandInfo) {
$scope.$emit(HIDE_INTERSTITIAL_SPINNER_EVENT);
$scope.getBrandedText = brBrandInfo.getBrandedText;
+ let loginController = this;
+ loginController.error = {};
+
+ var testAuthReq = {
+ method: 'HEAD',
+ url: '/v1/server/up/extended'
+ }
+
+ // If the user is already logged in then redirect to home
+ $http(testAuthReq).then( () => {$window.location.href = '/';} );
+
$scope.login = (user)=> {
var req = {
- method: 'GET',
- url: '/',
+ method: 'HEAD',
+ url: '/v1/server/up/extended',
headers: {
'Authorization': 'Basic ' + btoa(user.name +":" + user.password)
}
}
// ui registry metadata
$http(req)
- .then(function () {
- $window.location.href = '/';
- });
+ .then(() => { $window.location.href = '/'; },
+ () => { $window.alert("Login Failed") ; loginController.error.loginFailed = "Login Failed" });
Review comment:
Yup, would be nicer to use the `uibModal` service to display a popup rather than the default browser alert.
Also, it is just code style but I find the syntax
```
promise
.then(() => {})
.catch(() => {})
```
nicer than the one you are using
##########
File path: ui-modules/login/app/views/main/main.template.html
##########
@@ -20,6 +20,9 @@
<div class="row">
<div class="col-md-4 col-md-offset-4 col-sm-12">
<form>
+ <p ng-if="error['loginFailed']">
+ fkjldsfdlskjfdslkj
Review comment:
Hum, did a cat wrote that? :)
##########
File path: ui-modules/utils/server-status/server-status.js
##########
@@ -65,6 +65,9 @@ export function BrServerStatusDirective() {
}else if(response.status === 404) {
state = BrServerStatusModalController.STATES.NO_CONNECTION;
}else if(response.status === 401 || response.status === 403 ) {
+ if( response.headers("LOGIN_PAGE")) {
Review comment:
I don't think it is. But by convention, there should start with `X_`.
For instance, I think `X_BROOKLYN_LOGIN_PAGE` would be nicer
##########
File path: ui-modules/login/app/views/main/main.controller.js
##########
@@ -47,18 +47,28 @@ export function loginStateController($scope, $http, $window, brBrandInfo) {
$scope.$emit(HIDE_INTERSTITIAL_SPINNER_EVENT);
$scope.getBrandedText = brBrandInfo.getBrandedText;
+ let loginController = this;
+ loginController.error = {};
+
+ var testAuthReq = {
+ method: 'HEAD',
+ url: '/v1/server/up/extended'
+ }
+
+ // If the user is already logged in then redirect to home
+ $http(testAuthReq).then( () => {$window.location.href = '/';} );
Review comment:
There would be @duncangrant if within the same UI module. But you want to redirect to home, i.e. `/` so this is the right thing to do
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [brooklyn-ui] duncangrant merged pull request #211: Login page fixes
Posted by GitBox <gi...@apache.org>.
duncangrant merged pull request #211:
URL: https://github.com/apache/brooklyn-ui/pull/211
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org