Add support for OAuth/OpenID Connect#1300
Conversation
README.md
Outdated
|
|
||
| - [Synchronization with your private registry in order to fetch which images and tags are available](http://port.us.org/features/1_Synchronizing-the-Registry-and-Portus.html). | ||
| - [LDAP user authentication](http://port.us.org/features/2_LDAP-support.html). | ||
| - [LDAP, OAuth, OpenID-Connect user authentication](http://port.us.org/features/2_LDAP-support.html). |
There was a problem hiding this comment.
Note that the link is for LDAP only. We can write another page for other services later. In the meantime, move the OAuth, OpenID-Connect into another point (without link then).
|
|
||
| def gitlab | ||
| auth_user | ||
| end |
There was a problem hiding this comment.
Instead of:
def google_oauth2
auth_user
end
def open_id
auth_user
end
def github
auth_user
end
def gitlab
auth_user
endDo the following:
alias_method :google_oauth2, :auth_user
alias_method :open_id, :auth_user
alias_method :github, :auth_user
alias_method :gitlab, :auth_user|
|
||
| private | ||
|
|
||
| def auth_user |
| def new | ||
| return new_user_session_url unless (data = oauth_data) | ||
| @user = User.new username: data["info"]["username"], display_name: data["info"]["name"] | ||
| @user.suggest_username data["info"] |
There was a problem hiding this comment.
I'd prefer something like this:
def new
data = oauth_data
if data
redirect_to new_user_session_url
else
@user = User.new username: data["info"]["username"], display_name: data["info"]["name"]
@user.suggest_username data["info"]
end
endMainly because I think doing this (data = oauth_data) is super ugly 😀. Plus, since it's a short method, we don't have to write it with an early return when a good ol' if/else statement would do it.
There was a problem hiding this comment.
Also, since suggest_username might return a generated username, make sure to show a flashy message being more explicit with it. Like "Successfully registered as myusername!"
| parameters: { username: username } | ||
| end | ||
|
|
||
| def self.create_from_oauth(params, data) |
There was a problem hiding this comment.
Could you add documentation on this ? Things like where does this data come from, the expected values, etc.
app/models/user.rb
Outdated
| User.create params | ||
| end | ||
|
|
||
| # If usernsme exists then try variant username + "_nn". |
There was a problem hiding this comment.
Typo: usernsme -> username.
lib/portus/config.rb
Outdated
| self[objs[0]][objs[1]]["enabled"].eql?(true) | ||
| else | ||
| return false if !self[feature] || self[feature].empty? | ||
| # return false if !self[feature] || self[feature].empty? |
There was a problem hiding this comment.
Can't you just remove this ?
|
Overall it looks good to me. Just fix my issues. Besides this, I'm adding @vitoravelino as a reviewer and I'll proceed to further test this locally. |
|
You may also want to document which callback to be used for each provider. I've tested Github and Gitlab. A couple of comments:
|
| strong Note: | ||
| | The first user to be created will have admin permissions ! | ||
|
|
||
| - elsif APP_CONFIG["oauth"] && APP_CONFIG["oauth"].find_all {|k, v| v["enabled"]}.size > 0 |
There was a problem hiding this comment.
Extract this condition to a helper
| | The first user to be created will have admin permissions ! | ||
|
|
||
| - elsif APP_CONFIG["oauth"] && APP_CONFIG["oauth"].find_all {|k, v| v["enabled"]}.size > 0 | ||
| .row |
There was a problem hiding this comment.
Extract the whole social login to its own partial.
| .btn-link { | ||
| color: $white | ||
| } | ||
| h1, h2, h3, h4, h5 { |
There was a problem hiding this comment.
I would prefer to change this to something like .social-login-title and add it to the html element.
| margin-bottom: 5px; | ||
| border-radius: 4px; | ||
| } | ||
| p { |
There was a problem hiding this comment.
Same apply to this. Be more specific.
There was a problem hiding this comment.
Btw, can you tell me where this is being applied? I couldn't find it.
| h1, h2, h3, h4, h5 { | ||
| color: $white; | ||
| padding: 6px 12px; | ||
| font-family: arial; |
There was a problem hiding this comment.
No need of font-family.
| } | ||
| h1, h2, h3, h4, h5 { | ||
| color: $white; | ||
| padding: 6px 12px; |
There was a problem hiding this comment.
I usually prefer to use margin instead of padding for spacing titles. In this case, I would guess margin-top 30px (20px default + 10px that you were already doing with padding) and margin-bottom 15px. That's my personal preference. What do you think?
Agreed. 👍 |
| # If the deployment is done through Puma, include it in the bundle. | ||
| -gem "puma", "~> 3.7.0" if ENV["PORTUS_PUMA_DEPLOYMENT"] == "yes" || !packaging? | ||
| +gem "puma", "~> 2.16.0" if ENV["PORTUS_PUMA_DEPLOYMENT"] == "yes" || !packaging? | ||
| +gem "puma", "= 2.16.0" if ENV["PORTUS_PUMA_DEPLOYMENT"] == "yes" || !packaging? |
There was a problem hiding this comment.
@mssola Is this change necessary? Just wondering...
There was a problem hiding this comment.
I don't know where it comes from. Will try to find out.
There was a problem hiding this comment.
It comes from a rebase I think. I introduced this change in a commit that happened when this was being reviewed-developed.
| public_activity | ||
| - puma (~> 3.7.0) | ||
| + puma (~> 2.16.0) | ||
| + puma (= 2.16.0) |
| = form_for @user, url: users_oauth_url do |f| | ||
| = f.text_field :username, class: 'input form-control input-lg first', placeholder: 'Username', autofocus: true, required: true | ||
| = f.text_field :display_name, class: 'input form-control input-lg last', placeholder: 'Display name' | ||
| = f.button 'Create account', class: 'classbutton btn btn-primary btn-block btn-lg' |
There was a problem hiding this comment.
Add .fa-check icon as in the standard registration page.
| = image_tag 'layout/portus-logo-login-page.png', class: 'login-picture' | ||
| .row | ||
| .col-sm-12 | ||
| h5.create-new-account Define your Username and Display Name |
There was a problem hiding this comment.
I would suggest to change to change this to p and ignore the .col-sm-12 and .row, going right below the image_tag.
You can also associate it with a class and style it as you might need.
|
I'm not sure if this is a bug in the |
vitoravelino
left a comment
There was a problem hiding this comment.
I've made a few comments... If you have any question, please feel free to reach me out, ok? I'll be more than happy to help you with it.
Thanks a lot for the PR, a really great feature! 👍
Of course, it possible to leave the username empty, but since the feature is @Vad1mo suggestion, I think we should wait for his comment. |
|
Since the username can't be changed once it is set the user need to choose hist uername quite wisely as he is going to use it for each image push/pull. In the first iteration of OAuth there wasn't even an option to set username and password. Thats why we decided to give the user the option to set its username when it is possible. From the usability and uniformity point of view this process would fit best:
Leaving the username out in the second case, is personally OK for me, however it would break the uniformity of the flow. |
dda7f98 to
cd9e640
Compare
|
@mssola @vitoravelino, what changes should we implement for this PR? |
config/config.yml
Outdated
| github: | ||
| enabled: false | ||
| # Application credentials. | ||
| key: "" |
There was a problem hiding this comment.
As @mssola mentioned a few comments ago, it would be nice to rename this to client_id and client_secret below to avoid confusion.
config/config.yml
Outdated
| # Gitlab authentication support. | ||
| gitlab: | ||
| enabled: false | ||
| id: "" |
There was a problem hiding this comment.
Here could be application_id
|
Missing space between icon and text:
|
|
I'm not sure if I missed this somewhere but it would be nice to have somewhere documented which callback url should be used for each service. Also, which are the minimum permissions to be selected to make the integration work (gitlab and bitbucket forces you to choose something). |
vitoravelino
left a comment
There was a problem hiding this comment.
After implementing those changes, it's a huge 👍 from myself.
|
@vitoravelino I have no image there callback URLs could be documented because it is Devise's feature. Devise makes route |
I know. What I mean is that when we are creating the oauth application on the providers' websites (e.g.: gitlab, github, etc), we are asked about the callback redirect url. It would be nice to have somewhere (it could even be in the This is not required to merge the PR, this is just a nice to have thing because users might be confused when trying to configure all of this. I was confused in the beginning for example, I had to check to find something related to callback. |
Signed-off-by: Vadim Bauer <bauer.vadim@gmail.com>
mssola
left a comment
There was a problem hiding this comment.
Portus crashed on bitbucket support if the team option is left empty.
| options: | ||
| # Only members of team can sign in/up with Bitbucket. Need permission to read team membership. | ||
| team: "" | ||
| # Set first_user_admin to true if you want that the first user that signs up |
There was a problem hiding this comment.
Leave an empty line of separation 😉
Implemented anonymous browsing
|
@mssola I got error in Travis could you explain what does it mean? |
|
@andrew2net feel free to ignore this error. It's due some packaging issues that I'm fixing in another PR |
|
@mssola for me Portus with on Bitbucket and the empty team works without errors. May I have your log? |
|
@andrew2net here it goes: Could it be that it's because I'm using a local development instance instead of a production reachable one ? |
|
@mssola I also using local development instance. The error comes from Fraday. Looks like it is some network issue. The piece of code related to team doesn't even start. Could you try late test both with team and without? |
Signed-off-by: Vadim Bauer <bauer.vadim@gmail.com>
Signed-off-by: Vadim Bauer <bauer.vadim@gmail.com>
mssola
left a comment
There was a problem hiding this comment.
The bitbucket failure for me is a simple networking issue. Connecting to bitbucket takes ages for me. Since @vitoravelino assures me that it works for him, I'll give my 👍
I'll fix the failures reported in Travis myself.
|
Thanks a lot for your patience and hard work ! 🎉 |
|
🎉 awesome! |
Adding OAuth Support with omniauth Signed-off-by: Vadim Bauer <bauer.vadim@gmail.com>
This PR adds OAuth Social Logins via omniauth.
OAuth Providers:
Closes #645
Foundation for #527
Features:
Social Login Buttons:
Option for the user the choose/modify the Username before the account is finally created.
New configuration Options