Hotfix/only admin permission

[marla-singer]

Closes #1835

[brylie]

I am not confident of the conditions here. It is probably not a safe assumption to say that 'any error means the user can add an API'.

Rather, our code should be explicit, for safety and self documentation:

• if only admins can add API and
  • this user is not an admin, then
    • user cannot add an api
  • this user is an admin, then
    • user can add an api
• otherwise, if user is logged in
  • user can add an api
• otherwise
  • user cannot add an api

[brylie]

The above code is intentionally verbose, so that we can easily spot semantic errors. It also has a safe default, not allowing a user to add an API.

[brylie]

Ping @frenchbread

[marla-singer]

@brylie Should I change code in this PR? I just only removed it

[frenchbread]

Agreed that it looks a bit messy. Ideally we can create a controller class with methods for each check so that in the end helper's return will look like:

Template.registerHelper('userCanAddApi', () => {
    const permissionController = new PermissionController();
    return permissionController.userIsAdmin() || (permissionController.userIsAdmin() && permissionController.onlyAdminsCanAddApis()) || (permissionController.userIsLoggedIn() && !permissionController.onlyAdminsCanAddApis());
});

Simplifying what @brylie documented:
User can add API when:

• User is an admin
• User is admin and onlyAdminsCanAddApis setting set to true.
• User is logged in and onlyAdminsCanAddApis setting set to false.

[marla-singer]

@frenchbread Nope! It looks awful

[brylie]

While you are probably being ironic, words like 'instance', 'controller', and 'class' are not going to make it clearer what is going on.

[brylie]

The point is to make it clear 'why' the code is written, what is it trying to accomplish? To this end, we use clear comments, humanly meaningful variable names, reasonable line length, proper whitespace, and explicit flow.

This, while reducing unnecessary noise that is mainly relevant to the interpreter, such as:

• delimiters () {} ; [] ,
• programming constructs: words like 'class', 'module', 'variable', 'method', 'controller', 'canHasProps', 'onDidEatCheeseburger'
• implict, or otherwise overly efficient, coding style: e.g. using catch instead of else, where catch should be used for error handling
  • see also: code golf

[brylie]

“Debugging is twice as hard as writing the code in the first place. Therefore, if you write the code as cleverly as possible, you are, by definition, not smart enough to debug it.”

– Brian Kernighan
(source: Are You Smart Enough To Debug Your Own Code)

[brylie]

Keep it simple.

[marla-singer]

@brylie Hm. I learnt code detaily and don't actually understand, why this solution is bad? The main logic of this part is good. I added the few comments and show error if it is catched. As for me everithing is fine and obviously

[brylie]

I learnt code detaily and don't actually understand, why this solution is bad?

The code can be improved for the following reasons:

• it is difficult to reason about
  • we have to think about all of the possible cases that could cause an error, in order to understand when the 'catch' statement would be called
• it has unsafe defaults
  • if any error is thrown, the user is 'allowed' to add an API
• it is overly complicated
  • a simple if, else if, else flow would suffice here, and be more readable
• we are 'misusing' try/catch
  • try/catch are generally used for error handling, by design
  • in this case we are using try/catch as a conditional block, where if, else if, else would be more appropriate

In general, we are writing our code for humans to read and maintain. Our primary goal is to have simple, clear, and working code.

References and examples

• Arguments for or against using Try/Catch as logical operators
• Literate programming
  • Eve: Programming designed for humans - Eve is a programming language and IDE based on years of research into building a human-first programming platform.
• PEP 20- The Zen of Python
• Dr. Grace Hopper - invented the first compiler, so that people could write instructions for computers in natural language

[marla-singer]

@brylie Well... I can't understand yet what do you want from my side. I'm confused

we have to think about all of the possible cases that could cause an error, in order to understand when the 'catch' statement would be called

I'm not really sure that I should create the checking for every "sneeze". In our case I can only add the checking for settings exists or not and no more to replace try/catch bundle. What kind of errors can be else? I don't know.

Please point to me a line where how do you think the code isn't good enough. My eyes look at code too much and they get tunnel vision

[frenchbread]

Please correct me if I am wrong but I would leave try/catch since it's more safe. If changing it to if/else we would need to check each case 1. settings 2. settings.access 3. settings.access. onlyAdminsCanAddApis.

[frenchbread]

Also try/catch is much more readable than stacks of if/elses.

[marla-singer]

@frenchbread I think @brylie try to say that if error was cathced we wouldn't understand exactly why it happened. Because settings don't exist or field "access" isn't available. But yes - the long chain of if/elses doesn't look pretty and readable too.

[frenchbread]

@marla-singer The first that can happen there is that settings were not set for current user. To resolve this we can create an empty settings document when user is created.

Second error is onlyAdminsCanAddApis value not being available since it was not set. To bypass this we can give default value to it when Settings document is created. In this file changing this part

'access.onlyAdminsCanAddApis': {
    type: Boolean,
    optional: true,
    label: TAPi18n.__('settings_schema_onlyAdminsCanAddApis'),
},

to this

'access.onlyAdminsCanAddApis': {
    type: Boolean,
    label: TAPi18n.__('settings_schema_onlyAdminsCanAddApis'),
    autoValue: function () {
      // Allow everyone to add APIs by default
      return false;
    }
},

[frenchbread]

I can help with that.

[marla-singer]

@frenchbread Is it a good idea to allow user to add api on default?

[brylie]

@marla-singer the original feedback request was to structure the code somewhat like this:

1. Check if user is logged in,
   • store it in a variable: e.g. 'userIsloggedIn'
2. Check if user is an admin,
   • store it in a variable: e.g. 'userIsAdmin'
3. Get the value of 'onlyAdminsCanAddApis',
   • store it in a shorter variable name

Then, create a nested conditional, using if/else if/else or something similar (rather than try/catch)

• if only admins can add API
  • if this user is an admin, then
    • user can add an api
  • else
    • user cannot add an api
• else if user is logged in
  • user can add an api
• else
  • user cannot add an api

[brylie]

Feel free to make the code simpler or more elegant, while keeping descriptive comments. If there is a more logical order for the conditionals, use it instead.

The main guiding principles to consider, from PEP 20, are:

• Beautiful is better than ugly.
  • use as few if/else if/else statements as possible
  • consider a different flow control (aside from try/catch)
• Explicit is better than implicit.
  • the code and comments should clearly say what they are doing and why
• Simple is better than complex
  • if/else if/else is somewhat simple
  • I am not sure how to get rid of the nested conditions, aside from abstraction
• Complex is better than complicated.
  • suggestions like adding a controller may be overly complicated
• Flat is better than nested.
  • although try/catch is not desirable here without explicit error handling
• Readability counts.

[marla-singer]

Thanks @brylie. I'll try to improve code and make it simple and readable 👌

[brylie]

Cool. Thanks for your patience @marla-singer and @frenchbread

[brylie]

One last consideration. We probably don't even need to allow users to access the Dashboard when they are not admin and the onlyAdminCanAddApi is true. In other words, there is no reason for a regular user to see the dashboard if they cannot add an API.

Taking the above into consideration may mean a different outcome, such as only running this authorization method in the Navbar, and showing a 403 not authorized to normal users when they view the Dashboard with onlyAdminCanAddApi set to true.

I realize this is out of scope for your pull request @marla-singer, so it is just 'food for thought.'

Ping @bajiat

[marla-singer]

@brylie regular user is user, who sign up and don't have any api, right? If it's so, I agree, there is no reason to see the Dashboard for them.

[brylie]

regular users is user, who sign up and don't have any api, right?

Jep. We have basically four roles:

• anonymous - person not logged in
• regular user - registered for an account, currently logged in
• manager - has added an API
• admin - platform superuser

When the admin user enables onlyAdminCanAddApi = true, then regular users have no benefit from seeing the Dashboard.

[marla-singer]

@brylie Done. I rewrote the solution and did force pushing
Now regular users can't navigate to Dashboard via navbar. The enhacement is to add "forbidden" template, if regular user navigates to dashboard or add/new via url.

[brylie]

This should be called userCanViewDashboard, since it is only used in that context.

[brylie]

This should be called userCanViewDashboard, since we are only checking for permission to view Dashboard here. I.e. 'page' is ambiguous, and is not what we are really checking.

[marla-singer]

@brylie Done.