cs3219-ay2324s1/ay2324s1-course-assessment-g23

ay2324s1-course-assessment-g23 created by GitHub Classroom

ay2324s1-course-assessment-g23's Issues

Changing `session_id` to cookies

Below is just a rough draft of what the changes could look like. Someone pls help to implement them.

The backend can set cookies in the frontend via Response from fastapi:

from fastapi.responses import Response

@app.post("/sessions",  status_code=200)
async def user_login(r: rm.UserLogin, response: Response):
    result = sf.user_login(r.username, r.password)
    response.set_cookie(key="session_id", value=result['session_id'])
    return result

The session_id for all the other APIs can then be obtained from the cookies via Cookie from fastapi:

from fastapi import Cookie

@app.delete("/questions/{question_id}", status_code=200)
async def delete_question(question_id: str, session_id: str | None = Cookie(None)):
    return qf.delete_question(question_id, session_id)

Then finally, the session_id key needs to be removed from the request models, else it'll cause an error when the payload doesn't contain session_id:

class DeleteUser(BaseModel):
    session_id: str  # to be removed

Add type hints to backend functions

Suggestion to add parameter and return type hints to the backend functions. Here's the type hint for user_login in /database_functions/session_functions.py:

from typing import TypedDict

class SessionDetails(TypedDict):
    user_id: str
    session_id: str
    role: str
    message: str

def user_login(username: str, password: str) -> SessionDetails:
    # ... rest of the code ...
    return {
        'user_id': f'{user_id}',
        'session_id': f'{session_id}',
        'role': f'{role}',
        'message': f'User {username} successfully logged in'
    }

Then it'll be easier to tell what the function returns.

Maintainers can't delete logged in user (ie. user with an ongoing session)

When a user has an ongoing session (ie. logged in), maintainers can't delete them, as shown below.

Perhaps the correct implementation is to do some kind of cascade delete of sessions when user is deleted?

Missing error message for question's description-related errors

The error message from #95 isn't displayed on frontend.

For reference, the error in #95 due to the question's description being too long:

React warning

Updating users gives a error with OK 200 status

Moving env vars from `docker-compose.yaml` into `.env`

Perhaps we should move these environment variables into .env file, instead of putting them in the docker-compose.yaml

  api_gateway:
    build: ./backend_services/api_gateway
    container_name: api_gateway
    networks:
      - client-side
      - backend-apis
    ports:
      - 8000:8000
    environment:
      - USERS_SERVICE_HOST=users_api
      - SESSIONS_SERVICE_HOST=users_api
      - QUESTIONS_SERVICE_HOST=questions_api
      - MATCHING_SERVICE_HOST=matching_api
      - API_PORT=8000

Stylise profile details (if needed)

For now, I just added a basic display for role + email into the profile popup.
Idk if @afiqzu u wanna stylise it or smth

Session token expiry time to be extended as long as user is active

Current implementation:
User is given a 15 minute hard-limit session token upon login.

Proposed implementation:
User activeness can be tracked via the API calls they make. When they make a call, get their session_id from their cookie, and trigger a call to the database to extend their expiry time

Maintainer is able to delete last maintainer

Failing API call when updating questions/users

Here's what's in the database, obtained from GET /questions/all request:

[
  {
    "question_id": "d77fbf35-9950-42c4-8b89-7332c4035f76",
    "title": "Q1",
    "description": "Desc1",
    "category": "Cat1",
    "complexity": "Easy"
  }
]

But when I try to update the question's category with a PUT /questions request with this payload:
(also happens when changing any field except title)

{
  "question_id": "7682d632-a8a6-46e0-bb3e-447c3b3dc3ae",
  "title": "qq",
  "description": "q",
  "category": "CHANGING CATEGORY",
  "complexity": "Easy"
}

It returns me 409 status with this response body:

{"detail":"Title already exists"}

Something similar also happens when updating users

Failed POST questions/users HTTP returns 200 OK

Same for POST users

Edit/Delete icons infront of edit-profile popup

Error in console when viewing the web page

Bug in `is_account_owner` in `/database_functions/utils/users_util.py`

Here's the current code for is_account_owner:

def is_account_owner(user_id, session_id):
    return db.execute_sql_read_fetchone("SELECT user_id FROM sessions WHERE session_id = %s", params=(session_id))[0] == user_id

The line:

... params=(session_id))[0] == user_id

is missing a comma for =(session_id,). Without the comma, the brackets won't form a tuple, and it'll throw an error.

Also, I think the is_account_owner might throw an error too when the db.execute_sql_read_fetchone call returns an empty array/doesn't return an array. Might need extra checks for that.

Change of host url's for services

As mentioned in PR #109 . Due to refactoring of the containers and services, containers are no longer hosted on localhost, but instead will call something like http://api_gateway:8000 instead of http://localhost:8000. i.e. Call the container name instead of localhost.
Frontend should only be calling 1 container, api_gateway.

@Lava-Iris do correct me if I'm wrong, and/or provide more details that you think may be useful for frontend team to know, if any.

Store collab data in database (instead of just in memory)

idk if its a good idea to store every delta, so maybe store the in-memory full-doc string into database every 5s if there's a change.

`get_user` in `database_functions/user_functions.py` not returning correct format when `user_id != "all"`

Make this fix on ASSIGNMENT 4 branch, NOT ASS. 3 to avoid having merge conflicts

get_user in database_functions/user_functions.py returns a list instead of an object when user_id != "all".

def get_user(user_id, session_id):
    # ... rest of the code ...
    FIELD_NAMES = ['user_id', 'username', 'email', 'password', 'role']
    if user_id == "all":
        rows = db.execute_sql_read_fetchall(f"SELECT {', '.join(FIELD_NAMES)} FROM users")
        users = [dict(zip(FIELD_NAMES, row)) for row in rows]
        return users
    return db.execute_sql_read_fetchone(f"SELECT {', '.join(FIELD_NAMES)} FROM users WHERE user_id = %s",
                                        params=(user_id,))

A fix wout be to replace the last line with
The last line

    # ... rest of the code ...
        users = [dict(zip(FIELD_NAMES, row)) for row in rows]
        return users

    rows = db.execute_sql_read_fetchone(f"SELECT {', '.join(FIELD_NAMES)} FROM users WHERE user_id = %s",
                                        params=(user_id,))
    user = dict(zip(FIELD_NAMES, rows))
    return user

`UpdateQuestionInfo` request model has no `session_id` field

UpdateQuestionInfo has no session_id field:

backend_services/questions_service/api/app/main.py

@app.put("/questions", status_code=200)
async def update_question_info(r: rm.UpdateQuestionInfo):
    return qc.update_question_info(r.question_id, r.title, r.description, r.category, r.complexity, r.session_id)

Maintainers don't have matchmaking option

idk if it's intentional or not. @afiqzu

Standardise user session details

Currently user_login in /database_functions/session_functions.py returns the format:

{
    'user_id': str,
    'session_id': str,
    'role': str,
    'message': str,
}

But get_session returns the format:

{
    'user_id': str,
    'session_id': str,
    'role': str,
    'creation_time': str,
    'expiration_time': str,
}

How about we standardise to the get_session format, including the 2 times.
The for user_login we seperate the user details from the message:

{
    user_details: {
        'user_id': str,
        'session_id': str,
        'role': str,
        'creation_time': str,
        'expiration_time': str,
    },
    message: str,
}

React warning about TimerProvider & MatchingScreen

Tweak new view-only description for normal users

Since normal users can't edit description, I've removed the Save button and swap the TextField MUI component with a DialogContentText that was beside it, to prevent users from being able to edit/type into the textbox.

But the normal-user description popup now looks weird. Will need help tweaking the CSS of it.

As maintainer (no change)

As normal user

Here's the code change:

Unofficial instructions to run/demo each assignments

The commits havent been merged to master, nor have they been tagged
(waiting on TA Anab to reply with what the tag name should be)

So these are the unofficial instructions for @Lava-Iris to do the demos

Assignment 1

git checkout fbdb03fa778803ba1bb25c7fb5adb85d44717f76
cd frontend
npm install
npm run dev

Port is chosen randomly, so click on the link in the console. It should look like:

Local: http://127.0.0.1:1234/

Assignment 2

git checkout 2ca83a3fdf359783309fb33dc7704a9ac3b637f1
docker volume create sql-data
bash start_containers.sh

In browser, go to http://localhost

Assignment 3

git checkout 12672d615512eca8cd6e1f3ba982a807dd8ce5b1
docker volume create sql-data
bash start_containers.sh

In browser, go to http://localhost

Assignment 4

git checkout f1bbede9c29bf6f89ac9b6d3ad78bf5474afd88b
bash start_containers.sh

In browser, go to http://localhost

Assignment 5

git checkout e75e3d672d63bfac1f97b0ad7761772287fc9313
bash start_containers.sh

In browser, go to http://localhost

Should we remove "Add a User" options for maintainers from frontend?

It's kind of weird to create new users from the table. Like shouldn't u use the signup page for that?

Standardise question/user ID names

As discussed, lets set all references to question/user IDs as question_id / user_id same as in the database

Some stuff that needs fixing:

• Reverting commit 72c6c13
• Updating error messages to use the terms question_id / user_id
  For example:

  # In `/api/app/database_functions/question_functions.py`
  def _create_question_check_args(question_id, title, description, category, complexity):
      if question_id is None:
          raise HTTPException(status_code=422, detail='Missing question id')  # maybe change to question_id

  # in `/api/app/tests/user_functions_tests/update_user_info_tests.py`
  data = {
              "id": "121d480c-de96-4f72-abca-0a31586ccb28", # change key to "user_id"
              "username": "testuser1102",
              ...
  }

Remove `password` value from the `GET /users` API return

Even though the returned password is hashed, I think it's kinda weird to have passwords returned at all.

Edit:

After removing password from the API, what should we do about the "Hashed Password" column in the frontend?
Remove it?

Add loading animation when doing actions (eg. signing-up, logging-in)

We can use the react-query mutation's .isLoading property to tell if an action is being performed.

For example during signups:

const Signup = () => {
    const signupUserMutation = useSignupUser()

    signupUserMutation.isLoading // `true` while waiting for backend's reply during signup

    // ... rest of the code ...
}

Backend error when updating question

error when trying to update question:

questions_api       | INFO:     192.168.128.5:51128 - "PUT /questions HTTP/1.1" 500 Internal Server Error
api_gateway         | INFO:     192.168.128.1:35908 - "PUT /questions HTTP/1.1" 500 Internal Server Error
api_gateway         | ERROR:    Exception in ASGI application
api_gateway         | Traceback (most recent call last):
questions_api       | ERROR:    Exception in ASGI application
api_gateway         |   File "/usr/local/lib/python3.11/site-packages/uvicorn/protocols/http/h11_impl.py", line 408, in run_asgi
questions_api       | Traceback (most recent call last):
api_gateway         |     result = await app(  # type: ignore[func-returns-value]
questions_api       |   File "/usr/local/lib/python3.11/site-packages/uvicorn/protocols/http/h11_impl.py", line 408, in run_asgi
api_gateway         |              ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
questions_api       |     result = await app(  # type: ignore[func-returns-value]
api_gateway         |   File "/usr/local/lib/python3.11/site-packages/uvicorn/middleware/proxy_headers.py", line 84, in __call__
questions_api       |              ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
api_gateway         |     return await self.app(scope, receive, send)
questions_api       |   File "/usr/local/lib/python3.11/site-packages/uvicorn/middleware/proxy_headers.py", line 84, in __call__
api_gateway         |            ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
questions_api       |     return await self.app(scope, receive, send)
api_gateway         |   File "/usr/local/lib/python3.11/site-packages/fastapi/applications.py", line 292, in __call__
questions_api       |            ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
api_gateway         |     await super().__call__(scope, receive, send)
questions_api       |   File "/usr/local/lib/python3.11/site-packages/fastapi/applications.py", line 292, in __call__
api_gateway         |   File "/usr/local/lib/python3.11/site-packages/starlette/applications.py", line 122, in __call__
questions_api       |     await super().__call__(scope, receive, send)
api_gateway         |     await self.middleware_stack(scope, receive, send)
questions_api       |   File "/usr/local/lib/python3.11/site-packages/starlette/applications.py", line 122, in __call__
api_gateway         |   File "/usr/local/lib/python3.11/site-packages/starlette/middleware/errors.py", line 184, in __call__
questions_api       |     await self.middleware_stack(scope, receive, send)
api_gateway         |     raise exc
questions_api       |   File "/usr/local/lib/python3.11/site-packages/starlette/middleware/errors.py", line 184, in __call__
api_gateway         |   File "/usr/local/lib/python3.11/site-packages/starlette/middleware/errors.py", line 162, in __call__
questions_api       |     raise exc
api_gateway         |     await self.app(scope, receive, _send)
questions_api       |   File "/usr/local/lib/python3.11/site-packages/starlette/middleware/errors.py", line 162, in __call__
api_gateway         |   File "/usr/local/lib/python3.11/site-packages/starlette/middleware/cors.py", line 91, in __call__
questions_api       |     await self.app(scope, receive, _send)
api_gateway         |     await self.simple_response(scope, receive, send, request_headers=headers)
questions_api       |   File "/usr/local/lib/python3.11/site-packages/starlette/middleware/cors.py", line 83, in __call__
api_gateway         |   File "/usr/local/lib/python3.11/site-packages/starlette/middleware/cors.py", line 146, in simple_response        
questions_api       |     await self.app(scope, receive, send)
api_gateway         |     await self.app(scope, receive, send)
questions_api       |   File "/usr/local/lib/python3.11/site-packages/starlette/middleware/exceptions.py", line 79, in __call__
api_gateway         |   File "/usr/local/lib/python3.11/site-packages/starlette/middleware/exceptions.py", line 79, in __call__
questions_api       |     raise exc
api_gateway         |     raise exc
questions_api       |   File "/usr/local/lib/python3.11/site-packages/starlette/middleware/exceptions.py", line 68, in __call__
api_gateway         |   File "/usr/local/lib/python3.11/site-packages/starlette/middleware/exceptions.py", line 68, in __call__
questions_api       |     await self.app(scope, receive, sender)
api_gateway         |     await self.app(scope, receive, sender)
questions_api       |   File "/usr/local/lib/python3.11/site-packages/fastapi/middleware/asyncexitstack.py", line 20, in __call__        
api_gateway         |   File "/usr/local/lib/python3.11/site-packages/fastapi/middleware/asyncexitstack.py", line 20, in __call__        
questions_api       |     raise e
api_gateway         |     raise e
questions_api       |   File "/usr/local/lib/python3.11/site-packages/fastapi/middleware/asyncexitstack.py", line 17, in __call__        
api_gateway         |   File "/usr/local/lib/python3.11/site-packages/fastapi/middleware/asyncexitstack.py", line 17, in __call__        
questions_api       |     await self.app(scope, receive, send)
api_gateway         |     await self.app(scope, receive, send)
questions_api       |   File "/usr/local/lib/python3.11/site-packages/starlette/routing.py", line 718, in __call__
api_gateway         |   File "/usr/local/lib/python3.11/site-packages/starlette/routing.py", line 718, in __call__
questions_api       |     await route.handle(scope, receive, send)
api_gateway         |     await route.handle(scope, receive, send)
questions_api       |   File "/usr/local/lib/python3.11/site-packages/starlette/routing.py", line 276, in handle
api_gateway         |   File "/usr/local/lib/python3.11/site-packages/starlette/routing.py", line 276, in handle
questions_api       |     await self.app(scope, receive, send)
api_gateway         |     await self.app(scope, receive, send)
questions_api       |   File "/usr/local/lib/python3.11/site-packages/starlette/routing.py", line 66, in app
api_gateway         |   File "/usr/local/lib/python3.11/site-packages/starlette/routing.py", line 66, in app
questions_api       |     response = await func(request)
api_gateway         |     response = await func(request)
questions_api       |                ^^^^^^^^^^^^^^^^^^^
api_gateway         |                ^^^^^^^^^^^^^^^^^^^
questions_api       |   File "/usr/local/lib/python3.11/site-packages/fastapi/routing.py", line 273, in app
api_gateway         |   File "/usr/local/lib/python3.11/site-packages/fastapi/routing.py", line 273, in app
questions_api       |     raw_response = await run_endpoint_function(
api_gateway         |     raw_response = await run_endpoint_function(
questions_api       |                    ^^^^^^^^^^^^^^^^^^^^^^^^^^^^
api_gateway         |                    ^^^^^^^^^^^^^^^^^^^^^^^^^^^^
questions_api       |   File "/usr/local/lib/python3.11/site-packages/fastapi/routing.py", line 190, in run_endpoint_function
api_gateway         |   File "/usr/local/lib/python3.11/site-packages/fastapi/routing.py", line 190, in run_endpoint_function
questions_api       |     return await dependant.call(**values)
api_gateway         |     return await dependant.call(**values)
questions_api       |            ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
api_gateway         |            ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
questions_api       |   File "/app/main.py", line 36, in update_question_info
questions_api       |     return qc.update_question_info(r.question_id, r.title, r.description, r.category, r.complexity, r.session_id)  
questions_api       |                                                                                                     ^^^^^^^^^^^^   
api_gateway         |   File "/main.py", line 76, in handle_request
questions_api       |   File "/usr/local/lib/python3.11/site-packages/pydantic/main.py", line 734, in __getattr__
api_gateway         |     response = response.json()
questions_api       |     raise AttributeError(f'{type(self).__name__!r} object has no attribute {item!r}')
api_gateway         |                ^^^^^^^^^^^^^^^
questions_api       | AttributeError: 'UpdateQuestionInfo' object has no attribute 'session_id'
api_gateway         |   File "/usr/local/lib/python3.11/site-packages/httpx/_models.py", line 756, in json
api_gateway         |     return jsonlib.loads(self.text, **kwargs)
api_gateway         |            ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
api_gateway         |   File "/usr/local/lib/python3.11/json/__init__.py", line 346, in loads
api_gateway         |     return _default_decoder.decode(s)
api_gateway         |            ^^^^^^^^^^^^^^^^^^^^^^^^^^
api_gateway         |   File "/usr/local/lib/python3.11/json/decoder.py", line 337, in decode
api_gateway         |     obj, end = self.raw_decode(s, idx=_w(s, 0).end())
api_gateway         |                ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
api_gateway         |   File "/usr/local/lib/python3.11/json/decoder.py", line 355, in raw_decode
api_gateway         |     raise JSONDecodeError("Expecting value", s, err.value) from None
api_gateway         | json.decoder.JSONDecodeError: Expecting value: line 1 column 1 (char 0)

Incorrect date comparison in `is_expired_session`

In backend_services/users_service/api/app/utils/sessions_util.py

it's comparing string vs string. It probably needs to compare some kind of date type.

NOTE

Right now, the expiration date is a string. Idk what's the format of this date-string, so it'd be better to wait for the expiration-date to be changed into some kind of date-time type in the database. Thus this depends on issue #152

Backend doesn't hash new password when updating user's password

It doesn't rehash based on this code in /database_functions/user_functions.py:

def update_user_info(user_id, username, password, email, role, session_id):
    # ... rest of the code ...
    db.execute_sql_write("""UPDATE users
                        SET username = %s, password = %s, email = %s
                        WHERE user_id = %s""",
                        params=(username, new_password, email, user_id))
    return {'message': 'Successfully updated'}

Also, a related bug is #81 which prevents users from even updating their profile details.

Backend crashes when logging in with non-existing username/password

the function db.execute_sql_read_fetchone returns None when no such user exists, which cannot be unpacked into user_id, role and thus the code crashes.

def is_valid_login(username, hashed_password):
    user_id, role = db.execute_sql_read_fetchone("SELECT user_id, role FROM users where username = %s AND password = %s",
                                        params=(username, hashed_password))

Backend error when updating user info

The backend throws the below error when attempting to update a user's details in these cases:

• update with unchanged user details
• update when changing username/email, with no duplicates

Scroll bar in description popup

Vite complaining about path references to `/public/` dir

Missing type hints

The files:

• backend_services/users_service/api/app/controllers/users_controller.py
• backend_services/api_gateway/app/utils/api_gateway_util.py
• backend_services/users_service/api/app/utils/users_util.py

has some missing type hints.

Also, in backend_services/api_gateway/app/main.py, this line has some static-analysis error:

        # Send message to microservice
        if service == "matching-service":
            connect_matching_service_websocket(websocket, request)

Also, in backend_services/api_gateway/app/utils/api_gateway_util.py, these static-analysis errors need to have some type ignores:

Add loading screen when checking if user is logged in

I've implemented persistent login on the frontend.

When the user visits the login page but is already logged in, they'll be redirected to the /questions page.

However, it takes a split second for it to get confirmation from the backend that the user is indeed logged in, shown below.
Thus, we might need to add a loading screen while it's confirming with the backend.

Change SQL field type for session date-time

Should probably change time-related fields to somekind of date-time type

NOTE

Issue #148 depends on this change, so let me know when it's changed, and i can work on #148.

No checks are done for whether a user action needs to be specific to user

E.g. A user could update the information of another user, as long as they know the other user_id.

Reason:
Any user-specific action requires user_id in the API call.
Currently, there is only a check for whether the user is logged in, but not whether they are logged into the account that they supplied the user_id for.
It is a wrong assumption that the user_id is private to a user.

This is somewhat mitigated by the frontend, as the user_id that is supplied during a normal API call will be the user's personal one.

However, this can be exploited if a malicious user crafts their own API call.

Fix (either one):

1. user_id needs to be checked against session_id (verified login) before executing any user-specific action.
2. Change the implementation of user-specific actions to use session_id instead of user_id(session table would contain all other required information, like user_id)

Revoking cookies when they expire

When cookies expire, either the:

• frontend needs to remove them
• or the backend needs to tell frontend to remove them

I'm in favor of getting backend to remove, as the frontend really differentiate between "session doesn't exist" vs "session exists but u don't have permissions"

Edit:

After looking at the backend code, i think frontend should revoke the cookie when it gets 401 on GET /sessions

Add form doesn't clear after adding

Canceling question-description edit doesn't clear description textbox

Opening question description dialog for the 1st time gives a console error

But subsequent opening of the description dialog doesn't seem to log an error

`get_question` function has 2 declaration

get_question has 2 declaration

backend_services/questions_service/api/app/main.py

@app.get("/questions/{question_id}", status_code=200)
async def get_question(question_id: str):
    return qc.get_question(question_id)

@app.get("/questions_all", status_code=200)
async def get_question():
    return qc.get_all_questions()

Questions can't contain too long of a description

When entering a typical leetcode question like:

The backend throws an error saying:

My guess is that the description can't be >255 characters?

Add message when matched-user joins/leaves room

Inability to update user "password" field when editing

When editing a user, when u try to change the "password" field, it doesn't change the input value. Probably some problems with tracking the "password" state in the UserEditableRow.tsx

Errors on profile page doesn't show alert messages

On this page, if (for example)u try to change ur name to an already taken username, it won't show anything in the GUI

Need an API for updating user info (eg. username/email) without requiring password

Currently the API for updating user info requires all fields to be present.

To change just the username, we'll have to send the API a payload containing a different username while everything else stays the same.

The problem is, we aren't keeping track of the user's password. And the GET user API only returns the hashed of the password. If we send the hashed password during updating of user info, it'll set the password tho the hashed-password.

Refreshing on `/users` page will redirect to `/` then `/questions`

It should stay on /users page when refreshing