-
Notifications
You must be signed in to change notification settings - Fork 111
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
[PAY-2761] Add managed users endpoints #8238
Changes from 6 commits
d063999
ebc9e71
73b3299
3a121b0
cf6ba9b
2ed52e9
14e5ed1
a719b68
9b7e660
ca5ab6e
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,117 @@ | ||
import copy | ||
|
||
from integration_tests.utils import populate_mock_db | ||
from src.queries.get_managed_users import get_managed_users_with_grants | ||
from src.utils.db_session import get_db | ||
|
||
test_entities = { | ||
"users": [ | ||
{"user_id": 10, "name": "a", "wallet": "0x10"}, | ||
{"user_id": 20, "name": "b", "wallet": "0x20"}, | ||
{"user_id": 30, "name": "c", "wallet": "0x30"}, | ||
{"user_id": 40, "name": "d", "wallet": "0x40"}, | ||
{"user_id": 50, "name": "e", "wallet": "0x50"}, | ||
{"user_id": 60, "name": "f", "wallet": "0x60"}, | ||
], | ||
"grants": [ | ||
# Active grants | ||
{ | ||
"user_id": 20, | ||
"grantee_address": "0x10", | ||
"is_approved": True, | ||
"is_revoked": False, | ||
}, | ||
{ | ||
"user_id": 30, | ||
"grantee_address": "0x10", | ||
"is_approved": True, | ||
"is_revoked": False, | ||
}, | ||
# Not yet approved | ||
{ | ||
"user_id": 40, | ||
"grantee_address": "0x10", | ||
"is_approved": False, | ||
"is_revoked": False, | ||
}, | ||
# Approved then Revoked | ||
{ | ||
"user_id": 50, | ||
"grantee_address": "0x10", | ||
"is_approved": True, | ||
"is_revoked": True, | ||
}, | ||
# Revoked before approval | ||
{ | ||
"user_id": 60, | ||
"grantee_address": "0x10", | ||
"is_approved": False, | ||
"is_revoked": True, | ||
}, | ||
], | ||
} | ||
|
||
|
||
def test_get_managed_users_default(app): | ||
with app.app_context(): | ||
db = get_db() | ||
populate_mock_db(db, test_entities) | ||
|
||
managed_users = get_managed_users_with_grants( | ||
{"manager_wallet_address": "0x10", "current_user_id": 10} | ||
) | ||
|
||
# return all non-revoked records by default | ||
assert len(managed_users) == 3, "Expected exactly 3 records" | ||
assert ( | ||
record["grant"]["is_revoked"] == False for record in managed_users | ||
), "Revoked records returned" | ||
|
||
|
||
def test_get_managed_users_grants_without_users(app): | ||
with app.app_context(): | ||
db = get_db() | ||
|
||
entities = copy.deepcopy(test_entities) | ||
# Record for a user which won't be found | ||
entities["grants"].append( | ||
{ | ||
"user_id": 70, | ||
"grantee_address": "0x10", | ||
"is_approved": False, | ||
"is_revoked": False, | ||
} | ||
) | ||
populate_mock_db(db, entities) | ||
|
||
managed_users = get_managed_users_with_grants( | ||
{"manager_wallet_address": "0x10", "current_user_id": 10} | ||
) | ||
|
||
# return all non-revoked records by default | ||
assert len(managed_users) == 3, "Expected exactly 3 records" | ||
assert ( | ||
record["grant"]["user_id"] != 70 for record in managed_users | ||
), "Revoked records returned" | ||
|
||
|
||
def test_get_managed_users_invalid_parameters(app): | ||
with app.app_context(): | ||
db = get_db() | ||
populate_mock_db(db, test_entities) | ||
|
||
try: | ||
get_managed_users_with_grants( | ||
{"manager_wallet_address": None, "current_user_id": 10} | ||
) | ||
assert False, "Should have thrown an error for missing wallet address" | ||
except ValueError as e: | ||
assert str(e) == "manager_wallet_address is required" | ||
|
||
try: | ||
get_managed_users_with_grants( | ||
{"manager_wallet_address": "0x10", "current_user_id": None} | ||
) | ||
assert False, "Should have thrown an error for missing current user id" | ||
except ValueError as e: | ||
assert str(e) == "current_user_id is required" |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,25 @@ | ||
from flask_restx import fields | ||
|
||
from .common import ns | ||
from .users import user_model | ||
|
||
# Describes a grant made from user to another user | ||
grant = ns.model( | ||
"grant", | ||
{ | ||
"grantee_address": fields.String(required=True), | ||
"user_id": fields.String(required=True), | ||
"is_revoked": fields.Boolean(required=True), | ||
"is_approved": fields.Boolean(required=True), | ||
"created_at": fields.String(required=True), | ||
"updated_at": fields.String(required=True), | ||
}, | ||
) | ||
|
||
managed_user = ns.model( | ||
"managed_user", | ||
{ | ||
"user": fields.Nested(user_model, required=True), | ||
"grant": fields.Nested(grant, required=True), | ||
}, | ||
) |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,81 @@ | ||
import logging | ||
from typing import Dict, List, Optional, TypedDict | ||
|
||
from src.models.grants.grant import Grant | ||
from src.queries.get_unpopulated_users import get_unpopulated_users | ||
from src.queries.query_helpers import populate_user_metadata | ||
from src.utils import db_session | ||
from src.utils.helpers import query_result_to_list | ||
|
||
logger = logging.getLogger(__name__) | ||
|
||
|
||
class GetManagedUsersArgs(TypedDict): | ||
manager_wallet_address: str | ||
current_user_id: int | ||
is_approved: Optional[bool] | ||
is_revoked: Optional[bool] | ||
|
||
|
||
def make_managed_users_list(users: List[Dict], grants: List[Dict]) -> List[Dict]: | ||
managed_users = [] | ||
grants_map = {grant.get("user_id"): grant for grant in grants} | ||
|
||
for user in users: | ||
grant = grants_map.get(user.get("user_id")) | ||
if grant is None: | ||
continue | ||
|
||
managed_users.append( | ||
{ | ||
"user": user, | ||
"grant": grant, | ||
} | ||
) | ||
|
||
return managed_users | ||
|
||
|
||
def get_managed_users_with_grants(args: GetManagedUsersArgs) -> List[Dict]: | ||
""" | ||
Returns users managed by the given wallet address | ||
|
||
Args: | ||
manager_wallet_address: str wallet address of the manager | ||
is_approved: Optional[bool] If set, filters by approval status | ||
is_revoked: Optional[bool] If set, filters by revocation status, defaults to False | ||
|
||
Returns: | ||
List of Users with grant information | ||
""" | ||
is_approved = args.get("is_approved", None) | ||
is_revoked = args.get("is_revoked", False) | ||
current_user_id = args.get("current_user_id") | ||
grantee_address = args.get("manager_wallet_address") | ||
if grantee_address is None: | ||
raise ValueError("manager_wallet_address is required") | ||
if current_user_id is None: | ||
raise ValueError("current_user_id is required") | ||
|
||
db = db_session.get_db_read_replica() | ||
with db.scoped_session() as session: | ||
query = session.query(Grant).filter( | ||
Grant.grantee_address == grantee_address, Grant.is_current == True | ||
) | ||
|
||
if is_approved is not None: | ||
query = query.filter(Grant.is_approved == is_approved) | ||
if is_revoked is not None: | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. idt it's possible for is_revoked to actually be There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The input is a dict, so it's possible to not set the value at all. I had assumed that results in There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Added a test which sets both filters to |
||
query = query.filter(Grant.is_revoked == is_revoked) | ||
|
||
grants = query.all() | ||
if len(grants) == 0: | ||
return [] | ||
|
||
user_ids = [grant.user_id for grant in grants] | ||
users = get_unpopulated_users(session, user_ids) | ||
users = populate_user_metadata(session, user_ids, users, current_user_id) | ||
|
||
grants = query_result_to_list(grants) | ||
|
||
return make_managed_users_list(users, grants) |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -14,7 +14,9 @@ | |
SIGNATURE_HEADER = "Encoded-Data-Signature" | ||
|
||
|
||
def auth_middleware(parser: reqparse.RequestParser = None): | ||
def auth_middleware( | ||
parser: reqparse.RequestParser = None, include_wallet: bool = False | ||
): | ||
""" | ||
Auth middleware decorator. | ||
|
||
|
@@ -60,6 +62,7 @@ def decorator(func): | |
def wrapper(*args, **kwargs): | ||
message = request.headers.get(MESSAGE_HEADER) | ||
signature = request.headers.get(SIGNATURE_HEADER) | ||
wallet_lower = None | ||
|
||
authed_user_id = None | ||
if message and signature: | ||
|
@@ -68,13 +71,14 @@ def wrapper(*args, **kwargs): | |
wallet = web3.eth.account.recover_message( | ||
encoded_to_recover, signature=signature | ||
) | ||
wallet_lower = wallet.lower() | ||
db = db_session.get_db_read_replica() | ||
with db.scoped_session() as session: | ||
user = ( | ||
session.query(User.user_id) | ||
.filter( | ||
# Convert checksum wallet to lowercase | ||
User.wallet == wallet.lower(), | ||
User.wallet == wallet_lower, | ||
User.is_current == True, | ||
) | ||
# In the case that multiple wallets match (not enforced on the data layer), | ||
|
@@ -87,7 +91,16 @@ def wrapper(*args, **kwargs): | |
logger.info( | ||
f"auth_middleware.py | authed_user_id: {authed_user_id}" | ||
) | ||
return func(*args, **kwargs, authed_user_id=authed_user_id) | ||
return ( | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I learned today that if you pass a kv argument that isn't consumed, it will often generate an error. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think you could include in it kwargs, like this
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Oh interesting. Maybe we should do that for the other variable as well then? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. |
||
func( | ||
*args, | ||
**kwargs, | ||
authed_user_id=authed_user_id, | ||
authed_user_wallet=wallet_lower, | ||
) | ||
if include_wallet | ||
else func(*args, **kwargs, authed_user_id=authed_user_id) | ||
) | ||
|
||
return wrapper | ||
|
||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nice way of setting this up