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 5 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 @@ -8,6 +8,7 @@
"FinancialInsitutionDomainCreate",
"DeniedDomainDao",
"DeniedDomainDto",
"UserProfile",
"AuthenticatedUser",
]

Expand All @@ -23,5 +24,6 @@
FinancialInsitutionDomainDto,
FinancialInsitutionDomainCreate,
DeniedDomainDto,
UserProfile,
AuthenticatedUser,
)
12 changes: 10 additions & 2 deletions src/entities/models/dto.py
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,4 @@
from typing import Any, Dict, List

from typing import List, Dict, Any, Set
from pydantic import BaseModel
from starlette.authentication import BaseUser

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


class UserProfile(BaseModel):
firstName: str
lastName: str
Copy link
Collaborator

Choose a reason for hiding this comment

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

let's keep the variable naming to the python convention of snake case, and use alias_generator to convert them to camel case: https://docs.pydantic.dev/latest/api/config/#pydantic.alias_generators.to_camel
https://stackoverflow.com/questions/67995510/how-to-inflect-from-snake-case-to-camel-case-post-the-pydantic-schema-validation

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The body was to look like:
{ "firstName": "First", "lastName": "Last", "leis": ["LEI1", "LEI2"] }
So, we need aliasing for the request as well as when generating the dict?
To match the request reqs and the syntax that keycloak expects, we would need it in the form above, but to change to snake case would require aliasing to snake when request is sent and when creating dict being sent to keycloak.
Just double-checking here. Should the request body be changed to snake case?

Copy link
Collaborator

Choose a reason for hiding this comment

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

request body to the api should be snake case, we would then alias it to camel case for keycloak.

Copy link
Member

Choose a reason for hiding this comment

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

Just so I understand, our API's User resource will look like:

{
  "first_name":"Jane",
  "last_name":"Doe",
  "leis": ["123456789012","234567890123"],
}

Meanwhile, the Keycloak Admin API's UserRepresentation expects:

{
  "firstName": "Jane",
  "lastName": "Doe"
}

...and to associated LEIs to said User, it's reeeaaally associating a Keycloak User to a Keycloak Group which is named after a given LEI? So, to complete the transaction, and second call that's made to...

  • PUT /admin/realms/{realm}/users/{id}/groups/{groupId}

If yes, 👍. If no, I may need some help here. 😅

I think what @lchen-2101 is getting at is that we shouldn't couple our API format to Keycloak's, which I agree with.

As to Pydantic's to_camel, I'm not sure we want to do that. There's only 2 attributes we care about. Aaaand, what happens to leis on our model when you do the to_camel? Wouldn't that still be there in the data we'd be submitting to Keycloak? Seems like that might break.

To me, it doesn't seem that gross to just construct the dict at time of request in update_me(). 🤷 Looks like we're already doing it that way in upsert_group now anyway.

leis: Set[str] = None

def get_user(self) -> Dict[str, Any]:
return {"firstName": self.firstName, "lastName": self.lastName}
Copy link
Collaborator

Choose a reason for hiding this comment

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

pydantic should have something built-in that will generate the dict for you, I don't think you need to create a separate method for this.



class AuthenticatedUser(BaseUser, BaseModel):
claims: Dict[str, Any]
name: str
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_lei_set(self, user_id, leis: Set[str]):
Copy link
Collaborator

Choose a reason for hiding this comment

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

normally if we can, we'd like to keep the implementation detail of the collection out of the var name / method name; this one should be good with just associate_to_leis

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


oauth2_admin = OAuth2Admin()
12 changes: 7 additions & 5 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 @@ -19,12 +20,13 @@ async def get_me(request: Request):

@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)
async def update_me(request: Request, user: UserProfile):
Copy link
Member

Choose a reason for hiding this comment

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

Should API functions that use the Keycloak client be async? It looks like python-keycloak uses requests under the hood, which is not async. I think they'd need to use something like httpx to support that. As it stands now, I think this could lead to unintentional blocking during these calls.

Dropping async is not a bad thing. FastAPI will just run these requests in a new thread, which I think is what we'd want for synchronous processes like this.

oauth2_admin.update_user(request.user.id, user.get_user())
if user.leis:
oauth2_admin.associate_to_lei_set(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)
oauth2_admin.associate_to_lei_set(request.user.id, leis)
26 changes: 17 additions & 9 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_lei_set")
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,10 @@ 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_lei_set")
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)
associate_lei_mock.assert_called_once_with("testuser123", {"testlei1", "testlei2"})
assert res.status_code == 202