davido / gerrit-oauth-provider

OAuth2 authentication provider for Gerrit Code Review. Please upload changes for review to: https://gerrit-review.googlesource.com/#/admin/projects/plugins/oauth
Apache License 2.0
140 stars 84 forks source link

Untrusted identity problem #33

Open spinus opened 9 years ago

spinus commented 9 years ago

-> Lost admin group, my identitty is "untrusted"

solution: I had to add

[auth]
        allowedOpenID = ^.*$
        trustedOpenID = ^.*$

to the config (by default it accepts only ids starting with http:// or https://, so oauth "numbers" were not matched).

As gerrit "sees" oauth plugin as OpenID identities (it's probably to gerrit internals, I'm not sure) it would be nice to add some prefix to the EXTERNAL_IDS table. For example, ssh identities have "username:" prefix. OpenID identities have URL, but for this plugin the content of the field is just some number (user id on oauth provider side?).

Maybe this could be the URL to user account on provider side or something like:

Also would be nice to add this tip to readme, it took a while to figure it out.

davido commented 9 years ago

There is similar discussion in context of this change on gerrit-review: [1].

lucamilanesio commented 9 years ago

Agreed on the prefix. Btw the current OAuth / GitHub implementation (based on HTTP authentication) uses the following scheme: external:github_oauth:NNNNNN (using the Gerrit's ability to store external identities).

Defining a new scheme for oauth would then result in: oauth:github:NNNNNN

davido commented 9 years ago

@lucamilanesio So you are going to use oauth-provider-name as middle suffix (not plugin name!)? What when two different gerrit-oauth-provider plugin would use the same name?

Let's say, both gerrit-oauth-provider and GitHub plugin would choose "github" as middle prefix, and GitHub-login-id as external_id, then it would end up with:

for both plugins, when both OAuth-providers are deployed on the same gerrit site. That cannot work either.

lucamilanesio commented 9 years ago

@davido true, we need to keep the plugin name as well in the syntax somewhere :-)

davido commented 9 years ago

@lucamilanesio We should be able to agree on that ;-)

lucamilanesio commented 9 years ago

oauth:provider/plugin-name:NNNN ... how does it sound?

Example: oauth:github/github-plugin:lucamilanesio

davido commented 9 years ago

Or just plugin name (with or without provider-suffix in my case), to follow DRY principle?

GitHub:

Gerrit-OAuth-Provider:

or mit suffix:

?

spinus commented 9 years ago

I havent't looked at intetrnals yet but maybe if both plugins use the same user id the prefix and whole auth string could be used by both plugins (oauth procedure it done the same way)?

davido commented 9 years ago

Yes, I asked another gerrit-oauth-provider plugin stock holder, and he voted for your proposal:

oauth:github:lucamilanesio
oauth:google:394875623489576398756395

No matter which plugin identified this entry. This would even allow to replace the plugins and still all would keep working, at least this is the plan ;-)

spinus commented 9 years ago

Great stuff. Cheers.

lucamilanesio commented 9 years ago

@davido not really as different plugins for the same OAuth provider could use different external ids (gerrit-oauth-provider uses the GitHub internal ID whilst GitHub plugin uses GitHub username).

We do need to include the plugin name as well, in order to identity who is managing and has provided that identity over time.

davido commented 9 years ago

@lucamilanesio Good catch! I wonder why I did it and if changing the semantic in my plugin would be better approach? I was confused that Google OAuth doesn't such a thing as user id, and i went to if, and added GitHub later to the plugin and missed to switch to the login external id.