Ensure preservation of return_to through OAuth login flow #3884
Conversation
ghost
assigned 
Generated by 🚫 Danger |
|
Have you checked the behaviour on the local host? |
|
@SidharthBansal Nope I tried it on unstable.publiclab.org I can deploy it there in case you want to test it out. |
|
Yeah please deploy it there. We need to test it before publishing it to the production |
|
@SidharthBansal Sure, thanks! I have started the build here: https://jenkins.laboratoriopublico.org/job/Plots-Unstable/327/console |
| </a> | ||
| <ul id="login-dropdown" class="dropdown-menu" style="width:245px; color: white;"> |
There was a problem hiding this comment.
You have deleted the form by mistake I guess.
There was a problem hiding this comment.
Actually, it's w.r.t. this comment : #3879 (comment)
Though in case you feel we need it, I can try adding it back.
There was a problem hiding this comment.
I see.
When Jeff and I implemented this OAuth system we made this working on the localhost, unstable, stable and on the production.
So, once you deploy this onto the unstable and check the functionality for all the providers, ping me. I will check it.
Thanks for the awesome hard work.
There was a problem hiding this comment.
Oh. I wasn't sure how to get those credentials for local setup. So, I tried it on unstable only.
Yeah sure, I'll ping you once it's deployed. Build seems to be failed due to some reason and I don't have permission to start it manually from there. May be I'll push it again.
There was a problem hiding this comment.
This might help you
https://github.com/publiclab/plots2/blob/master/doc/OMNIAUTH.md
There was a problem hiding this comment.
No need to check for all providers on the development. Just create anyone say github and check it on local.
#2856
On unstable we need to check it on all providers.
My insights says that your solution will work. I did the same solution in February.
There was a problem hiding this comment.
https://github.com/publiclab/plots2/blob/master/doc/OMNIAUTH.md
Thanks, I understand the process now!
On unstable we need to check it on all providers.
Oh cool
My insights says that your solution will work. I did the same solution in February.
Oh, so did it get dropped mistakenly in refactoring or something ?
cool, I'll try it out.
|
In OAuth process there were a lot of use cases for the basic and
alternative flow. All of them were implemented with proper test in the
unstable, stable, production and local development mode with unit and
integration testing.
Now in the maintainance phase of OAuth project flow, we came across
integration of return_to field to preserve the web pages issue.
So we are trying to combat it.
Software always requires enhancements over time.:-)
…On Thu, Nov 8, 2018, 12:20 AM Radhika Dua ***@***.***> wrote:
***@***.**** commented on this pull request.
------------------------------
In app/views/layouts/_header.html.erb
<#3884 (comment)>:
> </a>
- <ul id="login-dropdown" class="dropdown-menu" style="width:245px; color: white;">
https://github.com/publiclab/plots2/blob/master/doc/OMNIAUTH.md
Thanks, I understand the process now!
On unstable we need to check it on all providers.
Oh cool
My insights says that your solution will work. I did the same solution in
February.
Oh, so did it get dropped mistakenly in refactoring or something ?
cool, I'll try it out.
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#3884 (comment)>, or mute
the thread
<https://github.com/notifications/unsubscribe-auth/AUACQ_x4XjqYfuPXMv1Bs5NVHWGVxj7dks5usyuNgaJpZM4YMvbU>
.
|
|
I see. Haha true. |
|
It was left by mistake. We did not think in this regard while building up
the oauth logic. Our main focus was integration of providers to the user
without disturbing the existing database.
OAuth logic was implemented in summers where as the login flow was built
from the start of the website.
You can checkout a lot of corner cases which Jeff and I have thought and
implemented in your free time in my Gsoc proposal.
Thanks
On Thu, Nov 8, 2018, 12:52 AM Sidharth Bansal <bansal.sidharth2996@gmail.com>
wrote:
… In OAuth process there were a lot of use cases for the basic and
alternative flow. All of them were implemented with proper test in the
unstable, stable, production and local development mode with unit and
integration testing.
Now in the maintainance phase of OAuth project flow, we came across
integration of return_to field to preserve the web pages issue.
So we are trying to combat it.
Software always requires enhancements over time.:-)
On Thu, Nov 8, 2018, 12:20 AM Radhika Dua ***@***.***>
wrote:
> ***@***.**** commented on this pull request.
> ------------------------------
>
> In app/views/layouts/_header.html.erb
> <#3884 (comment)>:
>
> > </a>
> - <ul id="login-dropdown" class="dropdown-menu" style="width:245px; color: white;">
>
> https://github.com/publiclab/plots2/blob/master/doc/OMNIAUTH.md
>
> Thanks, I understand the process now!
>
> On unstable we need to check it on all providers.
>
> Oh cool
>
> My insights says that your solution will work. I did the same solution in
> February.
>
> Oh, so did it get dropped mistakenly in refactoring or something ?
>
> cool, I'll try it out.
>
> —
> You are receiving this because you were mentioned.
> Reply to this email directly, view it on GitHub
> <#3884 (comment)>,
> or mute the thread
> <https://github.com/notifications/unsubscribe-auth/AUACQ_x4XjqYfuPXMv1Bs5NVHWGVxj7dks5usyuNgaJpZM4YMvbU>
> .
>
|
|
yes, gosh i remember how complex it was and how careful Sidharth's planning
was! This would have been just one more element to try to juggle. But now
we can approach it as a broken-out piece, so even though it's complex, we
have only one main project to complete at once.
Thanks you two for collaborating on this!
On Wed, Nov 7, 2018 at 2:29 PM Sidharth Bansal <notifications@github.com>
wrote:
… It was left by mistake. We did not think in this regard while building up
the oauth logic. Our main focus was integration of providers to the user
without disturbing the existing database.
OAuth logic was implemented in summers where as the login flow was built
from the start of the website.
You can checkout a lot of corner cases which Jeff and I have thought and
implemented in your free time in my Gsoc proposal.
Thanks
On Thu, Nov 8, 2018, 12:52 AM Sidharth Bansal <
***@***.***>
wrote:
> In OAuth process there were a lot of use cases for the basic and
> alternative flow. All of them were implemented with proper test in the
> unstable, stable, production and local development mode with unit and
> integration testing.
>
> Now in the maintainance phase of OAuth project flow, we came across
> integration of return_to field to preserve the web pages issue.
> So we are trying to combat it.
> Software always requires enhancements over time.:-)
>
> On Thu, Nov 8, 2018, 12:20 AM Radhika Dua ***@***.***>
> wrote:
>
>> ***@***.**** commented on this pull request.
>> ------------------------------
>>
>> In app/views/layouts/_header.html.erb
>> <#3884 (comment)>:
>>
>> > </a>
>> - <ul id="login-dropdown" class="dropdown-menu" style="width:245px;
color: white;">
>>
>> https://github.com/publiclab/plots2/blob/master/doc/OMNIAUTH.md
>>
>> Thanks, I understand the process now!
>>
>> On unstable we need to check it on all providers.
>>
>> Oh cool
>>
>> My insights says that your solution will work. I did the same solution
in
>> February.
>>
>> Oh, so did it get dropped mistakenly in refactoring or something ?
>>
>> cool, I'll try it out.
>>
>> —
>> You are receiving this because you were mentioned.
>> Reply to this email directly, view it on GitHub
>> <#3884 (comment)>,
>> or mute the thread
>> <
https://github.com/notifications/unsubscribe-auth/AUACQ_x4XjqYfuPXMv1Bs5NVHWGVxj7dks5usyuNgaJpZM4YMvbU
>
>> .
>>
>
—
You are receiving this because you are subscribed to this thread.
Reply to this email directly, view it on GitHub
<#3884 (comment)>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/AABfJ2hcrpCAMma2n4q-EUjRQ_I29iQnks5uszR2gaJpZM4YMvbU>
.
|
|
@jywarren I have tested this out on unstable.publiclab.org and it works fine for me. It's still live in case you want to try it out. :) |
|
Just checking it out |
|
Awesome!!! |
There was a problem hiding this comment.
Hi, changes looks good to me. Thanks for your work.
@jywarren can you please review this and merge this?
|
Restarted these builds to try to get them to pass! |
|
@jywarren all tests passed. |
|
Hooray!!! 🎉 🎉 |
|
Can one of you test on https://stable.publiclab.org now that it's merged? This is now auto-building from the master branch on each merge. It may take 10m to publish, you can monitor on Jenkins. Thanks! |
|
Thanks for merging it! Oh wow, so I believe we have stable.publiclab.org for master branch. I see. |
* Split create function into two login paths * Ensure preservation of return_to through OAuth login flow (#3884)
|
Yes, that's right - it used to be pointed at our `stable` branch but is now
running against `master` branch so that as soon as we merge a PR it's soon
available for testing :-)
…On Fri, Nov 9, 2018 at 7:26 PM Radhika Dua ***@***.***> wrote:
Thanks for merging it!
Oh wow, so I believe we have stable.publiclab.org for master branch. I
see.
—
You are receiving this because you modified the open/close state.
Reply to this email directly, view it on GitHub
<#3884 (comment)>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/AABfJ8ysNGSZy8PHVVo5Y5yppglyKrk7ks5uth0UgaJpZM4YMvbU>
.
|
* Split create function into two login paths * Ensure preservation of return_to through OAuth login flow (publiclab#3884)
Part of #3879