Issue 7326049: Adding server side OAuth 2.0 support. (Closed)
|
Created:
13 years, 3 months ago by dhermes Modified:
13 years, 3 months ago CC:
codereview-list_googlegroups.com Visibility:
Public. |
DescriptionAdding server side OAuth 2.0 support.
Patch Set 1 #
Total comments: 5
Patch Set 2 : Addressing review comments. #Patch Set 3 : Fixing broken test after moving RIETVELD_CLIENT_ID to settings. #
MessagesTotal messages: 16
A first step towards fixing http://code.google.com/p/rietveld/issues/detail?id=385 We also need the client to support OAuth 2.0 before we can fully close it out. https://codereview.appspot.com/7326049/diff/1/codereview/auth_utils.py File codereview/auth_utils.py (right): https://codereview.appspot.com/7326049/diff/1/codereview/auth_utils.py#newcode53 codereview/auth_utils.py:53: RIETVELD_CLIENT_ID = 'dummy29.apps.googleusercontent.com' This value is nonsense and I'd like to discuss it's future value with one of the maintainers.
Sign in to reply to this message.
BUMP
Sign in to reply to this message.
On 2013/02/20 05:18:39, dhermes wrote: > BUMP Sorry for the delay! I'll have a look this weekend.
Sign in to reply to this message.
Great news! Thanks Andi. On Thu, Feb 21, 2013 at 8:17 PM, <albrecht.andi@gmail.com> wrote: > On 2013/02/20 05:18:39, dhermes wrote: > >> BUMP >> > > Sorry for the delay! I'll have a look this weekend. > > https://codereview.appspot.**com/7326049/<https://codereview.appspot.com/7326... > -- Danny Hermes Developer Programs Engineer
Sign in to reply to this message.
LGTM in general, but it would be great if someone with more knowledge with OAuth could have a look. I like the abstraction in auth_utils. Do I understand it correctly that OAuth users and regular users are matched by their email address? https://codereview.appspot.com/7326049/diff/1/codereview/auth_utils.py File codereview/auth_utils.py (right): https://codereview.appspot.com/7326049/diff/1/codereview/auth_utils.py#newcode53 codereview/auth_utils.py:53: RIETVELD_CLIENT_ID = 'dummy29.apps.googleusercontent.com' On 2013/02/15 22:10:53, dhermes wrote: > This value is nonsense and I'd like to discuss it's future value with one of the > maintainers. I'm not very familiar with OAuth in general, but to me it seems that the value must be unique for each instance of Rietveld. A good place to configure this would be settings.py. Why is it "*.apps.googleusercontent.com"? I'd expected something like "codereview.appspot.com" (but as noted, I'm really not very familiar with that OAuth stuff). https://codereview.appspot.com/7326049/diff/1/tests/run_tests.py File tests/run_tests.py (right): https://codereview.appspot.com/7326049/diff/1/tests/run_tests.py#newcode69 tests/run_tests.py:69: sdk_path = '/Applications/GoogleAppEngineLauncher.app/Contents/Resources/GoogleAppEngine-default.bundle/Contents/Resources/google_appengine' Please don't change this, or if this is a common used path please add a something that chooses between both paths.
Sign in to reply to this message.
PTAL RE: Do I understand it correctly that OAuth users and regular users are matched by their email address? Yes. As you may have noticed in auth_utils, we are requiring that *only* the e-mail scope is associated with the OAuth 2.0 tokens sent, since we don't have our own scope and are using the Client ID as a proxy for our own scope (as is done in Cloud Endpoints). The OAuth API in App Engine only works if at least the e-mail scope is associated with the token in the current environ (via the env var HTTP_AUTHORIZATION). When this is the case, the current user returned can contain the e-mail since the user has granted that token the ability to retrieve the associated email. https://codereview.appspot.com/7326049/diff/1/codereview/auth_utils.py File codereview/auth_utils.py (right): https://codereview.appspot.com/7326049/diff/1/codereview/auth_utils.py#newcode53 codereview/auth_utils.py:53: RIETVELD_CLIENT_ID = 'dummy29.apps.googleusercontent.com' I am pretty familiar with OAuth and helped implement https://code.google.com/p/googleappengine/source/browse/trunk/python/google/a... for the new App Engine feature "Google Cloud Endpoints". I wasn't aware of settings.py, and will put this there. The value is created in a Google APIs Console (https://code.google.com/apis/console/) project. Each project get's a unique ID (e.g. the project associated with Google's APIs Explorer is 292824132082) and the corresponding client ID is {$PROJECT_ID}.project.googleusercontent.com. We actually need to create a project and add the appropriate people to it and then replace this value with the actual Client ID associated with that project. I'm happy to do this, if you tell me who needs to be added to it. If you don't want me listed as an owner on the project, I can show you how to do it. I'd be happy to jump on a video chat to explain some of this stuff. Particularly about the next step, which involves distributing the client ID *AND* client secret in upload.py. On 2013/02/24 09:20:06, Andi wrote: > On 2013/02/15 22:10:53, dhermes wrote: > > This value is nonsense and I'd like to discuss it's future value with one of > the > > maintainers. > > I'm not very familiar with OAuth in general, but to me it seems that the value > must be unique for each instance of Rietveld. A good place to configure this > would be settings.py. > > Why is it "*.apps.googleusercontent.com"? I'd expected something like > "codereview.appspot.com" (but as noted, I'm really not very familiar with that > OAuth stuff). https://codereview.appspot.com/7326049/diff/1/tests/run_tests.py File tests/run_tests.py (right): https://codereview.appspot.com/7326049/diff/1/tests/run_tests.py#newcode69 tests/run_tests.py:69: sdk_path = '/Applications/GoogleAppEngineLauncher.app/Contents/Resources/GoogleAppEngine-default.bundle/Contents/Resources/google_appengine' That was an accident and I didn't mean to upload this diff, just patched it so I could run the tests on my Mac without moving stuff around :) On 2013/02/24 09:20:06, Andi wrote: > Please don't change this, or if this is a common used path please add a > something that chooses between both paths.
Sign in to reply to this message.
Hi, On Sun, Feb 24, 2013 at 7:09 PM, <dhermes@google.com> wrote: > PTAL > > RE: Do I understand it correctly that OAuth users and regular users are > > matched by their email address? > > Yes. As you may have noticed in auth_utils, we are requiring that *only* > the e-mail scope is associated with the OAuth 2.0 tokens sent, since we > don't have our own scope and are using the Client ID as a proxy for our > own scope (as is done in Cloud Endpoints). > > The OAuth API in App Engine only works if at least the e-mail scope is > associated with the token in the current environ (via the env var > HTTP_AUTHORIZATION). When this is the case, the current user returned > can contain the e-mail since the user has granted that token the ability > to retrieve the associated email. ah, ok. Thanks! It's getting clearer to me now. > > > > https://codereview.appspot.com/7326049/diff/1/codereview/auth_utils.py > File codereview/auth_utils.py (right): > > https://codereview.appspot.com/7326049/diff/1/codereview/auth_utils.py#newcode53 > codereview/auth_utils.py:53: RIETVELD_CLIENT_ID = > 'dummy29.apps.googleusercontent.com' > I am pretty familiar with OAuth and helped implement > > https://code.google.com/p/googleappengine/source/browse/trunk/python/google/a... > > for the new App Engine feature "Google Cloud Endpoints". > > I wasn't aware of settings.py, and will put this there. > > The value is created in a Google APIs Console > (https://code.google.com/apis/console/) project. Each project get's a > unique ID (e.g. the project associated with Google's APIs Explorer is > 292824132082) and the corresponding client ID is > {$PROJECT_ID}.project.googleusercontent.com. > > We actually need to create a project and add the appropriate people to > it and then replace this value with the actual Client ID associated with > that project. I'm happy to do this, if you tell me who needs to be added > to it. If you don't want me listed as an owner on the project, I can > show you how to do it. > > I'd be happy to jump on a video chat to explain some of this stuff. > Particularly about the next step, which involves distributing the client > ID *AND* client secret in upload.py. A video chat to discuss the details sounds good. What about next monday? IMO we can commit this change and make it live to make sure that we don't run in any troubles. > > > On 2013/02/24 09:20:06, Andi wrote: >> >> On 2013/02/15 22:10:53, dhermes wrote: >> > This value is nonsense and I'd like to discuss it's future value > > with one of >> >> the >> > maintainers. > > >> I'm not very familiar with OAuth in general, but to me it seems that > > the value >> >> must be unique for each instance of Rietveld. A good place to > > configure this >> >> would be settings.py. > > >> Why is it "*.apps.googleusercontent.com"? I'd expected something like >> "codereview.appspot.com" (but as noted, I'm really not very familiar > > with that >> >> OAuth stuff). > > > https://codereview.appspot.com/7326049/diff/1/tests/run_tests.py > File tests/run_tests.py (right): > > https://codereview.appspot.com/7326049/diff/1/tests/run_tests.py#newcode69 > tests/run_tests.py:69: sdk_path = > '/Applications/GoogleAppEngineLauncher.app/Contents/Resources/GoogleAppEngine-default.bundle/Contents/Resources/google_appengine' > That was an accident and I didn't mean to upload this diff, just patched > it so I could run the tests on my Mac without moving stuff around :) > > > On 2013/02/24 09:20:06, Andi wrote: >> >> Please don't change this, or if this is a common used path please add > > a >> >> something that chooses between both paths. > > > https://codereview.appspot.com/7326049/
Sign in to reply to this message.
SGTM I added you to a GCal event with a Hangout link attached: https://plus.google.com/hangouts/_/6867f2c022ca2ddb212d97d7342bbe261b9ec51e You have edit privileges on the event, so you can change the time. It's currently set for 8am Pacific, 5pm in Bonn. On Thu, Feb 28, 2013 at 6:51 AM, Andi Albrecht <albrecht.andi@gmail.com>wrote: > Hi, > > On Sun, Feb 24, 2013 at 7:09 PM, <dhermes@google.com> wrote: > > PTAL > > > > RE: Do I understand it correctly that OAuth users and regular users are > > > > matched by their email address? > > > > Yes. As you may have noticed in auth_utils, we are requiring that *only* > > the e-mail scope is associated with the OAuth 2.0 tokens sent, since we > > don't have our own scope and are using the Client ID as a proxy for our > > own scope (as is done in Cloud Endpoints). > > > > The OAuth API in App Engine only works if at least the e-mail scope is > > associated with the token in the current environ (via the env var > > HTTP_AUTHORIZATION). When this is the case, the current user returned > > can contain the e-mail since the user has granted that token the ability > > to retrieve the associated email. > > ah, ok. Thanks! It's getting clearer to me now. > > > > > > > > > https://codereview.appspot.com/7326049/diff/1/codereview/auth_utils.py > > File codereview/auth_utils.py (right): > > > > > https://codereview.appspot.com/7326049/diff/1/codereview/auth_utils.py#newcode53 > > codereview/auth_utils.py:53: RIETVELD_CLIENT_ID = > > 'dummy29.apps.googleusercontent.com' > > I am pretty familiar with OAuth and helped implement > > > > > https://code.google.com/p/googleappengine/source/browse/trunk/python/google/a... > > > > for the new App Engine feature "Google Cloud Endpoints". > > > > I wasn't aware of settings.py, and will put this there. > > > > The value is created in a Google APIs Console > > (https://code.google.com/apis/console/) project. Each project get's a > > unique ID (e.g. the project associated with Google's APIs Explorer is > > 292824132082) and the corresponding client ID is > > {$PROJECT_ID}.project.googleusercontent.com. > > > > We actually need to create a project and add the appropriate people to > > it and then replace this value with the actual Client ID associated with > > that project. I'm happy to do this, if you tell me who needs to be added > > to it. If you don't want me listed as an owner on the project, I can > > show you how to do it. > > > > I'd be happy to jump on a video chat to explain some of this stuff. > > Particularly about the next step, which involves distributing the client > > ID *AND* client secret in upload.py. > > A video chat to discuss the details sounds good. What about next monday? > > IMO we can commit this change and make it live to make sure that we > don't run in any troubles. > > > > > > > On 2013/02/24 09:20:06, Andi wrote: > >> > >> On 2013/02/15 22:10:53, dhermes wrote: > >> > This value is nonsense and I'd like to discuss it's future value > > > > with one of > >> > >> the > >> > maintainers. > > > > > >> I'm not very familiar with OAuth in general, but to me it seems that > > > > the value > >> > >> must be unique for each instance of Rietveld. A good place to > > > > configure this > >> > >> would be settings.py. > > > > > >> Why is it "*.apps.googleusercontent.com"? I'd expected something like > >> "codereview.appspot.com" (but as noted, I'm really not very familiar > > > > with that > >> > >> OAuth stuff). > > > > > > https://codereview.appspot.com/7326049/diff/1/tests/run_tests.py > > File tests/run_tests.py (right): > > > > > https://codereview.appspot.com/7326049/diff/1/tests/run_tests.py#newcode69 > > tests/run_tests.py:69: sdk_path = > > > '/Applications/GoogleAppEngineLauncher.app/Contents/Resources/GoogleAppEngine-default.bundle/Contents/Resources/google_appengine' > > That was an accident and I didn't mean to upload this diff, just patched > > it so I could run the tests on my Mac without moving stuff around :) > > > > > > On 2013/02/24 09:20:06, Andi wrote: > >> > >> Please don't change this, or if this is a common used path please add > > > > a > >> > >> something that chooses between both paths. > > > > > > https://codereview.appspot.com/7326049/ > -- Danny Hermes Developer Programs Engineer
Sign in to reply to this message.
On Thu, Feb 28, 2013 at 3:51 PM, Andi Albrecht <albrecht.andi@gmail.com> wrote: > Hi, > > On Sun, Feb 24, 2013 at 7:09 PM, <dhermes@google.com> wrote: >> PTAL >> >> RE: Do I understand it correctly that OAuth users and regular users are >> >> matched by their email address? >> >> Yes. As you may have noticed in auth_utils, we are requiring that *only* >> the e-mail scope is associated with the OAuth 2.0 tokens sent, since we >> don't have our own scope and are using the Client ID as a proxy for our >> own scope (as is done in Cloud Endpoints). >> >> The OAuth API in App Engine only works if at least the e-mail scope is >> associated with the token in the current environ (via the env var >> HTTP_AUTHORIZATION). When this is the case, the current user returned >> can contain the e-mail since the user has granted that token the ability >> to retrieve the associated email. > > ah, ok. Thanks! It's getting clearer to me now. > >> >> >> >> https://codereview.appspot.com/7326049/diff/1/codereview/auth_utils.py >> File codereview/auth_utils.py (right): >> >> https://codereview.appspot.com/7326049/diff/1/codereview/auth_utils.py#newcode53 >> codereview/auth_utils.py:53: RIETVELD_CLIENT_ID = >> 'dummy29.apps.googleusercontent.com' >> I am pretty familiar with OAuth and helped implement >> >> https://code.google.com/p/googleappengine/source/browse/trunk/python/google/a... >> >> for the new App Engine feature "Google Cloud Endpoints". >> >> I wasn't aware of settings.py, and will put this there. >> >> The value is created in a Google APIs Console >> (https://code.google.com/apis/console/) project. Each project get's a >> unique ID (e.g. the project associated with Google's APIs Explorer is >> 292824132082) and the corresponding client ID is >> {$PROJECT_ID}.project.googleusercontent.com. >> >> We actually need to create a project and add the appropriate people to >> it and then replace this value with the actual Client ID associated with >> that project. I'm happy to do this, if you tell me who needs to be added >> to it. If you don't want me listed as an owner on the project, I can >> show you how to do it. >> >> I'd be happy to jump on a video chat to explain some of this stuff. >> Particularly about the next step, which involves distributing the client >> ID *AND* client secret in upload.py. > > A video chat to discuss the details sounds good. What about next monday? > > IMO we can commit this change and make it live to make sure that we > don't run in any troubles. Your change is committed and already live. I'll keep an eye on the logs for the next few hours. In case something goes wrong we could switch back to the previous version (963 in admin console). -- Andi > >> >> >> On 2013/02/24 09:20:06, Andi wrote: >>> >>> On 2013/02/15 22:10:53, dhermes wrote: >>> > This value is nonsense and I'd like to discuss it's future value >> >> with one of >>> >>> the >>> > maintainers. >> >> >>> I'm not very familiar with OAuth in general, but to me it seems that >> >> the value >>> >>> must be unique for each instance of Rietveld. A good place to >> >> configure this >>> >>> would be settings.py. >> >> >>> Why is it "*.apps.googleusercontent.com"? I'd expected something like >>> "codereview.appspot.com" (but as noted, I'm really not very familiar >> >> with that >>> >>> OAuth stuff). >> >> >> https://codereview.appspot.com/7326049/diff/1/tests/run_tests.py >> File tests/run_tests.py (right): >> >> https://codereview.appspot.com/7326049/diff/1/tests/run_tests.py#newcode69 >> tests/run_tests.py:69: sdk_path = >> '/Applications/GoogleAppEngineLauncher.app/Contents/Resources/GoogleAppEngine-default.bundle/Contents/Resources/google_appengine' >> That was an accident and I didn't mean to upload this diff, just patched >> it so I could run the tests on my Mac without moving stuff around :) >> >> >> On 2013/02/24 09:20:06, Andi wrote: >>> >>> Please don't change this, or if this is a common used path please add >> >> a >>> >>> something that chooses between both paths. >> >> >> https://codereview.appspot.com/7326049/
Sign in to reply to this message.
I've need to fix a call to is_current_user_admin() in models.py to make the issue page work. Here's the commit https://code.google.com/p/rietveld/source/detail?r=c379eb429d33267182473d603b... just a one line hotfix, so I skip to review process :)
Sign in to reply to this message.
So, the OAuth part will always fail since the Client ID in the review doesn't exist. However, it's fine as a test that cookie based auth still works. On Thu, Feb 28, 2013 at 11:39 PM, <albrecht.andi@gmail.com> wrote: > I've need to fix a call to is_current_user_admin() in models.py to make > the issue page work. > > Here's the commit > https://code.google.com/p/**rietveld/source/detail?r=** > c379eb429d33267182473d603bee9e**dcd3c5e624<https://code.google.com/p/rietveld/source/detail?r=c379eb429d33267182473d603bee9edcd3c5e624> > > > just a one line hotfix, so I skip to review process :) > > https://codereview.appspot.**com/7326049/<https://codereview.appspot.com/7326... > -- Danny Hermes Developer Programs Engineer
Sign in to reply to this message.
Am Freitag, 1. März 2013 schrieb Danny Hermes :
> So, the OAuth part will always fail since the Client ID in the review
> doesn't exist.
>
> However, it's fine as a test that cookie based auth still works.
>
Yes, this was the intention. Except for the trivial bug I've fixed already
everything works without problems. I'll just lower some warnings to debug
to keep the log viewer a bit cleaner.
>
>
> On Thu, Feb 28, 2013 at 11:39 PM, <albrecht.andi@gmail.com<javascript:_e({},
'cvml', 'albrecht.andi@gmail.com');>
> > wrote:
>
>> I've need to fix a call to is_current_user_admin() in models.py to make
>> the issue page work.
>>
>> Here's the commit
>> https://code.google.com/p/**rietveld/source/detail?r=**
>>
c379eb429d33267182473d603bee9e**dcd3c5e624<https://code.google.com/p/rietveld/source/detail?r=c379eb429d33267182473d603bee9edcd3c5e624>
>>
>>
>> just a one line hotfix, so I skip to review process :)
>>
>>
https://codereview.appspot.**com/7326049/<https://codereview.appspot.com/7326...
>>
>
>
>
> --
> Danny Hermes
> Developer Programs Engineer
>
Sign in to reply to this message.
test reply, please ignore On Fri, Mar 1, 2013 at 6:33 PM, Andi Albrecht <albrecht.andi@gmail.com> wrote: > Am Freitag, 1. März 2013 schrieb Danny Hermes : > >> So, the OAuth part will always fail since the Client ID in the review >> doesn't exist. >> >> However, it's fine as a test that cookie based auth still works. > > > Yes, this was the intention. Except for the trivial bug I've fixed already > everything works without problems. I'll just lower some warnings to debug to > keep the log viewer a bit cleaner. > >> >> >> >> On Thu, Feb 28, 2013 at 11:39 PM, <albrecht.andi@gmail.com> wrote: >>> >>> I've need to fix a call to is_current_user_admin() in models.py to make >>> the issue page work. >>> >>> Here's the commit >>> >>> https://code.google.com/p/rietveld/source/detail?r=c379eb429d33267182473d603b... >>> >>> >>> just a one line hotfix, so I skip to review process :) >>> >>> https://codereview.appspot.com/7326049/ >> >> >> >> >> -- >> Danny Hermes >> Developer Programs Engineer
Sign in to reply to this message.
Should I close this review since the code has been committed?
Sign in to reply to this message.
Yes, please close this issue. Am Dienstag, 5. März 2013 schrieb : > Should I close this review since the code has been committed? > > https://codereview.appspot.**com/7326049/<https://codereview.appspot.com/7326... >
Sign in to reply to this message.
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
