Issue 9998043: code review 9998043: oauth/appengine: add service account helpers
Descriptionappengine: add service account helpers
Patch Set 1 #Patch Set 2 : diff -r 2d711318934b https://code.google.com/p/goauth2 #
Total comments: 8
Patch Set 3 : diff -r 2d711318934b https://code.google.com/p/goauth2 #Patch Set 4 : diff -r 2d711318934b https://code.google.com/p/goauth2 #Patch Set 5 : diff -r 2dd63b800d01 https://code.google.com/p/goauth2 #
Total comments: 14
Patch Set 6 : diff -r 2dd63b800d01 https://code.google.com/p/goauth2 #Patch Set 7 : diff -r 2dd63b800d01 https://code.google.com/p/goauth2 #Patch Set 8 : diff -r 917d06da3b2b https://code.google.com/p/goauth2 #
Total comments: 3
Patch Set 9 : diff -r 917d06da3b2b https://code.google.com/p/goauth2 #
Total comments: 4
Patch Set 10 : diff -r 917d06da3b2b https://code.google.com/p/goauth2 #
Total comments: 8
Patch Set 11 : diff -r 917d06da3b2b https://code.google.com/p/goauth2 #
MessagesTotal messages: 33
Hello golang-dev@googlegroups.com (cc: adg@golang.org), I'd like you to review this change to https://code.google.com/p/goauth2
Sign in to reply to this message.
https://codereview.appspot.com/9998043/diff/2001/oauth/appengine/serviceaccou... File oauth/appengine/serviceaccount.go (right): https://codereview.appspot.com/9998043/diff/2001/oauth/appengine/serviceaccou... oauth/appengine/serviceaccount.go:47: func GetAccessToken(c appengine.Context, scopes ...string) (string, error) { s/Get// Maybe this should be CachedAccessToken, as distinct from appengine.AccessToken. The token is per-app, right? So there's no issue here with sharing the cache key between multiple instances of the same app? https://codereview.appspot.com/9998043/diff/2001/oauth/appengine/serviceaccou... oauth/appengine/serviceaccount.go:67: func NewClient(c appengine.Context, scopes ...string) (*http.Client, error) { A recent change to the oauth package means you should be able to just construct and use a regular oauth.Transport for this: return &http.Client{Transport:&oauth.Transport{ Token:&oauth.Token{AccessToken: token}, Transport: &urlfetch.Transport{...}, }}
Sign in to reply to this message.
https://codereview.appspot.com/9998043/diff/2001/oauth/appengine/serviceaccou... File oauth/appengine/serviceaccount.go (right): https://codereview.appspot.com/9998043/diff/2001/oauth/appengine/serviceaccou... oauth/appengine/serviceaccount.go:17: // client, err := serviceaccount.NewClient(c, "https://www.googleapis.com/auth/compute", "https://www.googleapis.com/auth/bigquery") check the errors in the example, please https://codereview.appspot.com/9998043/diff/2001/oauth/appengine/serviceaccou... oauth/appengine/serviceaccount.go:24: // Transport: &accesstoken.Transport{ accesstoken.Transport? that doesn't exist. but you won't need it (see lower comments)
Sign in to reply to this message.
https://codereview.appspot.com/9998043/diff/2001/oauth/appengine/serviceaccou... File oauth/appengine/serviceaccount.go (right): https://codereview.appspot.com/9998043/diff/2001/oauth/appengine/serviceaccou... oauth/appengine/serviceaccount.go:17: // client, err := serviceaccount.NewClient(c, "https://www.googleapis.com/auth/compute", "https://www.googleapis.com/auth/bigquery") On 2013/06/05 06:47:59, adg wrote: > check the errors in the example, please Done. https://codereview.appspot.com/9998043/diff/2001/oauth/appengine/serviceaccou... oauth/appengine/serviceaccount.go:24: // Transport: &accesstoken.Transport{ On 2013/06/05 06:47:59, adg wrote: > accesstoken.Transport? that doesn't exist. but you won't need it (see lower > comments) type, renamed the package since then, Done. https://codereview.appspot.com/9998043/diff/2001/oauth/appengine/serviceaccou... oauth/appengine/serviceaccount.go:47: func GetAccessToken(c appengine.Context, scopes ...string) (string, error) { On 2013/06/05 06:46:59, adg wrote: > s/Get// > > Maybe this should be CachedAccessToken, as distinct from appengine.AccessToken. > > The token is per-app, right? So there's no issue here with sharing the cache key > between multiple instances of the same app? I splitted the cache in a separate file, and implement oauth.Cache interface. The cache is indeed per app, but I made the scopes part of the key. https://codereview.appspot.com/9998043/diff/2001/oauth/appengine/serviceaccou... oauth/appengine/serviceaccount.go:67: func NewClient(c appengine.Context, scopes ...string) (*http.Client, error) { On 2013/06/05 06:46:59, adg wrote: > A recent change to the oauth package means you should be able to just construct > and use a regular oauth.Transport for this: > > return &http.Client{Transport:&oauth.Transport{ > Token:&oauth.Token{AccessToken: token}, > Transport: &urlfetch.Transport{...}, > }} Cool, I just looked into this, but I can't really reuse oauth.Transport there because the Refresh logic is different: but I did mimic the interface of it (by moving the AccessToken call in a Refresh method): see the new implementation.
Sign in to reply to this message.
https://codereview.appspot.com/9998043/diff/13001/oauth/appengine/serviceacco... File oauth/appengine/serviceaccount/cache.go (right): https://codereview.appspot.com/9998043/diff/13001/oauth/appengine/serviceacco... oauth/appengine/serviceaccount/cache.go:25: const KeyPrefix = "goauth2_appengine_serviceaccount_cache_" If you require the user specify the key, I wouldn't worry about the prefix. https://codereview.appspot.com/9998043/diff/13001/oauth/appengine/serviceacco... File oauth/appengine/serviceaccount/serviceaccount.go (right): https://codereview.appspot.com/9998043/diff/13001/oauth/appengine/serviceacco... oauth/appengine/serviceaccount/serviceaccount.go:5: // +build !appengine this is wrong, surely? we only want to build this on app engine https://codereview.appspot.com/9998043/diff/13001/oauth/appengine/serviceacco... oauth/appengine/serviceaccount/serviceaccount.go:21: // } indent https://codereview.appspot.com/9998043/diff/13001/oauth/appengine/serviceacco... oauth/appengine/serviceaccount/serviceaccount.go:57: transport := &Transport{ s/transport/t/ https://codereview.appspot.com/9998043/diff/13001/oauth/appengine/serviceacco... oauth/appengine/serviceaccount/serviceaccount.go:81: type Transport struct { This is OK for now, but we need to improve the oauth.Transport to make it easy to do this. https://codereview.appspot.com/9998043/diff/13001/oauth/appengine/serviceacco... oauth/appengine/serviceaccount/serviceaccount.go:125: newReq.Header.Set("Authorization", "Bearer "+t.AccessToken) you need to clone the headers, too. just copy the cloneRequest function from goauth2's oauth.go
Sign in to reply to this message.
PTAL https://codereview.appspot.com/9998043/diff/13001/oauth/appengine/serviceacco... File oauth/appengine/serviceaccount/cache.go (right): https://codereview.appspot.com/9998043/diff/13001/oauth/appengine/serviceacco... oauth/appengine/serviceaccount/cache.go:25: const KeyPrefix = "goauth2_appengine_serviceaccount_cache_" On 2013/06/06 00:35:54, adg wrote: > If you require the user specify the key, I wouldn't worry about the prefix. Done. https://codereview.appspot.com/9998043/diff/13001/oauth/appengine/serviceacco... File oauth/appengine/serviceaccount/serviceaccount.go (right): https://codereview.appspot.com/9998043/diff/13001/oauth/appengine/serviceacco... oauth/appengine/serviceaccount/serviceaccount.go:5: // +build !appengine On 2013/06/06 00:35:54, adg wrote: > this is wrong, surely? we only want to build this on app engine Done. In flagrante delicto of cargo cult programming. I was also confused because the go tool contained in the SDK wouldn't set the appengine build tag by default. https://codereview.appspot.com/9998043/diff/13001/oauth/appengine/serviceacco... oauth/appengine/serviceaccount/serviceaccount.go:21: // } On 2013/06/06 00:35:54, adg wrote: > indent Done. https://codereview.appspot.com/9998043/diff/13001/oauth/appengine/serviceacco... oauth/appengine/serviceaccount/serviceaccount.go:57: transport := &Transport{ On 2013/06/06 00:35:54, adg wrote: > s/transport/t/ Done. https://codereview.appspot.com/9998043/diff/13001/oauth/appengine/serviceacco... oauth/appengine/serviceaccount/serviceaccount.go:81: type Transport struct { On 2013/06/06 00:35:54, adg wrote: > This is OK for now, but we need to improve the oauth.Transport to make it easy > to do this. filed: https://code.google.com/p/goauth2/issues/detail?id=18 https://codereview.appspot.com/9998043/diff/13001/oauth/appengine/serviceacco... oauth/appengine/serviceaccount/serviceaccount.go:125: newReq.Header.Set("Authorization", "Bearer "+t.AccessToken) On 2013/06/06 00:35:54, adg wrote: > you need to clone the headers, too. > > just copy the cloneRequest function from goauth2's oauth.go Made oauth.CloneRequest public.
Sign in to reply to this message.
https://codereview.appspot.com/9998043/diff/13001/oauth/appengine/serviceacco... File oauth/appengine/serviceaccount/serviceaccount.go (right): https://codereview.appspot.com/9998043/diff/13001/oauth/appengine/serviceacco... oauth/appengine/serviceaccount/serviceaccount.go:81: type Transport struct { On 2013/06/06 14:58:10, proppy wrote: > On 2013/06/06 00:35:54, adg wrote: > > This is OK for now, but we need to improve the oauth.Transport to make it easy > > to do this. > > filed: https://code.google.com/p/goauth2/issues/detail?id=18 Thanks https://codereview.appspot.com/9998043/diff/13001/oauth/appengine/serviceacco... oauth/appengine/serviceaccount/serviceaccount.go:125: newReq.Header.Set("Authorization", "Bearer "+t.AccessToken) On 2013/06/06 14:58:10, proppy wrote: > On 2013/06/06 00:35:54, adg wrote: > > you need to clone the headers, too. > > > > just copy the cloneRequest function from goauth2's oauth.go > > Made oauth.CloneRequest public. No, don't do that. There is no reason that should be part of the oauth package's exported interface. Plus it only does the right thing in this specific instance. There is nothing wrong with copying a bit of trivial code.
Sign in to reply to this message.
On 2013/06/06 23:26:17, adg wrote: > https://codereview.appspot.com/9998043/diff/13001/oauth/appengine/serviceacco... > File oauth/appengine/serviceaccount/serviceaccount.go (right): > > https://codereview.appspot.com/9998043/diff/13001/oauth/appengine/serviceacco... > oauth/appengine/serviceaccount/serviceaccount.go:81: type Transport struct { > On 2013/06/06 14:58:10, proppy wrote: > > On 2013/06/06 00:35:54, adg wrote: > > > This is OK for now, but we need to improve the oauth.Transport to make it > easy > > > to do this. > > > > filed: https://code.google.com/p/goauth2/issues/detail?id=18 > > Thanks > > https://codereview.appspot.com/9998043/diff/13001/oauth/appengine/serviceacco... > oauth/appengine/serviceaccount/serviceaccount.go:125: > newReq.Header.Set("Authorization", "Bearer "+t.AccessToken) > On 2013/06/06 14:58:10, proppy wrote: > > On 2013/06/06 00:35:54, adg wrote: > > > you need to clone the headers, too. > > > > > > just copy the cloneRequest function from goauth2's oauth.go > > > > Made oauth.CloneRequest public. > > No, don't do that. There is no reason that should be part of the oauth package's > exported interface. Plus it only does the right thing in this specific instance. > > There is nothing wrong with copying a bit of trivial code. Very true, I might quote this in my OSCON talk :)
Sign in to reply to this message.
On 2013/06/07 00:29:41, proppy wrote: > On 2013/06/06 23:26:17, adg wrote: > > > https://codereview.appspot.com/9998043/diff/13001/oauth/appengine/serviceacco... > > File oauth/appengine/serviceaccount/serviceaccount.go (right): > > > > > https://codereview.appspot.com/9998043/diff/13001/oauth/appengine/serviceacco... > > oauth/appengine/serviceaccount/serviceaccount.go:81: type Transport struct { > > On 2013/06/06 14:58:10, proppy wrote: > > > On 2013/06/06 00:35:54, adg wrote: > > > > This is OK for now, but we need to improve the oauth.Transport to make it > > easy > > > > to do this. > > > > > > filed: https://code.google.com/p/goauth2/issues/detail?id=18 > > > > Thanks > > > > > https://codereview.appspot.com/9998043/diff/13001/oauth/appengine/serviceacco... > > oauth/appengine/serviceaccount/serviceaccount.go:125: > > newReq.Header.Set("Authorization", "Bearer "+t.AccessToken) > > On 2013/06/06 14:58:10, proppy wrote: > > > On 2013/06/06 00:35:54, adg wrote: > > > > you need to clone the headers, too. > > > > > > > > just copy the cloneRequest function from goauth2's oauth.go > > > > > > Made oauth.CloneRequest public. > > > > No, don't do that. There is no reason that should be part of the oauth > package's > > exported interface. Plus it only does the right thing in this specific > instance. > > > > There is nothing wrong with copying a bit of trivial code. > > Very true, I might quote this in my OSCON talk :) and I forgot to say PTAL.
Sign in to reply to this message.
Looking good, but can you put this in the root/appengine/serviceaccount directory (ie move it one level up?)
Sign in to reply to this message.
On 2013/06/18 03:54:20, adg wrote: > Looking good, but can you put this in the root/appengine/serviceaccount > directory (ie move it one level up?) Given that the jwt/ stuff is also under oauth/ do you want to also move it 1 level up? (Maybe we could do that in a separate re-org cl, if that's the case).
Sign in to reply to this message.
On 19 June 2013 07:59, <proppy@google.com> wrote: > Given that the jwt/ stuff is also under oauth/ do you want to also move > it 1 level up? > > (Maybe we could do that in a separate re-org cl, if that's the case). > I do want to do that at some point, but not now. There are people that depend on it.
Sign in to reply to this message.
On 2013/06/18 22:03:28, adg wrote: > On 19 June 2013 07:59, <mailto:proppy@google.com> wrote: > > > Given that the jwt/ stuff is also under oauth/ do you want to also move > > it 1 level up? > > > > (Maybe we could do that in a separate re-org cl, if that's the case). > > > > I do want to do that at some point, but not now. There are people that > depend on it. PTAL
Sign in to reply to this message.
https://codereview.appspot.com/9998043/diff/34001/appengine/serviceaccount/se... File appengine/serviceaccount/serviceaccount.go (right): https://codereview.appspot.com/9998043/diff/34001/appengine/serviceaccount/se... appengine/serviceaccount/serviceaccount.go:26: // transport := &serviceaccount.Transport{ I think it might be worth unexporting the Transport type; what do you think?
Sign in to reply to this message.
https://codereview.appspot.com/9998043/diff/34001/appengine/serviceaccount/se... File appengine/serviceaccount/serviceaccount.go (right): https://codereview.appspot.com/9998043/diff/34001/appengine/serviceaccount/se... appengine/serviceaccount/serviceaccount.go:26: // transport := &serviceaccount.Transport{ On 2013/06/19 03:32:47, adg wrote: > I think it might be worth unexporting the Transport type; what do you think? Then how do stack this on a customTransport?
Sign in to reply to this message.
R=adg
Sign in to reply to this message.
On 2013/06/19 18:13:12, proppy wrote: > https://codereview.appspot.com/9998043/diff/34001/appengine/serviceaccount/se... > File appengine/serviceaccount/serviceaccount.go (right): > > https://codereview.appspot.com/9998043/diff/34001/appengine/serviceaccount/se... > appengine/serviceaccount/serviceaccount.go:26: // transport := > &serviceaccount.Transport{ > On 2013/06/19 03:32:47, adg wrote: > > I think it might be worth unexporting the Transport type; what do you think? > > Then how do stack this on a customTransport? s/do/to/
Sign in to reply to this message.
https://codereview.appspot.com/9998043/diff/34001/appengine/serviceaccount/se... File appengine/serviceaccount/serviceaccount.go (right): https://codereview.appspot.com/9998043/diff/34001/appengine/serviceaccount/se... appengine/serviceaccount/serviceaccount.go:26: // transport := &serviceaccount.Transport{ On 2013/06/19 18:13:12, proppy wrote: > On 2013/06/19 03:32:47, adg wrote: > > I think it might be worth unexporting the Transport type; what do you think? > > Then how do stack this on a customTransport? Why would you ever want to use anything other than urlfetch?
Sign in to reply to this message.
You might want to also add googleapi.transport.ApiKey but I agree you can always stack them the other way around and put serviceaccount on the bottom. It really depend if you want to enforce that or not. On Jun 19, 2013 4:46 PM, <proppy@google.com> wrote: > On 2013/06/19 18:13:12, proppy wrote: > > https://codereview.appspot.**com/9998043/diff/34001/** > appengine/serviceaccount/**serviceaccount.go<https://codereview.appspot.com/9998043/diff/34001/appengine/serviceaccount/serviceaccount.go> > >> File appengine/serviceaccount/**serviceaccount.go (right): >> > > > https://codereview.appspot.**com/9998043/diff/34001/** > appengine/serviceaccount/**serviceaccount.go#newcode26<https://codereview.appspot.com/9998043/diff/34001/appengine/serviceaccount/serviceaccount.go#newcode26> > >> appengine/serviceaccount/**serviceaccount.go:26: // transport := >> &serviceaccount.Transport{ >> On 2013/06/19 03:32:47, adg wrote: >> > I think it might be worth unexporting the Transport type; what do >> > you think? > > Then how do stack this on a customTransport? >> > > s/do/to/ > > https://codereview.appspot.**com/9998043/<https://codereview.appspot.com/9998... >
Sign in to reply to this message.
What I want is the simplest possible API that is useful. It doesn't need to be a swiss army tool, just the thing that is unambiguously the right thing to use. For service-account auth on appengine, NewClient seems the thing. On 24 June 2013 14:39, Johan Euphrosine <proppy@google.com> wrote: > You might want to also add googleapi.transport.ApiKey but I agree you can > always stack them the other way around and put serviceaccount on the > bottom. It really depend if you want to enforce that or not. > On Jun 19, 2013 4:46 PM, <proppy@google.com> wrote: > >> On 2013/06/19 18:13:12, proppy wrote: >> >> https://codereview.appspot.**com/9998043/diff/34001/** >> appengine/serviceaccount/**serviceaccount.go<https://codereview.appspot.com/9998043/diff/34001/appengine/serviceaccount/serviceaccount.go> >> >>> File appengine/serviceaccount/**serviceaccount.go (right): >>> >> >> >> https://codereview.appspot.**com/9998043/diff/34001/** >> appengine/serviceaccount/**serviceaccount.go#newcode26<https://codereview.appspot.com/9998043/diff/34001/appengine/serviceaccount/serviceaccount.go#newcode26> >> >>> appengine/serviceaccount/**serviceaccount.go:26: // transport := >>> &serviceaccount.Transport{ >>> On 2013/06/19 03:32:47, adg wrote: >>> > I think it might be worth unexporting the Transport type; what do >>> >> you think? >> >> Then how do stack this on a customTransport? >>> >> >> s/do/to/ >> >> https://codereview.appspot.**com/9998043/<https://codereview.appspot.com/9998... >> >
Sign in to reply to this message.
I agree, this is specific enough (oauth+appengine+serviceaccount) I think nobody needs the flexibility of overriding the underlying transport. It will also make the doc simpler :) On Jun 23, 2013 9:41 PM, "Andrew Gerrand" <adg@golang.org> wrote: > What I want is the simplest possible API that is useful. It doesn't need > to be a swiss army tool, just the thing that is unambiguously the right > thing to use. For service-account auth on appengine, NewClient seems the > thing. > > > > > On 24 June 2013 14:39, Johan Euphrosine <proppy@google.com> wrote: > >> You might want to also add googleapi.transport.ApiKey but I agree you can >> always stack them the other way around and put serviceaccount on the >> bottom. It really depend if you want to enforce that or not. >> On Jun 19, 2013 4:46 PM, <proppy@google.com> wrote: >> >>> On 2013/06/19 18:13:12, proppy wrote: >>> >>> https://codereview.appspot.**com/9998043/diff/34001/** >>> appengine/serviceaccount/**serviceaccount.go<https://codereview.appspot.com/9998043/diff/34001/appengine/serviceaccount/serviceaccount.go> >>> >>>> File appengine/serviceaccount/**serviceaccount.go (right): >>>> >>> >>> >>> https://codereview.appspot.**com/9998043/diff/34001/** >>> appengine/serviceaccount/**serviceaccount.go#newcode26<https://codereview.appspot.com/9998043/diff/34001/appengine/serviceaccount/serviceaccount.go#newcode26> >>> >>>> appengine/serviceaccount/**serviceaccount.go:26: // transport := >>>> &serviceaccount.Transport{ >>>> On 2013/06/19 03:32:47, adg wrote: >>>> > I think it might be worth unexporting the Transport type; what do >>>> >>> you think? >>> >>> Then how do stack this on a customTransport? >>>> >>> >>> s/do/to/ >>> >>> https://codereview.appspot.**com/9998043/<https://codereview.appspot.com/9998... >>> >> >
Sign in to reply to this message.
Do you also want to hide other field like, Context, Scopes, and TokenCache? Should the cache be optional? On 2013/06/24 04:46:45, proppy wrote: > I agree, this is specific enough (oauth+appengine+serviceaccount) I think > nobody needs the flexibility of overriding the underlying transport. It > will also make the doc simpler :) > On Jun 23, 2013 9:41 PM, "Andrew Gerrand" <mailto:adg@golang.org> wrote: > > > What I want is the simplest possible API that is useful. It doesn't need > > to be a swiss army tool, just the thing that is unambiguously the right > > thing to use. For service-account auth on appengine, NewClient seems the > > thing. > > > > > > > > > > On 24 June 2013 14:39, Johan Euphrosine <mailto:proppy@google.com> wrote: > > > >> You might want to also add googleapi.transport.ApiKey but I agree you can > >> always stack them the other way around and put serviceaccount on the > >> bottom. It really depend if you want to enforce that or not. > >> On Jun 19, 2013 4:46 PM, <mailto:proppy@google.com> wrote: > >> > >>> On 2013/06/19 18:13:12, proppy wrote: > >>> > >>> https://codereview.appspot.**com/9998043/diff/34001/** > >>> > appengine/serviceaccount/**serviceaccount.go<https://codereview.appspot.com/9998043/diff/34001/appengine/serviceaccount/serviceaccount.go> > >>> > >>>> File appengine/serviceaccount/**serviceaccount.go (right): > >>>> > >>> > >>> > >>> https://codereview.appspot.**com/9998043/diff/34001/** > >>> > appengine/serviceaccount/**serviceaccount.go#newcode26<https://codereview.appspot.com/9998043/diff/34001/appengine/serviceaccount/serviceaccount.go#newcode26> > >>> > >>>> appengine/serviceaccount/**serviceaccount.go:26: // transport := > >>>> &serviceaccount.Transport{ > >>>> On 2013/06/19 03:32:47, adg wrote: > >>>> > I think it might be worth unexporting the Transport type; what do > >>>> > >>> you think? > >>> > >>> Then how do stack this on a customTransport? > >>>> > >>> > >>> s/do/to/ > >>> > >>> > https://codereview.appspot.**com/9998043/%3Chttps://codereview.appspot.com/99...> > >>> > >> > >
Sign in to reply to this message.
On 2013/06/25 00:51:45, proppy wrote: > Do you also want to hide other field like, Context, Scopes, and TokenCache? > > Should the cache be optional? Let's start by only exporting NewClient. The fields on the Transport struct can stay exported, but Transport should be "transport" (unexported).
Sign in to reply to this message.
On 2013/06/25 04:09:22, adg wrote: > On 2013/06/25 00:51:45, proppy wrote: > > Do you also want to hide other field like, Context, Scopes, and TokenCache? > > > > Should the cache be optional? > > Let's start by only exporting NewClient. > The fields on the Transport struct can stay exported, but Transport should be > "transport" (unexported). Done. PTAL
Sign in to reply to this message.
On 2013/06/25 17:24:19, proppy wrote: > On 2013/06/25 04:09:22, adg wrote: > > On 2013/06/25 00:51:45, proppy wrote: > > > Do you also want to hide other field like, Context, Scopes, and TokenCache? > > > > > > Should the cache be optional? > > > > Let's start by only exporting NewClient. > > The fields on the Transport struct can stay exported, but Transport should be > > "transport" (unexported). > > Done. > > PTAL FYI, I also updated the service account sample to make use of this: https://github.com/GoogleCloudPlatform/appengine-goshorten/pull/2
Sign in to reply to this message.
https://codereview.appspot.com/9998043/diff/44001/appengine/serviceaccount/ca... File appengine/serviceaccount/cache.go (right): https://codereview.appspot.com/9998043/diff/44001/appengine/serviceaccount/ca... appengine/serviceaccount/cache.go:20: type Cache struct { and this? https://codereview.appspot.com/9998043/diff/44001/appengine/serviceaccount/se... File appengine/serviceaccount/serviceaccount.go (right): https://codereview.appspot.com/9998043/diff/44001/appengine/serviceaccount/se... appengine/serviceaccount/serviceaccount.go:63: type Transport struct { I thought we said this would be unexported?
Sign in to reply to this message.
PTAL https://codereview.appspot.com/9998043/diff/44001/appengine/serviceaccount/ca... File appengine/serviceaccount/cache.go (right): https://codereview.appspot.com/9998043/diff/44001/appengine/serviceaccount/ca... appengine/serviceaccount/cache.go:20: type Cache struct { On 2013/06/26 01:12:05, adg wrote: > and this? Done. https://codereview.appspot.com/9998043/diff/44001/appengine/serviceaccount/se... File appengine/serviceaccount/serviceaccount.go (right): https://codereview.appspot.com/9998043/diff/44001/appengine/serviceaccount/se... appengine/serviceaccount/serviceaccount.go:63: type Transport struct { On 2013/06/26 01:12:05, adg wrote: > I thought we said this would be unexported? Oh, sorry for the missunderding I though you were refering to Transport field not service.Transport type. (hence my question about the other member). Done.
Sign in to reply to this message.
https://codereview.appspot.com/9998043/diff/51002/appengine/serviceaccount/ca... File appengine/serviceaccount/cache.go (right): https://codereview.appspot.com/9998043/diff/51002/appengine/serviceaccount/ca... appengine/serviceaccount/cache.go:18: // A TokenCache implementation that use memcache to store AccessToken // cache implements TokenCache using memcache... https://codereview.appspot.com/9998043/diff/51002/appengine/serviceaccount/se... File appengine/serviceaccount/serviceaccount.go (right): https://codereview.appspot.com/9998043/diff/51002/appengine/serviceaccount/se... appengine/serviceaccount/serviceaccount.go:15: // // simple usage. delete https://codereview.appspot.com/9998043/diff/51002/appengine/serviceaccount/se... appengine/serviceaccount/serviceaccount.go:37: // NewClient returns a new *http.Client authorized for the s/a new/an/ https://codereview.appspot.com/9998043/diff/51002/appengine/serviceaccount/se... appengine/serviceaccount/serviceaccount.go:38: // given scopes with the service account owned by the application. mention caching
Sign in to reply to this message.
PTAL https://codereview.appspot.com/9998043/diff/51002/appengine/serviceaccount/ca... File appengine/serviceaccount/cache.go (right): https://codereview.appspot.com/9998043/diff/51002/appengine/serviceaccount/ca... appengine/serviceaccount/cache.go:18: // A TokenCache implementation that use memcache to store AccessToken On 2013/06/26 01:23:51, adg wrote: > // cache implements TokenCache using memcache... Done. https://codereview.appspot.com/9998043/diff/51002/appengine/serviceaccount/se... File appengine/serviceaccount/serviceaccount.go (right): https://codereview.appspot.com/9998043/diff/51002/appengine/serviceaccount/se... appengine/serviceaccount/serviceaccount.go:15: // // simple usage. On 2013/06/26 01:23:51, adg wrote: > delete Done. https://codereview.appspot.com/9998043/diff/51002/appengine/serviceaccount/se... appengine/serviceaccount/serviceaccount.go:37: // NewClient returns a new *http.Client authorized for the On 2013/06/26 01:23:51, adg wrote: > s/a new/an/ Done. https://codereview.appspot.com/9998043/diff/51002/appengine/serviceaccount/se... appengine/serviceaccount/serviceaccount.go:38: // given scopes with the service account owned by the application. On 2013/06/26 01:23:51, adg wrote: > mention caching Done.
Sign in to reply to this message.
Upload? On 26 June 2013 11:33, <proppy@google.com> wrote: > PTAL > > > > https://codereview.appspot.**com/9998043/diff/51002/** > appengine/serviceaccount/**cache.go<https://codereview.appspot.com/9998043/diff/51002/appengine/serviceaccount/cache.go> > File appengine/serviceaccount/**cache.go (right): > > https://codereview.appspot.**com/9998043/diff/51002/** > appengine/serviceaccount/**cache.go#newcode18<https://codereview.appspot.com/9998043/diff/51002/appengine/serviceaccount/cache.go#newcode18> > appengine/serviceaccount/**cache.go:18: // A TokenCache implementation > that use memcache to store AccessToken > On 2013/06/26 01:23:51, adg wrote: > >> // cache implements TokenCache using memcache... >> > > Done. > > > https://codereview.appspot.**com/9998043/diff/51002/** > appengine/serviceaccount/**serviceaccount.go<https://codereview.appspot.com/9998043/diff/51002/appengine/serviceaccount/serviceaccount.go> > File appengine/serviceaccount/**serviceaccount.go (right): > > https://codereview.appspot.**com/9998043/diff/51002/** > appengine/serviceaccount/**serviceaccount.go#newcode15<https://codereview.appspot.com/9998043/diff/51002/appengine/serviceaccount/serviceaccount.go#newcode15> > appengine/serviceaccount/**serviceaccount.go:15: // // simple usage. > On 2013/06/26 01:23:51, adg wrote: > >> delete >> > > Done. > > > https://codereview.appspot.**com/9998043/diff/51002/** > appengine/serviceaccount/**serviceaccount.go#newcode37<https://codereview.appspot.com/9998043/diff/51002/appengine/serviceaccount/serviceaccount.go#newcode37> > appengine/serviceaccount/**serviceaccount.go:37: // NewClient returns a > new *http.Client authorized for the > On 2013/06/26 01:23:51, adg wrote: > >> s/a new/an/ >> > > Done. > > > https://codereview.appspot.**com/9998043/diff/51002/** > appengine/serviceaccount/**serviceaccount.go#newcode38<https://codereview.appspot.com/9998043/diff/51002/appengine/serviceaccount/serviceaccount.go#newcode38> > appengine/serviceaccount/**serviceaccount.go:38: // given scopes with the > service account owned by the application. > On 2013/06/26 01:23:51, adg wrote: > >> mention caching >> > > Done. > > https://codereview.appspot.**com/9998043/<https://codereview.appspot.com/9998... >
Sign in to reply to this message.
Done, sorry On Tue, Jun 25, 2013 at 6:37 PM, Andrew Gerrand <adg@golang.org> wrote: > Upload? > > > On 26 June 2013 11:33, <proppy@google.com> wrote: > >> PTAL >> >> >> >> https://codereview.appspot.**com/9998043/diff/51002/** >> appengine/serviceaccount/**cache.go<https://codereview.appspot.com/9998043/diff/51002/appengine/serviceaccount/cache.go> >> File appengine/serviceaccount/**cache.go (right): >> >> https://codereview.appspot.**com/9998043/diff/51002/** >> appengine/serviceaccount/**cache.go#newcode18<https://codereview.appspot.com/9998043/diff/51002/appengine/serviceaccount/cache.go#newcode18> >> appengine/serviceaccount/**cache.go:18: // A TokenCache implementation >> that use memcache to store AccessToken >> On 2013/06/26 01:23:51, adg wrote: >> >>> // cache implements TokenCache using memcache... >>> >> >> Done. >> >> >> https://codereview.appspot.**com/9998043/diff/51002/** >> appengine/serviceaccount/**serviceaccount.go<https://codereview.appspot.com/9998043/diff/51002/appengine/serviceaccount/serviceaccount.go> >> File appengine/serviceaccount/**serviceaccount.go (right): >> >> https://codereview.appspot.**com/9998043/diff/51002/** >> appengine/serviceaccount/**serviceaccount.go#newcode15<https://codereview.appspot.com/9998043/diff/51002/appengine/serviceaccount/serviceaccount.go#newcode15> >> appengine/serviceaccount/**serviceaccount.go:15: // // simple >> usage. >> On 2013/06/26 01:23:51, adg wrote: >> >>> delete >>> >> >> Done. >> >> >> https://codereview.appspot.**com/9998043/diff/51002/** >> appengine/serviceaccount/**serviceaccount.go#newcode37<https://codereview.appspot.com/9998043/diff/51002/appengine/serviceaccount/serviceaccount.go#newcode37> >> appengine/serviceaccount/**serviceaccount.go:37: // NewClient returns a >> new *http.Client authorized for the >> On 2013/06/26 01:23:51, adg wrote: >> >>> s/a new/an/ >>> >> >> Done. >> >> >> https://codereview.appspot.**com/9998043/diff/51002/** >> appengine/serviceaccount/**serviceaccount.go#newcode38<https://codereview.appspot.com/9998043/diff/51002/appengine/serviceaccount/serviceaccount.go#newcode38> >> appengine/serviceaccount/**serviceaccount.go:38: // given scopes with the >> service account owned by the application. >> On 2013/06/26 01:23:51, adg wrote: >> >>> mention caching >>> >> >> Done. >> >> https://codereview.appspot.**com/9998043/<https://codereview.appspot.com/9998... >> > > -- Johan Euphrosine (proppy) Developer Programs Engineer Google Developer Relations
Sign in to reply to this message.
LGTM Thanks!
Sign in to reply to this message.
*** Submitted as https://code.google.com/p/goauth2/source/detail?r=ecc4c1308422 *** appengine: add service account helpers R=golang-dev, adg CC=golang-dev https://codereview.appspot.com/9998043 Committer: Andrew Gerrand <adg@golang.org>
Sign in to reply to this message.
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
