846039 - Create additional authorization-state-specific routing rules

Status: VERIFIED FIXED

(Participation Infrastructure :: Phonebook, defect)

Description

[Justin Crawford [:hoosteeno] [:jcrawford]]

When we add privacy states to profile fields, we need to anticipate some new interactions between users with a particular authorization state ("public users", "vouched users") and pages with particular visibility rules ("visible to public users," "visible to vouched users"). 

Specifically:

We should do the exact same thing (send a 404? send a "No Permissions" message? redirect to homepage?) when someone requests a URL for...
* A profile that they do not have permissions to see any fields on, or
* A profile that doesn't exist

We should redirect a public user to some public place, or send an error message, if s/he
* Requests a group or tag (or any other non-profile) page
* Submits a search query

giorgos has been mapping specific HTTP responses to URLs + authorization states in an etherpad. We may want to copy it over here when it's ready.

Comment 1

[Justin Crawford [:hoosteeno] [:jcrawford]]

The below spreadsheet contains DRAFT spec for routing requests based on their authZ status and the visibility state of the content they're requesting.

https://docs.google.com/a/mozilla.com/spreadsheet/ccc?key=0Algxlsc2cLlldFFsNldKNzI5YXIwMzFNb1A2MEszYVE#gid=0

Ping me if you want to edit it.

Comment 2

[[github robot]]

Commits pushed to master at https://github.com/mozilla/mozillians

https://github.com/mozilla/mozillians/commit/10be0888b46df3fb1aa7f0a029c0177146fd7d32
[bug 846039] Implement 'Stronghold'.

https://github.com/mozilla/mozillians/commit/e04772bb8de16353d3bc247782208f118ee8235b
[bug 846039] Return location listing page even when nonexistent country.

Comment 3

[Matt Brandt [:mbrandt]]

Reopening:
If I'm following this correctly, with an unvouched account I am receiving a HTTP 302 for a the following urls - the spec suggests the final status code should be a HTTP 301 (comment 1):

With an Unvouched user follow the below urls:
https://mozillians.allizom.org/en-US/country/us/
https://mozillians.allizom.org/en-US/country/us/region/California/
https://mozillians.allizom.org/en-US/country/us/city/
https://mozillians.allizom.org/en-US/group/mozilla-mexico
https://mozillians.allizom.org/en-US/group/258-l10n/toggle

https://mozillians.allizom.org/en-US/u/mbrandt (valid user)
https://mozillians.allizom.org/en-US/country/broccoli (invalid country)

Comment 4

[Giorgos Logiotatidis [:giorgos]]

I would argue that the spec is wrong, since 301 is Permanent Redirect, while we want Temporary Redirect, which is 302 (or at least what the web falsely uses for TempRedirect).

Comment 5

[Matt Brandt [:mbrandt]]

That wfm -- I admit that I had to refresh my memory on what the intended difference was between a 301 and 302 but didn't think through the implications. 

hoosteeno how think you?

Comment 6

[Justin Crawford [:hoosteeno] [:jcrawford]]

Yes, I think it should be a 302. Which, I think, means this is again resolved.

Comment 7

[Matt Brandt [:mbrandt]]

Pl(In reply to Justin Crawford [:hoosteeno] from comment #6)
> Yes, I think it should be a 302. Which, I think, means this is again
> resolved.

Please review the spec sheet and update it accordingly :)

Comment 8

[Justin Crawford [:hoosteeno] [:jcrawford]]

Done!

Comment 9

[Justin Crawford [:hoosteeno] [:jcrawford]]

I'm reopening this bug because the /u/ behavior is not conforming to the spec (comment 1).

Here's a user with no public fields:
https://mozillians.org/en-US/u/4234aef0ab/

Expected behavior (according to the spec):
* As a logged out user, if I visit a valid user account with no public fields I should get a 302.
* As a logged out user, if I visit a user URL that doesn't exist, I should get a 302.

Actual behavior: 
In both instances I get a 404.

Comment 10

[Giorgos Logiotatidis [:giorgos]]

https://github.com/mozilla/mozillians/pull/451

Pull request with a few tests to cover all scenarios.

Anonymous, unvouched, vouched, incomplete users request:
 - Public profile
 - Private profile
 - Non-existent profile
 - Own profile
 - Pending (unvouched) profile
 - Incomplete profile

Comment 11

[[github robot]]

Commits pushed to master at https://github.com/mozilla/mozillians

https://github.com/mozilla/mozillians/commit/8ae87a41ae77cef6cef8579d4b31a38ed2f8602a
[fix bug 846039] Authorization routes for Profile View.

* Allow all users to visit public profiles.
* Allow all users visit own profile.
* Redirect anonymous and unvouched users when requesting private or non-existent profile
* Raise 404 when vouched user requests non-existent profile

https://github.com/mozilla/mozillians/commit/2d974799f9d2c51414ae18e2f45251c5c0e98aff
Merge pull request #451 from glogiotatidis/routes

[fix bug 846039] Authorization routes for Profile View.

Comment 12

[Matt Brandt [:mbrandt]]

QA verified - automation is passing. Thanks all!

Comment 13

[[:AndreiH]]

Reopening this bug because the behavior is not conforming to the spec.

These two urls:
https://mozillians-dev.allizom.org/pl/opensearch.xml and 
https://mozillians-dev.allizom.org/nl/u/MozilliansUser/ should return 200 as in "comment 1".

Actual code retuned is 302.

Comment 14

[Matt Brandt [:mbrandt]]

(In reply to Andrei Hutusoru from comment #13)
> Reopening this bug because the behavior is not conforming to the spec.
> 
> These two urls:
> https://mozillians-dev.allizom.org/pl/opensearch.xml and 
> https://mozillians-dev.allizom.org/nl/u/MozilliansUser/ should return 200 as
> in "comment 1".
> 
> Actual code retuned is 302.

Odd, it looks like this was a small hiccup, the automated tests are passing again for dev. Anonymous user reveive a HTTP 200 for https://mozillians-dev.allizom.org/pl/opensearch.xml and https://mozillians-dev.allizom.org/nl/u/MozilliansUser/

For reference here is the report showing the failures - http://qa-selenium.mv.mozilla.com:8080/view/Mozillians/job/mozillians.dev/3152/testReport/tests.test_redirects/TestRedirects/test_200_for_anonymous_users/

Comment 15

[[:AndreiH]]

Thanks Matt! Verified FIXED.

Comment 16

[[:AndreiH]]

Sorry about this Matt. My mistake. The automated test is xfailed, but the fail is still there. Reopening this.

Comment 17

[Giorgos Logiotatidis [:giorgos]]

I can't confirm this error. I get 200 - OK for both urls provided in comment 13.

Comment 18

[Matt Brandt [:mbrandt]]

Bumping to fixed per comment 17

Comment 19

[Matt Brandt [:mbrandt]]

Bumping to QA verified on stage and prod:

The tests manually pass for me on both stage and prod. I'll investigate our automation to see if there is an incorrect assumption in the code that is causing the test to falsely report a failure.

There appears to be a separate issue occurring on dev (bug 864362) where the server is down, I'm not sure if this is what you were experiencing.

AndreiH, please provide more information - which environment were you seeing the failure and are you able to manually reproduce the errors that automation is hitting.

Comment 20

[Justin Crawford [:hoosteeno] [:jcrawford]]

I'm seeing a deviation from spec.

Steps to reproduce:

1. Login.
2. Visit a user who exists but has no public fields. Example: https://mozillians-dev.allizom.org/en-US/?next=/en-US/u/timw/

Actual behavior:

3. Get redirected to logged-out homepage and see error, "You must be logged in to continue"

Expected behavior:

3. D18 here: https://docs.google.com/a/mozilla.com/spreadsheet/ccc?key=0Algxlsc2cLlldFFsNldKNzI5YXIwMzFNb1A2MEszYVE#gid=0

Comment 21

[Justin Crawford [:hoosteeno] [:jcrawford]]

Er, this is user error, not a regression. Closing again.

Comment 22

[Matt Brandt [:mbrandt]]

Bumping back to verified http://f.cl.ly/items/0q0C1O1W3A313g3W0e1R/yeah!.gif