Initial implementation of Pbench user model and associated APIs in server by npalaska · Pull Request #1937 · distributed-system-analysis/pbench

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": "user@domain.com"
}

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...

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.

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.

portante

First pass set of review comments ...

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

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

This includes some comments that were pending from a day or two ago that I apparently forgot to commit; maybe they're still meaningful!

lgtm-com · 2020-11-17T16:15:22Z

This pull request introduces 2 alerts when merging a661931 into 8bb4bef - view on LGTM.com

new alerts:

2 for Unused import

lgtm-com · 2020-11-18T13:24:03Z

This pull request introduces 2 alerts when merging 49fdd87 into 8bb4bef - view on LGTM.com

new alerts:

2 for Unused import

webbnh

Hi Nikhil, sorry for the volume, below -- there's a lot of brand-new code in this PR, and I found a lot to comment on this time around.

Some of it is nits and small code-quality stuff. A lot of it is "content" stuff (like what we should have in diagnostics and similar). But, I think I did find a few bugs:

Something is odd with Auth.verify_auth() -- as coded, I think it has no effect, and so I think you're getting a silent failure that you're not noticing.
I don't think the code is handling an email address collision correctly in user creation, resulting in another silent failure.
I don't think the code correctly handles an unspecified authorization validity duration.
I'm concerned that we're not managing the User list of auth tokens correctly: I'm not sure how it gets initialized; I don't see where we ever delete from it; and, I don't think we're protecting it from being updated through the RESTful API. On the other hand, I don't think I saw anywhere where we actually use it...so maybe it can just be removed?

I have one other issue which I would like to bring up: what should we be doing if the authorization token validity duration is unspecified? I'm not convinced that defaulting to "forever" is the best choice (and I think that doing so poses a challenge for the code). I think it would be better if we picked a finite interval for the default.

dbutenhof

Rebase needed ...

ndokos

Lots of nits and some questions in-line.

aquibbaig · 2021-03-04T10:39:08Z

Hey @npalaska, just a general question? In the current implementation, when some error occurs we abort using the flask's abort method with a message. However in the dashboard, I do not see the custom message, it just prints an exception to the console. Any reason why it might be such?

aquibbaig · 2021-03-04T11:31:14Z

Hey @npalaska, just a general question? In the current implementation, when some error occurs we abort using the flask's abort method with a message. However in the dashboard, I do not see the custom message, it just prints an exception to the console. Any reason why it might be such?

EDIT: umi-request uses certain properties with its errorhandlers, an errorhandler returns a ResponseError type, as seen in <umi-request source code>/types/index.d.ts:

export interface ResponseError<D = any> extends Error {
  name: string;
  data: D;
  response: Response;
  request: {
    url: string;
    options: RequestOptionsInit;
  };
  type: string;
}

Fix: We were using the response parameter of umi-request which did not contain the embedded message, however data parameter certainly does.

webbnh

Just small stuff left (I'll change to "Approved" once I've reviewed my previous concerns).

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

+        # 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

+        # 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

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.

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

Comment thread lib/pbench/server/api/__init__.py Outdated

dbutenhof requested changes Nov 4, 2020

View reviewed changes

dbutenhof requested changes Nov 6, 2020

View reviewed changes

npalaska requested review from dbutenhof, ndokos, portante, riya-17 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

Comment thread lib/pbench/cli/server/shell.py Outdated

Comment thread lib/pbench/server/api/__init__.py Outdated

Comment thread lib/pbench/server/api/__init__.py Outdated

dbutenhof reviewed Nov 13, 2020

View reviewed changes

npalaska force-pushed the pbench_user branch from cd922c3 to a661931 Compare November 17, 2020 16:05

npalaska force-pushed the pbench_user branch from 1e1f423 to c413ea4 Compare March 2, 2021 18:49

dbutenhof reviewed Mar 2, 2021

View reviewed changes

Comment thread lib/pbench/server/api/resources/users_api.py Outdated

Comment thread lib/pbench/server/api/resources/users_api.py Outdated

Comment thread lib/pbench/server/database/models/users.py

dbutenhof reviewed Mar 2, 2021

View reviewed changes

Comment thread lib/pbench/server/database/models/users.py Outdated

dbutenhof approved these changes Mar 3, 2021

View reviewed changes

webbnh requested changes Mar 3, 2021

View reviewed changes

dbutenhof requested changes Mar 3, 2021

View reviewed changes

ndokos previously requested changes Mar 4, 2021

View reviewed changes

npalaska force-pushed the pbench_user branch from 2cd563c to e712880 Compare March 4, 2021 06:08

npalaska mentioned this pull request Mar 4, 2021

We need to restrict the external user from updating auth_tokens #2127

Closed

npalaska force-pushed the pbench_user branch from e712880 to 385eb1c Compare March 4, 2021 06:37

dbutenhof previously approved these changes Mar 4, 2021

View reviewed changes

Comment thread lib/pbench/server/api/resources/users_api.py Outdated

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

webbnh reviewed Mar 4, 2021

View reviewed changes

webbnh previously approved these changes Mar 4, 2021

View reviewed changes

dbutenhof requested a review from ndokos March 4, 2021 16:34

npalaska mentioned this pull request Mar 4, 2021

On logout we should return the success irrespective of internal failures #2130

Closed

webbnh previously approved these changes Mar 5, 2021

View reviewed changes

dbutenhof previously approved these changes Mar 5, 2021

View reviewed changes

dbutenhof approved these changes Mar 5, 2021

View reviewed changes

webbnh approved these changes Mar 5, 2021

View reviewed changes

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

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

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

Conversation

npalaska commented Nov 2, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

lgtm-com Bot commented Nov 2, 2020

Uh oh!

dbutenhof left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

dbutenhof left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

lgtm-com Bot commented Nov 5, 2020

Uh oh!

dbutenhof left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

portante left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

lgtm-com Bot commented Nov 9, 2020

Uh oh!

Uh oh!

Uh oh!

Uh oh!

lgtm-com Bot commented Nov 10, 2020

Uh oh!

lgtm-com Bot commented Nov 12, 2020

Uh oh!

lgtm-com Bot commented Nov 13, 2020

Uh oh!

lgtm-com Bot commented Nov 13, 2020

Uh oh!

dbutenhof left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

lgtm-com Bot commented Nov 17, 2020

Uh oh!

lgtm-com Bot commented Nov 18, 2020

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

webbnh left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

npalaska commented Nov 2, 2020 •

edited

Loading

aquibbaig commented Mar 4, 2021 •

edited

Loading

aquibbaig commented Mar 4, 2021 •

edited

Loading