Add helper generate and validate token #3907

shubhscoder · 2018-11-07T03:57:58Z

Fixes #3848
Additions :

Added generate_token functions that generates a token for the user
Added validate_token function that verifies the generated token and makes sure that the token is generated within 24 hours.

Thanks!

shubhscoder · 2018-11-07T04:00:36Z

@Radhikadua123 , Does this seem fine?
@publiclab/reviewers

plotsbot · 2018-11-07T04:12:15Z

	1 Message
📖	@shubhscoder 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.

Generated by 🚫 Danger

…_token' into add_helper_generate_and_validate_token

jywarren · 2018-11-07T04:48:00Z

Aha, cool! Would you consider adding a simple test to /test/unit/user_test.rb to confirm these work? Thank you!

jywarren · 2018-11-07T04:48:18Z

@Radhikadua123 this is looking good, huh? 😄

shubhscoder · 2018-11-07T06:56:56Z

Yeah i ll do that 👍

…

On Wed, Nov 7, 2018, 10:18 AM Jeffrey Warren ***@***.***> wrote: Aha, cool! Would you consider adding a simple test to /test/unit/user_test.rb to confirm these work? Thank you! — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#3907 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/ARofxeWcB6fk8KLEDEqlWrx8dQ45RUEgks5usmYBgaJpZM4YRvcy> .

shubhscoder · 2018-11-07T11:18:59Z

@jywarren . I have added the tests. Thank you!

Radhikadua123 · 2018-11-07T17:07:35Z

test/unit/user_test.rb

+
+  test 'generate token and validate token for user email verification' do
+    all_users = User.where("id<?",3)
+    for i in all_users


It's fine to run it for one user only. :)

Radhikadua123 · 2018-11-07T17:08:27Z

Hi @shubhscoder,

Thanks a lot for the PR! That's the way we wanted to implement it. :)

Can you please add more test for it to cover all the cases ?

e.g.

Test to confirm that token generated more than 24hrs back from now should be treated as invalid.
Test to confirm that token for one user can't be used for any other user.
Test to confirm that invalid token raises respective exception

I'll close your other PRs to keep reviewboard clean.

jywarren · 2018-11-08T00:03:36Z

oh, this is looking awesome. Great work! And thanks Radhika for helping this along!

…

On Wed, Nov 7, 2018 at 12:08 PM Radhika Dua ***@***.***> wrote: Hi @shubhscoder <https://github.com/shubhscoder>, Thanks a lot for the PR! :) Can you please add more test for it to cover all the cases ? e.g. 1. Test to confirm that token generated 24hrs back from now should be treated as invalid. 2. Test to confirm that token for one user can't be used for any other user. 3. Test to confirm that invalid token raises respective exception — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#3907 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AABfJzCdOizo8xGyu3dIlUxj2qUSwXdAks5usxOLgaJpZM4YRvcy> .

shubhscoder · 2018-11-08T01:45:19Z

Yeah I ll do that 👍

…

On Wed, Nov 7, 2018, 10:38 PM Radhika Dua ***@***.***> wrote: Hi @shubhscoder <https://github.com/shubhscoder>, Thanks a lot for the PR! :) Can you please add more test for it to cover all the cases ? e.g. 1. Test to confirm that token generated 24hrs back from now should be treated as invalid. 2. Test to confirm that token for one user can't be used for any other user. 3. Test to confirm that invalid token raises respective exception — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#3907 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/ARofxWJtfM9axrr8SZOi3F2f-iK2j--Pks5usxONgaJpZM4YRvcy> .

shubhscoder · 2018-11-09T10:34:20Z

@jywarren @Radhikadua123 I have added the failing tests. Check if this looks good. Thank you!

Radhikadua123

Awesome work @shubhscoder!

Sorry for these requested changes! Though things would work even without these fixes. It's just to maintain quality of code. :)

Radhikadua123 · 2018-11-09T11:25:39Z

test/unit/user_test.rb

+    generated_token = generated_token[2,generated_token.length]
+    begin 
+      assert_not_equal all_users[0].validate_token(generated_token), true
+    rescue => error


Please use assert_raise for testing this.

Radhikadua123 · 2018-11-09T11:30:26Z

test/unit/user_test.rb

@@ -220,4 +221,27 @@ class UserTest < ActiveSupport::TestCase
    #as the username as "jeff" exists, hence username = "jeff" + 2 digit alphanumeric code will be created
    assert_not_equal jeffrey.username, "jeff"
  end
+
+  test 'generate token and validate token for user email verification' do


Almost there, just split these all these tests into multiple functions.

In unit tests, you should try tests to be as small as possible so that they test one small functionality properly. When they fail, developer easily get to know which test failed.

Radhikadua123 · 2018-11-09T11:32:09Z

app/models/user.rb

@@ -416,6 +417,22 @@ def customize_digest(type)
    end
  end

+  def generate_token
+    user_id_and_time = [id, Time.now]


May be we should use hash here instead of array ?
{ :id => id, :timestamp => Time.now }

That would be more descriptive than using something like decrypted_data[0].

shubhscoder · 2018-11-09T12:53:04Z

@Radhikadua123 I thinked I have fixed them all! And thank you for guiding me through the process. If this goes fine, then I will start working on #3850 .

Radhikadua123 · 2018-11-09T14:14:05Z

@shubhscoder Thanks for fixing the issues!

If this goes fine, then I will start working on #3850 .

Awesome!

@jywarren It looks good to me. Please review the changes.

jywarren · 2018-11-09T20:08:06Z

Oh, lovely - thanks!!! Great tests, too!

* Added is_verified column to users with default value false * added helper functions for generation and validation of tokens * Delete 20181103114645_add_is_verified_to_users.rb * Minor code fixes * Added tests for implemented helper functions * Added failing tests for helper functions * Added test to make sure that a token is not validated 24 hours after gen * Code quality changes

shubhscoder added 2 commits November 7, 2018 09:18

Added is_verified column to users with default value false

616d1c8

added helper functions for generation and validation of tokens

b947a1a

shubhscoder added 2 commits November 7, 2018 09:31

Delete 20181103114645_add_is_verified_to_users.rb

0987c8a

Minor code fixes

1b5b27c

Merge remote-tracking branch 'origin/add_helper_generate_and_validate…

a28f394

…_token' into add_helper_generate_and_validate_token

Added tests for implemented helper functions

464b6dd

Radhikadua123 reviewed Nov 7, 2018

View reviewed changes

This was referenced Nov 7, 2018

Email authentication changes #3880

Closed

Added is_verified column to users with default value false #3885

Closed

shubhscoder added 2 commits November 9, 2018 15:48

Added failing tests for helper functions

82e5017

Added test to make sure that a token is not validated 24 hours after gen

53fba1b

Radhikadua123 reviewed Nov 9, 2018

View reviewed changes

Code quality changes

e4e17b8

jywarren merged commit 788ff6f into publiclab:master Nov 9, 2018

shubhscoder deleted the add_helper_generate_and_validate_token branch November 10, 2018 09:17

SidharthBansal added this to the OAuth milestone Dec 18, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add helper generate and validate token #3907

Add helper generate and validate token #3907

shubhscoder commented Nov 7, 2018

shubhscoder commented Nov 7, 2018

plotsbot commented Nov 7, 2018 •

edited

Loading

jywarren commented Nov 7, 2018

jywarren commented Nov 7, 2018

shubhscoder commented Nov 7, 2018 via email

shubhscoder commented Nov 7, 2018

Radhikadua123 Nov 7, 2018

Radhikadua123 commented Nov 7, 2018 •

edited

Loading

jywarren commented Nov 8, 2018 via email

shubhscoder commented Nov 8, 2018 via email

shubhscoder commented Nov 9, 2018

Radhikadua123 left a comment

Radhikadua123 Nov 9, 2018

Radhikadua123 Nov 9, 2018

Radhikadua123 Nov 9, 2018

shubhscoder commented Nov 9, 2018

Radhikadua123 commented Nov 9, 2018 •

edited

Loading

jywarren commented Nov 9, 2018

Add helper generate and validate token #3907

Add helper generate and validate token #3907

Conversation

shubhscoder commented Nov 7, 2018

shubhscoder commented Nov 7, 2018

plotsbot commented Nov 7, 2018 • edited Loading

jywarren commented Nov 7, 2018

jywarren commented Nov 7, 2018

shubhscoder commented Nov 7, 2018 via email

shubhscoder commented Nov 7, 2018

Radhikadua123 Nov 7, 2018

Choose a reason for hiding this comment

Radhikadua123 commented Nov 7, 2018 • edited Loading

jywarren commented Nov 8, 2018 via email

shubhscoder commented Nov 8, 2018 via email

shubhscoder commented Nov 9, 2018

Radhikadua123 left a comment

Choose a reason for hiding this comment

Radhikadua123 Nov 9, 2018

Choose a reason for hiding this comment

Radhikadua123 Nov 9, 2018

Choose a reason for hiding this comment

Radhikadua123 Nov 9, 2018

Choose a reason for hiding this comment

shubhscoder commented Nov 9, 2018

Radhikadua123 commented Nov 9, 2018 • edited Loading

jywarren commented Nov 9, 2018

plotsbot commented Nov 7, 2018 •

edited

Loading

Radhikadua123 commented Nov 7, 2018 •

edited

Loading

Radhikadua123 commented Nov 9, 2018 •

edited

Loading