Skip to content
This repository has been archived by the owner on May 24, 2024. It is now read-only.

SEP-10 3.0 #285

Merged
merged 7 commits into from
Nov 10, 2020
Merged

SEP-10 3.0 #285

merged 7 commits into from
Nov 10, 2020

Conversation

Kirbyrawr
Copy link
Collaborator

Added homeDomain check in WebAuthentication.cs

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)
  • I have read the CONTRIBUTING document.
  • My code follows the code style of this project.
  • My change requires a change to the documentation.
  • I have updated the documentation accordingly.
  • I have added tests to cover my changes.
  • All new and existing tests passed.

Added homeDomain check in WebAuthentication.cs
@coveralls
Copy link

coveralls commented Nov 9, 2020

Coverage Status

Coverage decreased (-0.005%) to 88.753% when pulling d73a896 on sep-10-3.0 into afe345b on master.

@Kirbyrawr Kirbyrawr linked an issue Nov 9, 2020 that may be closed by this pull request
Copy link
Contributor

@leighmcculloch leighmcculloch left a comment

Choose a reason for hiding this comment

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

🎉 I think this looks great. I left one suggestion (💡) that I don't think is required to support SEP-10 v3.0.0, but may be relevant depending on the use cases the SDK wishes to support.

@@ -145,6 +145,9 @@ public static class WebAuthentication
if (operation.SourceAccount is null)
throw new InvalidWebAuthenticationException("Challenge transaction operation must have source account");

if (operation.Name.Split(' ')[0] != homeDomain)
Copy link
Contributor

Choose a reason for hiding this comment

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

💡 Clients will only ever read a transaction expecting a single home domain, but a server may wish to support multiple home domains and if the SDK wishes to support that use case the list of home domains passed into it maybe should be a collection. That's really up to you though for if that is an important use case to this SDK.

Copy link
Collaborator Author

@Kirbyrawr Kirbyrawr Nov 10, 2020

Choose a reason for hiding this comment

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

Hi, this is fixed now, thank you!

Copy link
Contributor

Choose a reason for hiding this comment

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

Nice! Looks great!

Copy link
Collaborator

@fracek fracek left a comment

Choose a reason for hiding this comment

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

Looks good to me. Thanks for this.

@fracek fracek merged commit c07fbd0 into master Nov 10, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

SEP-10 v3.0 Changes (from the SDF)
4 participants