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

Fix authorization for /maker/home #20804

Merged
merged 3 commits into from Feb 22, 2018
Merged

Fix authorization for /maker/home #20804

merged 3 commits into from Feb 22, 2018

Conversation

islemaster
Copy link
Contributor

Before, we were checking :maker_discount authorization before rendering /maker/home, which restricted access to teachers.

Now, we skip that check for /maker/home. Instead, when rendering home we have an explicit authenticate_user! call which redirects to the sign-in page if the current user is not signed in, and will redirect them back to /maker/home after they sign in successfully.

Before, we were checking `:maker_discount` authorization before rendering /maker/home, which restricted access to teachers.

Now, we skip that check for /maker/home.  Instead, when rendering home we have an explicit `authenticate_user!` call which redirects to the sign-in page if the current user is not signed in, and will redirect them back to /maker/home after they sign in successfully.
@islemaster
Copy link
Contributor Author

Review requests sent to

  • @Erin007 who is helping create the new /maker/home page.
  • @mehalshah who probably knows more about Rails, CanCanCan, and authorization than either of us.

# Fake CSD6 script for progress info
csd6_script = create :script, name: Script::CSD6_NAME
create :script_level, script: csd6_script
student = create :student
Copy link
Contributor

Choose a reason for hiding this comment

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

Why not create the student in the setup step?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Only because we're not reusing it anywhere else right now. It might make sense to move it there for consistency.

Copy link
Contributor

@Erin007 Erin007 left a comment

Choose a reason for hiding this comment

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

LGTM! I've seen authenticate_user! used like this for the signed-in home page and projects, so it seems like a good use here.

@teacher = create :teacher
@admin = create :admin
@school = create :school
end

test "home redirects to sign-in when user is signed out" do
Copy link
Contributor

Choose a reason for hiding this comment

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

You can on-line this with test_redirect_to_sign_in_for (defined in test_helper.rb by Andrew)

@teacher = create :teacher
@admin = create :admin
@school = create :school
end

test "home redirects to sign-in when user is signed out" do
assert_queries 0 do
Copy link
Contributor

Choose a reason for hiding this comment

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

There isn't really a need to assert this if the redir happens

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sounds good, I'll simplify this case. I was modeling it on this test in HomeControllerTest:

test "redirect index when signed out" do
assert_queries 0 do
get :index
end
assert_redirected_to '/courses'
end

But this page will get a lot less traffic than HomeController#index, so maybe we don't care that much anyway.

@islemaster islemaster merged commit 3b6d378 into staging Feb 22, 2018
@islemaster islemaster deleted the maker-home-auth branch February 22, 2018 00:41
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.

None yet

3 participants