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

OAuth2 support allowing public and user access #2099

Closed
wants to merge 19 commits into from

Conversation

wasade
Copy link
Contributor

@wasade wasade commented Apr 4, 2017

This was partially pair programmed with @ElDeveloper.

The goal of this PR is to lay the ground work for allowing handlers to optionally allow defaulting to a public user, and to also make the User available to the handler so that access control can be performed if necessary.

What has been done so far is:

  • added a means to associate a oauth2 client_id with a User
  • added a means to instantiate a User from a client_id
  • added tests for these User modifications
  • decomposed the oauth2 handling logic
  • added explicit tests for the oauth2 handling logic using a mocked out handler
  • creation of a parameterized oauth2 decorator
  • added tests for the parameterized oauth2 decorator using a mocked out handler
  • replacement of the original authenticate_oauth decorator across all of Qiita

The way the new decorator works is to allow, per method, a means to describe whether it can default to access if a) a valid token is provided or b) if the Authorization header is not provided. If a oauth2 token is provided, and it is invalid, rate limited, etc, the decorator will error as expected. This approach requires the developer indicate explicitly that a method can default to public, and that statement is proximal to the function signature. Second, and per method as well, the User object associated with the oauth2 client_id can be made available by monkey patching get_current_user. As of right now, the public user is demo@microbio.me. This monkey patching is parameterized as the original authenticate_oauth decorator did not do this, so it was added to minimize possible side effects from replacing the authentication mechanism.

@coveralls
Copy link

Coverage Status

Coverage increased (+0.03%) to 91.974% when pulling b839bfe on wasade:death-april into c309aa6 on biocore:master.

@ElDeveloper
Copy link
Member

👍 looks good to me!!

Copy link
Member

@antgonza antgonza left a comment

Choose a reason for hiding this comment

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

Code looks fine except:

  • for a couple of tests missing
  • I think it's worth deploying in the test env and testing there
  • this will not really change anything in the front end, right? What will be missing to finish it?
  • I think is worth removing the demo@microbio.me default
  • it's not clear for me how this generated the client_id for existing users

# no error, or no authorization header. We should error if
# oauth is actually attempted but there was an auth issue
# (e.g., rate limit hit)
if errtype not in (None, 'invalid_request'):
Copy link
Member

Choose a reason for hiding this comment

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

test?

return
if self.inject_user:
if cid is None:
Copy link
Member

Choose a reason for hiding this comment

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

test?

@wasade
Copy link
Contributor Author

wasade commented Apr 5, 2017

Thanks @antgonza! Regarding your comments,

for a couple of tests missing

Here, and one is not possible to reach as noted in the code so I'm not sure how to test.

I think it's worth deploying in the test env and testing there

sounds good

this will not really change anything in the front end, right? What will be missing to finish it?

It was not intended to change the front end, this was to allow for per-user support against the rest API.

I think is worth removing the demo@microbio.me default

@josenavas suggested this. We're fine with anything but it does need to be a valid qiita account.

it's not clear for me how this generated the client_id for existing users

We have not touched the existing code which already does this. What is missing though is a UI to generate per-user API tokens, which effectively needs to call the existing logic, and then issue a call to User(foo).client_id = bar.

@wasade
Copy link
Contributor Author

wasade commented Apr 5, 2017 via email

@coveralls
Copy link

Coverage Status

Coverage increased (+0.04%) to 91.992% when pulling 15211de on wasade:death-april into c309aa6 on biocore:master.

@josenavas josenavas changed the base branch from master to dev September 15, 2017 18:13
@josenavas josenavas closed this Nov 1, 2017
@josenavas josenavas changed the base branch from dev to master November 1, 2017 14:33
@josenavas josenavas mentioned this pull request Nov 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
Development

Successfully merging this pull request may close these issues.

None yet

5 participants