Ensure that uid in OAuth is a string #7141

Merged
merged 1 commit into from Jul 23, 2014

Conversation

5 participants
@katafrakt
Contributor

katafrakt commented Jun 17, 2014

In our company we use Redmine as OAuth provider (with https://github.com/suer/redmine_oauth_provider and https://github.com/suer/omniauth-redmine on GitLab side). In this case uid returned from Redmine is its database ID as integer. This leads to:

PG::Error: ERROR:  operator does not exist: character varying = integer
LINE 1: ..."."provider" = 'redmine' AND "users"."extern_uid" = 422  ORD...
                                                             ^
HINT:  No operator matches the given name and argument type(s). You might need to add explicit type casts.
: SELECT  "users".* FROM "users"  WHERE "users"."provider" = 'redmine' AND "users"."extern_uid" = 422  ORDER BY "users"."id" DESC LIMIT 1
Completed 500 Internal Server Error in 27ms

ActiveRecord::StatementInvalid (PG::Error: ERROR:  operator does not exist: character varying = integer
LINE 1: ..."."provider" = 'redmine' AND "users"."extern_uid" = 422  ORD...
                                                             ^
HINT:  No operator matches the given name and argument type(s). You might need to add explicit type casts.
: SELECT  "users".* FROM "users"  WHERE "users"."provider" = 'redmine' AND "users"."extern_uid" = 422  ORDER BY "users"."id" DESC LIMIT 1):
  lib/gitlab/oauth/user.rb:56:in `find_by_uid_and_provider'
  lib/gitlab/oauth/user.rb:14:in `find'
  app/controllers/omniauth_callbacks_controller.rb:36:in `handle_omniauth'
  app/controllers/omniauth_callbacks_controller.rb:4:in `block (2 levels) in <class:OmniauthCallbacksController>'
  app/controllers/application_controller.rb:59:in `set_current_user_for_thread'

Casting uid to string fixes this issue.

lib/gitlab/oauth/user.rb
@@ -61,7 +61,8 @@ def find_by_uid_and_provider
end
def uid
- auth.info.uid || auth.uid
+ uid = auth.info.uid || auth.uid
+ uid.to_s rescue uid

This comment has been minimized.

@houndci-bot

houndci-bot Jun 17, 2014

Avoid using rescue in its modifier form.

@houndci-bot

houndci-bot Jun 17, 2014

Avoid using rescue in its modifier form.

This comment has been minimized.

@sodabrew

sodabrew Jun 18, 2014

Contributor

Is the rescue needed? What data types could uid be that would not support .to_s?

@sodabrew

sodabrew Jun 18, 2014

Contributor

Is the rescue needed? What data types could uid be that would not support .to_s?

This comment has been minimized.

@katafrakt

katafrakt Jun 19, 2014

Contributor

You are right. This was just me being overcautious. It also didn't handle nils properly and converted them into empty strings. I'm fixing it with next commit.

@katafrakt

katafrakt Jun 19, 2014

Contributor

You are right. This was just me being overcautious. It also didn't handle nils properly and converted them into empty strings. I'm fixing it with next commit.

@sodabrew

This comment has been minimized.

Show comment
Hide comment
@sodabrew

sodabrew Jun 20, 2014

Contributor

Looks great! I think I've hit this issue as well. Could you squash these commits using git rebase -i master?

Contributor

sodabrew commented Jun 20, 2014

Looks great! I think I've hit this issue as well. Could you squash these commits using git rebase -i master?

@katafrakt

This comment has been minimized.

Show comment
Hide comment
@katafrakt

katafrakt Jun 21, 2014

Contributor

I just did that.

Contributor

katafrakt commented Jun 21, 2014

I just did that.

@jvanbaarsen

This comment has been minimized.

Show comment
Hide comment
@jvanbaarsen

jvanbaarsen Jul 9, 2014

Contributor

@katafrakt Thanks!
@randx Looks ok to merge

Contributor

jvanbaarsen commented Jul 9, 2014

@katafrakt Thanks!
@randx Looks ok to merge

dzaporozhets added a commit that referenced this pull request Jul 23, 2014

@dzaporozhets dzaporozhets merged commit 931f27a into gitlabhq:master Jul 23, 2014

1 check failed

continuous-integration/travis-ci The Travis CI build could not complete due to an error
Details

@katafrakt katafrakt deleted the PuzzleFlow:uid_should_always_be_string branch Jul 23, 2014

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment