Identity model created

[SidharthBansal]

#2388
Make sure these boxes are checked before your pull request (PR) is ready to be reviewed and merged. Thanks!

• tests pass -- look for a green checkbox ✔️ a few minutes after opening your PR -- or run tests locally with rake test
• code is in uniquely-named feature branch and has no merge conflicts
• PR is descriptively titled
• PR body includes fixes #0000-style reference to original issue #
• ask @publiclab/reviewers for help, in a comment below

We're happy to help you get this ready -- don't be afraid to ask for help, and don't be discouraged if your tests fail at first!

If tests do fail, click on the red X to learn why by reading the logs.

Please be sure you've reviewed our contribution guidelines at https://publiclab.org/contributing-to-public-lab-software

We have a loose schedule of reviewing and pulling in changes every Tuesday and Friday, and publishing changes on Fridays.

Thanks!

[SidharthBansal]

@publiclab/reviewers I have ran rake db:migrate on my local machine but travis shows that the migrations are pending. How shall I run migrations on travis?

[seafr]

You should probably use a validation here to check for the presence of user_id

[SidharthBansal]

Oh thanks @seafr
I did change it.

[jywarren]

You can update the db/schema.rb.example file and it'll use the latest schema. Looking good!

[SidharthBansal]

Hi @jywarren still the Travis is failing what I need to do??

[grvsachdeva]

hi @SidharthBansal , you just need to copy the checksum of migration into the top of schema.rb.example and test will pass.

[PublicLabBot]

	1 Warning
⚠️	New migrations added. Please update schema.rb.example by overwriting it with a copy of the up-to-date db/schema.rb. Also, be aware to preserve the MySQL-specific conditions for full-text indices.

	2 Messages
📖	@SidharthBansal Thank you for your pull request! I’m here to help with some tips and recommendations. Please take a look at the list provided and help us review and accept your contribution! And don’t be discouraged if you see errors – we’re here to help.
📖	It looks like you haven’t marked all the checkboxes. Help us review and accept your suggested changes by going through the steps one by one. If it is still a ‘Work in progresss’, please include ‘[WIP]’ in the title.

Generated by 🚫 Danger

[SidharthBansal]

Thanks @Gauravano .
@jywarren, @publiclab/reviewers can you kindly review it?

[jywarren]

Looks awesome! Should I merge now, or are you going to add some unit tests? Or will that be in a follow-up PR?

[SidharthBansal]

I will add them in following prs

[jywarren]

Hi, I was thinking a bit here -- is there a reason to create a new database model for this, or do you think we could store this information in a UserTag? I think the key is - is any of the information now or in the future "secret" -- like, an auth token, or a secret key -- because then a unique table makes sense.

Sorry this took me a few days of thinking to articulate this question. But I don't think going the other route would be too much trouble given how well you've modularized your planning. What do you think, and are there pros/cons to the UserTag approach? Would it make your life easier or harder?

[SidharthBansal]

Hi @jywarren there is a need to create a new table to hold the provider (Facebook,twitter ,google ,github etc) ,uid for the id given by the provider of the account and the user-id to associate the identity with the user model. We would login through identity. We require multiple providers aka identities for a single user. When try to login or logout we will use these models.
We can't do it in usertag model. If I am not wrong, usertag is the tags which are displayed on the profile page rt? We require those tags for all the providers. We require a separate model for login and logout.

Yes we require future information the uid given by the provider and the name of the provider. The app secret and the app id we need to write them in the config files.
The user tag model has uid , value ,updated at and created at fields in the schema. We do require other parameters uid given by the provider and name of provider as in multi party login two or more author service may give us the same uid. Does this makes sense?
Also, there would be ability to delete the identity at time of unlinking of the account and making new one at on linking new provider.

[jywarren]

So in UserTag, we would follow the Power Tags convention of `key:value` as strings, so my thought was to do `accountProvider:user-id` -- but if we ever need a third parameter, or a secret parameter, then this won't work as well. and for multi party login, we could have: `twitter:jywarren` `twitter:warren` for example. Would that work? Or can you add in here some example cases that would demonstrate how UserTag would not be possible? Sorry if I'm being dense here, just trying to see if we can save complexity! But if we cannot, this Identity model looks fantastic.

…

On Fri, Apr 6, 2018 at 10:30 AM, Sidharth Bansal ***@***.***> wrote: Hi @jywarren <https://github.com/jywarren> there is a need to create a new table to hold the provider (Facebook,twitter ,google ,github etc) ,uid for the id given by the provider of the account and the user-id to associate the identity with the user model. We would login through identity. We require multiple providers aka identities for a single user. When try to login or logout we will use these models. We can't do it in usertag model. If I am not wrong, usertag is the tags which are displayed on the profile page rt? We require those tags for all the providers. We require a separate model for login and logout. Yes we require future information the uid given by the provider and the name of the provider. The app secret and the app id we need to write them in the config files. The user tag model has uid , value ,updated at and created at fields in the schema. We do require other parameters uid given by the provider and name of provider as in multi party login two or more author service may give us the same uid. Does this makes sense? Also, there would be ability to delete the identity at time of unlinking of the account and making new one at on linking new provider. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#2556 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AABfJ4gQtUwYeSSJGp1bswC7gjJ9Mu28ks5tl4ovgaJpZM4S__Bl> .

[SidharthBansal]

@jywarren we can adopt the value as "facebook:warren", "twitter:jwarren" etc. But we may need to retrieve and store other apart from these in future like these

{
  :provider => "twitter",
  :uid => "123456",
  :info => {
    :nickname => "johnqpublic",
    :name => "John Q Public",
    :location => "Anytown, USA",
    :image => "http://si0.twimg.com/sticky/default_profile_images/default_profile_2_normal.png",
    :description => "a very normal guy.",
    :urls => {
      :Website => nil,
      :Twitter => "https://twitter.com/johnqpublic"
    }
  },
  :credentials => {
    :token => "a1b2c3d4...", # The OAuth 2.0 access token
    :secret => "abcdef1234"
  },
  :extra => {
    :access_token => "", # An OAuth::AccessToken object
    :raw_info => {
      :name => "John Q Public",
      :listed_count => 0,
      :profile_sidebar_border_color => "181A1E",
      :url => nil,
      :lang => "en",
      :statuses_count => 129,
      :profile_image_url => "http://si0.twimg.com/sticky/default_profile_images/default_profile_2_normal.png",
      :profile_background_image_url_https => "https://twimg0-a.akamaihd.net/profile_background_images/229171796/pattern_036.gif",
      :location => "Anytown, USA",
      :time_zone => "Chicago",
      :follow_request_sent => false,
      :id => 123456,
      :profile_background_tile => true,
      :profile_sidebar_fill_color => "666666",
      :followers_count => 1,
      :default_profile_image => false,
      :screen_name => "",
      :following => false,
      :utc_offset => -3600,
      :verified => false,
      :favourites_count => 0,
      :profile_background_color => "1A1B1F",
      :is_translator => false,
      :friends_count => 1,
      :notifications => false,
      :geo_enabled => true,
      :profile_background_image_url => "http://twimg0-a.akamaihd.net/profile_background_images/229171796/pattern_036.gif",
      :protected => false,
      :description => "a very normal guy.",
      :profile_link_color => "2FC2EF",
      :created_at => "Thu Jul 4 00:00:00 +0000 2013",
      :id_str => "123456",
      :profile_image_url_https => "https://si0.twimg.com/sticky/default_profile_images/default_profile_2_normal.png",
      :default_profile => false,
      :profile_use_background_image => false,
      :entities => {
        :description => {
          :urls => []
        }
      },
      :profile_text_color => "666666",
      :contributors_enabled => false
    }
  }
}

Consider the case of image it may be possible that the user has uploaded Image1 on fb as profile pic and Image2 on the twitter then we do need to show the same user different profile pics on public lab login through providers. If the user logs in through fb he will get the Image1 otherwise Image2.
We may require to know how many people are his friends in future. We may require location. It all depends on future needs.
Does this makes sense?
Don't be sorry, I like to dive deep into coding as it then becomes interesting 😃

[SidharthBansal]

Also, if we use the usertags we need not to shows the oauth related user tags on the profile page.

[jywarren]

ohhhhh wow! There's so much info there! Exciting, although also want to be considerate of people /not/ wanting to share it all. I think we could add fields like `time_zone:______` and `location:______` and `facebook-picture:_______` if we want, but one thing I'm thinking of that would /save/ us time is if the profile tag UI were what we used to store all of these, making it unnecessary to develop a separate Identities storage/management UI. I think most of these I can see are ones we could adapt to a UserTag in one way or another. The picture URL maybe wouldn't fit into a key:value format, now that I think of it. But we can do that in a follow-up; we don't have to come up with a way to use *every* piece of data here, at least not all at once. What do you think? we could automatically import a few of these into UserTags, then prompt people to remove the ones they don't want to share? Pros/cons to that idea?

…

On Fri, Apr 6, 2018 at 11:23 AM, Sidharth Bansal ***@***.***> wrote: Also, if we use the usertags we need not to shows the oauth related user tags on the profile page. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#2556 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AABfJ7PZlg3eSTSGi6yo8Oa8HVcYfxRUks5tl5adgaJpZM4S__Bl> .

[SidharthBansal]

Actually the user will grant the permissions for these fields. Some of the user grant some permission while the other grant other permissions. Also the various informations which we can get are different in case of different providers like we discussed the email case of twitter.
If we are using usertags then we need to migrate the database table to use these different information whenever needed in future.

[SidharthBansal]

One problem is if any malicious user(like me 😄 ) creates a usertag named "facebook:jwarren" same as yours then the OAuth logic will get corrupted.

We have given the capability to the user to add user tags then we need to change that.

[SidharthBansal]

One more thing to consider is the Usertag model has many power tags like "lat:"," lon:" etc. So, searching the "provider:uid" field among them all is having higher time complexity then searching in the identity model where we only need to search those fields which all have these "provider" and "uid".
We can optimise the search by first selecting the "provider" and then the "uid". Regarding space complexity I guess we will require the same space if we are using any model.

[jywarren]

Aha -- so if we were to use UserTag we'd have to perhaps forbid creation of ones prefixed with facebook:___ twitter:_____ etc UNLESS via the Oauth process. Could that work, hypothetically?

DB optimization is a good point, but relative to the complexity of searching the NodeTag table, this is not so bad, and indexing helps a lot.

What do you think of the above? We already prevent some types of powertags on nodes on these lines:

plots2/app/models/node.rb

Lines 835 to 845 in 9795328

	def can_tag(tagname, user, errors = false)
	if tagname[0..4] == 'with:'
	if User.find_by_username_case_insensitive(tagname.split(':')[1]).nil?
	errors ? I18n.t('node.cannot_find_username') : false
	elsif author.uid != user.uid
	errors ? I18n.t('node.only_author_use_powertag') : false
	elsif tagname.split(':')[1] == user.username
	errors ? I18n.t('node.cannot_add_yourself_coauthor') : false
	else
	true
	end

[SidharthBansal]

Yeah this way, we can do. I was also thinking of this way. Yeah, indexing helps you are right. So, let's close this PR and first quickly write validation for provider:uid first.
Thanks

[SidharthBansal]

Hi, I think the code which you have posted is not working

We should not be able to coauthor ourself. But we are able to add that tag. Also, some other tags are working which suppose to be not working.
We want that the user will not be able to create the usertags by himself (from /profile/username) but he would be able to create by OAuth process(from /signup).
One more problem which we will face later by not making user tags and identities as separate models is if there is some problem in any one logic then both the user tagging and oauth will be affected. And we dont want the oauth to be affected as if it will get affected then the whole user will be affected.

[SidharthBansal]

We are not using the can_tag method in the create action of the usertag controller but we are using it in the tagcontroller. So, we are able to create tags with the 'format:rss' etc which we should not be able to make.

plots2/app/controllers/user_tags_controller.rb

Lines 14 to 39 in ac3dce1

	if current_user && (current_user.role == 'admin' || current_user == user)
	if params[:name]
	tagnames = params[:name].split(',')
	tagnames.each do |tagname|
	name = tagname.downcase
	if UserTag.exists?(current_user.id, name)
	@output[:errors] << I18n.t('user_tags_controller.tag_already_exists')
	exist = true
	end
	unless exist
	user_tag = user.user_tags.build(value: name)
	if user_tag.save
	@output[:saved] << [name, user_tag.id]
	else
	@output[:errors] << I18n.t('user_tags_controller.cannot_save_value')
	end
	end
	end
	else
	@output[:errors] << I18n.t('user_tags_controller.value_cannot_be_empty')
	end
	else
	@output[:errors] << I18n.t('user_tags_controller.admin_user_manage_tags')
	end

plots2/app/controllers/tag_controller.rb

Lines 271 to 301 in ac3dce1

	if Tag.exists?(tagname, nid)
	@output[:errors] << I18n.t('tag_controller.tag_already_exists')
	elsif node.can_tag(tagname, current_user) === true || current_user.role == 'admin' # || current_user.role == "moderator"
	saved, tag = node.add_tag(tagname.strip, current_user)
	if tagname.split(':')[0] == "barnstar"
	CommentMailer.notify_barnstar(current_user, node)
	barnstar_info_link = '<a href="//' + request.host.to_s + '/wiki/barnstars">barnstar</a>'
	node.add_comment(subject: 'barnstar',
	uid: current_user.uid,
	body: "@#{current_user.username} awards a #{barnstar_info_link} to #{node.drupal_user.name} for their awesome contribution!")
	elsif tagname.split(':')[0] == "with"
	user = User.find_by_username_case_insensitive(tagname.split(':')[1])
	CommentMailer.notify_coauthor(user, node)
	node.add_comment(subject: 'co-author',
	uid: current_user.uid,
	body: " @#{current_user.username} has marked #{tagname.split(':')[1]} as a co-author. ")
	end
	if saved
	@tags << tag
	@output[:saved] << [tag.name, tag.id]
	else
	@output[:errors] << I18n.t('tag_controller.error_tags') + tag.errors[:name].first
	end
	else
	@output[:errors] << node.can_tag(tagname, current_user, true)
	end
	end

So I think first of all we should make the usertag's controller create action to work right.
I just want to know that can we apply the whole can_tag method on the usertag or we need only some parts of it? Was it left without can_tag intentionally or by mistake?
If it is left without can_tag method used, I am happy to send the patch @jywarren
The tags are working fine

[jywarren]

Aha - great catch. I think we should implement can_tag in UserTag as well!

[jywarren]

Great, deep work here, @SidharthBansal !!!

[SidharthBansal]

Yeah working on this today. Was not able to do it because I was having exams.