Plugins oauth2 #1584

josenavas · 2015-12-21T04:03:30Z

This modifies the plugins so they work with OAuth2.

This includes the commits from @squirrelo in #1562

…fact-oauth

antgonza · 2015-12-21T16:03:23Z

Just to double check, the plan is to close #1562 and fully review this one, right?

mortonjt · 2015-12-22T07:59:51Z

qiita_core/support_files/server.key

Maybe I'm missing something here...

Should this be a public key, not a private key?

Nope, the server needs the private key to run.

Will the server key be automatically regenerated? Otherwise, distributing a
private key is usually not a good idea
On Dec 22, 2015 09:13, "Jose Navas" notifications@github.com wrote:

In qiita_core/support_files/server.key
#1584 (comment):

@@ -0,0 +1,27 @@
+-----BEGIN RSA PRIVATE KEY-----

Nope, the server needs the private key to run.

—
Reply to this email directly or view it on GitHub
https://github.com/biocore/qiita/pull/1584/files#r48275147.

antgonza · 2015-12-24T14:41:32Z

@josenavas could you update your branch? Note that I merged #1562 with the idea that all test should pass on this one.

mortonjt · 2015-12-30T19:38:55Z

qiita_db/handlers/oauth2.py

Over-indent!

@mortonjt this still needs a pull and update before review.

…fact-oauth

josenavas · 2016-01-03T14:07:13Z

Done

antgonza · 2016-01-03T15:00:50Z

Now this has conflicts ... could you fix? Thanks.

josenavas · 2016-01-03T15:55:07Z

Done

antgonza · 2016-01-03T16:00:34Z

👍 when tests pass.

antgonza · 2016-01-03T16:50:28Z

Note that this PR is going to have failures, @josenavas can you fix?

squirrelo · 2016-01-04T11:05:27Z

qiita_plugins/target_gene/tgp/tests/test_qiita_client.py

This is duplicated from setUp

I can't remove this one - tests fail. The other 2, however, have been removed.

josenavas · 2016-01-07T17:26:24Z

Tests should be passing right now.

antgonza · 2016-01-07T20:33:10Z

Thanks, one more question, should the values of the keys be shown in the qiita-test-install output?

josenavas · 2016-01-07T21:17:36Z

That's probably a good point. However, how is that going to scale? Meaning, as the number of plugins increase, how ugly is going to be showing that list?

Do you think that adding to qiita-test-install output is the best one, or should we add a flag, --show-plugin-info that show such information? If so, we can potentially remove the print statements from the patch and just point to the script.

What do you think?

antgonza · 2016-01-07T21:18:50Z

I thought the plan was to have a certificate per system not per plugin, is that inaccurate?

josenavas · 2016-01-07T21:21:47Z

The certificate is for the system but each plugin has its own client id and client secret.

antgonza · 2016-01-07T21:24:03Z

IMOO is fine to display all the plugins ids/secrets in the qiita-test-install by default but fine if you want to add a flag for this.

josenavas · 2016-01-07T22:03:51Z

Ok, thanks! Will try to update later today - busy with classes right now...

squirrelo · 2016-01-08T02:58:51Z

That seems like a bad security issue if we are going to have people post their qiita-test-install outputs anywhere for debugging, like we do with print_qiime_config, as it will expose all the system's client username and passwords. Flag would be safer, but this issue may still happen if someone accidentally posts their output with the flag on.

antgonza · 2016-01-08T03:49:08Z

IMOO this is fine and why we have an email for help and not a forum. At least for the moment.

josenavas · 2016-01-08T17:23:35Z

Ok, I just realized that to do this correctly I need to do some modifications on the DB. The issue is that the system does not know which client_id belongs to each plugin and although @squirrelo suggested that is not needed for the oauth2 workflow, it is actually needed for the system to keep track of ownerships.

I have to work in something else right now, but I will update the PR soon.

josenavas · 2016-01-08T21:05:48Z

PR updated

antgonza · 2016-01-08T21:48:24Z

qiita_db/support_files/qiita-db.dbs

This has an extra h

antgonza · 2016-01-08T21:49:53Z

Thanks. One minor comment and then 👍

josenavas · 2016-01-08T22:07:48Z

Comments addressed

squirrelo · 2016-01-09T03:11:55Z

What do you mean ownerships? By oauth2 standards, there are no specific ownerships for keys. All you transmit is the client id and client secret, and no other information. If you want to transmit the plugin id or other information as well, that is no longer following oauth2 specification, and we need to discuss this.

josenavas · 2016-01-09T04:11:51Z

I'm not transmitting this information anywhere - is a information that the system needs to know to inform the user which client id and client secret needs to provide to the plugin config file - does it make sense?

squirrelo · 2016-01-09T04:20:15Z

Not really. There's no reason we couldn't use a single client id/secret per system for app plugins if we wanted to, and this may actually make sense for single-computer systems like laptop deploys, since it means the user only needs one client login instead of many. The multi-client login does makes sense for larger or distributed setups, however.
I do not think we need to tie specific logins to specific plugins when displaying, since it creates a tie in the display that does not actually exist in the system. The system does not actually connect a login to a plugin in at all, as you could just as easily use the client id/secret for plugin A on plugin B and still have it work fine. The display should reflect that.

josenavas · 2016-01-09T04:26:43Z

Ok this is getting annoying - in one of my previous comments I stated that each plugin has its own cliend_id/client_secret pair. If you didn't agree with that, you should raise up your concern then, instead of wasting my time. I'm not going to continue working on this until we reach a consensus.

antgonza · 2016-01-09T14:22:45Z

There is nothing in the code that limits the use of the same id/secret if the user chooses to, right? In that case, we are covering both scenarios, right?

josenavas · 2016-01-09T17:17:03Z

Yes - right now we are covering both scenarios.

josenavas · 2016-01-09T17:59:00Z

I'm unsure why the tests are failing here - my guess is that it has to do with the Oauth2 authorization. I've done a small change to see if tests pass now as I can't reproduce in my machine.

josenavas · 2016-01-09T19:53:59Z

Ok, it looks like it was a timing issue

josenavas · 2016-01-11T18:03:00Z

@squirrelo ping

Plugins oauth2

josenavas added 8 commits December 16, 2015 13:22

Merge branch 'artifact' of https://github.com/biocore/qiita into oauth2

cb99e04

Merge branch 'oauth2' of https://github.com/squirrelo/qiita into oauth2

e8169f3

Merge branch 'artifact' of https://github.com/biocore/qiita into oauth2

1004130

Merge branch 'artifact' of https://github.com/biocore/qiita into arti…

2aa5876

…fact-oauth

Merge branch 'oauth2' into artifact-oauth

f963137

Fixing the qiita client to use OAuth2

100b354

Fixing plugin tests

be34c7c

Adding python patch to create the target gene client oauth2 info

3a305d8

antgonza mentioned this pull request Dec 21, 2015

Oauth2 addition #1562

Merged

mortonjt reviewed Dec 22, 2015
View reviewed changes

josenavas mentioned this pull request Dec 23, 2015

Artifact plugin setup #1587

Merged

mortonjt reviewed Dec 30, 2015
View reviewed changes

Merge branch 'artifact' of https://github.com/biocore/qiita into arti…

b9d6931

…fact-oauth

Fixing conflicts

12dab96

squirrelo reviewed Jan 4, 2016
View reviewed changes

josenavas added 2 commits January 7, 2016 02:09

Fixing failing tests

dbccfca

Addressing comments

1c4ea5b

josenavas added 2 commits January 8, 2016 13:04

Adding relation table between software and oauth identifiers

e474219

Adding plugin information to the qiita-test-install script

1b2bdbc

antgonza reviewed Jan 8, 2016
View reviewed changes

qiita_db/support_files/qiita-db.dbs Outdated

Copy link

Member

antgonza Jan 8, 2016

Choose a reason for hiding this comment

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

This has an extra h

Fixing comments

c6eebfb

Fixing expire time

64e0713

squirrelo added a commit that referenced this pull request Jan 11, 2016

Merge pull request #1584 from josenavas/artifact-oauth

0909755

Plugins oauth2

squirrelo merged commit 0909755 into qiita-spots:artifact Jan 11, 2016

Uh oh!

Plugins oauth2 #1584

Plugins oauth2 #1584

Uh oh!

Conversation

josenavas commented Dec 21, 2015

Uh oh!

antgonza commented Dec 21, 2015

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

antgonza commented Dec 24, 2015

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

josenavas commented Jan 3, 2016

Uh oh!

antgonza commented Jan 3, 2016

Uh oh!

josenavas commented Jan 3, 2016

Uh oh!

antgonza commented Jan 3, 2016

Uh oh!

antgonza commented Jan 3, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

josenavas commented Jan 7, 2016

Uh oh!

antgonza commented Jan 7, 2016

Uh oh!

josenavas commented Jan 7, 2016

Uh oh!

antgonza commented Jan 7, 2016

Uh oh!

josenavas commented Jan 7, 2016

Uh oh!

antgonza commented Jan 7, 2016

Uh oh!

josenavas commented Jan 7, 2016

Uh oh!

squirrelo commented Jan 8, 2016

Uh oh!

antgonza commented Jan 8, 2016

Uh oh!

josenavas commented Jan 8, 2016

Uh oh!

josenavas commented Jan 8, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

antgonza commented Jan 8, 2016

Uh oh!

josenavas commented Jan 8, 2016

Uh oh!

squirrelo commented Jan 9, 2016

Uh oh!

josenavas commented Jan 9, 2016

Uh oh!

squirrelo commented Jan 9, 2016

Uh oh!

josenavas commented Jan 9, 2016

Uh oh!

antgonza commented Jan 9, 2016

Uh oh!

josenavas commented Jan 9, 2016

Uh oh!

josenavas commented Jan 9, 2016

Uh oh!

josenavas commented Jan 9, 2016

Uh oh!

josenavas commented Jan 11, 2016