ocket8888 commented on a change in pull request #6009:
URL: https://github.com/apache/trafficcontrol/pull/6009#discussion_r669778843
##########
File path: docs/source/api/v4/users.rst
##########
@@ -68,29 +68,30 @@ Request Structure
Response Structure
------------------
-:addressLine1: The user's address - including street name and number
-:addressLine2: An additional address field for e.g. apartment number
-:city: The name of the city wherein the user resides
-:company: The name of the company for which the user works
-:country: The name of the country wherein the user resides
-:email: The user's email address
-:fullName: The user's full name, e.g. "John Quincy Adams"
-:gid: A deprecated field only kept for legacy compatibility
reasons that used to contain the UNIX group ID of the user - now it is always
``null``
-:id: An integral, unique identifier for this user
-:lastUpdated: The date and time at which the user was last modified, in
:ref:`non-rfc-datetime`
-:newUser: A meta field with no apparent purpose that is usually
``null`` unless explicitly set during creation or modification of a user via
some API endpoint
-:phoneNumber: The user's phone number
-:postalCode: The postal code of the area in which the user resides
-:publicSshKey: The user's public key used for the SSH protocol
-:registrationSent: If the user was created using the
:ref:`to-api-users-register` endpoint, this will be the date and time at which
the registration email was sent - otherwise it will be ``null``
-:role: The integral, unique identifier of the highest-privilege
role assigned to this user
-:rolename: The name of the highest-privilege role assigned to this user
-:stateOrProvince: The name of the state or province where this user resides
-:tenant: The name of the tenant to which this user belongs
-:tenantId: The integral, unique identifier of the tenant to which this
user belongs
-:uid: A deprecated field only kept for legacy compatibility
reasons that used to contain the UNIX user ID of the user - now it is always
``null``
-:username: The user's username
-:changeLogCount: The number of change log entries created by the user
+:addressLine1: The user's address - including street name and number
+:addressLine2: An additional address field for e.g. apartment number
+:city: The name of the city wherein the user resides
+:company: The name of the company for which the user works
+:country: The name of the country wherein the user resides
+:email: The user's email address
+:fullName: The user's full name, e.g. "John Quincy Adams"
+:gid: A deprecated field only kept for legacy compatibility
reasons that used to contain the UNIX group ID of the user - now it is always
``null``
+:id: An integral, unique identifier for this user
+:lastUpdated: The date and time at which the user was last modified, in
:ref:`non-rfc-datetime`
+:newUser: A meta field with no apparent purpose that is usually
``null`` unless explicitly set during creation or modification of a user via
some API endpoint
+:phoneNumber: The user's phone number
+:postalCode: The postal code of the area in which the user resides
+:publicSshKey: The user's public key used for the SSH protocol
+:registrationSent: If the user was created using the
:ref:`to-api-users-register` endpoint, this will be the date and time at which
the registration email was sent - otherwise it will be ``null``
+:role: The integral, unique identifier of the highest-privilege
role assigned to this user
+:rolename: The name of the highest-privilege role assigned to this
user
+:stateOrProvince: The name of the state or province where this user resides
+:tenant: The name of the tenant to which this user belongs
+:tenantId: The integral, unique identifier of the tenant to which
this user belongs
+:uid: A deprecated field only kept for legacy compatibility
reasons that used to contain the UNIX user ID of the user - now it is always
``null``
+:username: The user's username
+:changeLogCount: The number of change log entries created by the user
+:lastAuthenticated: The date and time at which the user was last
authenticated, in :ref:`non-rfc-datetime`
Review comment:
Same as above RE: date format and alphabetize
##########
File path: traffic_ops/v4-client/user.go
##########
@@ -33,9 +33,9 @@ func (to *Session) GetUsers(opts RequestOptions)
(tc.UsersResponseV40, toclientl
}
// GetUserCurrent retrieves the currently authenticated User.
-func (to *Session) GetUserCurrent(opts RequestOptions)
(tc.UserCurrentResponse, toclientlib.ReqInf, error) {
+func (to *Session) GetUserCurrent(opts RequestOptions)
(tc.UserCurrentResponseV40, toclientlib.ReqInf, error) {
Review comment:
If we release APIv4.1 that adds some new field to the current user
response, we'll need to make a new method to avoid breaking the call signature
here. So instead what we should do is define a type alias for the latest minor
version and use that in the client:
```go
type UserCurrentResponseV4 = UserCurrentResponseV40
```
So then later the client can be upgraded for non-breaking changes by just
changing that type alias to:
```go
type UserCurrentResponseV4 = UserCurrentResponseV41
```
with no changes necessary in `traffic_ops/v4-client`.
##########
File path: docs/source/api/v4/users_id.rst
##########
@@ -48,28 +48,29 @@ Request Structure
Response Structure
------------------
-:addressLine1: The user's address - including street name and number
-:addressLine2: An additional address field for e.g. apartment number
-:city: The name of the city wherein the user resides
-:company: The name of the company for which the user works
-:country: The name of the country wherein the user resides
-:email: The user's email address
-:fullName: The user's full name, e.g. "John Quincy Adams"
-:gid: A deprecated field only kept for legacy compatibility
reasons that used to contain the UNIX group ID of the user - now it is always
``null``
-:id: An integral, unique identifier for this user
-:lastUpdated: The date and time at which the user was last modified, in
:ref:`non-rfc-datetime`
-:newUser: A meta field with no apparent purpose that is usually
``null`` unless explicitly set during creation or modification of a user via
some API endpoint
-:phoneNumber: The user's phone number
-:postalCode: The postal code of the area in which the user resides
-:publicSshKey: The user's public key used for the SSH protocol
-:registrationSent: If the user was created using the
:ref:`to-api-users-register` endpoint, this will be the date and time at which
the registration email was sent - otherwise it will be ``null``
-:role: The integral, unique identifier of the highest-privilege
role assigned to this user
-:rolename: The name of the highest-privilege role assigned to this user
-:stateOrProvince: The name of the state or province where this user resides
-:tenant: The name of the tenant to which this user belongs
-:tenantId: The integral, unique identifier of the tenant to which this
user belongs
-:uid: A deprecated field only kept for legacy compatibility
reasons that used to contain the UNIX user ID of the user - now it is always
``null``
-:username: The user's username
+:addressLine1: The user's address - including street name and number
+:addressLine2: An additional address field for e.g. apartment number
+:city: The name of the city wherein the user resides
+:company: The name of the company for which the user works
+:country: The name of the country wherein the user resides
+:email: The user's email address
+:fullName: The user's full name, e.g. "John Quincy Adams"
+:gid: A deprecated field only kept for legacy compatibility
reasons that used to contain the UNIX group ID of the user - now it is always
``null``
+:id: An integral, unique identifier for this user
+:lastUpdated: The date and time at which the user was last modified, in
:ref:`non-rfc-datetime`
+:newUser: A meta field with no apparent purpose that is usually
``null`` unless explicitly set during creation or modification of a user via
some API endpoint
+:phoneNumber: The user's phone number
+:postalCode: The postal code of the area in which the user resides
+:publicSshKey: The user's public key used for the SSH protocol
+:registrationSent: If the user was created using the
:ref:`to-api-users-register` endpoint, this will be the date and time at which
the registration email was sent - otherwise it will be ``null``
+:role: The integral, unique identifier of the highest-privilege
role assigned to this user
+:rolename: The name of the highest-privilege role assigned to this
user
+:stateOrProvince: The name of the state or province where this user resides
+:tenant: The name of the tenant to which this user belongs
+:tenantId: The integral, unique identifier of the tenant to which
this user belongs
+:uid: A deprecated field only kept for legacy compatibility
reasons that used to contain the UNIX user ID of the user - now it is always
``null``
+:username: The user's username
+:lastAuthenticated: The date and time at which the user was last
authenticated, in :ref:`non-rfc-datetime`
Review comment:
Same as above RE: date format and alphabetize
##########
File path: traffic_ops/traffic_ops_golang/user/current.go
##########
@@ -105,12 +105,31 @@ func Current(w http.ResponseWriter, r *http.Request) {
return
}
defer inf.Close()
- currentUser, err := getUser(inf.Tx.Tx, inf.User.ID)
+
+ var currentUser tc.UserCurrent
+ var currentUserV40 tc.UserCurrentV40
Review comment:
The advantage to the structure nesting we do with our library structures
is that you don't need to do this. A `tc.UserCurrentV40` contains a
`tc.UserCurrent`, so you can just do:
```go
currentUser := getUser()
if inf.Version >=4.0 {
api.WriteResp(w, r, currentUser)
} else {
api.WriteResp(w, r, currentUser.CurrentUser)
}
```
(pseudocode-ish) and that wy you only need to check the API version once,
when you write the response, and you don't need to make a new `getUserVx`
function for each new API version.
##########
File path: docs/source/api/v4/user_current.rst
##########
@@ -35,28 +35,29 @@ No parameters available.
Response Structure
------------------
-:addressLine1: The user's address - including street name and number
-:addressLine2: An additional address field for e.g. apartment number
-:city: The name of the city wherein the user resides
-:company: The name of the company for which the user works
-:country: The name of the country wherein the user resides
-:email: The user's email address
-:fullName: The user's full name, e.g. "John Quincy Adams"
-:gid: A deprecated field only kept for legacy compatibility
reasons that used to contain the UNIX group ID of the user
+:addressLine1: The user's address - including street name and number
+:addressLine2: An additional address field for e.g. apartment number
+:city: The name of the city wherein the user resides
+:company: The name of the company for which the user works
+:country: The name of the country wherein the user resides
+:email: The user's email address
+:fullName: The user's full name, e.g. "John Quincy Adams"
+:gid: A deprecated field only kept for legacy compatibility
reasons that used to contain the UNIX group ID of the user
:id: An integral, unique identifier for this user
Review comment:
This field isn't aligned anymore
##########
File path: traffic_ops/testing/api/v4/user_test.go
##########
@@ -454,6 +454,9 @@ func GetTestUsers(t *testing.T) {
if err != nil {
t.Errorf("cannot get users: %v - alerts: %+v", err, resp.Alerts)
}
+ if resp.Response[0].LastAuthenticated == nil {
Review comment:
This line will segfault if Traffic Ops returns an empty list
##########
File path: docs/source/api/v4/user_current.rst
##########
@@ -35,28 +35,29 @@ No parameters available.
Response Structure
------------------
-:addressLine1: The user's address - including street name and number
-:addressLine2: An additional address field for e.g. apartment number
-:city: The name of the city wherein the user resides
-:company: The name of the company for which the user works
-:country: The name of the country wherein the user resides
-:email: The user's email address
-:fullName: The user's full name, e.g. "John Quincy Adams"
-:gid: A deprecated field only kept for legacy compatibility
reasons that used to contain the UNIX group ID of the user
+:addressLine1: The user's address - including street name and number
+:addressLine2: An additional address field for e.g. apartment number
+:city: The name of the city wherein the user resides
+:company: The name of the company for which the user works
+:country: The name of the country wherein the user resides
+:email: The user's email address
+:fullName: The user's full name, e.g. "John Quincy Adams"
+:gid: A deprecated field only kept for legacy compatibility
reasons that used to contain the UNIX group ID of the user
:id: An integral, unique identifier for this user
-:lastUpdated: The date and time at which the user was last modified, in
:ref:`non-rfc-datetime`
-:newUser: A meta field with no apparent purpose that is usually
``null`` unless explicitly set during creation or modification of a user via
some API endpoint
-:phoneNumber: The user's phone number
-:postalCode: The postal code of the area in which the user resides
-:publicSshKey: The user's public key used for the SSH protocol
-:registrationSent: If the user was created using the
:ref:`to-api-users-register` endpoint, this will be the date and time at which
the registration email was sent - otherwise it will be ``null``
-:role: The integral, unique identifier of the highest-privilege
:term:`Role` assigned to this user
-:rolename: The name of the highest-privilege :term:`Role` assigned to
this user
-:stateOrProvince: The name of the state or province where this user resides
-:tenant: The name of the :term:`Tenant` to which this user belongs
-:tenantId: The integral, unique identifier of the :term:`Tenant` to
which this user belongs
-:uid: A deprecated field only kept for legacy compatibility
reasons that used to contain the UNIX user ID of the user
-:username: The user's username
+:lastUpdated: The date and time at which the user was last modified, in
:ref:`non-rfc-datetime`
+:newUser: A meta field with no apparent purpose that is usually
``null`` unless explicitly set during creation or modification of a user via
some API endpoint
+:phoneNumber: The user's phone number
+:postalCode: The postal code of the area in which the user resides
+:publicSshKey: The user's public key used for the SSH protocol
+:registrationSent: If the user was created using the
:ref:`to-api-users-register` endpoint, this will be the date and time at which
the registration email was sent - otherwise it will be ``null``
+:role: The integral, unique identifier of the highest-privilege
:term:`Role` assigned to this user
+:rolename: The name of the highest-privilege :term:`Role` assigned to
this user
+:stateOrProvince: The name of the state or province where this user resides
+:tenant: The name of the :term:`Tenant` to which this user belongs
+:tenantId: The integral, unique identifier of the :term:`Tenant` to
which this user belongs
+:uid: A deprecated field only kept for legacy compatibility
reasons that used to contain the UNIX user ID of the user
+:username: The user's username
+:lastAuthenticated: The date and time at which the user was last
authenticated, in :ref:`non-rfc-datetime`
Review comment:
This isn't non-RFC formatted, it's RFC3339 now. Also, this should go
right above `lastUpdated` to keep the list alphabetically sorted.
##########
File path: traffic_ops/traffic_ops_golang/login/login.go
##########
@@ -149,9 +150,25 @@ func LoginHandler(db *sqlx.DB, cfg config.Config)
http.HandlerFunc {
if authenticated {
httpCookie := tocookie.GetCookie(form.Username,
defaultCookieDuration, cfg.Secrets[0])
http.SetCookie(w, httpCookie)
- resp = struct {
- tc.Alerts
- }{tc.CreateAlerts(tc.SuccessLevel,
"Successfully logged in.")}
+
+ //If all's well until here, then update last
authenticated time
+ tx, txErr := db.Begin()
+ if txErr != nil {
+ api.HandleErr(w, r, tx,
http.StatusInternalServerError, nil, fmt.Errorf("beginning transaction: %v",
txErr))
+ return
+ }
+ defer tx.Commit()
+ _, dbErr := tx.Exec(UpdateLoginTimeQuery,
form.Username)
+ if dbErr != nil {
+ resp = struct {
+ tc.Alerts
+ }{tc.CreateAlerts(tc.ErrorLevel,
"Unable to update authentication time for a given user")}
Review comment:
We should be logging this error somewhere.
##########
File path: traffic_ops/traffic_ops_golang/login/login.go
##########
@@ -149,9 +150,25 @@ func LoginHandler(db *sqlx.DB, cfg config.Config)
http.HandlerFunc {
if authenticated {
httpCookie := tocookie.GetCookie(form.Username,
defaultCookieDuration, cfg.Secrets[0])
http.SetCookie(w, httpCookie)
- resp = struct {
- tc.Alerts
- }{tc.CreateAlerts(tc.SuccessLevel,
"Successfully logged in.")}
+
+ //If all's well until here, then update last
authenticated time
+ tx, txErr := db.Begin()
+ if txErr != nil {
+ api.HandleErr(w, r, tx,
http.StatusInternalServerError, nil, fmt.Errorf("beginning transaction: %v",
txErr))
Review comment:
`fmt.Errorf` should use wrapping format parameter for errors: `%w`
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
