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

bug fixed #47

Merged
merged 4 commits into from
Jan 14, 2019
Merged

bug fixed #47

merged 4 commits into from
Jan 14, 2019

Conversation

PengfeiLi0218
Copy link
Contributor

bug: Mismatch between validRedirectURL and requestedRedirectURL

Copy link
Contributor

@chrisjsimpson chrisjsimpson left a comment

Choose a reason for hiding this comment

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

I would advise caution on this for the following reasons:

It will break existing apitester deployments which expect the /obp route to exist
Since it's inception the apitester has used the route '/obp' for the return url, any other developers (and including documentation being made) will expect this to still be the case
It's not clear what bug this commit is fixing (I assume it's related to oauth debugging internally) but the effect of this commit is probably unintentional.
I propose there should be consistency on which route oauth apps, be recommending which route apps should delegate for their oauth returl url.
@PengfeiLi0218 @simonredfern

urlpatterns = [
url(r'^$', HomeView.as_view(), name="home"),
url(r'^obp/', include('obp.urls')),
Copy link
Contributor

Choose a reason for hiding this comment

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

I would advise caution on this for the following reasons:

  • It will break existing apitester deployments which expect the /obp route to exist
  • Since it's inception the apitester has used the route '/obp' for the return url, any other developers (and including documentation being made) will expect this to still be the case
  • It's not clear what bug this commit is fixing (I assume it's related to oauth debugging internally) but the effect of this commit is probably unintentional.
  • I propose there should be consistency on which route oauth apps, be recommending which route apps should delegate for their oauth returl url.

@PengfeiLi0218 @simonredfern

Copy link
Contributor

Choose a reason for hiding this comment

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

Yeah we'd have to update API Tester configurations on all sandboxes once this gets deployed. Tedious, but IMHO increasing consistency, as none of our other apps need additions to the redirect URL.

@chrisjsimpson
Copy link
Contributor

Thanks @PengfeiLi0218 @simonredfern

Please could alias the /obp route so that it still works for older deployments.

A separate issue (#48 ) has been created to RECOMMEND app developers use a standard route for their oauth return urls (I suggest /oauth_redirect and then we will put this into the development docs..

@simonredfern simonredfern merged commit f9e5350 into OpenBankProject:master Jan 14, 2019
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.

4 participants