Page MenuHomePhabricator

Bug: App lets you navigate to url(s) without being logged in
Closed, ResolvedPublic3 Story Points

Description

Steps to reproduce:

Expected: User gets redirected to login page if they are not logged in at any time.

Event Timeline

Niharika created this task.Dec 14 2017, 3:11 PM
Restricted Application added a subscriber: Aklapper. · View Herald TranscriptDec 14 2017, 3:11 PM
kaldari set the point value for this task to 3.Jan 9 2018, 11:56 PM
DannyH triaged this task as Normal priority.Jan 17 2018, 12:59 AM
DannyH raised the priority of this task from Normal to Needs Triage.
DannyH triaged this task as Normal priority.
DannyH edited projects, added Community-Tech-Sprint; removed Community-Tech.
MusikAnimal moved this task from Ready to In Development on the Community-Tech-Sprint board.

There's now some basic, custom-built role-based authentication. There are only three roles:

  • Anonymous -- only can see the homepage. If you browse anywhere else, you're redirected to login on Meta, then it goes back to /programs. I'm going to make it redirect back to where you were, but that requires some OAuth tweaks. I've got it on my to-dos.
  • User -- can view any program/event, but can only created/edit/delete their own programs and events (or else they're shown an error page). Not sure if this is desirable. I just assumed there's no harm in letting people see others' events, and the situation may arise where they'd want to.
  • Admin -- can view/edit/delete anything. The list of admins is hard coded in parameters.yml, and currently only includes MusikAnimal (WMF) and NKohli (WMF). This file is not in version control, so you can go in and add people directly on Toolforge in app/config/parameters.yml, then clear the cache with php bin/console c:c --env=prod --no-warmup

Relevant commits: https://github.com/wikimedia/grantmetrics/compare/c58b0fa9badcad681fda7ca9c78642933cb3c5d1...7186f8bad83a615d9fe56fc4bed28261d4c29cd1

There's now some basic, custom-built role-based authentication.

This is awesome! Thank you!

There are only three roles:

  • Anonymous -- only can see the homepage. If you browse anywhere else, you're redirected to login on Meta, then it goes back to /programs. I'm going to make it redirect back to where you were, but that requires some OAuth tweaks. I've got it on my to-dos.

If they're only able to see the homepage, how is it possible that they'd be somewhere else and you'd have to redirect them there?

  • User -- can view any program/event, but can only created/edit/delete their own programs and events (or else they're shown an error page). Not sure if this is desirable. I just assumed there's no harm in letting people see others' events, and the situation may arise where they'd want to.

I raised this in one of the earlier discussions we had with Sati and Danny about grant metrics and was told that this is a strict requirement. A user should only be able to see their own programs and events. We might come back to this after the beta testing. So you can create another role for user only seeing their own programs and events and we can make that default for now.

  • Admin -- can view/edit/delete anything. The list of admins is hard coded in parameters.yml, and currently only includes MusikAnimal (WMF) and NKohli (WMF). This file is not in version control, so you can go in and add people directly on Toolforge in app/config/parameters.yml, then clear the cache with php bin/console c:c --env=prod --no-warmup

Any reason for it to not be in version control?

If they're only able to see the homepage, how is it possible that they'd be somewhere else and you'd have to redirect them there?

In case they have their program/event bookmarked, or were given a link.

A user should only be able to see their own programs and events. We might come back to this after the beta testing. So you can create another role for user only seeing their own programs and events and we can make that default for now.

No problemo, I'll get to work on this.

This file is not in version control, so you can go in and add people directly on Toolforge in app/config/parameters.yml

Any reason for it to not be in version control?

Hard-coding is bad enough for something like this, I think, but additionally this file contains database credentials. Being able to edit the file on Toolforge should make it easier, so you don't have to commit and do a deploy. parameters.yml.dist is in version control, however, and provides the initial default values.

Makes sense. Thanks!

@MusikAnimal I can only see programs I am an organizer for. Shouldn't I be able to see all programs per above?

@MusikAnimal I can only see programs I am an organizer for. Shouldn't I be able to see all programs per above?

And you're logged in as NKohli (WMF)?

@MusikAnimal I can only see programs I am an organizer for. Shouldn't I be able to see all programs per above?

And you're logged in as NKohli (WMF)?

Yup.

Oh sorry, I misunderstood. It doesn't show you all programs, no, but it will allow you to browse to them directly and delete them/etc. Do we want it to show all programs, always?

Oh sorry, I misunderstood. It doesn't show you all programs, no, but it will allow you to browse to them directly and delete them/etc. Do we want it to show all programs, always?

I think it'd be nice if you and I could see them all - they'd come in handy for testing when we are trying to troubleshoot why someone can't add an organizer or why someone's all data isn't loading etc. What say?

Alrighty, you should no longer be able to view a program/event unless you're an organizer or admin. I've also prettified the error pages.

Commits: https://github.com/wikimedia/grantmetrics/compare/9b6b2afb6d01e62843413b5c48498f4e47c6fe98...c1432b9d5b7cbb7f5f14eae50b17cd73ab01b635

I've created T187926 for having admins see all programs in My Programs. I have some concerns about performance, should this tool become really popular, which is one of those good problems :) Either way T187380 would affect how this is implemented, so I'd like to do that first. In the meantime, if anyone has issues, we could ask them for a link to the program/event.

This looks good. I have one concern. Right now, if someone is not logged in and they try to go to say, https://tools.wmflabs.org/grantmetrics/programs/Danny%27s_test, they're directly taken to the OAuth dialog. This is a bit confusing and the user could potentially take this as spam (I know I would). I would rather we take them to the error page where we say tell them that they need to login to access it and let them click "login with OAuth".
Thoughts?

Yeah that works! I have also created T187934, as I think redirecting back to where you were trying to go would be the expected behaviour.

I've now go it redirecting back to the homepage (which has a big login button), with the message "Please login to continue". Hope this is OK. Otherwise we need to create a new page, or a custom exception for attempting to view program/events while logged out.

Niharika closed this task as Resolved.Feb 22 2018, 11:15 PM
Niharika moved this task from Needs Review/Feedback to Q1 2018-19 on the Community-Tech-Sprint board.

I've now go it redirecting back to the homepage (which has a big login button), with the message "Please login to continue". Hope this is OK. Otherwise we need to create a new page, or a custom exception for attempting to view program/events while logged out.

Works fine for me!

MusikAnimal moved this task from In progress to Done on the Grant-Metrics board.Feb 22 2018, 11:57 PM
TBolliger moved this task from Estimated to Archive on the Community-Tech board.Feb 28 2018, 1:47 AM