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

[C-3921] Stop sending signature headers as params #7774

Merged
merged 4 commits into from
Apr 23, 2024
Merged

[C-3921] Stop sending signature headers as params #7774

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
a1511e4
[C-3921] Stop sending signature headers as params
raymondjacobson Mar 3, 2024
69f1f39
Fix lint
raymondjacobson Mar 3, 2024
7e92db3
Merge remote-tracking branch 'origin/main' into rj-c-3921
raymondjacobson Apr 22, 2024
1882563
Move to auth_middelware helper and regenerate
raymondjacobson Apr 23, 2024
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
4 changes: 1 addition & 3 deletions packages/common/src/api/library.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -51,9 +51,7 @@ const fetchLibraryCollections = async ({
query,
sortMethod,
sortDirection,
type: category,
encodedDataMessage: '', // TODO: remove, handled by sdk
encodedDataSignature: '' // TODO: remove, handled by sdk
type: category
}
const { data: rawCollections = [] } =
collectionType === 'album'
Expand Down
16 changes: 4 additions & 12 deletions packages/common/src/api/purchases.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -64,9 +64,7 @@ const purchasesApi = createApi({
sortDirection,
sortMethod,
id: Id.parse(userId!),
userId: Id.parse(userId!),
encodedDataMessage: '', // TODO: remove, handled by sdk
encodedDataSignature: ''
Copy link
Contributor

Choose a reason for hiding this comment

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

Love all of these getting removed. Will make migration easier for sure.

userId: Id.parse(userId!)
})
const purchases = data.map(parsePurchase)

Expand Down Expand Up @@ -95,9 +93,7 @@ const purchasesApi = createApi({
const sdk = await audiusSdk()
const { data } = await sdk.full.users.getPurchasesCount({
id: Id.parse(userId!),
userId: Id.parse(userId!),
encodedDataMessage: '', // TODO: remove, handled by sdk
encodedDataSignature: '' // TODO: remove, handled by sdk
userId: Id.parse(userId!)
})
return data ?? 0
},
Expand All @@ -121,9 +117,7 @@ const purchasesApi = createApi({
sortDirection,
sortMethod,
id: Id.parse(userId!),
userId: Id.parse(userId!),
encodedDataMessage: '', // TODO: remove, handled by sdk
encodedDataSignature: '' // TODO: remove, handled by sdk
userId: Id.parse(userId!)
})

const purchases = data.map(parsePurchase)
Expand Down Expand Up @@ -153,9 +147,7 @@ const purchasesApi = createApi({
const sdk = await audiusSdk()
const { data } = await sdk.full.users.getSalesCount({
id: Id.parse(userId!),
userId: Id.parse(userId!),
encodedDataMessage: '', // TODO: remove, handled by sdk
encodedDataSignature: '' // TODO: remove, handled by sdk
userId: Id.parse(userId!)
})
return data ?? 0
},
Expand Down
8 changes: 2 additions & 6 deletions packages/common/src/api/user.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -129,9 +129,7 @@ const userApi = createApi({
sortMethod,
id: Id.parse(userId!),
type,
method,
encodedDataMessage: '', // TODO: remove, handled by sdk
encodedDataSignature: '' // TODO: remove, handled by sdk
method
})

return data.map((transaction) => parseTransaction({ transaction }))
Expand All @@ -151,9 +149,7 @@ const userApi = createApi({
const { data } = await sdk.full.users.getUSDCTransactionCount({
id: Id.parse(userId!),
type,
method,
encodedDataMessage: '', // TODO: remove, handled by sdk
encodedDataSignature: '' // TODO: remove, handled by sdk
method
})
return data ?? 0
},
Expand Down
20 changes: 0 additions & 20 deletions packages/discovery-provider/src/api/v1/helpers.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,7 +20,6 @@
SortMethod,
)
from src.queries.reactions import ReactionResponse
from src.utils.auth_middleware import MESSAGE_HEADER, SIGNATURE_HEADER
from src.utils.get_all_nodes import get_all_healthy_content_nodes_cached
from src.utils.helpers import decode_string_id, encode_int_id
from src.utils.redis_connection import get_redis
Expand Down Expand Up @@ -683,23 +682,6 @@ def __schema__(self):
return param


# Helper to allow consumer to pass message and signature headers as request params
def add_auth_headers_to_parser(parser, required=True):
parser.add_argument(
MESSAGE_HEADER,
required=required,
Copy link
Contributor

Choose a reason for hiding this comment

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

Feels like we should keep this but make them non-required, so that the REST API docs/swagger pick them up?

Copy link
Member Author

Choose a reason for hiding this comment

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

Revitalizing this PR. Yes this is is a good point, but I find it somewhat error prone to remember to call this helper in places, so instead I added this into @auth_middleware.

@schottra also adding you as a reviewer since this is relevant to your current work. Lmk if either of you have concerns.

The gist of this change is removing add_auth_headers_to_parser and having it picked up with the middleware

class SalesDownload(Resource):
    @ns.expect(sales_download_parser)
    @auth_middleware(sales_download_parser)
    def get(self, id, authed_user_id=None):
         pass

I dream for a day when we have one parser/response marshaller/endpoint per file...

Copy link
Contributor

Choose a reason for hiding this comment

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

Love this! It's unfortunate that we have to trade-off optional fields in the SDK with missing documentation on the REST/Swagger side, but I like where it's landed!

Copy link
Member Author

Choose a reason for hiding this comment

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

yeah ugh!

description="The data that was signed by the user for signature recovery",
location="headers",
)
parser.add_argument(
SIGNATURE_HEADER,
required=required,
description="The signature of data, used for signature recovery",
location="headers",
)
return parser


current_user_parser = reqparse.RequestParser(argument_class=DescriptiveArgument)
current_user_parser.add_argument(
"user_id", required=False, description="The user ID of the user making the request"
Expand Down Expand Up @@ -742,7 +724,6 @@ def add_auth_headers_to_parser(parser, required=True):
type=str,
choices=SortDirection._member_names_,
)
add_auth_headers_to_parser(track_history_parser, False)

user_favorited_tracks_parser = pagination_with_current_user_parser.copy()
user_favorited_tracks_parser.add_argument(
Expand Down Expand Up @@ -773,7 +754,6 @@ def add_auth_headers_to_parser(parser, required=True):
choices=LibraryFilterType._member_names_,
default=LibraryFilterType.favorite,
)
add_auth_headers_to_parser(user_tracks_library_parser)

user_collections_library_parser = user_tracks_library_parser.copy()
# Replace just the sort method args with the CollectionLibrarySortMethod version
Expand Down
17 changes: 6 additions & 11 deletions packages/discovery-provider/src/api/v1/transactions.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,6 @@
abort_bad_request_param,
abort_forbidden,
abort_unauthorized,
add_auth_headers_to_parser,
decode_with_abort,
extend_transaction_details,
make_full_response,
Expand Down Expand Up @@ -64,8 +63,6 @@
default=SortDirection.desc,
)

add_auth_headers_to_parser(transaction_history_parser)


@full_user_ns.route("/<string:id>/transactions/audio")
class GetTransactionHistory(Resource):
Expand All @@ -75,8 +72,8 @@ class GetTransactionHistory(Resource):
params={"id": "A User ID"},
)
@full_user_ns.expect(transaction_history_parser)
@auth_middleware(transaction_history_parser)
@full_user_ns.marshal_with(transaction_history_response)
@auth_middleware()
def get(self, id, authed_user_id=None):
user_id = decode_with_abort(id, full_user_ns)
return self._get(user_id, authed_user_id)
Expand Down Expand Up @@ -106,8 +103,8 @@ class LegacyGetTransactionHistory(GetTransactionHistory):
deprecated=True,
)
@full_ns.expect(transaction_history_parser)
@auth_middleware(transaction_history_parser)
@full_ns.marshal_with(transaction_history_response)
@auth_middleware()
def get(self, authed_user_id=None):
"""Gets the user's $AUDIO transaction history within the App

Expand All @@ -123,7 +120,6 @@ def get(self, authed_user_id=None):
transaction_history_count_parser = reqparse.RequestParser(
argument_class=DescriptiveArgument
)
add_auth_headers_to_parser(transaction_history_count_parser)


@full_user_ns.route("/<string:id>/transactions/audio/count")
Expand All @@ -134,8 +130,8 @@ class GetTransactionHistoryCount(Resource):
params={"id": "A User ID"},
)
@full_user_ns.expect(transaction_history_count_parser)
@auth_middleware(transaction_history_count_parser)
@full_user_ns.marshal_with(transaction_history_count_response)
@auth_middleware()
def get(self, id, authed_user_id=None):
user_id = decode_with_abort(id, full_ns)
if authed_user_id is None:
Expand All @@ -154,8 +150,8 @@ class LegacyGetTransactionHistoryCount(Resource):
deprecated=True,
)
@full_ns.expect(transaction_history_count_parser)
@auth_middleware(transaction_history_count_parser)
@full_ns.marshal_with(transaction_history_count_response)
@auth_middleware()
def get(self, authed_user_id=None):
"""Gets the count of the user's $AUDIO transaction history within the App.

Expand Down Expand Up @@ -208,8 +204,8 @@ class GetUSDCTransactionHistory(Resource):
params={"id": "A User ID"},
)
@full_user_ns.expect(usdc_transaction_history_parser)
@auth_middleware(usdc_transaction_history_parser)
@full_user_ns.marshal_with(transaction_history_response)
@auth_middleware()
def get(self, id, authed_user_id=None):
user_id = decode_with_abort(id, full_ns)
if authed_user_id is None:
Expand Down Expand Up @@ -238,7 +234,6 @@ def get(self, id, authed_user_id=None):
argument_class=DescriptiveArgument
)
add_transaction_history_filters(usdc_transaction_history_count_parser)
add_auth_headers_to_parser(usdc_transaction_history_count_parser)


@full_user_ns.route("/<string:id>/transactions/usdc/count")
Expand All @@ -249,8 +244,8 @@ class GetUSDCTransactionHistoryCount(Resource):
params={"id": "A User ID"},
)
@full_user_ns.expect(usdc_transaction_history_count_parser)
@auth_middleware(usdc_transaction_history_count_parser)
@full_user_ns.marshal_with(transaction_history_count_response)
@auth_middleware()
def get(self, id, authed_user_id=None):
user_id = decode_with_abort(id, full_ns)
if authed_user_id is None:
Expand Down
Loading