Skip to content
Snippets Groups Projects

Restrict public users for private instances

Merged Restrict public users for private instances
issue_3508 into master
Merged Felipe Artur requested to merge issue_3508 into master

Implements #3508 (closed)

Merge request reports

Loading
Loading

Activity

Filter activity
  • Approvals
  • Assignees & reviewers
  • Comments (from bots)
  • Comments (from users)
  • Commits & branches
  • Edits
  • Labels
  • Lock status
  • Mentions
  • Merge request status
  • Tracking
  • Robert Speicher
  • Robert Speicher
  • Robert Speicher
  • Robert Speicher
  • Robert Speicher
  • Felipe Artur Added 1 commit:

    Added 1 commit:

    • 42fa138e - Add specs and fix code
  • Felipe Artur
    Felipe Artur @felipe_artur ·
    Author Maintainer

    @rspeicher Made the changes and added specs.

  • Robert Speicher
  • Robert Speicher
  • Robert Speicher
  • Robert Speicher
  • Robert Speicher
  • Robert Speicher
    Robert Speicher @rspeicher started a thread on the diff
    • spec/controllers/groups/group_members_controller_spec.rb 0 → 100644
    1 require 'spec_helper'
    2
    3 describe Groups::GroupMembersController do
    4 let(:user) { create(:user) }
    5 let(:group) { create(:group) }
    6
    7
  • Robert Speicher
  • Robert Speicher
  • Robert Speicher
  • Felipe Artur Marked this merge request as a Work In Progress

    Marked this merge request as a Work In Progress

  • Felipe Artur Added 1 commit:

    Added 1 commit:

    • 4ef3b0d8 - Fix code
  • Felipe Artur Added 276 commits:

    Added 276 commits:

    • 4ef3b0d8...c3875b5f - 272 commits from branch master
    • 64e339fc - Restrict user profiles based on restricted visibility levels
    • 218f20eb - Move verification to abilities
    • 742b8ceb - Add specs and fix code
    • 96902099 - Fix code
    Toggle commit list
  • Felipe Artur Added 1 commit:

    Added 1 commit:

    • 3f32f40a - Fix code
  • Felipe Artur Unmarked this merge request as a Work In Progress

    Unmarked this merge request as a Work In Progress

  • Felipe Artur Added 1 commit:

    Added 1 commit:

    • 3f8c7f49 - Fix specs
  • Felipe Artur
    Felipe Artur @felipe_artur ·
    Author Maintainer

    @rspeicher Can you please review again?

    This change should be for companies with ONLY private projects. So i think this should be it.

  • Robert Speicher
  • Robert Speicher
    Robert Speicher @rspeicher ·
    Owner

    LGTM, passing to Douwe since he's better about noticing weird permissions issues.

  • Robert Speicher Reassigned to @DouweM

    Reassigned to @DouweM

  • Douwe Maan
  • Douwe Maan
  • Douwe Maan
  • Douwe Maan
  • Douwe Maan
  • Douwe Maan
  • Douwe Maan
  • Douwe Maan Reassigned to @felipe_artur

    Reassigned to @felipe_artur

  • Douwe Maan
    Douwe Maan @DouweM ·
    Maintainer

    @felipe_artur Are there other places where we expose users to unauthenticated users? I'm thinking of autocompletion, API, that sort of thing.

  • Douwe Maan
    Douwe Maan @DouweM ·
    Maintainer

    @felipe_artur We should also document this in the permissions document, and add a description to the "Restricted visibility levels" option in the admin area to say that if public is restricted, so are user profiles.

  • Felipe Artur
    Felipe Artur @felipe_artur ·
    Author Maintainer

    @DouweM If we have to worry with the scenario where there are public groups with private projects then YES there are other places. Do we?

    I will check API.

  • Felipe Artur Added 1 commit:

    Added 1 commit:

    • 02c5213b - Code fixes
  • Felipe Artur Marked this merge request as a Work In Progress

    Marked this merge request as a Work In Progress

  • Felipe Artur Added 229 commits:

    Added 229 commits:

    • 02c5213b...77e178b5 - 223 commits from branch master
    • 8688007e - Restrict user profiles based on restricted visibility levels
    • 6ac6416f - Move verification to abilities
    • 6a845eac - Add specs and fix code
    • 53815231 - Fix code
    • c9d75812 - Fix specs
    • 6b9cb0ef - Code fixes
    Toggle commit list
  • Felipe Artur
    Felipe Artur @felipe_artur ·
    Author Maintainer

    I don't see a way to get users data using API without being authenticated. So it is ok.

  • Felipe Artur Added 1 commit:

    Added 1 commit:

    • 166edca6 - Insert users check into api
  • Douwe Maan
  • Felipe Artur Added 1 commit:

    Added 1 commit:

    • 67b2cc19 - Insert users check into api
  • Felipe Artur Added 1 commit:

    Added 1 commit:

    • 4fcb468c - Insert users check into api
  • Felipe Artur Unmarked this merge request as a Work In Progress

    Unmarked this merge request as a Work In Progress

  • Felipe Artur Reassigned to @DouweM

    Reassigned to @DouweM

  • Felipe Artur Reassigned to @felipe_artur

    Reassigned to @felipe_artur

  • Felipe Artur Added 1 commit:

    Added 1 commit:

    • e579d8de - Remove group members check
  • Felipe Artur Added 1 commit:

    Added 1 commit:

    • a3ccb8b0 - Remove group members check
  • Felipe Artur Added 1 commit:

    Added 1 commit:

    • 214197e7 - Remove group members check
  • Felipe Artur Added 1 commit:

    Added 1 commit:

    • b4094ad8 - Remove group members check
  • Felipe Artur Added 1 commit:

    Added 1 commit:

    • 38242b9c - Remove group members check
  • Felipe Artur Reassigned to @DouweM

    Reassigned to @DouweM

  • Felipe Artur
    Felipe Artur @felipe_artur started a thread on the diff
    • app/models/ability.rb
    470 480
    471 481 private
    472 482
    483 def restricted_public_level?
    484 current_application_settings.restricted_visibility_levels.include?(Gitlab::VisibilityLevel::PUBLIC)
    • Felipe Artur
      Felipe Artur @felipe_artur ·
      Author Maintainer

      Had to remove memoization here. Specs were failing when running in prod build. Looks like instances of this class are not being cleaned properly.

  • Felipe Artur
    Felipe Artur @felipe_artur started a thread on the diff
    • spec/controllers/groups/group_members_controller_spec.rb 0 → 100644
    1 require 'spec_helper'
  • Felipe Artur
    Felipe Artur @felipe_artur ·
    Author Maintainer

    Sorry for the noise. I amended and pushed a lot of small commits. I was not aware they were notified in the MR.

  • Felipe Artur Added 1 commit:

    Added 1 commit:

    • 869582bd - Insert instructions in admin page and permissions document
  • Douwe Maan
  • Douwe Maan
  • Douwe Maan
  • Douwe Maan
  • Douwe Maan Reassigned to @felipe_artur

    Reassigned to @felipe_artur

  • Felipe Artur Added 1 commit:

    Added 1 commit:

    • 75228cdc - Fix documentation and improve permissions code
  • Felipe Artur Reassigned to @DouweM

    Reassigned to @DouweM

  • Felipe Artur Added 1 commit:

    Added 1 commit:

    • 0e3cda09 - Add changelog entry
  • Felipe Artur Added 1 commit:

    Added 1 commit:

    • 39d853f5 - Add changelog entry
  • Felipe Artur Added 464 commits:

    Added 464 commits:

    • 39d853f5...c0678f2d - 463 commits from branch master
    • 5395e617 - Merge branch 'master' into issue_3508
  • Felipe Artur
    Felipe Artur @felipe_artur ·
    Author Maintainer

    @DouweM Done

  • Felipe Artur
    Felipe Artur @felipe_artur ·
    Author Maintainer

    @DouweM Can we merge this one?

  • Douwe Maan
    Douwe Maan @DouweM ·
    Maintainer

    @felipe_artur Looks good :) Please fix the merge conflict, ping me on Slack and I'll merge.

  • Douwe Maan Reassigned to @felipe_artur

    Reassigned to @felipe_artur

  • Felipe Artur Added 279 commits:

    Added 279 commits:

    • 5395e617...5ae4fd21 - 268 commits from branch master
    • b05f0a48 - Restrict user profiles based on restricted visibility levels
    • 57519565 - Move verification to abilities
    • 668d6ffa - Add specs and fix code
    • e8a77c0a - Fix code
    • 147879ae - Fix specs
    • 07b38c3b - Code fixes
    • ce96d482 - Insert users check into api
    • 09c8cf9d - Remove group members check
    • 7d54e721 - Insert instructions in admin page and permissions document
    • 820c08ce - Fix documentation and improve permissions code
    • 2366768d - Add changelog entry
    Toggle commit list
  • Douwe Maan Status changed to merged

    Status changed to merged

  • Douwe Maan mentioned in commit 58665b64

    mentioned in commit 58665b64

  • Douwe Maan mentioned in issue #14142 (closed)

    mentioned in issue #14142 (closed)

  • Felipe Artur mentioned in merge request gitlab-com/www-gitlab-com!1886 (merged)

    mentioned in merge request gitlab-com/www-gitlab-com!1886 (merged)

    • Please register or sign in to reply