From 227cd81e3ff27ab0747f29d9a2bba6032c5ad35a Mon Sep 17 00:00:00 2001 From: Srijeet Chatterjee Date: Fri, 7 Jan 2022 20:14:05 -0700 Subject: [PATCH 1/8] Add migrations and other modifications --- ...00_remove_null_last_authenticated.down.sql | 20 +++++++++++++++++++ ...1600_remove_null_last_authenticated.up.sql | 20 +++++++++++++++++++ traffic_ops/traffic_ops_golang/login/login.go | 14 +++++++++++++ .../modules/private/user/UserController.js | 3 +++ 4 files changed, 57 insertions(+) create mode 100644 traffic_ops/app/db/migrations/2022010715281600_remove_null_last_authenticated.down.sql create mode 100644 traffic_ops/app/db/migrations/2022010715281600_remove_null_last_authenticated.up.sql diff --git a/traffic_ops/app/db/migrations/2022010715281600_remove_null_last_authenticated.down.sql b/traffic_ops/app/db/migrations/2022010715281600_remove_null_last_authenticated.down.sql new file mode 100644 index 0000000000..3ed83d7364 --- /dev/null +++ b/traffic_ops/app/db/migrations/2022010715281600_remove_null_last_authenticated.down.sql @@ -0,0 +1,20 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one or more + * contributor license agreements. See the NOTICE file distributed with this + * work for additional information regarding copyright ownership. The ASF + * licenses this file to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, WITHOUT + * WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. See the + * License for the specific language governing permissions and limitations under + * the License. + */ + +UPDATE public.tm_user +SET last_authenticated = NULL +WHERE last_authenticated = '1970-01-01 00:00:00.000000'; \ No newline at end of file diff --git a/traffic_ops/app/db/migrations/2022010715281600_remove_null_last_authenticated.up.sql b/traffic_ops/app/db/migrations/2022010715281600_remove_null_last_authenticated.up.sql new file mode 100644 index 0000000000..818809ef46 --- /dev/null +++ b/traffic_ops/app/db/migrations/2022010715281600_remove_null_last_authenticated.up.sql @@ -0,0 +1,20 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one or more + * contributor license agreements. See the NOTICE file distributed with this + * work for additional information regarding copyright ownership. The ASF + * licenses this file to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, WITHOUT + * WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. See the + * License for the specific language governing permissions and limitations under + * the License. + */ + +UPDATE public.tm_user +SET last_authenticated = '1970-01-01 00:00:00.000000' +WHERE last_authenticated IS NULL; \ No newline at end of file diff --git a/traffic_ops/traffic_ops_golang/login/login.go b/traffic_ops/traffic_ops_golang/login/login.go index 0f31a355cb..d173fa9c22 100644 --- a/traffic_ops/traffic_ops_golang/login/login.go +++ b/traffic_ops/traffic_ops_golang/login/login.go @@ -232,6 +232,13 @@ func TokenLoginHandler(db *sqlx.DB, cfg config.Config) http.HandlerFunc { return } + _, dbErr := db.Exec(UpdateLoginTimeQuery, username) + if dbErr != nil { + log.Errorf("unable to update authentication time for a given user: %s\n", dbErr.Error()) + api.HandleErr(w, r, nil, http.StatusInternalServerError, nil, dbErr) + return + } + w.Header().Set(rfc.ContentType, rfc.ApplicationJSON) api.WriteAndLogErr(w, r, append(respBts, '\n')) @@ -371,6 +378,13 @@ func OauthLoginHandler(db *sqlx.DB, cfg config.Config) http.HandlerFunc { log.Errorf("checking local user: %s\n", err.Error()) } + _, dbErr := db.Exec(UpdateLoginTimeQuery, form.Username) + if dbErr != nil { + log.Errorf("unable to update authentication time for a given user: %s\n", dbErr.Error()) + api.HandleErr(w, r, nil, http.StatusInternalServerError, nil, dbErr) + return + } + if userAllowed && authenticated { httpCookie := tocookie.GetCookie(userId, defaultCookieDuration, cfg.Secrets[0]) http.SetCookie(w, httpCookie) diff --git a/traffic_portal/app/src/modules/private/user/UserController.js b/traffic_portal/app/src/modules/private/user/UserController.js index e4d29d39a7..1533ceffca 100644 --- a/traffic_portal/app/src/modules/private/user/UserController.js +++ b/traffic_portal/app/src/modules/private/user/UserController.js @@ -51,6 +51,9 @@ var UserController = function($scope, $state, $location, $uibModal, formUtils, l $scope.user = userModel.user; $scope.confirmSave = function(user, usernameField) { + if (usernameField == undefined) { + usernameField = user.username; + } if (usernameField.$dirty) { var params = { title: 'Reauthentication Required', From a710ed3af5d6bf66a553e4190ecf5ecfb42ca8f4 Mon Sep 17 00:00:00 2001 From: Srijeet Chatterjee Date: Fri, 7 Jan 2022 20:19:35 -0700 Subject: [PATCH 2/8] changelog entry --- CHANGELOG.md | 1 + 1 file changed, 1 insertion(+) diff --git a/CHANGELOG.md b/CHANGELOG.md index c2d67fa1f2..9fac928d24 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -35,6 +35,7 @@ The format is based on [Keep a Changelog](http://keepachangelog.com/en/1.0.0/). - [#6175](https://github.com/apache/trafficcontrol/issues/6175) - POST request to /api/4.0/phys_locations accepts mismatch values for regionName. - Fixed Traffic Monitor parsing stats_over_http output so that multiple stats for the same underlying delivery service (when the delivery service has more than 1 regex) are properly summed together. This makes the resulting data more accurate in addition to fixing the "new stat is lower than last stat" warnings. - Traffic Ops: Sanitize username before executing LDAP query +- [#6457](https://github.com/apache/trafficcontrol/issues/6457) - Fix broken user registration and password reset, due to the last_authenticated value being null. - [#6367](https://github.com/apache/trafficcontrol/issues/6367) - Fix PUT `user/current` to work with v4 User Roles and Permissions - [#6266](https://github.com/apache/trafficcontrol/issues/6266) - Removed postgresql13-devel requirement for traffic_ops - [#6446](https://github.com/apache/trafficcontrol/issues/6446) - Revert Traffic Router rollover file pattern to the one previously used in `log4j.properties` with Log4j 1.2 From 6206a4cde767bb635db32daace0f54f4620d65fe Mon Sep 17 00:00:00 2001 From: Srijeet Chatterjee Date: Fri, 7 Jan 2022 20:20:37 -0700 Subject: [PATCH 3/8] add new line at the end of migration --- .../2022010715281600_remove_null_last_authenticated.down.sql | 2 +- .../2022010715281600_remove_null_last_authenticated.up.sql | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/traffic_ops/app/db/migrations/2022010715281600_remove_null_last_authenticated.down.sql b/traffic_ops/app/db/migrations/2022010715281600_remove_null_last_authenticated.down.sql index 3ed83d7364..67489b7827 100644 --- a/traffic_ops/app/db/migrations/2022010715281600_remove_null_last_authenticated.down.sql +++ b/traffic_ops/app/db/migrations/2022010715281600_remove_null_last_authenticated.down.sql @@ -17,4 +17,4 @@ UPDATE public.tm_user SET last_authenticated = NULL -WHERE last_authenticated = '1970-01-01 00:00:00.000000'; \ No newline at end of file +WHERE last_authenticated = '1970-01-01 00:00:00.000000'; diff --git a/traffic_ops/app/db/migrations/2022010715281600_remove_null_last_authenticated.up.sql b/traffic_ops/app/db/migrations/2022010715281600_remove_null_last_authenticated.up.sql index 818809ef46..b75a3a626a 100644 --- a/traffic_ops/app/db/migrations/2022010715281600_remove_null_last_authenticated.up.sql +++ b/traffic_ops/app/db/migrations/2022010715281600_remove_null_last_authenticated.up.sql @@ -17,4 +17,4 @@ UPDATE public.tm_user SET last_authenticated = '1970-01-01 00:00:00.000000' -WHERE last_authenticated IS NULL; \ No newline at end of file +WHERE last_authenticated IS NULL; From 1232637e12f4eb73678109d11796bf57725337f6 Mon Sep 17 00:00:00 2001 From: Srijeet Chatterjee Date: Mon, 10 Jan 2022 11:24:15 -0700 Subject: [PATCH 4/8] fix the way TP updates users --- .../app/src/common/api/UserService.js | 22 +++++++++++++++---- 1 file changed, 18 insertions(+), 4 deletions(-) diff --git a/traffic_portal/app/src/common/api/UserService.js b/traffic_portal/app/src/common/api/UserService.js index 11195afc2c..da426db85c 100644 --- a/traffic_portal/app/src/common/api/UserService.js +++ b/traffic_portal/app/src/common/api/UserService.js @@ -83,12 +83,26 @@ var UserService = function($http, locationUtils, userModel, messageModel, ENV) { }; // todo: change to use query param when it is supported - this.updateUser = function(user) { - return $http.put(ENV.api.unstable + "users/" + user.id, user).then( + this.updateUser = function(userData) { + // We should be using PUT 'user/current' to update the current user + // Use PUT `users` only if the current user is not the same as the user being update + let path = 'users/' + userData.id; + if (userModel.user.id === userData.id) { + path = 'user/current'; + var userObject = { + user: userData + }; + userData = userObject; + } + return $http.put(ENV.api.unstable + path, userData).then( function(result) { - if (userModel.user.id === user.id) { + if (userData.user != undefined) { + userData = userData.user; + } + console.log(userData); + if (userModel.user.id === userData.id) { // if you are updating the currently logged in user... - userModel.setUser(user); + userModel.setUser(userData); } messageModel.setMessages(result.data.alerts, false); return result; From 846580fdd14cbc8f02c385aee0ece0bc65a2663a Mon Sep 17 00:00:00 2001 From: Srijeet Chatterjee Date: Mon, 10 Jan 2022 15:01:59 -0700 Subject: [PATCH 5/8] fix unit tests --- cache-config/t3c-generate/cfgfile/cfgfile_test.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/cache-config/t3c-generate/cfgfile/cfgfile_test.go b/cache-config/t3c-generate/cfgfile/cfgfile_test.go index 286475322f..60817f223b 100644 --- a/cache-config/t3c-generate/cfgfile/cfgfile_test.go +++ b/cache-config/t3c-generate/cfgfile/cfgfile_test.go @@ -214,7 +214,7 @@ func randDS() *atscfg.DeliveryService { ds.DSCP = randInt() ds.EdgeHeaderRewrite = randStr() ds.GeoLimit = randInt() - ds.GeoLimitCountries = randStr() + ds.GeoLimitCountries = nil ds.GeoLimitRedirectURL = randStr() ds.GeoProvider = randInt() ds.GlobalMaxMBPS = randInt() From 96cf27e7687cbc0797f99a755dc73c41e62d25d2 Mon Sep 17 00:00:00 2001 From: Srijeet Chatterjee Date: Mon, 10 Jan 2022 15:03:38 -0700 Subject: [PATCH 6/8] revert:fix unit tests --- cache-config/t3c-generate/cfgfile/cfgfile_test.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/cache-config/t3c-generate/cfgfile/cfgfile_test.go b/cache-config/t3c-generate/cfgfile/cfgfile_test.go index 60817f223b..286475322f 100644 --- a/cache-config/t3c-generate/cfgfile/cfgfile_test.go +++ b/cache-config/t3c-generate/cfgfile/cfgfile_test.go @@ -214,7 +214,7 @@ func randDS() *atscfg.DeliveryService { ds.DSCP = randInt() ds.EdgeHeaderRewrite = randStr() ds.GeoLimit = randInt() - ds.GeoLimitCountries = nil + ds.GeoLimitCountries = randStr() ds.GeoLimitRedirectURL = randStr() ds.GeoProvider = randInt() ds.GlobalMaxMBPS = randInt() From ebbc20b2f9b2dae6a25650e212b8f965e4b56e9d Mon Sep 17 00:00:00 2001 From: Srijeet Chatterjee Date: Mon, 10 Jan 2022 15:11:35 -0700 Subject: [PATCH 7/8] code review --- ...00_remove_null_last_authenticated.down.sql | 4 -- ...1600_remove_null_last_authenticated.up.sql | 2 +- traffic_ops/traffic_ops_golang/login/login.go | 15 +++---- .../app/src/common/api/UserService.js | 44 ++++++++++--------- .../modules/private/user/UserController.js | 2 +- 5 files changed, 33 insertions(+), 34 deletions(-) diff --git a/traffic_ops/app/db/migrations/2022010715281600_remove_null_last_authenticated.down.sql b/traffic_ops/app/db/migrations/2022010715281600_remove_null_last_authenticated.down.sql index 67489b7827..084106c81e 100644 --- a/traffic_ops/app/db/migrations/2022010715281600_remove_null_last_authenticated.down.sql +++ b/traffic_ops/app/db/migrations/2022010715281600_remove_null_last_authenticated.down.sql @@ -14,7 +14,3 @@ * License for the specific language governing permissions and limitations under * the License. */ - -UPDATE public.tm_user -SET last_authenticated = NULL -WHERE last_authenticated = '1970-01-01 00:00:00.000000'; diff --git a/traffic_ops/app/db/migrations/2022010715281600_remove_null_last_authenticated.up.sql b/traffic_ops/app/db/migrations/2022010715281600_remove_null_last_authenticated.up.sql index b75a3a626a..651c9f5044 100644 --- a/traffic_ops/app/db/migrations/2022010715281600_remove_null_last_authenticated.up.sql +++ b/traffic_ops/app/db/migrations/2022010715281600_remove_null_last_authenticated.up.sql @@ -16,5 +16,5 @@ */ UPDATE public.tm_user -SET last_authenticated = '1970-01-01 00:00:00.000000' +SET last_authenticated = now() WHERE last_authenticated IS NULL; diff --git a/traffic_ops/traffic_ops_golang/login/login.go b/traffic_ops/traffic_ops_golang/login/login.go index d173fa9c22..f4507d782e 100644 --- a/traffic_ops/traffic_ops_golang/login/login.go +++ b/traffic_ops/traffic_ops_golang/login/login.go @@ -234,7 +234,7 @@ func TokenLoginHandler(db *sqlx.DB, cfg config.Config) http.HandlerFunc { _, dbErr := db.Exec(UpdateLoginTimeQuery, username) if dbErr != nil { - log.Errorf("unable to update authentication time for a given user: %s\n", dbErr.Error()) + dbErr = fmt.Errorf("unable to update authentication time for user '%s': %w", username, dbErr) api.HandleErr(w, r, nil, http.StatusInternalServerError, nil, dbErr) return } @@ -378,14 +378,13 @@ func OauthLoginHandler(db *sqlx.DB, cfg config.Config) http.HandlerFunc { log.Errorf("checking local user: %s\n", err.Error()) } - _, dbErr := db.Exec(UpdateLoginTimeQuery, form.Username) - if dbErr != nil { - log.Errorf("unable to update authentication time for a given user: %s\n", dbErr.Error()) - api.HandleErr(w, r, nil, http.StatusInternalServerError, nil, dbErr) - return - } - if userAllowed && authenticated { + _, dbErr := db.Exec(UpdateLoginTimeQuery, form.Username) + if dbErr != nil { + dbErr = fmt.Errorf("unable to update authentication time for user '%s': %w", form.Username, dbErr) + api.HandleErr(w, r, nil, http.StatusInternalServerError, nil, dbErr) + return + } httpCookie := tocookie.GetCookie(userId, defaultCookieDuration, cfg.Secrets[0]) http.SetCookie(w, httpCookie) resp = struct { diff --git a/traffic_portal/app/src/common/api/UserService.js b/traffic_portal/app/src/common/api/UserService.js index da426db85c..7ec787d033 100644 --- a/traffic_portal/app/src/common/api/UserService.js +++ b/traffic_portal/app/src/common/api/UserService.js @@ -82,28 +82,14 @@ var UserService = function($http, locationUtils, userModel, messageModel, ENV) { ); }; - // todo: change to use query param when it is supported - this.updateUser = function(userData) { + this.updateCurrentUser = function(user) { // We should be using PUT 'user/current' to update the current user - // Use PUT `users` only if the current user is not the same as the user being update - let path = 'users/' + userData.id; - if (userModel.user.id === userData.id) { - path = 'user/current'; - var userObject = { - user: userData - }; - userData = userObject; - } - return $http.put(ENV.api.unstable + path, userData).then( + var currUser = { + user: user + }; + return $http.put(ENV.api.unstable + 'user/current', currUser).then( function(result) { - if (userData.user != undefined) { - userData = userData.user; - } - console.log(userData); - if (userModel.user.id === userData.id) { - // if you are updating the currently logged in user... - userModel.setUser(userData); - } + userModel.setUser(user); messageModel.setMessages(result.data.alerts, false); return result; }, @@ -114,6 +100,24 @@ var UserService = function($http, locationUtils, userModel, messageModel, ENV) { ); }; + // todo: change to use query param when it is supported + this.updateUser = function(user) { + if (userModel.user.id === user.id) { + return this.updateCurrentUser(user); + } else { + return $http.put(ENV.api.unstable + "users/" + user.id, user).then( + function(result) { + messageModel.setMessages(result.data.alerts, false); + return result; + }, + function(err) { + messageModel.setMessages(err.data.alerts, false); + throw err; + } + ); + } + }; + this.registerUser = function(registration) { return $http.post(ENV.api.unstable + "users/register", registration).then( function(result) { diff --git a/traffic_portal/app/src/modules/private/user/UserController.js b/traffic_portal/app/src/modules/private/user/UserController.js index 1533ceffca..f8afd5d14c 100644 --- a/traffic_portal/app/src/modules/private/user/UserController.js +++ b/traffic_portal/app/src/modules/private/user/UserController.js @@ -51,7 +51,7 @@ var UserController = function($scope, $state, $location, $uibModal, formUtils, l $scope.user = userModel.user; $scope.confirmSave = function(user, usernameField) { - if (usernameField == undefined) { + if (usernameField === undefined) { usernameField = user.username; } if (usernameField.$dirty) { From f87dc4ef96c57d0f03fe27a773c90194aeefd659 Mon Sep 17 00:00:00 2001 From: Srijeet Chatterjee Date: Mon, 10 Jan 2022 16:56:56 -0700 Subject: [PATCH 8/8] code review changes --- traffic_portal/app/src/common/api/UserService.js | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) diff --git a/traffic_portal/app/src/common/api/UserService.js b/traffic_portal/app/src/common/api/UserService.js index 7ec787d033..4e7fe31fb6 100644 --- a/traffic_portal/app/src/common/api/UserService.js +++ b/traffic_portal/app/src/common/api/UserService.js @@ -84,9 +84,7 @@ var UserService = function($http, locationUtils, userModel, messageModel, ENV) { this.updateCurrentUser = function(user) { // We should be using PUT 'user/current' to update the current user - var currUser = { - user: user - }; + const currUser = { user }; return $http.put(ENV.api.unstable + 'user/current', currUser).then( function(result) { userModel.setUser(user);