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

Adding multi-provider draft and enabling dev OAuth2 provider #25

Merged
merged 15 commits into from Sep 1, 2020
Merged

Adding multi-provider draft and enabling dev OAuth2 provider #25

merged 15 commits into from Sep 1, 2020

Conversation

danielbdias
Copy link

@danielbdias danielbdias commented Aug 19, 2020

Motivation and Context

The first time that you run Will.IAM you could have problems to setup a valid OAuth2 provider. It is implicit that you need to follow these steps and make a setup to enable Google OAuth2 to execute tests in development.

Description

To help with that I've changed the API to choose another OAuth2 provider from config and added a provider that calls a test OAuth2 server.

In future we can modify this dev server to allow a developer to simulate interactions with many users at same time using another systems that use Will.IAM to manage permissions.

How Has This Been Tested?

  1. Start Will.IAM auxiliary services using make compose-up. This command will start three docker containers: database, oauth2-server and Will.IAM itself.
    a. (extra) If it is your first time running Will.IAM, make be sure to run make db/setup first to create Will.IAM database;
  2. Assuming a Referer link as http://localhost:3000, just access the following link on a browser:
http://localhost:4040/sso/auth/do?referer=http%3A%2F%2Flocalhost%3A3000

It should trigger the OAuth2 authentication procedure (very similar to this explanation), returning to the referer link at the end.

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)

Checklist:

  • My code has tests.
  • My change requires a change to the documentation.
  • I have updated the documentation accordingly.

Makefile Outdated
@@ -32,7 +32,11 @@ docker/build:

.PHONY: run
run:
@reflex -c reflex.conf -- sh -c ./bin/Will.IAM start-api
make build && ./bin/Will.IAM start-api --host=localhost -v3
Copy link
Author

Choose a reason for hiding this comment

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

Without adding --host=localhost command, every time we compile and run Will.IAM we receive a MacOS Firewall message box asking us to allow connections to this app.
Telling that you are using localhost avoid it on development.

@@ -1,5 +1,10 @@
version: '2'
services:
oauth2-server:
Copy link
Author

Choose a reason for hiding this comment

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

To help us with OAuth2 I'm using oauth2-mock-server that mocks some requests to use.

switch verbosity {
case 0:
log.Level = logrus.InfoLevel
log.Level = logrus.ErrorLevel
Copy link
Author

Choose a reason for hiding this comment

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

I think that the verbosity code was added here by mistake. I added this hierarchy to make more sense to devs who want to use this option.

)

// ProviderBlankMock is a Provider mock will all dummy implementations
type ProviderBlankMock struct {
Copy link
Author

Choose a reason for hiding this comment

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

This provider is used only as a mock to some unit/integration tests on Will.IAM, so I've moved it untouched to another file.

@danielbdias danielbdias marked this pull request as ready for review August 20, 2020 19:43
Copy link

@rodopoulos rodopoulos left a comment

Choose a reason for hiding this comment

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

Besides the above, LGTM!

Makefile Outdated
@@ -32,7 +32,11 @@ docker/build:

.PHONY: run
run:
@reflex -c reflex.conf -- sh -c ./bin/Will.IAM start-api
make build && ./bin/Will.IAM start-api --host=localhost -v3

Choose a reason for hiding this comment

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

Instead of using make command inside each other, use target dependency:

Suggested change
make build && ./bin/Will.IAM start-api --host=localhost -v3
run: build
./bin/Will.IAM start-api --host=localhost -v3

compose-up:
@mkdir -p docker_data && docker-compose up -d
@until docker exec $(pg_docker_image) pg_isready; do echo 'Waiting Postgres...' && sleep 1; done
@sleep 2

Choose a reason for hiding this comment

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

Why having this?

Copy link
Author

Choose a reason for hiding this comment

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

Because the other similar command only start the database container postgres, skipping the new oauth2-server container.
Thinking now, to simplify this logic I'll an parameter here and edit db/up to send a container parameter.

Choose a reason for hiding this comment

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

Hm, having a better look, I see that the docker-compose only starts dependencies. Why not starting Will.IAM as well. An upside on this approach is using compose's depends_on feature.

With that, you can declare Will.IAM dependencies and having it wait until they are fully initialized. You only need to provide a healthcheck operations for each dependency, which should be pretty simple.

This is only a suggestion, if you'd like to address this in another PR, feel free to do it.

oauth2/dev.go Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
Daniel Dias and others added 3 commits August 21, 2020 16:36
Co-authored-by: Felipe Rodopoulos <felipekss@gmail.com>
Makefile Outdated
@mkdir -p docker_data && docker-compose up -d postgres
@until docker exec $(pg_docker_image) pg_isready; do echo 'Waiting Postgres...' && sleep 1; done
@sleep 2
make compose-up container=postgres

Choose a reason for hiding this comment

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

I don't think this approach will work if you need to wait for the dependency to be ready. The end of db/up target is not a guarantee that Postgres is accepting connections on the container.

Copy link

@rodopoulos rodopoulos left a comment

Choose a reason for hiding this comment

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

Everything good!

@danielbdias danielbdias merged commit c54320b into topfreegames:master Sep 1, 2020
@danielbdias danielbdias deleted the features/dev-provider branch September 1, 2020 17:14
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
2 participants