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.

Sign up for GitHub

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

Feature/11 combine profile update and fi association #38

Merged
merged 15 commits into from
Oct 5, 2023
Merged

Feature/11 combine profile update and fi association #38

Show file tree
Hide file tree
Changes from 10 commits
Commits
Show all changes
15 commits
Select commit Hold shift + click to select a range
f90c4f1
Changed admin update user api to associate lei
guffee23 Sep 26, 2023
2ed0d01
Merge branch 'main' into Feature/11_combine_profile_update_and_fi_ass…
guffee23 Sep 26, 2023
a1453c0
Updated admin update test to include association
guffee23 Sep 26, 2023
e16b624
Linting
guffee23 Sep 26, 2023
1057704
Merge branch 'main' into Feature/11_combine_profile_update_and_fi_ass…
guffee23 Sep 26, 2023
face017
Various formatting changes
guffee23 Sep 28, 2023
fe0a473
Black linting
guffee23 Sep 28, 2023
562a06f
Removed unused line
guffee23 Sep 28, 2023
086a75f
Changed admin apis from async to sync
guffee23 Sep 29, 2023
80861ee
Merge branch 'main' into Feature/11_combine_profile_update_and_fi_ass…
guffee23 Oct 3, 2023
ff582cb
Updated User Profile dto and tests
guffee23 Oct 4, 2023
df3522f
Updated no lei test and associate func params
guffee23 Oct 4, 2023
d5a746c
Updated dto and admin update api
guffee23 Oct 5, 2023
5a75928
Linting
guffee23 Oct 5, 2023
9e6d2aa
Removed user profile dto config
guffee23 Oct 5, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions src/entities/models/__init__.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,7 @@
"FinanicialInstitutionAssociationDto",
"DeniedDomainDao",
"DeniedDomainDto",
"UserProfile",
"AuthenticatedUser",
]

Expand All @@ -25,5 +26,6 @@
FinancialInsitutionDomainCreate,
FinanicialInstitutionAssociationDto,
DeniedDomainDto,
UserProfile,
AuthenticatedUser,
)
15 changes: 12 additions & 3 deletions src/entities/models/dto.py
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
from typing import Any, Dict, List

from pydantic import BaseModel
from typing import List, Dict, Any, Set, Optional
from pydantic import BaseModel, Field
from pydantic.utils import to_camel
from starlette.authentication import BaseUser


Expand Down Expand Up @@ -41,6 +41,15 @@ class Config:
orm_mode = True


class UserProfile(BaseModel):
first_name: str = Field(alias="firstName")
last_name: str = Field(alias="lastName")
leis: Optional[Set[str]] = Field(alias="leis")

class Config:
alias_generator = to_camel
Copy link
Collaborator

@lchen-2101 lchen-2101 Oct 3, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

found a couple of things with the configs, the to_camel generator is actually doing pascal case; there is a to_lower_camel which would achieve the goal, without you having to specify Field; there is also an allow_population_by_field_name option which would allow the request payload to be snake case instead of just camel case; while it doesn't change the swagger doc to show camel case, it does allow the API to accept both camel case and snake case. So what you end up with would be:

class UserProfile(BaseModel):
    first_name: str
    last_name: str
    leis: Optional[Set[str]]

    class Config(BaseConfig):
        alias_generator = to_lower_camel
        allow_population_by_field_name = True

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

just a side note; when we update fastapi later, the included pydantic package also changes to 2.x from 1.x, and some of these configurations will be changed. Nothing to worry about here, but in the pydantic settings issue, the dependency will be updated, so more changes will happen here as well.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why do we need/want to have our API accept camelCase? Is that just an artifact of us using the same UserProfile model for both our API and for submitting to Keycloak's API? To me it seems more explicit and future-proof to decouple the two, and just do a simple translation to the expected JSON format at the time we call the Keycloak API. If we go this route, we have one oddball model object with extra Config section that likely needs an explainer comment for what it's doing there, and we'll need to keep the exclude={"leis"} bit up-to-date every time you add another field UserProfile.

This is all what I was getting at when I asked #38 (comment), so it's also possible I'm just missing something.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@hkeeler It has to do with keycloak and the dictionary vals it's expecting, which are in camelCase.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I get that. My question is, why are we doing these Pydantic gymnastics when we could just...

def update_me(request: Request, user: UserProfile):
    keycloak_user = {'firstName': user.first_name, 'lastName': user.last_name}
    oauth2_admin.update_user(request.user.id, keycloak_user)
    if user.leis:
        oauth2_admin.associate_to_leis(request.user.id, user.leis)

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

my thought would be still to have a well defined model to communicate with Keycloak instead of constructing a dict on the spot. I was hoping to leverage what's built into pydantic, and having the config take care of SerDe; but if we want to do away with the configs, having a method on the model would be more preferable.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

But that model is for our API, not Keycloak's, right? It feels like we're using the fact that the two APIs happen to be similar enough in message structure (currently) that we're using the same model for both, but doing so comes with the baggage of having their fields now coupled, having to understand/explain why one model is different from the rest, and having to adjust a disconnected method to exclude future fields we add.

Now, we could have a dedicated model for calls to Keycloak. I mean really, that should be the job of python-keycloak, but that seems to just be a very a light wrapper around their API. I think that's probably more than we need for this PR, though.

For now, I agree having a to_keycloak_user() on the user model works. It's simple and explicit.



class FinanicialInstitutionAssociationDto(FinancialInstitutionDto):
approved: bool

Expand Down
6 changes: 5 additions & 1 deletion src/oauth2/oauth2_admin.py
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
from http import HTTPStatus
import logging
import os
from typing import Dict, Any
from typing import Dict, Any, Set

import jose.jwt
import requests
Expand Down Expand Up @@ -88,5 +88,9 @@ def associate_to_lei(self, user_id: str, lei: str) -> None:
detail="No institution found for given LEI",
)

def associate_to_leis(self, user_id, leis: Set[str]):
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
def associate_to_leis(self, user_id, leis: Set[str]):
def associate_to_leis(self, user_id: str, leis: Set[str]):

for lei in leis:
self.associate_to_lei(user_id, lei)


oauth2_admin = OAuth2Admin()
16 changes: 9 additions & 7 deletions src/routers/admin.py
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,9 +1,10 @@
from http import HTTPStatus
from typing import Dict, Any, Set
from typing import Set
from fastapi import Depends, Request
from starlette.authentication import requires
from dependencies import check_domain
from util import Router
from entities.models import UserProfile

from entities.models import AuthenticatedUser
from oauth2 import oauth2_admin
Expand All @@ -13,18 +14,19 @@

@router.get("/me/", response_model=AuthenticatedUser)
@requires("authenticated")
async def get_me(request: Request):
def get_me(request: Request):
return request.user


@router.put("/me/", status_code=HTTPStatus.ACCEPTED, dependencies=[Depends(check_domain)])
@requires("manage-account")
async def update_me(request: Request, user: Dict[str, Any]):
oauth2_admin.update_user(request.user.id, user)
def update_me(request: Request, user: UserProfile):
oauth2_admin.update_user(request.user.id, user.dict(by_alias=True, exclude={"leis"}))
if user.leis:
oauth2_admin.associate_to_leis(request.user.id, user.leis)


@router.put("/me/institutions/", status_code=HTTPStatus.ACCEPTED, dependencies=[Depends(check_domain)])
@requires("manage-account")
async def associate_lei(request: Request, leis: Set[str]):
for lei in leis:
oauth2_admin.associate_to_lei(request.user.id, lei)
def associate_lei(request: Request, leis: Set[str]):
oauth2_admin.associate_to_leis(request.user.id, leis)
29 changes: 18 additions & 11 deletions tests/api/routers/test_admin_api.py
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
from unittest.mock import Mock, call
from unittest.mock import Mock

from fastapi import FastAPI
from fastapi.testclient import TestClient
Expand Down Expand Up @@ -40,7 +40,7 @@ def test_get_me_authed_with_institutions(self, app_fixture: FastAPI, auth_mock:

def test_update_me_unauthed(self, app_fixture: FastAPI, unauthed_user_mock: Mock):
client = TestClient(app_fixture)
res = client.put("/v1/admin/me", json={"firstName": "testFirst", "lastName": "testLast"})
res = client.put("/v1/admin/me", json={"firstName": "testFirst", "lastName": "testLast", "leis": ["testLei"]})
assert res.status_code == 403

def test_update_me_no_permission(self, app_fixture: FastAPI, auth_mock: Mock):
Expand All @@ -55,10 +55,22 @@ def test_update_me_no_permission(self, app_fixture: FastAPI, auth_mock: Mock):
AuthenticatedUser.from_claim(claims),
)
client = TestClient(app_fixture)
res = client.put("/v1/admin/me", json={"firstName": "testFirst", "lastName": "testLast"})
res = client.put("/v1/admin/me", json={"firstName": "testFirst", "lastName": "testLast", "leis": ["testLei"]})
assert res.status_code == 403

def test_update_me(self, mocker: MockerFixture, app_fixture: FastAPI, authed_user_mock: Mock):
update_user_mock = mocker.patch("oauth2.oauth2_admin.OAuth2Admin.update_user")
associate_lei_mock = mocker.patch("oauth2.oauth2_admin.OAuth2Admin.associate_to_leis")
update_user_mock.return_value = None
associate_lei_mock.return_value = None
client = TestClient(app_fixture)
data = {"firstName": "testFirst", "lastName": "testLast", "leis": ["testLei1", "testLei2"]}
res = client.put("/v1/admin/me", json=data)
update_user_mock.assert_called_once_with("testuser123", {"firstName": "testFirst", "lastName": "testLast"})
associate_lei_mock.assert_called_once_with("testuser123", {"testLei1", "testLei2"})
assert res.status_code == 202

def test_update_me_no_lei(self, mocker: MockerFixture, app_fixture: FastAPI, authed_user_mock: Mock):
update_user_mock = mocker.patch("oauth2.oauth2_admin.OAuth2Admin.update_user")
update_user_mock.return_value = None
client = TestClient(app_fixture)
Expand All @@ -68,14 +80,9 @@ def test_update_me(self, mocker: MockerFixture, app_fixture: FastAPI, authed_use
assert res.status_code == 202

def test_associate_institutions(self, mocker: MockerFixture, app_fixture: FastAPI, authed_user_mock: Mock):
associate_lei_mock = mocker.patch("oauth2.oauth2_admin.OAuth2Admin.associate_to_lei")
associate_lei_mock = mocker.patch("oauth2.oauth2_admin.OAuth2Admin.associate_to_leis")
associate_lei_mock.return_value = None
client = TestClient(app_fixture)
data = ["testlei1", "testlei2"]
res = client.put("/v1/admin/me/institutions", json=data)
expected_calls = [
call("testuser123", "testlei1"),
call("testuser123", "testlei2"),
]
associate_lei_mock.assert_has_calls(expected_calls, any_order=True)
res = client.put("/v1/admin/me/institutions", json=["testlei1", "testlei2"])
associate_lei_mock.assert_called_once_with("testuser123", {"testlei1", "testlei2"})
assert res.status_code == 202