Initial implementation of Pbench user model and associated APIs in server #1937

npalaska · 2020-11-02T02:22:14Z

Initial framework to build Pbench user authentication:

This implements 6 basic user APIs

Register User:

Handles Pbench User registration via JSON request

POST /v1/register

json={
    "username": "username",
    "password": "password",
    "firstName": first_name,
    "lastName": "last_name",
    "email": "[email protected]"
}

Login User
- Handles user login and returns a valid pbench auth token
- POST /v1/login
```
json={
    "username": "username",
    "password": "password"
}
```
- Returns a valid pbench auth token for accessing other APIs.
- User is allowed to create multiple login requests and thus generating multiple auth tokens,
  however, in the future there will be limit on the number of auth token a user can generate.
- We plan to maintain an active_tokens table in our db.
- Each auth token has its own expiry and is associated with a user id.
- Any subsequent API request will only pass if the auth token in the header is not expired and is present in active_tokens table.
Logout User
- Handles the user logout mechanism.
- POST /v1/logout
```
headers={ 
Authorization:   Bearer <Pbench authentication token (user received upon login)>
}
```
- Deletes the authentication token provided in the headers from the active_tokens table.
- Once logged out user, can not use the same auth token for other API access.
Get User
- Returns the user's self information that was registered, the username must be provided in the url
- GET /v1/user/<string:username>
```
headers={ 
Authorization:   Bearer <Pbench authentication token (user received upon login)>
}
```
- If the Authorization header does not belong to the username provided in the url, we reject the request unless the Authorization token belongs to the admin user.
Delete User
- An API for a user to delete an account from the pbench database.
- DELETE /v1/user/<string:username>" headers={
  Authorization: Bearer <Pbench authentication token (user received upon login)>
  }
- A user can only perform the DELETE action on another account if the presented auth token belongs to an admin user.
- If the Authorization header does not belong to the username provided in the url, we reject the request unless the Authorization token belongs to the admin user.
Update User
- An API for updating the User registration fields, the username must be provided in the url
- PUT /v1/user/<string:username>
```
Example Json:
{
    "first_name": "new_name",
    "password": "password",
    ...
}
headers={ 
Authorization:   Bearer <Pbench authentication token (user received upon login)>
}
```
- If the Authorization header does not belong to the username provided in the url, we reject the request unless the Authorization token belongs to the admin user.
- Update is allowed on all the user model fields except registered_on

lgtm-com · 2020-11-02T02:29:49Z

This pull request introduces 1 alert when merging bfeccfd into 82c193a - view on LGTM.com

new alerts:

1 for Unused import

dbutenhof

A comment left over from the other day that I forgot to actually submit...

lib/pbench/server/api/__init__.py

dbutenhof

I'm a little curious why you haven't hooked up the endpoint to your new class yet, but I'm not objecting to "slow and steady" either.

lib/pbench/server/api/resources/db_models.py

lib/pbench/server/api/resources/pbench_users.py

lgtm-com · 2020-11-05T05:10:28Z

This pull request introduces 1 alert when merging a9be97c into a86cbb6 - view on LGTM.com

new alerts:

1 for Unused import

dbutenhof

Great progress, Nikhil!

A bunch of architecture/structure comments, most of which shouldn't radically change what you have.

lib/pbench/server/api/__init__.py

lib/pbench/server/api/resources/db_models.py

lib/pbench/server/api/resources/pbench_users.py

lib/pbench/server/api/__init__.py

portante

First pass set of review comments ...

lib/pbench/cli/server/shell.py

lib/pbench/server/api/resources/pbench_users.py

lgtm-com · 2020-11-09T20:00:58Z

This pull request introduces 1 alert when merging db53a4a into 659d90d - view on LGTM.com

new alerts:

1 for Unused local variable

lib/pbench/cli/server/shell.py

lib/pbench/server/api/__init__.py

lgtm-com · 2020-11-10T04:45:35Z

This pull request introduces 1 alert when merging 4fabf2c into 659d90d - view on LGTM.com

new alerts:

1 for Unused local variable

lgtm-com · 2020-11-12T07:06:27Z

This pull request introduces 1 alert when merging dbdd077 into ad3e4d3 - view on LGTM.com

new alerts:

1 for Unused import

lgtm-com · 2020-11-13T03:40:00Z

This pull request introduces 2 alerts when merging 3b2fd36 into ad3e4d3 - view on LGTM.com

new alerts:

2 for Unused import

lgtm-com · 2020-11-13T03:55:34Z

This pull request introduces 2 alerts when merging cd922c3 into ad3e4d3 - view on LGTM.com

new alerts:

2 for Unused import

dbutenhof

Let's put this one in the can; we've got important things to put on top of it. There are a few of the issues brought up during discussions that deserve consideration in a follow-up.

dbutenhof · 2021-03-05T15:01:03Z

lib/pbench/server/api/resources/users_api.py

+        # check if the user is already logged in, in case the request has a an authorization header included
+        # We would still proceed to login and return a new auth token to the user
+        if user == self.auth.token_auth.current_user():
+            self.logger.warning("User already logged in and trying to re-login")


We've had some discussions about how to handle this, and I think some changes are warranted but not in this PR ... when you do that, if this comment survives, I just spotted a typo: "has a an authorization header". ;-)

dbutenhof · 2021-03-05T15:04:49Z

lib/pbench/server/database/database.py

+        # Make sure all the models are imported before this function gets called
+        # so that they will be registered properly on the metadata. Otherwise
+        # metadata will not have any tables and create_all functionality will do nothing
+
+        Database.Base.query = Database.db_session.query_property()
+
+        engine = create_engine(Database.get_engine_uri(server_config, logger))
+        Database.Base.metadata.create_all(bind=engine)
+        Database.db_session.configure(bind=engine)


Another area that could use some cleanup in a follow-on PR. (I assume that you'll move on to finish your metadata PR #2049, so that's a possibility if not handled separately.)

Operationally; if we can do anything here to ensure/detect that everything's good, that would be excellent. If not, let's at least clean up the comment to describe the assumptions and current implementation, providing a simple and actionable warning about the impact of changes on those assumptions ... rather than a vague and disturbing warning with no actionable mitigation.

dbutenhof · 2021-03-05T15:07:15Z

Oh; and (sigh) you now need a rebase before we can merge.

This implements 5 basic user APIs 1. Register User Handles Pbench User registration via JSON request POST /v1/register 2. Login User: POST /v1/login Returns a valid pbench auth token User is allowed to issue multiple login requests and thus generating multiple auth tokens, Each token is stored in a active_tokens table and has its own expiry User is authenticated for subsequest API calls if token not expired and present in the active_tokens table 3. Logout user: POST /v1/logout Deletes the auth_token from the active_tokens table. Once logged out user can not use the same auth token for other API access. 4. Get User: GET /v1/user/<string:username> Returns the user's self information that was registered, the username must be provided in the url If the Auth header does not belong to the username, reject the request unless auth token belongs to the admin 5. Delete User: DELETE /v1/user/<string:username>" An API for a user to delete himself from the pbench database. A user can only perform delete action on himself unless the auth token belongs to the admin user. 6. Updare User: PUT /v1/user/<string:username> An API for updating the User registration fields, the username must be provided in the url Update is not allowed on registerd_on field

Outdated

webbnh

I don't know how to figure out what has changed in the last three hours, and I don't have it in me to review this whole thing from scratch, so I'm going to just assume that, at this point, it's all good....

npalaska · 2021-03-05T17:28:32Z

Nothing changed in last 3 hours, it just got rebase :)

webbnh · 2021-03-05T17:31:32Z

Then I'm sure that it's all good. ;-)

dbutenhof · 2021-03-05T17:34:57Z

Nothing changed in last 3 hours, it just got rebase :)

Yeah, we really need to consider doing merge commits in the future. Or just rely on GitHub to automatically squash and merge so at least we can see the real changes up to the last moment.

webbnh · 2021-03-05T17:44:59Z

Having the review feedback incorporated as separate commits which then get squashed (somehow) would be a huge step forward (at least as long as we continue to rely on GitHub for doing reviews). Having GitHub do the squash as part of the merge is perfectly acceptable in many, many cases. Having it do a rebase as part of the merge is fine, too, so long as we want to retain the individual the commits in the PR, but we probably won't in most cases. (Sadly, there is no "rebase AND squash" option.) And, having it create merge commits is a very good idea, but only in a fairly small set of cases, so I'd rather focus on easing review and then squashing.

This includes more change than it should, because I'd originally segregated the database files into a `db` directory while Nikhil went with `database`.

lib/pbench/server/api/resources/users_api.py

…system-analysis#1937) This implements 5 basic user APIs 1. Register User Handles Pbench User registration via JSON request POST /v1/register 2. Login User: POST /v1/login Returns a valid pbench auth token User is allowed to issue multiple login requests and thus generating multiple auth tokens, Each token is stored in a active_tokens table and has its own expiry User is authenticated for subsequest API calls if token not expired and present in the active_tokens table 3. Logout user: POST /v1/logout Deletes the auth_token from the active_tokens table. Once logged out user can not use the same auth token for other API access. 4. Get User: GET /v1/user/<string:username> Returns the user's self information that was registered, the username must be provided in the url If the Auth header does not belong to the username, reject the request unless auth token belongs to the admin 5. Delete User: DELETE /v1/user/<string:username>" An API for a user to delete himself from the pbench database. A user can only perform delete action on himself unless the auth token belongs to the admin user. 6. Updare User: PUT /v1/user/<string:username> An API for updating the User registration fields, the username must be provided in the url Update is not allowed on registerd_on field

portante self-assigned this Nov 3, 2020

portante added Contrib enhancement Dashboard Of and relating to the Dashboard GUI Server and removed Contrib labels Nov 3, 2020

portante added this to the v0.71 milestone Nov 3, 2020

portante assigned npalaska Nov 3, 2020

dbutenhof reviewed Nov 4, 2020

View reviewed changes

lib/pbench/server/api/__init__.py Outdated Show resolved Hide resolved

dbutenhof requested changes Nov 4, 2020

View reviewed changes

dbutenhof requested changes Nov 6, 2020

View reviewed changes

npalaska requested review from portante, riya-17, ndokos, dbutenhof and zulcss November 9, 2020 14:07

portante requested changes Nov 9, 2020

View reviewed changes

npalaska force-pushed the pbench_user branch from b7982be to db53a4a Compare November 9, 2020 19:51

dbutenhof reviewed Nov 9, 2020

View reviewed changes

lib/pbench/cli/server/shell.py Outdated Show resolved Hide resolved

lib/pbench/server/api/__init__.py Outdated Show resolved Hide resolved

lib/pbench/server/api/__init__.py Outdated Show resolved Hide resolved

webbnh previously approved these changes Mar 5, 2021

View reviewed changes

dbutenhof previously approved these changes Mar 5, 2021

View reviewed changes

npalaska dismissed stale reviews from dbutenhof and webbnh via 60aa002 March 5, 2021 15:51

npalaska force-pushed the pbench_user branch from f155228 to 60aa002 Compare March 5, 2021 15:51

dbutenhof requested review from webbnh and dbutenhof March 5, 2021 15:56

dbutenhof approved these changes Mar 5, 2021

View reviewed changes

webbnh approved these changes Mar 5, 2021

View reviewed changes

dbutenhof merged commit 8caf11d into distributed-system-analysis:main Mar 5, 2021

npalaska mentioned this pull request Mar 19, 2021

Fix minor bug in the login workflow #2144

Merged

webbnh reviewed Mar 24, 2021

View reviewed changes

lib/pbench/server/api/resources/users_api.py Show resolved Hide resolved

webbnh mentioned this pull request Mar 24, 2021

It looks like the server is returning different responses for bad username and bad password #2148

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Initial implementation of Pbench user model and associated APIs in server #1937

Initial implementation of Pbench user model and associated APIs in server #1937

npalaska commented Nov 2, 2020 •

edited

Loading

lgtm-com bot commented Nov 2, 2020

dbutenhof left a comment

dbutenhof left a comment

lgtm-com bot commented Nov 5, 2020

dbutenhof left a comment

portante left a comment

lgtm-com bot commented Nov 9, 2020

lgtm-com bot commented Nov 10, 2020

lgtm-com bot commented Nov 12, 2020

lgtm-com bot commented Nov 13, 2020

lgtm-com bot commented Nov 13, 2020

dbutenhof left a comment

dbutenhof Mar 5, 2021

dbutenhof Mar 5, 2021

dbutenhof commented Mar 5, 2021

webbnh left a comment

npalaska commented Mar 5, 2021 •

edited

Loading

webbnh commented Mar 5, 2021

dbutenhof commented Mar 5, 2021

webbnh commented Mar 5, 2021

Initial implementation of Pbench user model and associated APIs in server #1937

Initial implementation of Pbench user model and associated APIs in server #1937

Conversation

npalaska commented Nov 2, 2020 • edited Loading

lgtm-com bot commented Nov 2, 2020

dbutenhof left a comment

Choose a reason for hiding this comment

dbutenhof left a comment

Choose a reason for hiding this comment

lgtm-com bot commented Nov 5, 2020

dbutenhof left a comment

Choose a reason for hiding this comment

portante left a comment

Choose a reason for hiding this comment

lgtm-com bot commented Nov 9, 2020

lgtm-com bot commented Nov 10, 2020

lgtm-com bot commented Nov 12, 2020

lgtm-com bot commented Nov 13, 2020

lgtm-com bot commented Nov 13, 2020

dbutenhof left a comment

Choose a reason for hiding this comment

dbutenhof Mar 5, 2021

Choose a reason for hiding this comment

dbutenhof Mar 5, 2021

Choose a reason for hiding this comment

dbutenhof commented Mar 5, 2021

webbnh left a comment

Choose a reason for hiding this comment

npalaska commented Mar 5, 2021 • edited Loading

webbnh commented Mar 5, 2021

dbutenhof commented Mar 5, 2021

webbnh commented Mar 5, 2021

npalaska commented Nov 2, 2020 •

edited

Loading

npalaska commented Mar 5, 2021 •

edited

Loading