OAuth examples #153

mpguerra · 2016-11-23T14:09:42Z

Replaces #145 and closes #150

mayorova · 2016-11-23T14:32:19Z

examples/oauth2/README.md

+3scale setup
+------------
+
+To get this working with a 3scale instance 


Looks like the end of the phrase is missing...

Also, it would be good to mention here that:

Self-managed deployment type and OAuth authentication method should be selected

Login URL on the Integration page need to be configured, for the current example I guess it would be http://localhost:3000/auth/login

Agreed, please complete the sentence Pili

mayorova · 2016-11-23T15:51:25Z

Tested and it works 👍

One other point to include to the documentation is that "Request access token locally" should be set in Postman in order to make a successful /oauth/token call.

andrewdavidmackenzie · 2016-11-25T15:35:29Z

apicast/conf.d/apicast.conf

@@ -86,6 +86,7 @@ location = /_threescale/oauth_store_token {
  internal;
  proxy_set_header  X-Real-IP  $remote_addr;
  proxy_set_header  X-Forwarded-For $proxy_add_x_forwarded_for;
+  proxy_set_header  Host  "$backend_host";


Pili, can you tell us what the purpose of adding the header is please?

I haven't tried without, but I think it wasn't working.
All other calls to the backend do already have this header.

Authorization against backend fails when this header is missing

andrewdavidmackenzie · 2016-11-25T15:38:44Z

examples/oauth2/README.md

+3scale setup
+------------
+
+To get this working with a 3scale instance 


Agreed, please complete the sentence Pili

andrewdavidmackenzie · 2016-11-25T15:39:27Z

examples/oauth2/.env-sample

@@ -0,0 +1,20 @@
+# URI to fetch gateway configuration from. Expected format is: https?://[password@]hostname


Is .env-sample the best name or being inside an example .env is fine?
Where is the code that loads this?

I didn't want to commit my .env since it would be specific to my environment, so I added a sample of what a .env file could look like. Could also be removed since this is documented in the README

andrewdavidmackenzie · 2016-11-25T15:41:20Z

examples/oauth2/auth-server/.dockerignore

@@ -0,0 +1,3 @@
+.git
+.dockerignore
+Gemfile.lock


Sure you don't want this to lock the versions of the gems that your bundle install installs?

andrewdavidmackenzie · 2016-11-25T15:42:39Z

examples/oauth2/auth-server/Dockerfile

+
+# Set an environment variable to store where the app is installed to inside
+# of the Docker image.
+ENV INSTALL_PATH /auth-server


can we follow the (emerging) convention of ours to install in /opt/app?

andrewdavidmackenzie · 2016-11-25T15:43:26Z

examples/oauth2/auth-server/Dockerfile

+
+# Copy in the application code from your work station at the current directory
+# over to the working directory.
+COPY . .


This will copy Gemfile, which is already copied above. Remove above copy?

andrewdavidmackenzie · 2016-11-25T15:51:48Z

examples/oauth2/config.json

@@ -0,0 +1,126 @@
+{


Wouldn't this be a better example if it used the config.json downloaded from the config API, for the user account doing the example. That would make it flexible to whatever the user has configured in 3scale Admin Portal, would avoid having to redact secrets (making sample non-functional), and supply service IDs, shared secrets etc.

The only data data needed would the service_id and token that is already needed by apicast, to get the config from the config API.

this is here as an example config to show the data that is necessary, the example would work with any config.json downloaded from the admin portal as long as they are using oauth authentication mode AND they have filled in the login url/Authorization endpoint in the Integration screen.

andrewdavidmackenzie · 2016-11-25T15:52:18Z

examples/oauth2/config.json

+  "services": [
+    {
+      "id": 2555417736402,
+      "account_id": 2445581608319,


lots of data here, tied to your test account

this is just as an example, I could also remove this config from the example

andrewdavidmackenzie

I'd suggest we try and create an example that isn't tied to your single test account, so that user can modify their API config and we avoid exposing ids, secrets etc.

andrewdavidmackenzie · 2016-11-25T16:01:33Z

Further comment:

Can we please separate out the fixes for OAuth into one PR....for including in RC1.... and continue the discussion and polishing of the examples, via a separate PR...

The ".... and ....." in the commit title is a givaway, it's adding two things in one commit/PR.

thanks.

andrewdavidmackenzie · 2016-11-25T16:04:24Z

apicast/src/get_token.lua

@@ -10,7 +12,7 @@ local function extract_params()
  params.authorization = {}

  if header_params['Authorization'] then
-    params.authorization = ngx.decode_base64(header_params['Authorization']:split(" ")[2]):split(":")
+    params.authorization = split(ngx.decode_base64(split(header_params['Authorization']," ")[2]),":")


Is this a functional change, or just syntax?

We're trying to understand the changes that fix the broken OAuth....

This is a functional change which fixes the bug.
There used to be a global function function string:split, which didn't make it to APIcast, so strvariable:split is not a valid syntax.

andrewdavidmackenzie · 2016-11-29T10:41:28Z

@mpguerra Now that you submitted the fixes in a separate PR, you should modify this one to remove them and only add the examples.

You should rebase on v2 branch first to get latest changes to readme etc....

mayorova · 2016-12-04T22:27:37Z

Rebased on top of v2

mikz · 2016-12-05T09:38:14Z

examples/oauth2/docker-compose.yml

+version: '2'
+services:
+  gateway:
+    image: apicast-test:latest


shouldn't this point to quay.io/3scale/apicast:v2 ? people will not have apicst-test unless they built it locally

Yes, makes sense. I guess it was like that before because there were some fixes for the apicast needed for this to work properly. Not relevant now, as the fixes are already in master.

think about whether you want to point to a (potentially, constantly changing) "latest", or a known numbered version that you know worked when you wrote the example...

As this example is using a default APIcast functionality, it should work for the later versions, unless some breaking changes are introduced.
In case of breaking changes I think we need to go through the examples to verify they still work.so, I guess quay.io/3scale/apicast:v2 should work...

mayorova · 2016-12-14T07:44:37Z

examples/oauth2/auth-server/auth-server.rb

+require 'sinatra'
+
+set :bind, '0.0.0.0'
+nginx_redirect_uri =  "http://localhost:8080/callback?"  #nginx callback


I guess this example will only work when the Public Base URL is "localhost". Or it should be probably mentioned somewhere in README that this url needs to be adjusted to coincide with the API public base URL

We should probably just add this to the README for now.

mpguerra · 2016-12-14T09:42:36Z

I'm going to add some of these comments to my list of things to improve for a second iteration of this example, e.g for the nginx callback, this would ideally be a parameter that is provided to the auth server on startup.

However, I'm getting concerned that the comments on this PR are deviating from what was the intended purpose of this example which was something that "just works" for anyone that wants to try it out to see how to set up an example, run it, go through the token request process and see how it would work, the PR as it is does the trick when using docker-compose.

Otherwise it becomes an example that doesn't really run unless you have the correct variables plugged in which in a way defeats the purpose of having something anyone can run :)

mikz · 2016-12-14T10:04:00Z

@mpguerra if this is supposed to "just work"™️ it has to change the image in docker compose file to released apicast. Otherwise you'd have to build it locally first.

mikz

We have to replace apicast-test:latest in the docker compose with the normal release image. Otherwise people would have to build it locally for the example to work.

mikz · 2016-12-23T18:00:02Z

.env

@@ -1,27 +0,0 @@
-# URI to fetch gateway configuration from. Expected format is: https?://[password@]hostname
-THREESCALE_PORTAL_ENDPOINT=http://127.0.0.1.xip.io:8090/config/


I guess this was not intentional?

… Authorization server for testing remove sample env as it's already documented in README Update README Keep Gemfile.lock to lock gem versions Remove example config.json Docker fixes: remove Gemfile.lock from .dockerignore, copy application into /opt/app, remove redundant COPY from Dockerfile and move bundle install after Gemfile is copied

Change image to point to quay.io/3scale/apicast:v2

mayorova · 2016-12-29T13:33:29Z

Tested with the provided client, and it works.
Probably it would be easier to modify the same .env file instead of creating a new one in each directory client and auth-server if the user wants to override the gateway host, but probably it's fine for now.

mikz

It looks like there is some dead code in the html layouts.

mikz · 2016-12-29T13:40:30Z

examples/oauth2/auth-server/views/layout.erb

+
+    <!-- HTML5 shim, for IE6-8 support of HTML5 elements -->
+    <!--[if lt IE 9]>
+      <script src="/js/html5shiv.js"></script>


This is not in the repo so it should not be in the code either.

mikz · 2016-12-29T13:41:18Z

examples/oauth2/client/views/layout.erb

+
+    <!-- HTML5 shim, for IE6-8 support of HTML5 elements -->
+    <!--[if lt IE 9]>
+      <script src="/js/html5shiv.js"></script>


This has no value for the example and should not be here.

mikz · 2016-12-30T10:04:32Z

examples/oauth2/client/client.rb

+
+get("/") do	
+	if params[:action] == "get_token"
+		get_token(params[:code])


This can't work. This is not defined anywhere.

* cleanup docker-compose * fix ports * fix uuid generation * cleanup dockerignore files * use bundler

mikz · 2016-12-30T10:25:28Z

I did major cleanup to the docker setup and ruby dependencies. Would be good to get some 👍 and we can merge it.

Also if someone feels like providing steps in a form of a bash script we can run it as part of the test suite.

mayorova · 2016-12-30T10:31:43Z

examples/oauth2/docker-compose.yml

-      - "3001:3001"
-    volumes: 
-      - ./client:/opt/app/client
+      - "3001:3000"


why? should still be 3001:3001, no?

No. Both dockers expose port 3000. It just maps 3000 in docker to 3001 on local machine.

OK, I see. I was confused by this line I guess it's not needed?

Yep. Not needed. Will remove it.

mikz · 2017-01-02T10:57:55Z

examples/oauth2/client/client.rb

+set :bind, '0.0.0.0'
+enable :sessions
+set :session_secret, '*&(^B234'
+set :port, 3001


This should be removed. Also the bind is not necessary.

SidonieGaffori added the in progress label Nov 23, 2016

mpguerra mentioned this pull request Nov 23, 2016

Update apicast.conf #145

Closed

mayorova reviewed Nov 23, 2016

View reviewed changes

mpguerra force-pushed the merge-oauth-examples branch from 0d85fdb to 732b581 Compare November 25, 2016 14:50

andrewdavidmackenzie reviewed Nov 25, 2016

View reviewed changes

andrewdavidmackenzie requested changes Nov 25, 2016

View reviewed changes

andrewdavidmackenzie reviewed Nov 25, 2016

View reviewed changes

andrewdavidmackenzie changed the title ~~Merge oauth examples~~ OAuth examples Nov 29, 2016

andrewdavidmackenzie added the needs: rework label Nov 29, 2016

mayorova force-pushed the merge-oauth-examples branch from b54799b to 27332b9 Compare December 4, 2016 22:26

mayorova force-pushed the merge-oauth-examples branch from 27332b9 to b21f87b Compare December 4, 2016 22:35

mayorova added needs: review and removed needs: rework labels Dec 4, 2016

andrewdavidmackenzie approved these changes Dec 5, 2016

View reviewed changes

mikz reviewed Dec 5, 2016

View reviewed changes

mayorova reviewed Dec 14, 2016

View reviewed changes

mikz suggested changes Dec 23, 2016

View reviewed changes

mikz reviewed Dec 23, 2016

View reviewed changes

Maria Pilar Guerra Arias and others added 2 commits December 27, 2016 11:57

Update docker-compose.yml

2d70779

Change image to point to quay.io/3scale/apicast:v2

mpguerra force-pushed the merge-oauth-examples branch from 8379c61 to 2827990 Compare December 27, 2016 11:00

Maria Pilar Guerra Arias added 3 commits December 27, 2016 12:05

Allow Gateway host to be overridden via a .env file

cc514d3

Add client implementation to request access token only

7dcca41

Update README with client and updated auth-server instructions

551fb7f

mpguerra force-pushed the merge-oauth-examples branch from 2827990 to 551fb7f Compare December 27, 2016 11:05

mayorova added 2 commits December 29, 2016 12:49

Some README tweaks

24face1

Formatting and styles

4389f16

mikz suggested changes Dec 29, 2016

View reviewed changes

mikz added 2 commits December 30, 2016 10:49

[examples] remove unused comments in oauth2

f95b0d2

[examples] oauth example is not in apicast folder

aaa4f01

mikz reviewed Dec 30, 2016

View reviewed changes

mikz added 3 commits December 30, 2016 11:10

[examples] cleanup oauth2 ruby and docker files

aeecf13

[examples] oauth2 expects .env file to not be in git

8b2937e

[examples] cleanup oauth2 example

37eb122

* cleanup docker-compose * fix ports * fix uuid generation * cleanup dockerignore files * use bundler

mayorova reviewed Dec 30, 2016

View reviewed changes

mikz approved these changes Dec 30, 2016

View reviewed changes

mikz suggested changes Jan 2, 2017

View reviewed changes

Clean up client code: remove bind and port definitions

0a8e3d1

mikz approved these changes Jan 2, 2017

View reviewed changes

mikz merged commit 498ecf4 into v2 Jan 2, 2017

mikz deleted the merge-oauth-examples branch January 2, 2017 12:55

mikz removed the in progress label Jan 2, 2017

		@@ -0,0 +1,20 @@
		# URI to fetch gateway configuration from. Expected format is: https?://[password@]hostname

		@@ -1,27 +0,0 @@
		# URI to fetch gateway configuration from. Expected format is: https?://[password@]hostname
		THREESCALE_PORTAL_ENDPOINT=http://127.0.0.1.xip.io:8090/config/

OAuth examples #153

OAuth examples #153

Conversation

mpguerra commented Nov 23, 2016

mayorova Nov 23, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mayorova commented Nov 23, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

andrewdavidmackenzie left a comment

Choose a reason for hiding this comment

andrewdavidmackenzie commented Nov 25, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

andrewdavidmackenzie commented Nov 29, 2016

mayorova commented Dec 4, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mpguerra commented Dec 14, 2016

mikz commented Dec 14, 2016

mikz left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mayorova commented Dec 29, 2016

mikz left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mikz commented Dec 30, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mayorova Nov 23, 2016 •

edited

Loading