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

remove query string from redirect_uri when comparing #336

Closed
wants to merge 1 commit into from

Conversation

timfeirg
Copy link

The request.redirect_uri could be a url, with query string components, comparison with grant.redirect_uri could fail even if the URI parts of two are actually the same.

I may be misunderstanding oauth2 here, I knew so little about it.

@lepture

@timfeirg
Copy link
Author

timfeirg commented May 28, 2017

I tried to add some tests for this change, but using make test inside the project directory results in such error:

OperationalError: (sqlite3.OperationalError) no such table: user [SQL: u'SELECT user.id AS user_id, user.username AS user_username \nFROM user \nWHERE user.id = ?'] [parameters: (1,)]

But I did see some db.create_alls inside the tests, not sure what's going on here.

I'll finish the tests if this PR is approved.

@timfeirg
Copy link
Author

你好啊伟大的开发者 @lepture
我来询问下这个 PR 里我做的对不对..
对的话我去研究下测试怎么搞

@lepture
Copy link
Owner

lepture commented Jun 1, 2017

It will compare redirect_uri and the one in grant token strictly, you can
add a validate_redirect_uri function on grant for a customized validation.

@lepture
Copy link
Owner

lepture commented Jun 1, 2017

The function is designed to compare strictly, if what you need is a flexible compare function, define it in your own service.

@timfeirg
Copy link
Author

timfeirg commented Jun 1, 2017

But I couldn't help thinking that, redirect_uri, by definition, does not include the query string components, and that the current implementation is unnecessarily too strict.

@timfeirg timfeirg closed this Jun 1, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
2 participants