Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Authentication and Authorisation #227

Merged
merged 1 commit into from
Nov 10, 2021
Merged

Conversation

rsnyman
Copy link
Contributor

@rsnyman rsnyman commented Oct 14, 2021

Add authentication and authorisation to Ibutsu

  • Add a login page
  • Add a login controller
  • Use JWT authentication
  • Make HttpClient service object that transparently auths requests
  • Add OAuth2 login to Ibutsu
  • Add tests for the login controller
  • Add Keycloak integration
  • Add authorisation, users can only see resources for projects they are part of
  • Use the 'from_file' method if it exists
  • Add support for running the dev servers under TLS
  • Add signup, reset password pages
  • Add creating and deleting tokens
  • Update the db updates

@john-dupuy
Copy link
Contributor

When trying to sign up for a new account and clicking the "Register" button, I get:
Screenshot from 2021-10-15 07-33-35

In the backend logs I see:

  File "/home/jdupuy/iqe/iqe-repos/ibutsu/ibutsu-server/backend/.ibutsu-env/lib/python3.9/site-packages/connexion/decorators/uri_parsing.py", line 149, in wrapper
    response = function(request)
  File "/home/jdupuy/iqe/iqe-repos/ibutsu/ibutsu-server/backend/.ibutsu-env/lib/python3.9/site-packages/connexion/decorators/validation.py", line 193, in wrapper
    response = function(request)
  File "/home/jdupuy/iqe/iqe-repos/ibutsu/ibutsu-server/backend/.ibutsu-env/lib/python3.9/site-packages/connexion/decorators/parameter.py", line 115, in wrapper
    return function(**kwargs)
  File "/home/jdupuy/iqe/iqe-repos/ibutsu/ibutsu-server/backend/ibutsu_server/controllers/login_controller.py", line 162, in register
    activation_code = urlsafe_b64encode(bytes(uuid4())).strip(b"=")
TypeError: cannot convert 'UUID' object to bytes

@john-dupuy
Copy link
Contributor

After logging in via Gitlab and trying to view the Run/Results pages, the request fails and I see the following in the backend logs:

  File "/home/jdupuy/iqe/iqe-repos/ibutsu/ibutsu-server/backend/.ibutsu-env/lib/python3.9/site-packages/connexion/decorators/parameter.py", line 115, in wrapper
    return function(**kwargs)
  File "/home/jdupuy/iqe/iqe-repos/ibutsu/ibutsu-server/backend/ibutsu_server/util/query.py", line 41, in query
    return function(**kwargs)
  File "/home/jdupuy/iqe/iqe-repos/ibutsu/ibutsu-server/backend/ibutsu_server/controllers/run_controller.py", line 67, in get_run_list
    query = add_user_filter(query, user, Run.project)
  File "/home/jdupuy/iqe/iqe-repos/ibutsu/ibutsu-server/backend/ibutsu_server/util/projects.py", line 40, in add_user_filter
    query = query.filter(or_(project in user.projects, project.owner == user))
  File "/home/jdupuy/iqe/iqe-repos/ibutsu/ibutsu-server/backend/.ibutsu-env/lib/python3.9/site-packages/sqlalchemy/orm/attributes.py", line 238, in __getattr__
    util.raise_(
  File "/home/jdupuy/iqe/iqe-repos/ibutsu/ibutsu-server/backend/.ibutsu-env/lib/python3.9/site-packages/sqlalchemy/util/compat.py", line 182, in raise_
    raise exception
AttributeError: Neither 'InstrumentedAttribute' object nor 'Comparator' object associated with Run.project has an attribute 'owner'

@rsnyman
Copy link
Contributor Author

rsnyman commented Oct 19, 2021

After logging in via Gitlab and trying to view the Run/Results pages, the request fails and I see the following in the backend logs:

I know this error. Let me see if I can figure out the problem.

@rsnyman
Copy link
Contributor Author

rsnyman commented Oct 19, 2021

After logging in via Gitlab and trying to view the Run/Results pages, the request fails and I see the following in the backend logs:

K, I think I fixed this.

@john-dupuy
Copy link
Contributor

john-dupuy commented Oct 21, 2021

OK taking another look at this.

  1. When I enter an incorrect email/password combo the page crashes with:
Error: Objects are not valid as a React child (found: TypeError: Cannot read properties of null (reading 'message')). If you meant to render a collection of children, use an array instead.
  1. Now logging in via gitlab, I see the following chain of requests:
127.0.0.1 - - [21/Oct/2021 10:23:40] "GET /api/login/auth/gitlab?code=f748560cb6e996493b41a2101bc7ac7bcda9d9abb075f72667351e74e1cf2841 HTTP/1.1" 200 -
127.0.0.1 - - [21/Oct/2021 10:23:41] "GET /api/project HTTP/1.1" 401 -
127.0.0.1 - - [21/Oct/2021 10:23:41] "GET /api/widget-config?filter=type%3Dview&filter=navigable%3Dtrue HTTP/1.1" 401 -
127.0.0.1 - - [21/Oct/2021 10:23:41] "GET /api/widget/types?type=widget HTTP/1.1" 401 -

so the login works (200 at /login/auth/gitlab) but then the subsequent requests all 401. Perhaps the token isn't being created? I see the isLoggedIn method checks for a token, and there are no tokens in the DB.

  1. Trying to register a new user still results in that error
    activation_code = urlsafe_b64encode(bytes(uuid4())).strip(b"=")
TypeError: cannot convert 'UUID' object to bytes

@rsnyman
Copy link
Contributor Author

rsnyman commented Oct 27, 2021

@john-dupuy I've updated the code, and fixed the issues.

@john-dupuy
Copy link
Contributor

@rsnyman

From the previous comment

  1. Fixed
  2. Fixed
  3. I'm still seeing this issue

Also I'm still seeing the issue mentioned in a previous comment:

After logging in via Gitlab and trying to view the Run/Results pages, the request fails with error

  File "/home/jdupuy/iqe/iqe-repos/ibutsu/ibutsu-server/backend/.ibutsu-env/lib/python3.9/site-packages/sqlalchemy/orm/attributes.py", line 238, in __getattr__
    util.raise_(
  File "/home/jdupuy/iqe/iqe-repos/ibutsu/ibutsu-server/backend/.ibutsu-env/lib/python3.9/site-packages/sqlalchemy/util/compat.py", line 182, in raise_
    raise exception
AttributeError: Neither 'InstrumentedAttribute' object nor 'Comparator' object associated with Run.project has an attribute 'owner'

I think this is coming from the add_user_filter method in util.projects. Here the 'Result/Run.project' is passed in as project.

@john-dupuy
Copy link
Contributor

john-dupuy commented Oct 28, 2021

In creating a project, I came across a potential authorization issue:

  • Once a user logs in they have access to the projects API, and can make themselves the owner of a project after getting their UUID via the users API.

Also, after creating a project and selecting it via the dropdown, I got the following FE exception:

Unhandled Rejection (TypeError): Cannot read properties of undefined (reading 'dashboards')
(anonymous function)
src/dashboard.js:90
  87 | .then(response => HttpClient.handleResponse(response))
  88 | .then(data => {
  89 |   this.setState(
> 90 |     {dashboards: data['dashboards']},
     | ^  91 |     () => {
  92 |       // If the current dashboard is not in current list
  93 |       if (this.state.selectedDashboard) {

@rsnyman
Copy link
Contributor Author

rsnyman commented Oct 29, 2021

  1. I'm still seeing this issue

I changed that line of code completely, are you seeing the same line still? I cannot reproduce it.

AttributeError: Neither 'InstrumentedAttribute' object nor 'Comparator' object associated with Run.project has an attribute 'owner'

I cannot reproduce this error either.

@rsnyman
Copy link
Contributor Author

rsnyman commented Oct 29, 2021

Also, after creating a project and selecting it via the dropdown, I got the following FE exception:

Unhandled Rejection (TypeError): Cannot read properties of undefined (reading 'dashboards')
(anonymous function)
src/dashboard.js:90
  87 | .then(response => HttpClient.handleResponse(response))
  88 | .then(data => {
  89 |   this.setState(
> 90 |     {dashboards: data['dashboards']},
     | ^  91 |     () => {
  92 |       // If the current dashboard is not in current list
  93 |       if (this.state.selectedDashboard) {

This might be related to the 401 issues you were having earlier. I'm not seeing this.

@rsnyman rsnyman force-pushed the registration-page branch 4 times, most recently from 16671f3 to 242bfaf Compare November 4, 2021 21:28
@rsnyman
Copy link
Contributor Author

rsnyman commented Nov 5, 2021

OK, I've fixed the tests, fixed the linting, and I think I've addressed all your issues (including the potential elevated privilege bug).

@john-dupuy
Copy link
Contributor

Testing in a clean database, I'm not seeing any of the previous issues I mentioned. I noticed now that the fetch upon hitting register on the sign-up page will hang and eventually fail. But I don't think we need to prioritize fixing this.

I will test with an "unclean" database on Monday.

@rsnyman rsnyman moved this from In Progress to In Review in Authentication and Authorization Nov 8, 2021
@john-dupuy
Copy link
Contributor

john-dupuy commented Nov 8, 2021

I manually added my user to an existing project and a new project by performing an INSERT into the users_projects table and was able to see my results.

I then installed ibutsu/ibutsu-client-python#19 and ibutsu/pytest-ibutsu#26 locally to try to push up some data with the pytest-plugin. I tried:

  • Only specifying --ibutsu, this gave an error like
Ibutsu server: http://localhost:8080
Error in call to Ibutsu API

I think this is fine, but it'd be nice if the pytest-plugin printed out a more helpful message.

  • Only specifying --ibutsu and --ibutsu-project, this gave an error like
There was an error while uploading results, and not all results were uploaded to the server.
All results were written to archive, partial results can be viewed on: http://localhost:3000/runs/b53fe7cc-bbb5-4cdd-94eb-580f5e06a5dd

Again, I think this is ok, but it may be helpful to have a note about the token.

  • Specifying --ibutsu, --ibutsu-project, and --ibutsu-token
    This worked great! And I was able to see my results being added to the server.

However, when I tried to view the run on the run details page, I see the following exception in the BE and the FE crashes:

  File "/home/jdupuy/iqe/iqe-repos/ibutsu/ibutsu-server/backend/ibutsu_server/controllers/artifact_controller.py", line 94, in get_artifact_list
    query = add_user_filter(query, user, Result.project)
  File "/home/jdupuy/iqe/iqe-repos/ibutsu/ibutsu-server/backend/ibutsu_server/util/projects.py", line 40, in add_user_filter
    query = query.filter(or_(User.projects.any(id=project.id), project.owner == user))
  File "/home/jdupuy/iqe/iqe-repos/ibutsu/ibutsu-server/backend/.ibutsu-env/lib/python3.9/site-packages/sqlalchemy/orm/attributes.py", line 238, in __getattr__
    util.raise_(
  File "/home/jdupuy/iqe/iqe-repos/ibutsu/ibutsu-server/backend/.ibutsu-env/lib/python3.9/site-packages/sqlalchemy/util/compat.py", line 182, in raise_
    raise exception
AttributeError: Neither 'InstrumentedAttribute' object nor 'Comparator' object associated with Result.project has an attribute 'id'

I'm pretty sure this is the same error we were hitting earlier - passing that Result.project into the add_user_filter

    query = add_user_filter(query, user, Result.project)

EDIT: yup that was it, making the change to

    query = add_user_filter(query, user)

Fixed the issue.

@john-dupuy
Copy link
Contributor

john-dupuy commented Nov 8, 2021

Another issue I noticed is that the pagination on the "Results" tab of the run details page looks like it is an estimate. I think the fetch there just needs estimate=false.

EDIT: I actually don't think estimation is the issue - query.count() is returning twice the number of results. I think something must be messed up with the query, as the DB only contains 37 rows:

ibutsu=# select COUNT(*) from results where run_id='364123ac-657c-49bf-b8b0-351300960061';
 count 
-------
    37

But the backend is returning 74 as the count.

I'll do a bit more investigation. Printed query:

print(query)
SELECT results.id AS results_id, results.component AS results_component, results.data AS results_data, results.duration AS results_duration, results.env AS results_env, results.params AS results_params, results.project_id AS results_project_id, results.result AS results_result, results.run_id AS results_run_id, results.source AS results_source, results.start_time AS results_start_time, results.test_id AS results_test_id 
FROM results, projects 
WHERE ((EXISTS (SELECT 1 
FROM users_projects, users 
WHERE projects.id = users_projects.project_id AND users.id = users_projects.user_id AND users.id = %(id_1)s)) OR %(param_1)s = projects.owner_id) AND results.run_id = %(run_id_1)s

I think it might be the or_ that's in add_user_filter, it is double counting things.

@rsnyman
Copy link
Contributor Author

rsnyman commented Nov 9, 2021

SELECT results.id AS results_id, results.component AS results_component, results.data AS results_data, results.duration AS results_duration, results.env AS results_env, results.params AS results_params, results.project_id AS results_project_id, results.result AS results_result, results.run_id AS results_run_id, results.source AS results_source, results.start_time AS results_start_time, results.test_id AS results_test_id 
FROM results, projects 
WHERE ((EXISTS (SELECT 1 
FROM users_projects, users 
WHERE projects.id = users_projects.project_id AND users.id = users_projects.user_id AND users.id = %(id_1)s)) OR %(param_1)s = projects.owner_id) AND results.run_id = %(run_id_1)s

The query looks OK -- by my logic, anyway. I'm going to poke around a bit more and see if I can figure out what's going wrong.

- Add a login page
- Add a login controller
- Use JWT authentication
- Make HttpClient service object that transparently auths requests
- Add OAuth2 login to Ibutsu
- Add tests for the login controller
- Add Keycloak integration
- Add authorisation, users can only see resources for projects they are part of
- Use the 'from_file' method if it exists
- Add support for running the dev servers under TLS
- Add signup, reset password pages
- Add creating and deleting tokens
- Update the db updates
@john-dupuy
Copy link
Contributor

Giving this another look, I noticed the following:

  • The result/run lists are not gated by the projects a user is a part of, so I can see all results if I de-select a project. If I try to click on a specific result and the user is not part of the project, I get a 403 and the UI will display the "Run/result not found" message.

However, I am ok with fixing this and the other 403 issue I mentioned in a followup. If you want to merge these before your break.

@john-dupuy john-dupuy closed this Nov 10, 2021
Authentication and Authorization automation moved this from In Review to Done Nov 10, 2021
@john-dupuy john-dupuy reopened this Nov 10, 2021
Authentication and Authorization automation moved this from Done to In Progress Nov 10, 2021
@john-dupuy
Copy link
Contributor

Why is the close button so close to the comment button?? 😆

Authentication and Authorization automation moved this from In Progress to In Review Nov 10, 2021
@rsnyman
Copy link
Contributor Author

rsnyman commented Nov 10, 2021

Yes, let's address them in a follow-up.

@rsnyman rsnyman merged commit 0724f95 into ibutsu:master Nov 10, 2021
@rsnyman rsnyman deleted the registration-page branch November 10, 2021 21:49
Authentication and Authorization automation moved this from In Review to Done Nov 10, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
No open projects
Development

Successfully merging this pull request may close these issues.

None yet

2 participants