REST API support #199
REST API support #199
Conversation
* By default ```APP_ALLOWED_HOSTS``` is None, which allows all hosts. Signed-off-by: Harris Tzovanakis <me@drjova.com>
Signed-off-by: Harris Tzovanakis <me@drjova.com>
|
Copy of the accidentally merged #197 |
|
Need to resolve this #201 so tests can pass! |
There was a problem hiding this comment.
Hey, I've been through the PR now. I see two major issues that would need to be addressed.
- Code duplication: There's a lot of code that's duplicated. This is ok for smaller amounts, but it's at the handlers, views, testing and contrib level, hence this can potentially have a large impact on fixing bugs in the future. And with OAuthClient having a history of hard to find bugs, I think it's important that we ensure that bugs get's fixed once, and not have to remember to fix also the same bug in another file.
- Safe redirect target: I think there's a security issue with the changes made.
| @@ -0,0 +1,157 @@ | |||
| # -*- coding: utf-8 -*- | |||
There was a problem hiding this comment.
Comment: Was it not possible to use somehow the existing example application for the REST APIs so that we don't have to copy/paste all the code?
|
|
||
| from invenio_oauthclient.contrib import cern | ||
|
|
||
| OAUTH_REMOTE_APP = cern.REMOTE_REST_APP |
There was a problem hiding this comment.
OAUTH_REMOTE_APP is just a reference to cern.REMOTE_REST_APP thus, changing OAUTH_REMOTE_APP ['...'] = ... will also change cern.REMOTE_REST_APP['...'].
The example here seems to either be missing:
import copy
OAUTH_REMOTE_APP = copy.deepcopy(cern.REMOTE_REST_APP)or, just edit CERN.REMOTE_REST_APP instead of making a new variable.
| params=dict( | ||
| base_url='https://oauth.web.cern.ch/', | ||
| request_token_url=None, | ||
| access_token_url='https://oauth.web.cern.ch/OAuth/Token', |
There was a problem hiding this comment.
Question: Is this the new or the old SSO?
| @@ -159,6 +219,44 @@ | |||
| ) | |||
| """CERN Remote Application.""" | |||
|
|
|||
|
|
|||
| REMOTE_REST_APP = dict( | |||
There was a problem hiding this comment.
Minor: There's a high degree of similarity between the REMOTE_APP and REMOTE_REST_APP. Can we not make a base config with all the common fields (ditto for all the other providers as well).
_BASE_APP = dict(
# ...
)
REMOTE_APP = dict(_BASE_APP)
REMOTE_APP.update(dict(
# ...
))
REMOTE_REST_APP = dict(_BASE_APP)
REMOTE_REST_APP.update(dict(
# ...
))| @@ -99,7 +100,7 @@ | |||
| 'show_login': 'true'}, | |||
| base_url='https://pub.orcid.org/v1.2/', | |||
| request_token_url=None, | |||
| access_token_url='https://pub.orcid.org/oauth/token', | |||
| access_token_url="https://api.orcid.org/oauth/token", | |||
There was a problem hiding this comment.
Comment: Just checking, is this intended in the UI app? (just resolve comment if it is).
|
|
||
| """Handlers for customizing oauthclient endpoints.""" | ||
|
|
||
| from __future__ import absolute_import, print_function |
There was a problem hiding this comment.
Comment: These are no longer necessary due to Python 2 end of life (feel free to leave them).
| @@ -0,0 +1,393 @@ | |||
| # -*- coding: utf-8 -*- | |||
There was a problem hiding this comment.
Major: There's a very high code overlap between rest.py and ui.py. The differences, as far as I can tell, are mainly the HTTP responses. The code in the handlers are very touchy, in that bugs can easily sneak in, and a bug can have a high impact (e.g. unauthorized access). In my opinion, this has to be refactored, so there's a clear business logic side and only the responses differs. I'm happy to take a chat on how best to achieve this.
| @@ -105,11 +126,7 @@ def oauth_register(form): | |||
| """ | |||
| if form.validate(): | |||
| data = form.to_dict() | |||
| if not data.get('password'): | |||
There was a problem hiding this comment.
Comment: What's the reason for removing this? Was there a bug?
There was a problem hiding this comment.
| for target in request.args.get(arg), request.referrer: | ||
| if target: | ||
| redirect_uri = urisplit(target) | ||
| allowed_hosts = current_app.config.get('APP_ALLOWED_HOSTS', []) | ||
| if allowed_hosts is None: |
There was a problem hiding this comment.
Major: APP_ALLOWED_HOSTS is an optional security feature, so it's not required to be set for a secure operation of Invenio. What happens now is that if it's not set, then the check of the redirect target is completely skipped.
I think the previous behaviour should be restored:
If a redirect target has a host (i.e. redirect_uri.host), then it should be checked that it exists in allowed hosts. If allowed hosts is not set, then we strip off the host part, and we only rely on the path part to redirect. This ensures that when an host is used in a redirect target, it's only from an allowed set of hosts. Otherwise, now, you can basically inject a malicious redirect target.
| @@ -0,0 +1,161 @@ | |||
| # -*- coding: utf-8 -*- | |||
There was a problem hiding this comment.
Ditto here, regarding code duplication.
lnielsen
moved this from Backlog
to Todo: v3.3 release
in Sprint Week 19-20 - Invenio v3.3
|
For the upcoming sprint, see my comments. Please ping me, once ready to have it rereviewed. |
ppanero
moved this from Todo: v3.3 Beta 1
to In progress
in Sprint Week 19-20 - Invenio v3.3
|
Deprecated by #205 |
zzacharo
moved this from In progress
to Pending Release
in Sprint Week 19-20 - Invenio v3.3
zzacharo
moved this from Pending Release
to Blocked
in Sprint Week 19-20 - Invenio v3.3
|
Closed in favour of #205 |
No description provided.