[WIP] Ensure preservation of return_to through OAuth login flow #3879

Radhikadua123 · 2018-11-01T16:01:58Z

@jywarren I didn't have push access to oauth-return-to branch. That's why I pushed to new one in my forked repo.

Reference: #3762

Radhikadua123 · 2018-11-01T16:03:24Z

@jywarren I couldn't test the whole functionality because oauth is disabled for local setup. How can I test it out ?

plotsbot · 2018-11-01T16:12:58Z

	2 Messages
📖	@Radhikadua123 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.
📖	Pull Request is marked as Work in Progress

Generated by 🚫 Danger

jywarren · 2018-11-01T18:16:25Z

Oh, this is cool! I'll add you as a contributor and you'll be able to push to unstable.publiclab.org where we do testing. It'll build for a bit (follow at https://jenkins.laboratoriopublico.org) and then you should be able to try this out. I'm not 100% sure OAuth will work the same, but it's worth a try.

jywarren · 2018-11-01T18:17:19Z

Also Code Climate has made some suggestions about your code, we don't have to adopt all of them but they're at least interesting to look at: https://codeclimate.com/github/publiclab/plots2/pull/3879

I can approve this to get CodeClimate to return an OK, if you like, or adopt some but not all -- up to you!

jywarren · 2018-11-01T18:19:05Z

Oh i'm sorry i already added you! So, to start testing on unstable, first just chime in on the chatroom to warn others (since it only supports one build at a time, maybe someone else is using it... best to ask!) then run git push -f git@github.com:publiclab/plots2.git unstable and it will begin to build.

Radhikadua123 · 2018-11-01T20:23:26Z

you'll be able to push to unstable.publiclab.org where we do testing. It'll build for a bit (follow at https://jenkins.laboratoriopublico.org) and then you should be able to try this out. I'm not 100% sure OAuth will work the same, but it's worth a try.

I see.

I can approve this to get CodeClimate to return an OK, if you like, or adopt some but not all -- up to you!

Sure, I'll check them out.

Oh i'm sorry i already added you! So, to start testing on unstable, first just chime in on the chatroom to warn others (since it only supports one build at a time, maybe someone else is using it... best to ask!) then run git push -f git@github.com:publiclab/plots2.git unstable and it will begin to build.

~~Oh. Where can I access chatroom ?~~ Nevermind, I found it. I didn't know we also had a chatroom.

jywarren · 2018-11-01T20:27:28Z

Ah, sorry! https://publiclab.org/chat has several options for joining! welcome!

…

On Thu, Nov 1, 2018 at 4:23 PM Radhika Dua ***@***.***> wrote: you'll be able to push to unstable.publiclab.org where we do testing. It'll build for a bit (follow at https://jenkins.laboratoriopublico.org) and then you should be able to try this out. I'm not 100% sure OAuth will work the same, but it's worth a try. I see. I can approve this to get CodeClimate to return an OK, if you like, or adopt some but not all -- up to you! Sure, I'll check them out. Oh i'm sorry i already added you! So, to start testing on unstable, first just chime in on the chatroom to warn others (since it only supports one build at a time, maybe someone else is using it... best to ask!) then run git push -f ***@***.***:publiclab/plots2.git unstable and it will begin to build. Oh. Where's the chatroom ? — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#3879 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AABfJ1QDhykTQ5S7sAAumt8Id466QynAks5uq1g_gaJpZM4YHFk2> .

Radhikadua123 · 2018-11-01T21:18:45Z

@jywarren It's working fine now on https://unstable.publiclab.org

@publiclab/reviewers @SidharthBansal Please review the code.

Also it involves some UI related changes. I removed the "Login" dropdown from the navigation bar. And clicking on "Login" redirects to login page. This removes up duplicate code and also has better UX for mobile users.

You can check it out at https://unstable.publiclab.org

Radhikadua123 · 2018-11-01T21:22:01Z

app/controllers/user_sessions_controller.rb

+
+    if @user&.drupal_user&.status == 1
+      # an existing Rails user
+      if @user


@jywarren Is there any difference in @user and @user.nil in this case ? If not, then we can remove if condition because we already have check for that in above code.

I think you're right, here. Let's just be very careful and make changes incrementally since this is a pretty complex piece of code. It'll be easier to track any bugs we may introduce if we change fewer things at a time. I know this is not as satisfying as more thorough refactor here, but our login code is relatively critical, so we have to be careful, and not every thing can be protected by tests. Thanks!

Radhikadua123 · 2018-11-01T21:22:41Z

app/controllers/user_sessions_controller.rb

+      return
+    end
+
+    if @user&.drupal_user&.status.zero?


Banned users have status code 0, right ? I got to know that from the tests. Just wanted to confirm this.

This is a little different, this is our legacy drupal users status. We should check against status of just @user. But I believe that is correct, yes! And statuses should be synced but let's not rely on that as we are slowly working to remove drupal_user code in #2838

Radhikadua123 · 2018-11-01T21:23:36Z

app/controllers/user_sessions_controller.rb

+    end
+
+    if @user.nil?
+      flash[:warning] = "There is nobody in our system by that name, are you sure you have the right username?"


Generally sites don't tell this up. They just say "Invalid username or password".

yeah, you can change it if you think that'd be best, good catch!

Radhikadua123 · 2018-11-01T21:24:44Z

app/controllers/user_sessions_controller.rb

+      return
+    end
+
+    params[:user_session][:username] = params[:openid] if params[:openid] # second runthrough must preserve username


I thought of simplifying these parts but I don't know internals of the system. So, I didn't touch them. Things which I changed, I commented about them. :)

GREAT approach! As i mention below, we want to be conservative about refactoring the signup process.

jywarren · 2018-11-02T21:59:01Z

OK, sounds like there are a couple spots you'd like to refine before merging this? Thanks so much!!!

Radhikadua123 · 2018-11-02T22:53:52Z

I know this is not as satisfying as more thorough refactor here, but our login code is relatively critical, so we have to be careful, and not every thing can be protected by tests.
OK, sounds like there are a couple spots you'd like to refine before merging this? Thanks so much!!!

I can't agree more to this. I'll split this PR into multiple small patches by which it would be easier to catch bugs in whole process. But before doing that, just wanted to confirm if you agree with the following:

Also it involves some UI related changes. I removed the "Login" dropdown from the navigation bar. And clicking on "Login" redirects to login page. This removes up duplicate code and also has better UX for mobile users.

You can check it out at https://unstable.publiclab.org

jywarren · 2018-11-03T04:37:05Z

Hmm, I think this is ok, though I do wonder about if the non mobile page is slightly smoother with the drop-down... Are we able to have a separate page just for mobile, even if it is still code duplication a bit? Thank you for your consideration on this! I agree it'll make a big difference for mobile users!

…

On Fri, Nov 2, 2018, 6:54 PM Radhika Dua ***@***.*** wrote: I know this is not as satisfying as more thorough refactor here, but our login code is relatively critical, so we have to be careful, and not every thing can be protected by tests. OK, sounds like there are a couple spots you'd like to refine before merging this? Thanks so much!!! I can't agree more to this. I'll split this PR into multiple small patches by which it would be easier to catch bugs in whole process. But before doing that, just wanted to confirm if you agree with the following: Also it involves some UI related changes. I removed the "Login" dropdown from the navigation bar. And clicking on "Login" redirects to login page. This removes up duplicate code and also has better UX for mobile users. You can check it out at https://unstable.publiclab.org — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#3879 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AABfJxoIAHLgRwvYGp8epXn2OGVxG1o7ks5urM0AgaJpZM4YHFk2> .

Radhikadua123 · 2018-11-03T10:09:53Z

Hmm, I think this is ok, though I do wonder about if the non mobile page is slightly smoother with the drop-down... Are we able to have a separate page just for mobile, even if it is still code duplication a bit?

May be in future, we can create a modal for the login page ? So, on any page, clicking on "Login", will just open popup in which user will enter details and login. There won't be any separate login page.

Radhikadua123 · 2018-11-03T10:38:44Z

Deferring to #3883 and #3884

Radhikadua123 added 3 commits November 1, 2018 19:43

Refactor login code for readability

354b941

Fix redirection in oauth

c37280a

Refactor normal login code for readability

288d7df

ghost assigned Radhikadua123 Nov 1, 2018

ghost added the in progress label Nov 1, 2018

Fix auth parameter in function

29b4889

Fix simpler code climate issues

5ff455c

Radhikadua123 commented Nov 1, 2018

View reviewed changes

jywarren mentioned this pull request Nov 2, 2018

[WIP - Help wanted!] Ensure preservation of return_to through OAuth login flow #3762

Closed

This was referenced Nov 3, 2018

Split create function into two login paths #3883

Merged

Ensure preservation of return_to through OAuth login flow #3884

Merged

Radhikadua123 closed this Nov 3, 2018

ghost removed the in progress label Nov 3, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[WIP] Ensure preservation of return_to through OAuth login flow #3879

[WIP] Ensure preservation of return_to through OAuth login flow #3879

Radhikadua123 commented Nov 1, 2018

Radhikadua123 commented Nov 1, 2018

plotsbot commented Nov 1, 2018 •

edited

Loading

jywarren commented Nov 1, 2018

jywarren commented Nov 1, 2018

jywarren commented Nov 1, 2018

Radhikadua123 commented Nov 1, 2018 •

edited

Loading

jywarren commented Nov 1, 2018 via email

Radhikadua123 commented Nov 1, 2018

Radhikadua123 Nov 1, 2018

jywarren Nov 2, 2018

Radhikadua123 Nov 1, 2018

jywarren Nov 2, 2018

Radhikadua123 Nov 1, 2018

jywarren Nov 2, 2018

Radhikadua123 Nov 1, 2018

jywarren Nov 2, 2018

jywarren commented Nov 2, 2018

Radhikadua123 commented Nov 2, 2018

jywarren commented Nov 3, 2018 via email

Radhikadua123 commented Nov 3, 2018

Radhikadua123 commented Nov 3, 2018

[WIP] Ensure preservation of return_to through OAuth login flow #3879

[WIP] Ensure preservation of return_to through OAuth login flow #3879

Conversation

Radhikadua123 commented Nov 1, 2018

Radhikadua123 commented Nov 1, 2018

plotsbot commented Nov 1, 2018 • edited Loading

jywarren commented Nov 1, 2018

jywarren commented Nov 1, 2018

jywarren commented Nov 1, 2018

Radhikadua123 commented Nov 1, 2018 • edited Loading

jywarren commented Nov 1, 2018 via email

Radhikadua123 commented Nov 1, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jywarren commented Nov 2, 2018

Radhikadua123 commented Nov 2, 2018

jywarren commented Nov 3, 2018 via email

Radhikadua123 commented Nov 3, 2018

Radhikadua123 commented Nov 3, 2018

plotsbot commented Nov 1, 2018 •

edited

Loading

Radhikadua123 commented Nov 1, 2018 •

edited

Loading