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

Feature/user roles #380

Merged
merged 73 commits into from Aug 5, 2018

Conversation

Projects
None yet
2 participants
@axeloz
Copy link
Contributor

axeloz commented Jun 20, 2018

This feature add an authorization layer on authentication. There are two levels:

  • globally where a user can be a super administrator of the application. This admin can access the admin panel, edit any setting, add projects, groups and users and view and edit any projet.
  • locally per project where a user can be either "regular user" or "manager". A manager can view, edit and deploy the projects he has access to when a regular user can only view and deploy the projects he has access to. None of them can access the admin panel

Changes are the following:

  • users listed into the admin panel's users page can be set as "administrators".
  • projects listed into the admin panel's projects page can have zero, one or more managers and/or regular users attached to them.
  • projects pages are now protected by Laravel authorizations: a non-admin user can only access the projects he has authorization for.
  • projects views local features are now protected by the @can Laravel's feature to allow edition of some settings.
  • installation process now set the first user as admin
  • the deployer:create-user command now accepts the --admin option in order to define a new user as admin
  • the Laravel's seeders now include a user with the admin role.

Todos:

  • the autocomplete of users (using bootstrap-tagsinput and bootstrap-3-typeahead) to be attached to a project does work but the data source IS NOT the real actual users list. The users collection has been added into the view but I didn't get how to use this collection from the Projects view.
  • test the deployment process on behalf of a "regular user" and "manager" user
  • prevent an administrator to attach the same user as "regular user" and "manager" for the same project
@@ -77,6 +78,8 @@ public function handle(Dispatcher $dispatcher, Validation $validation)
$send_email = (!$this->option('no-email'));
$arguments['is_admin'] = (!$this->option('admin') ? 0 : 1);

This comment has been minimized.

@REBELinBLUE

REBELinBLUE Jun 20, 2018

Owner

Could you change this to $arguments['is_admin'] = (!$this->option('admin')); as with the previous line

This comment has been minimized.

@axeloz

axeloz Jun 20, 2018

Author Contributor

Done

@@ -360,7 +360,7 @@ private function generateKey()
*/
private function createAdminUser($name, $email, $password)
{
$process = $this->artisanProcess('deployer:create-user', [$name, $email, $password, '--no-email']);
$process = $this->artisanProcess('deployer:create-user', [$name, $email, $password, '--no-email --admin']);

This comment has been minimized.

@REBELinBLUE

REBELinBLUE Jun 20, 2018

Owner

Can you split the --admin out into a separate array item

This comment has been minimized.

@axeloz

axeloz Jun 20, 2018

Author Contributor

Done

@@ -51,12 +52,16 @@ public function index(
) {
$projects = $this->repository->getAll();
// Getting all users
$users = User::where('is_admin', 0)->get();

This comment has been minimized.

@REBELinBLUE

REBELinBLUE Jun 20, 2018

Owner

This should probably be changed a method on the user repository and inject the repository in the method

public function handle($request, Closure $next)
{
// If user is authenticated ...
if (Auth::user()) {

This comment has been minimized.

@REBELinBLUE

REBELinBLUE Jun 20, 2018

Owner

Take a look at the Authenticate middle, I am injecting the AuthFactory as I really don't like Laravel's "facades"

This comment has been minimized.

@axeloz

axeloz Jun 20, 2018

Author Contributor

I will check this in the morning

public function before($user, $ability)
{
if ($user->is_admin === 1) {

This comment has been minimized.

@REBELinBLUE

REBELinBLUE Jun 20, 2018

Owner

Add is_admin to the casts array on the User entity to cast it to a boolean then you can just do $user->is_admin

This comment has been minimized.

@axeloz

axeloz Jun 20, 2018

Author Contributor

Done

This comment has been minimized.

@axeloz

axeloz Jun 20, 2018

Author Contributor

Oops, breaks my code.

This comment has been minimized.

@axeloz

axeloz Jun 20, 2018

Author Contributor

fixed

axeloz and others added some commits Jun 20, 2018

@REBELinBLUE

This comment has been minimized.

Copy link
Owner

REBELinBLUE commented Jun 20, 2018

In the servers view you can replace the autocomplete with the following

$(`#${element} #${element}_name`).typeahead({
  autoSelect: false,
  source: (query, process) => $.ajax({
    type: 'GET',
    url: routes.serversAutocomplete,
    data: { query },
  }).done(response => process($.map(response.suggestions, dataItem => ({
    name: `${dataItem.name} (${dataItem.user}@${dataItem.ip_address})`,
    data: dataItem,
  })))),
  afterSelect: (suggestion) => {
    fields.forEach((field) => {
      $(`#${element}_${field}`).val(suggestion.data[field]);
    });

    $(`#${element}_deploy_code`).prop('checked', suggestion.data.deploy_code);
  },
});

Then you can remove devbridge-autocomplete from the package.json, no point in having 2 different autocomplete packages.

You should then be able to use yarn as well

@REBELinBLUE

This comment has been minimized.

Copy link
Owner

REBELinBLUE commented Jun 20, 2018

Here is the autocomplete for the users. It still needs some work but should be enough to get you started

/**
 * Autocomplete feature for project's members
 */
$('.members_autocomplete').tagsinput({
  allowDuplicates: false,
  freeInput: false,
  itemText: 'text',
  itemValue: 'value',
  typeahead: {
    name: 'users',
    source: (query) => {
      return UserCollection.filter((user) => {
        // TODO: Maybe move this to a method on the collection instead, but it would require the collection to be changed so it isn't simply generated
        return user.get('name').toLowerCase().startsWith(query.toLowerCase());
      }).map(user => ({
        id: user.get('id'),
        text: user.get('name'),
      }));
    },
    afterSelect: function() {
    	this.$element[0].value = '';
    }
  }
});
// Needs work to exclude already added users and maybe to change the filter so it is more than just startsWith

You obviously need to add import UserCollection from '../../collections/Users';

Whilst you're making changes could you fix a mistake for me, in src/Models/User.js the class is called Group, can you change it to User

@codecov

This comment has been minimized.

Copy link

codecov bot commented Jul 5, 2018

Codecov Report

Merging #380 into master will decrease coverage by 0.83%.
The diff coverage is 43.47%.

@@             Coverage Diff              @@
##             master     #380      +/-   ##
============================================
- Coverage     93.66%   92.82%   -0.84%     
- Complexity     1260     1290      +30     
============================================
  Files           195      198       +3     
  Lines          4151     4210      +59     
============================================
+ Hits           3888     3908      +20     
- Misses          263      302      +39
@axeloz

This comment has been minimized.

Copy link
Contributor Author

axeloz commented Jul 5, 2018

Cool it passed. It's been harder to write the unit testing than adding this feature ...

@REBELinBLUE

This comment has been minimized.

Copy link
Owner

REBELinBLUE commented Jul 8, 2018

Can you open the PR for the frontend code as well so I can merge this?

@axeloz

This comment has been minimized.

Copy link
Contributor Author

axeloz commented Jul 9, 2018

Thanks, just added a merge request.

@axeloz

This comment has been minimized.

Copy link
Contributor Author

axeloz commented Jul 17, 2018

Hello @REBELinBLUE is everything OK ? Thanks

@REBELinBLUE

This comment has been minimized.

Copy link
Owner

REBELinBLUE commented Jul 17, 2018

Yeah sorry just been really busy and haven't had a chance to go through everything yet, will try to before the end of the week

@REBELinBLUE

This comment has been minimized.

Copy link
Owner

REBELinBLUE commented Jul 29, 2018

Sorry for the delay, you know how life gets in the way sometimes, I've set aside time tomorrow evening to go over this

@axeloz

This comment has been minimized.

Copy link
Contributor Author

axeloz commented Jul 30, 2018

Hello @REBELinBLUE, I know exactly what you mean so don't worry. Let me know
Axel

@REBELinBLUE

This comment has been minimized.

Copy link
Owner

REBELinBLUE commented Jul 31, 2018

Mainly looks good, only one small thing I can see, can you update the migration so that all the existing users have is_admin set to true so that we don't break existing installs?

@axeloz

This comment has been minimized.

Copy link
Contributor Author

axeloz commented Jul 31, 2018

Done (sorry I wrote "but" instead of "when" in the commit message)

@REBELinBLUE REBELinBLUE merged commit 2e7c99b into REBELinBLUE:master Aug 5, 2018

1 of 5 checks passed

codeclimate 148 issues to fix
Details
codecov/patch 43.47% of diff hit (target 93.66%)
Details
codecov/project 92.82% (-0.84%) compared to b881852
Details
continuous-integration/styleci/pr Issues have been identified with 1 file
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

@axeloz axeloz deleted the axeloz:feature/user-roles branch Aug 6, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.