Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Ensure avatar auth #1587

Merged
merged 4 commits into from
Jul 2, 2018
Merged

Ensure avatar auth #1587

merged 4 commits into from
Jul 2, 2018

Conversation

premachb
Copy link
Contributor

Description

Ensure user is logged in before going through onboarding flow by redirecting to GitHub login if a user is not authenticated.

Checklist
  • linter status: 100% pass
  • changes don't break existing behavior
  • commit message follows commit guidelines
Affected core subsystem(s)

Dashboard folder more specifically the Onboarding Flow.

Testing

Simple change that checks and redirects and I've confirmed this doesn't break if the user is logged in.

Refers/Fixes

Refs: #1554

If user is unauthenticated forward to github login so that they don't waste time
@codecov
Copy link

codecov bot commented Jun 30, 2018

Codecov Report

Merging #1587 into master will increase coverage by 1.53%.
The diff coverage is 0%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #1587      +/-   ##
==========================================
+ Coverage    28.4%   29.93%   +1.53%     
==========================================
  Files         130      130              
  Lines        9592     9596       +4     
  Branches     1241     1243       +2     
==========================================
+ Hits         2725     2873     +148     
+ Misses       6769     6619     -150     
- Partials       98      104       +6
Impacted Files Coverage Δ
app/dashboard/views.py 14.62% <0%> (-0.1%) ⬇️
app/app/settings.py 80.43% <0%> (+80.43%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 13500cf...633c7a9. Read the comment docs.

owocki
owocki previously approved these changes Jul 1, 2018
Copy link
Contributor

@owocki owocki left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@@ -522,6 +522,11 @@ def send_tip_2(request):

def onboard(request, flow):
"""Handle displaying the first time user experience flow."""

if not request.user or not request.user.is_authenticated:
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is wrong. This will cause anyone who goes to the onboarding flow to be redirected to the github login. That is not expected. We want this only for the standalone avatar page. So this check should be only if someone is trying to access endpoints like: /onboard/contributor/?steps=avatar or /onboard/profile and so on.

@mbeacom
Copy link
Contributor

mbeacom commented Jul 2, 2018

@premachb Thanks for your contribution!

cc @owocki @SaptakS
I have adjusted this PR to only redirect to login when the user is hitting onboard outside of a flow containing the github step.

This will ensure we're redirecting to auth for any user passing steps without github or entering with the profile flow.

Also, I updated the if check by dropping if not request.user - request.user will always be present, but it might represent an unauthenticated user. In cases of auth, we want to check request.user.is_authenticated and hasattr(request.user, 'profile') to verify the user's auth flow has successfully completed and they have been assigned a profile object.

Thanks again for the contribution!

@mbeacom mbeacom merged commit 1680e60 into gitcoinco:master Jul 2, 2018
@ghost ghost removed the in progress label Jul 2, 2018
@SaptakS
Copy link
Contributor

SaptakS commented Jul 2, 2018

@mbeacom makes sense.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants