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

Use user provided params on silent login #318

Merged
merged 4 commits into from
Jan 24, 2020

Conversation

nkete
Copy link
Contributor

@nkete nkete commented Dec 19, 2019

Description

This PR adds support for custom query parameters in getTokenSilently similar to how loginWithRedirect does this.

References

Testing

Add a custom property to the getTokenSilently method call and inspect the generated url.

auth0.getTokenSilently({
    scope,
    audience,
    foo: 'bar',
});

I was looking at integration tests I noticed that getTokenSilently > Builds URL correctly test is skipped. Can someone provide some context on why is that? Should the test be updated to reflect custom parameters as well?

Checklist

  • All active GitHub checks for tests, formatting, and security are passing

@stevehobbsdev
Copy link
Contributor

@nkete Thanks for raising this. Before we bring it in, let me get a look at that integration test which is being skipped and get back to you. You're right, we'll want to update that with your changes if we can.

@stevehobbsdev stevehobbsdev requested review from stevehobbsdev and removed request for a team January 7, 2020 11:05
@stevehobbsdev stevehobbsdev added CH: Fixed PR is fixing a bug review:tiny labels Jan 7, 2020
@stevehobbsdev
Copy link
Contributor

stevehobbsdev commented Jan 10, 2020

@nkete I'm doing some other work on this SDK and in there I've ended up removing the integration test that is being skipped. It was being skipped because it just doesn't work, so I've removed it and taken a different approach. However, it's only available in another branch so I would ignore this test for now.

However, we would require some unit test coverage around this new functionality you've provided - could you please add those tests to the PR?

@nkete
Copy link
Contributor Author

nkete commented Jan 10, 2020

@stevehobbsdev sure, I'll get back to you once I add the tests.

@stevehobbsdev
Copy link
Contributor

Thanks @nkete. To clarify above, when I mentioned "test coverage" I was referring to unit tests. I'd think they'd be enough for this particular case.

@nkete
Copy link
Contributor Author

nkete commented Jan 10, 2020

@stevehobbsdev I added a test for custom params to the getTokenSilently section, please take a look if that'll do. Also rebased against latest master.

@stevehobbsdev stevehobbsdev added this to the vNext milestone Jan 15, 2020
@stevehobbsdev stevehobbsdev added CH: Changed PR is changing something CH: Added PR is adding feature or functionality CH: Fixed PR is fixing a bug and removed CH: Fixed PR is fixing a bug CH: Changed PR is changing something CH: Added PR is adding feature or functionality labels Jan 15, 2020
stevehobbsdev
stevehobbsdev previously approved these changes Jan 22, 2020
Copy link
Contributor

@stevehobbsdev stevehobbsdev 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, thanks. I've scheduled it for release, just waiting on a couple of other things coming in before that happens. Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CH: Fixed PR is fixing a bug
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants