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

[SDK-2161] Allow to skip the redirect callback #86

Merged
merged 12 commits into from
Dec 2, 2020

Conversation

frederikprijck
Copy link
Member

When using multiple OAuth providers on the same page, it happens that another provider redirects back to a certain URL, including state and code parameters.

Currently, our SDK processed this and tries to retrieve a token. However, it should ignore these as they do not belong to Auth0.

@frederikprijck frederikprijck requested a review from a team as a code owner November 17, 2020 12:42
@frederikprijck frederikprijck added the review:small Small review label Nov 17, 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, just had a couple of questions.

@@ -17,6 +17,7 @@ import {
const authConfig: AuthConfig = {
clientId: 'wLSIP47wM39wKdDmOj6Zb5eSEw3JVhVp',
domain: 'brucke.auth0.com',
skipRedirectCallback: window.location.pathname === '/other-callback',
Copy link
Contributor

Choose a reason for hiding this comment

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

Do we need to have a route set up to handle this? How does this work in the playground from a user's perspective?

Copy link
Member Author

Choose a reason for hiding this comment

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

If you want to use this, yes you do.
I added it to the playground as a matter of showing how to configure it, but we do not realy integrate our playground with another OAuth provider tho.

Copy link
Contributor

Choose a reason for hiding this comment

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

In that case it might be better to instead show a usage example in the docs against the new property - I can imagine people will look there first before they dive into the playground code.

Do you think this also merits a section in the readme about usage with other providers?

Copy link
Member Author

@frederikprijck frederikprijck Nov 18, 2020

Choose a reason for hiding this comment

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

Yes I think that is a good idea and I updated the PR.

API Docs:
image

README:
image

Comment on lines 179 to 182
{
provide: AuthClientConfig,
useValue: authClientConfigSpy,
},
Copy link
Contributor

Choose a reason for hiding this comment

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

Thinking about our other conversations we've had about mocking less, do we need to use a Spy here or can we just give it an an instance of AuthClientConfig with the right config?

Copy link
Member Author

Choose a reason for hiding this comment

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

As the config is different between tests, mocking it makes it a bit easier I think.

@@ -311,7 +312,7 @@ export class MyComponent {
}
```

## Dynamic Configuration
### Dynamic Configuration
Copy link
Member Author

Choose a reason for hiding this comment

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

It looks like this header was wrong, updated it to be the same level of heading as the other titles.

stevehobbsdev
stevehobbsdev previously approved these changes Nov 18, 2020
@frederikprijck
Copy link
Member Author

Updated the PR to avoid mocking the config.

stevehobbsdev
stevehobbsdev previously approved these changes Nov 18, 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, all minor suggestions.

README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
projects/auth0-angular/src/lib/auth.config.ts Outdated Show resolved Hide resolved
projects/auth0-angular/src/lib/auth.service.spec.ts Outdated Show resolved Hide resolved
@frederikprijck frederikprijck merged commit a5a7a30 into master Dec 2, 2020
@frederikprijck frederikprijck deleted the frederik/sdk-2161 branch December 2, 2020 17:02
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CH: Added PR is adding feature or functionality review:small Small review
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants