You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@trafficcontrol.apache.org by GitBox <gi...@apache.org> on 2022/01/08 03:19:28 UTC

[GitHub] [trafficcontrol] srijeet0406 opened a new pull request #6458: User registration and password reset are broken due to the last_authenticated value being null

srijeet0406 opened a new pull request #6458:
URL: https://github.com/apache/trafficcontrol/pull/6458


   <!--
   Thank you for contributing! Please be sure to read our contribution guidelines: https://github.com/apache/trafficcontrol/blob/master/CONTRIBUTING.md
   If this closes or relates to an existing issue, please reference it using one of the following:
   
   Closes: #6457 
   Related: #ISSUE
   
   If this PR fixes a security vulnerability, DO NOT submit! Instead, contact
   the Apache Traffic Control Security Team at security@trafficcontrol.apache.org and follow the
   guidelines at https://apache.org/security regarding vulnerability disclosure.
   -->
   
   This PR closes https://github.com/apache/trafficcontrol/issues/6457
   <!-- **^ Add meaningful description above** --><hr>
   
   ## Which Traffic Control components are affected by this PR?
   <!-- Please delete all components from this list that are NOT affected by this PR.
   Feel free to add the name of a tool or script that is affected but not on the list.
   -->
   - Traffic Ops
   - Traffic Portal
   
   ## What is the best way to verify this PR?
   <!-- Please include here ALL the steps necessary to test your PR.
   If your PR has tests (and most should), provide the steps needed to run the tests.
   If not, please provide step-by-step instructions to test the PR manually and explain why your PR does not need tests. -->
   Run TP and TO, and verify that you can successfully register new users and reset passwords of existing users.
   
   ## If this is a bugfix, which Traffic Control versions contained the bug?
   <!-- Delete this section if the PR is not a bugfix, or if the bug is only in the master branch.
   Examples:
   - 5.1.2
   - 5.1.3 (RC1)
    -->
   - master
   
   ## PR submission checklist
   - [x] This PR does not have tests <!-- If not, please delete this text and explain why this PR does not need tests. -->
   - [x] This PR does not have documentation <!-- If not, please delete this text and explain why this PR does not need documentation. -->
   - [x] This PR has a CHANGELOG.md entry <!-- A fix for a bug from an ATC release, an improvement, or a new feature should have a changelog entry. -->
   - [x] This PR **DOES NOT FIX A SERIOUS SECURITY VULNERABILITY** (see [the Apache Software Foundation's security guidelines](https://apache.org/security) for details)
   
   <!--
   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.
   -->
   


-- 
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: issues-unsubscribe@trafficcontrol.apache.org

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

[GitHub] [trafficcontrol] srijeet0406 commented on a change in pull request #6458: User registration and password reset are broken due to the last_authenticated value being null

Posted by GitBox <gi...@apache.org>.
srijeet0406 commented on a change in pull request #6458:
URL: https://github.com/apache/trafficcontrol/pull/6458#discussion_r781559172



##########
File path: 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;

Review comment:
       Sounds good, I'll set it to `now()` wherever it is `null`. Fun fact, I had it as `now()` when I first started debugging this issue, and then changed it to epoch start later.




-- 
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: issues-unsubscribe@trafficcontrol.apache.org

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

[GitHub] [trafficcontrol] ocket8888 commented on a change in pull request #6458: User registration and password reset are broken due to the last_authenticated value being null

Posted by GitBox <gi...@apache.org>.
ocket8888 commented on a change in pull request #6458:
URL: https://github.com/apache/trafficcontrol/pull/6458#discussion_r781547289



##########
File path: 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())

Review comment:
       Actually, disregard, because it looks like you're double-logging this, because `api.HandleErr` will log it too. It looks like what you actually want something like:
   ```go
   if dbErr != nil {
       dbErr = fmt.Errorf("unable to update authentication time for user '%s': %w", username, dbErr)
       api.HandleErr(w, r, nil, http.StatusInternalServerError, nil, dbErr)
       return
   }
   ```
   ... instead of logging it twice.
   
   (I think the username is helpful information)




-- 
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: issues-unsubscribe@trafficcontrol.apache.org

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

[GitHub] [trafficcontrol] ocket8888 commented on a change in pull request #6458: User registration and password reset are broken due to the last_authenticated value being null

Posted by GitBox <gi...@apache.org>.
ocket8888 commented on a change in pull request #6458:
URL: https://github.com/apache/trafficcontrol/pull/6458#discussion_r781496878



##########
File path: 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;

Review comment:
       well, I think you have to be the tiebreaker here. I vote we let it be null and it looks to me like Rawlin is voting we don't, at the risk of speaking for him.




-- 
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: issues-unsubscribe@trafficcontrol.apache.org

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

[GitHub] [trafficcontrol] rawlinp commented on a change in pull request #6458: User registration and password reset are broken due to the last_authenticated value being null

Posted by GitBox <gi...@apache.org>.
rawlinp commented on a change in pull request #6458:
URL: https://github.com/apache/trafficcontrol/pull/6458#discussion_r781398499



##########
File path: 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;

Review comment:
       It doesn't hurt really -- I think we could even make this column non-nullable with a default of beginning of epoch. There'd be one less nullable column to worry about at least.




-- 
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: issues-unsubscribe@trafficcontrol.apache.org

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

[GitHub] [trafficcontrol] srijeet0406 commented on a change in pull request #6458: User registration and password reset are broken due to the last_authenticated value being null

Posted by GitBox <gi...@apache.org>.
srijeet0406 commented on a change in pull request #6458:
URL: https://github.com/apache/trafficcontrol/pull/6458#discussion_r781606279



##########
File path: 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) {

Review comment:
       This happens when the user tries to Update the password after clicking on the `Reset Password` link in TP 




-- 
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: issues-unsubscribe@trafficcontrol.apache.org

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

[GitHub] [trafficcontrol] ocket8888 commented on a change in pull request #6458: User registration and password reset are broken due to the last_authenticated value being null

Posted by GitBox <gi...@apache.org>.
ocket8888 commented on a change in pull request #6458:
URL: https://github.com/apache/trafficcontrol/pull/6458#discussion_r781334023



##########
File path: 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;

Review comment:
       Is this migration necessary now that the `last_authenticated` column is being properly updated on token/oauth login?




-- 
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: issues-unsubscribe@trafficcontrol.apache.org

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

[GitHub] [trafficcontrol] srijeet0406 commented on a change in pull request #6458: User registration and password reset are broken due to the last_authenticated value being null

Posted by GitBox <gi...@apache.org>.
srijeet0406 commented on a change in pull request #6458:
URL: https://github.com/apache/trafficcontrol/pull/6458#discussion_r781660959



##########
File path: traffic_portal/app/src/common/api/UserService.js
##########
@@ -82,14 +82,14 @@ 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.updateCurrentUser = function(user) {
+        // We should be using PUT 'user/current' to update the current user
+        var currUser = {
+            user: user

Review comment:
       Yeah, I'm not used to the JS syntax at all. But I'll change 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.

To unsubscribe, e-mail: issues-unsubscribe@trafficcontrol.apache.org

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

[GitHub] [trafficcontrol] ocket8888 commented on a change in pull request #6458: User registration and password reset are broken due to the last_authenticated value being null

Posted by GitBox <gi...@apache.org>.
ocket8888 commented on a change in pull request #6458:
URL: https://github.com/apache/trafficcontrol/pull/6458#discussion_r781429401



##########
File path: 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;

Review comment:
       This is one thing that I think ought to be "nullable", because if a user has never logged in `null` is correct but the epoch value is a lie. Yes, it is impossible for a user to have actually logged in before ATC existed, but it's just nice to never lie, so that users/clients don't need to translate our responses themselves into what they actually probably mean - our system should be doing that for them, IMO.
   
   I won't hold up the PR for it, but if we're gonna make it non-nullable the Go structures should use `time.Time` instead of `*time.Time` to reflect that.




-- 
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: issues-unsubscribe@trafficcontrol.apache.org

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

[GitHub] [trafficcontrol] rawlinp commented on a change in pull request #6458: User registration and password reset are broken due to the last_authenticated value being null

Posted by GitBox <gi...@apache.org>.
rawlinp commented on a change in pull request #6458:
URL: https://github.com/apache/trafficcontrol/pull/6458#discussion_r781535127



##########
File path: 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;

Review comment:
       A null last_authenticated is still technically a lie for users that have authenticated before the feature was added, but I don't really care. We can't really answer the question of who has ever authenticated or not, only who has authenticated since this feature was deployed. If @ocket8888 prefers to keep the null values and delete these migrations, I think that's fine. I just generally like the idea of reducing the amount of nullable columns is all.




-- 
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: issues-unsubscribe@trafficcontrol.apache.org

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

[GitHub] [trafficcontrol] ocket8888 commented on a change in pull request #6458: User registration and password reset are broken due to the last_authenticated value being null

Posted by GitBox <gi...@apache.org>.
ocket8888 commented on a change in pull request #6458:
URL: https://github.com/apache/trafficcontrol/pull/6458#discussion_r781658369



##########
File path: traffic_portal/app/src/common/api/UserService.js
##########
@@ -82,14 +82,14 @@ 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.updateCurrentUser = function(user) {
+        // We should be using PUT 'user/current' to update the current user
+        var currUser = {

Review comment:
       `currUser` is never reassigned, could be `const`.

##########
File path: traffic_portal/app/src/common/api/UserService.js
##########
@@ -82,14 +82,14 @@ 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.updateCurrentUser = function(user) {
+        // We should be using PUT 'user/current' to update the current user
+        var currUser = {
+            user: user

Review comment:
       nit but `: user` is superfluous; `{ user }` is equivalent to `{ user: user }`. I find that to be cleaner, but if you're not used to it I bet you could be confused by that syntax. So change it or don't, just wanted to point it out.




-- 
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: issues-unsubscribe@trafficcontrol.apache.org

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

[GitHub] [trafficcontrol] ocket8888 commented on a change in pull request #6458: User registration and password reset are broken due to the last_authenticated value being null

Posted by GitBox <gi...@apache.org>.
ocket8888 commented on a change in pull request #6458:
URL: https://github.com/apache/trafficcontrol/pull/6458#discussion_r781544277



##########
File path: traffic_ops/traffic_ops_golang/login/login.go
##########
@@ -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)

Review comment:
       The "Last Authenticated" time of a user should only be updated if the user successfully authenticates, which isn't checked until line 388 currently.

##########
File path: traffic_ops/traffic_ops_golang/login/login.go
##########
@@ -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())

Review comment:
       same nitpick as above.

##########
File path: 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())

Review comment:
       nit but you don't need `\n` because logging handles line ending for you and you don't need `.Error()` because the string coercion performed by the logging library on `%s` arguments of `error` type call `.Error()` implicitly.

##########
File path: 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) {

Review comment:
       comparisons should use `===` - under what conditions was this occurring?




-- 
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: issues-unsubscribe@trafficcontrol.apache.org

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

[GitHub] [trafficcontrol] srijeet0406 commented on a change in pull request #6458: User registration and password reset are broken due to the last_authenticated value being null

Posted by GitBox <gi...@apache.org>.
srijeet0406 commented on a change in pull request #6458:
URL: https://github.com/apache/trafficcontrol/pull/6458#discussion_r781436453



##########
File path: 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;

Review comment:
       I had it in there just as an added layer of protection, as Rawlin mentioned. I do agree with Brennan that if it is changed to non- nullable, we should change the Go structures as well. I'll let you guys decide which way we want to go.




-- 
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: issues-unsubscribe@trafficcontrol.apache.org

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

[GitHub] [trafficcontrol] ocket8888 commented on a change in pull request #6458: User registration and password reset are broken due to the last_authenticated value being null

Posted by GitBox <gi...@apache.org>.
ocket8888 commented on a change in pull request #6458:
URL: https://github.com/apache/trafficcontrol/pull/6458#discussion_r781542071



##########
File path: 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;

Review comment:
       For the record, I also like that idea, in general, just not in this case. It does feel a little strange arguing _in favor_ of nullability.
   
   We could improve it a bit - without, still, being able to always tell the complete truth in all situations - by adding a migration that sets `last_authenticated = now() WHERE last_authenticated IS NULL` with no "down" statements and no `NOT NULL` constraint/nullability change. That way you know that all users haven't logged in since _at least_ their `lastAuthenticated` time, which is set to the date/time at which the feature was added (roughly - it's been at least one minor release since then in reality, but it's behind an unstable API atm). I'd find that more acceptable than setting it to Jan 1, 1970. That way there's no synonym for `null` being used and `null` is still allowed for new users moving forward.




-- 
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: issues-unsubscribe@trafficcontrol.apache.org

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

[GitHub] [trafficcontrol] ocket8888 merged pull request #6458: User registration and password reset are broken due to the last_authenticated value being null

Posted by GitBox <gi...@apache.org>.
ocket8888 merged pull request #6458:
URL: https://github.com/apache/trafficcontrol/pull/6458


   


-- 
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: issues-unsubscribe@trafficcontrol.apache.org

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

[GitHub] [trafficcontrol] ocket8888 commented on pull request #6458: User registration and password reset are broken due to the last_authenticated value being null

Posted by GitBox <gi...@apache.org>.
ocket8888 commented on pull request #6458:
URL: https://github.com/apache/trafficcontrol/pull/6458#issuecomment-1010465061


   Solving that problem is more complicated than I initially thought, because of LDAP and OAuth users. I'll open an issue for it probably tomorrow. This PR doesn't need to try to fix the problem.


-- 
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: issues-unsubscribe@trafficcontrol.apache.org

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

[GitHub] [trafficcontrol] ocket8888 commented on a change in pull request #6458: User registration and password reset are broken due to the last_authenticated value being null

Posted by GitBox <gi...@apache.org>.
ocket8888 commented on a change in pull request #6458:
URL: https://github.com/apache/trafficcontrol/pull/6458#discussion_r781547289



##########
File path: 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())

Review comment:
       Actually, disregard, because it looks like you're double-logging this, because `api.HandleErr` will log it too. It looks like what you actually want something like:
   ```go
   if dbErr != nil {
       dbErr = fmt.Errorf("unable to update authentication time for user '%s': %w", username, dbErr)
       api.HandleErr(w, r, nil, http.StatusInternalServerError, nil, dbErr)
       return
   }
   ```
   (I think the username is helpful information)




-- 
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: issues-unsubscribe@trafficcontrol.apache.org

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

[GitHub] [trafficcontrol] ocket8888 commented on a change in pull request #6458: User registration and password reset are broken due to the last_authenticated value being null

Posted by GitBox <gi...@apache.org>.
ocket8888 commented on a change in pull request #6458:
URL: https://github.com/apache/trafficcontrol/pull/6458#discussion_r781505979



##########
File path: 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);

Review comment:
       To keep things simple, I'd recommend just adding a new method e.g. `updateCurrentUser` and have the controller determine which method to call, the same way we have `getUsers` as well as `getCurrentUser`.




-- 
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: issues-unsubscribe@trafficcontrol.apache.org

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