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.

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

Support for passing a wildcard of extra params in an authRequest #569

Merged
merged 3 commits into from
Dec 20, 2018

Conversation

hstove
Copy link
Contributor

@hstove hstove commented Nov 30, 2018

This PR adds one new parameter to makeAuthRequest called extraParams.

This parameter is essentially a "wildcard" to allow passing extra parameters in the auth request. This allows apps to pass in parameters that might not be part of the standard Blockstack auth protocol, but may be supported by various authenticators.

The immediate need for this is to support #568 , which requires a developer to be able to pass a few new 'beta' parameters to the Browser, which will trigger a modified onboarding flow that prompts the user to specify their Gaia hub.

I opted for this wildcard approach instead of hard-coded parameters, because this supports a future with multiple authenticators, each that might allow for a more customized onboarding flow.

Hypothetically, you might imagine one authenticator allowing parameters that change the styling and display of the onboarding flow to match the style of an app.

@hstove hstove self-assigned this Nov 30, 2018
@hstove hstove requested review from kantai and yknl November 30, 2018 02:55
Copy link
Collaborator

@kantai kantai left a comment

Choose a reason for hiding this comment

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

This looks good to me, @hstove

@yknl yknl changed the base branch from master to develop November 30, 2018 18:26
Copy link
Contributor

@yknl yknl 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 👍

@hstove
Copy link
Contributor Author

hstove commented Nov 30, 2018

Thanks for the quick reviews! Good to merge?

Who should now be in charge of deploying new releases to NPM? @yknl ?

@yknl
Copy link
Contributor

yknl commented Nov 30, 2018

I think it's good to merge. I know @larrysalibra is working on the v19 release and this can probably be included.

@markmhendrickson
Copy link

@larrysalibra Could we get this merged by Monday? We're hoping to soft-launch Gaia hub functionality that depends on this then per this forum thread.

@larrysalibra larrysalibra self-requested a review December 7, 2018 13:08
Copy link
Contributor

@larrysalibra larrysalibra left a comment

Choose a reason for hiding this comment

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

I really like making the auth request spec extensible so that other implementors of authenticators can support custom functionality. Great call @hstove !

We've had problems in the past with authentication request tokens being too long and breaking things in other ways. In view of that, I'd propose that extra params should have a length limit so that developers don't add arbitrarirly large amounts of data and expect to keep working.

Given that this is a change in the authentication request token format, it should also bump VERSION and have tests to make sure this new authentication request token format works with previous versions. There should also be new/updated tests that test this functionality with the logic that generates a response token. It also needs a change log entry.

For an example of testing authentication flow changes, take a look at this pull request:
#503 (comment)

I'm taking a look at this at this because @markmhx tagged me indicating that this is a time sensitive release.

I'd typically request that these changes be addressed before merging, however, @yknl is the new maintainer of this library, so I defer that decision to him!

@markmhendrickson
Copy link

@larrysalibra thanks!

@markmhendrickson
Copy link

It appears that @larrysalibra is suggesting three changes in total before we merge:

  • Set limit for extra param lengths
  • Bump the version
  • Add tests for new authentication request token format against previous versions

@yknl do you feel these need to be resolved before we merge or can they be added as issues for resolution afterwards?

@hstove can you create issues for these regardless so we can start tracking them?

@hstove
Copy link
Contributor Author

hstove commented Dec 20, 2018

I made #586 to track the possibility of limiting the length of the authRequest. I'm not sure I have the information necessary to implement this right now, and I'd have to take more time to test out the impact that these new params could have, and investigate limits for different browsers.

Otherwise, I've added tests regarding the new authRequest, and bumped the changelog. I have not bumped the version in package.json, I wasn't sure if @yknl you wanted to do that when merging and pushing to NPM.

@yknl
Copy link
Contributor

yknl commented Dec 20, 2018

Thanks @hstove, this looks good to go. Going to merge and publish.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants