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

User pages are public #6177

Merged
merged 1 commit into from Feb 11, 2014

Conversation

3 participants
@cirosantilli
Contributor

cirosantilli commented Jan 27, 2014

Fixes ACCEPTING MERGE/PULL REQUESTS at http://feedback.gitlab.com/forums/176466-general/suggestions/5374154-create-a-public-index-of-users-and-let-users-be-pu.

Behaviour Changes

  • current user profile pages at u/:username can be visited seen by any user without login.
  • only projects and project events that the logged in user can see are listed.

The content of the page is otherwise unchanged, except of course the public header in case the user is not logged in.

Screenshots

  • u/:username while signed out:

screenshot from 2014-01-27 18 21 45 signed out

  • u/:username while signed in as someone else:

screenshot from 2014-01-27 18 26 15 signed in as someone else

  • u/:username while signed as the user himself:

screenshot from 2014-01-27 18 26 54 signed in as the user

Implementation notes

Factored out test User creation under group and admin/group into shared/user.
Used name "John Doe" everywhere instead of just "John" to keep uniformity with projects.

Part of features/steps/public/projects_feature.rb spilled out to shared/project.rb since it
is now used in both Public projects and User tests.

CSS class header.project_name renamed to .title since it can now contain both project names and user names.

Please consider merging this other MR as it would make it much easier to do interactive tests and screenshots that depend on having multiple project visibility levels.

@@ -1,11 +1,20 @@
class UsersController < ApplicationController
layout 'navless'
skip_before_filter :authenticate_user!, only: [:show]

This comment has been minimized.

@ghost

ghost Jan 27, 2014

Adding something like (roughly):

skip_before_filter :authenticate_user!, only: [:show] unless config.disable_public_user_pages

Would easily allow this to be enabled or disabled. I think this would be useful and added a comment on the feedback page.

@cirosantilli

This comment has been minimized.

Contributor

cirosantilli commented Feb 5, 2014

Updated the behaviour to match what was required by the Gitlab Team on the feedback discussion.

The only UI change is that the help for users has a new "Visibility of users" section explaining in detail the new behavior:

screenshot from 2014-02-05 11 27 11

@projects = @user.authorized_projects.includes(:namespace).select {|project| can?(current_user, :read_project, project)}
if @projects.empty?
if current_user
if @user != current_user

This comment has been minimized.

@ghost

ghost Feb 6, 2014

This is more restrictive than before. Logged in users can only see profiles if the user is a member of a public/internal or common project?

This seems like an unnecessary restriction. Shouldn't logged in users be able to see any profile? Due to the way this works, it is more of an inconvenience than security. I just have to grant a user access to a dummy project I am a member of and I can visit their profile.

Was there a part of the http://feedback.gitlab.com/forums/176466-general/suggestions/5374154-create-a-public-index-of-users-and-let-users-be-pu that specified this should happen? Did I miss that?

This comment has been minimized.

@cirosantilli

cirosantilli Feb 6, 2014

Contributor

@jhollingsworth Ah, OK, you mean logged users can see all profiles is that it?

Reading again, it was probably what the GitLab Team wanted, and it does make more sense.

I'll update.

@dzaporozhets

This comment has been minimized.

Member

dzaporozhets commented Feb 10, 2014

@cirosantilli I think user should be visible outside only if user has membership in public project. Otherwise its security issue since GitLab is private hosting solution at first.

@cirosantilli

This comment has been minimized.

Contributor

cirosantilli commented Feb 10, 2014

@randx Maybe I don't quite understand what you mean, but isn't this the current behavior?

See

  • Scenario: I visit user "John Doe" page while not signed in when he has a public project
    • OK
  • Scenario: I visit user "John Doe" page while not signed in when he does not have a public project
    • Redirect to sign in.
@cirosantilli

This comment has been minimized.

Contributor

cirosantilli commented Feb 10, 2014

This works because on the controller the projects are filtered by the projects that the current user can see. So if he is logged out, and he can see any projects, then there must be a public one there:

@projects = @user.authorized_projects.includes(:namespace).select {|project| can?(current_user, :read_project, project)}
if !current_user && @projects.empty?
    return authenticate_user!
end
CHANGELOG Outdated
@@ -1,6 +1,7 @@
v 6.6.0
- Permissions: Developer now can manage issue tracker (modify any issue)
- Improve Code Compare page performance
- User pages are publicly visible

This comment has been minimized.

@dosire

dosire Feb 11, 2014

Member

=> User pages are publicly visible if they have public projects

if current_user
'navless'
else
'public_users'

This comment has been minimized.

@dzaporozhets

dzaporozhets Feb 11, 2014

Member

why create separate layout for public users?

This comment has been minimized.

@cirosantilli

cirosantilli Feb 11, 2014

Contributor

Because the header can either be the signed off header, or the signed in header. I copied the technique used for the public project pages.

@dzaporozhets

This comment has been minimized.

Member

dzaporozhets commented Feb 11, 2014

@cirosantilli ok lets describe cases:

Scenario: I visit user "John Doe" page while not signed in when he has a public project
-> I see his profile with public projects and activity in it

Scenario: I visit user "John Doe" page while not signed in when he has no public project
-> Redirect to sign-in

Scenario: I visit user "John Doe" page while signed in when he has no shared projects with me
-> I see profile but w/o projects and activity

Scenario: I visit user "John Doe" page while signed in when he has shared projects with me
-> I see profile with shared projects and activity

Scenario: I visit user "John Doe" page while signed in when he has no shared projects with me but has public/internal projects
-> I see profile with public/internal projects and activity

@cirosantilli

This comment has been minimized.

Contributor

cirosantilli commented Feb 11, 2014

@randx That's what I meant it to do.

Do you mean I should:

  1. rename the scenarios using just no instead of does not have (I agree, better wording)
  2. also add the expected outcome to the scenario description (I agree, clearer)
@dzaporozhets

This comment has been minimized.

Member

dzaporozhets commented Feb 11, 2014

@cirosantilli ok

That's what I meant it to do.

Sorry I missed that. Cool

Do you mean I should:

yes if you agree

User pages are visible to users without login
... if the user is authorized to at least one public project.
@cirosantilli

This comment has been minimized.

Contributor

cirosantilli commented Feb 11, 2014

I have tried to address the issues raised:

  • CHANGELOG is more precise
  • test names use shorter wording
  • new scenario added: "I visit user "John Doe" page while signed in when he has no shared projects with me"

I decided not to put the test outcomes on the test name because I could not find a way to break lines, and the test name lines would be too long otherwise.

Also rebased to master.

@dzaporozhets

This comment has been minimized.

Member

dzaporozhets commented Feb 11, 2014

@cirosantilli thanks. I will merge it when CI pass

dzaporozhets added a commit that referenced this pull request Feb 11, 2014

@dzaporozhets dzaporozhets merged commit 39aeac7 into gitlabhq:master Feb 11, 2014

1 check passed

default The Travis CI build passed
Details
@dosire

This comment has been minimized.

Member

dosire commented Feb 11, 2014

Well done @cirosantilli and @randx

@cirosantilli cirosantilli deleted the booktree:public-user-pages branch Feb 12, 2014

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment