[Anchor-418] Support auth with SEP10 in SEP38 #1130
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
philipliu
reviewed
Sep 29, 2023
| fun testAll() { | ||
| println("Performing SEP38 tests...") | ||
| `test sep38 info, price and prices endpoints`() | ||
| `test selling over asset limit throws an exception`() | ||
| `test endpoints does not required valid token when auth is disabled`() |
There was a problem hiding this comment.
Can we also add tests for when authentication is enabled?
| @@ -451,6 +451,8 @@ sep38: | |||
| # Whether to enable SEP-38 | |||
| # | |||
| enabled: false | |||
| # Whether to require SEP-10 authentication for SEP-38 requests. | |||
| requires_sep10: false | |||
There was a problem hiding this comment.
SEP-38 has other endpoints that require authentication such as POST /quote. Should we change the name of this flag or schema (nesting it under sep38.endpoints.info.authentication_required, for example) so that this is clear?
lijamie98
approved these changes
Oct 2, 2023
| @@ -451,6 +451,8 @@ sep38: | |||
| # Whether to enable SEP-38 | |||
| # | |||
| enabled: false | |||
| # Whether to require SEP-10 authentication for SEP-38 requests. | |||
| requires_sep10: false | |||
There was a problem hiding this comment.
I feel requires_sep10 is vague because some of the endpoints already requires sep10.
How about:
Suggested change
| requires_sep10: false | |
| enforce_sep10: false |
sep38.endpoints.info.authentication_required seems a bit over engineering.
|
|
||
| init { | ||
| sep38Client = Sep38Client(toml.getString("ANCHOR_QUOTE_SERVER"), jwt) | ||
| sep38ClientWithoutJwt = Sep38Client(toml.getString("ANCHOR_QUOTE_SERVER"), "Invalid Token") |
There was a problem hiding this comment.
This is only used in test endpoints does not required valid token when auth is disabled(). It's better to use local var.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Description
Add sep38.requires_sep10 configuration with default value to FALSE
Context
Currently, according to the SEP-38 spec, some of the SEP38 endpoints have SEP-10 as optional. This task seeks ensure we support this optionality by add configurability to these endpoints' SEP-10 requirements.
Testing
Tests were added to verify url pattern was added to filter if auth is required