StefanMaron / BusinessCentral.LinterCop

Community driven code linter for AL (MS Dynamics 365 Business Central)
https://stefanmaron.com
MIT License
76 stars 31 forks source link

LC0068: suggestions #780

Open NavNab opened 1 month ago

NavNab commented 1 month ago

First of all, thank you for this rule. Very useful. It detects all 'missing' permissions (with few false-positive-ish πŸ˜… / see exemple below). The downside now is that my 'Problems' pane is flooded with 1k+ messages: The current object is missing permission "abc" for tabledata "xyz". So, I may easily miss more important warning. If I recall, this rule originated from a discussion about indirect permissions. This issue usually is about a missing modify. With that in mind, could you please:

  1. add a setup to enable/disable each permission 'rimd' separately. Or maybe (doable easily-quickly) split the rule to 4 rules so we can enable/disable them separately. I, for example, currently don't care about a potential missing read permission for table Item as almost everyone is allowed to read it πŸ˜…
  2. add a setup to (include) focus on tables that really matter (17, 21, 25, 32, etc.) --> The 'include' implies 'exclude all other tables that are not on te list'

false-positive-ish

Do not apply te rule for a table that calls itself --> GetNextEntryNo for example:

table 50000 "MyTable"
{
    fields
    { field(1; EntryNo; Integer){ } }

    keys
    { key(PK; "EntryNo") { Clustered = true;} }

    internal procedure GetNextEntryNo(): Integer
    var
        MyTable: Record "MyTable";
    begin
        if MyTable.FindLast() then
            exit(MyTable.EntryNo + 1);
       exit(1);
    end;
}
StefanMaron commented 1 month ago

I will write up a detailed blog and put it on the wiki page.

But this is not a false positive because users can run the procedure on the table with just table x permissions

NavNab commented 1 month ago

Thanks for the feedback @StefanMaron I agree that it's not a white or black false-positive hence the -ish in the end πŸ˜‰. The common sens here is that some codeunit will call the MyTable.GetNextEntryNo and that codeunit will/should have permissions to read and insert to MyTable. In this context, do you have in mind a use case where a feature would work correctly with only x permission on the table? Broadly speaking, it is rarely, if ever, advisable to let an object or entity assign permissions to itself.

NavNab commented 1 month ago

Any input about points 1 & 2?

StefanMaron commented 1 month ago

Well, if you just follow the linter rule, each object will handle the tabledata permissions it need, therefore the table would have the permission for itself, the codeunit would not need tabledata permissions since it's just calling the function and just needs table x

On point 1&2 I also have thoughts but that requires deeper elaboration and will be covered by the blog post

NavNab commented 1 month ago

Will the blog address the points I mentioned in my previous comment (copied below)? I'm looking forward to reading it once it's available, although I do think that GitHub comments are intended for discussing ideas πŸ˜…

StefanMaron commented 1 month ago

I will try to cover my view on the entire topic and as many aspects as possible.

I don't think it about what feature can work in what scenario, it's about support the permission model by the code we write. If you do not set the permission property properly, an admin just can not make use of the indirect permissions feature.

And as far as I can see there is absolutely no difference at all between a procedure on the table with permissions on it self versus the procedure being placed in a codeunit.

NavNab commented 1 month ago

Okay, we'll continue the discussion after the blog πŸ™ƒ

StefanMaron commented 1 month ago

Here you go :)

https://stefanmaron.com/posts/indirectpermissions/

StefanMaron commented 1 month ago

Lets maybe continue the discussion here, and not on twitter.

I have been thinking about "focus on what really matters"

And I think the best solution here that I could live with would be to split this into two rules. 0068 stays as it is right now, you can disable it if it doesnt fit you. But I would like to keep this the default rule, since I think we should always set all the permissions.

A new rule with additional setup to define all tables "that matter" with their individual indirect permissions. This will include all 250 tables from my blog post.

With that rule you can just focus on avoiding runtime permission errors.

I dont want to split RIMD into separate rules or something.

Would that be a solution for you that works?

NavNab commented 1 month ago

@StefanMaron Yes, let's continue our discussion here.

Regarding [A new rule with additional setup to define all tables "that matter" with their individual indirect permissions. This will include all 250 tables from my blog post.] absolutely, and thank you! πŸ™ƒ

For clarity, could you explain the rationale behind not wanting to divide RIMD/rimd into separate rules? If there are any drawbacks or negative impacts associated with splitting the rule, could you provide some specific examples?

From what I understand, direct permissions (RIMD) generally aren’t a concern as they are managed effectively through proper Permission Sets. I struggle to think of a realistic scenario where a user would need indirect permissions to the Purchase Line to complete a task, since appropriate permissions are typically assigned through Permission Sets. If you have any real-world examples, please share them. --> preferably actual cases rather than hypothetical situations πŸ˜….

On the other hand, indirect permissions (rimd) restricted by the license are a real issue. When reviewing code, I often look for operations like PurchRcptLine.Modify() and ensure the developer has added the appropriate permissions to the codeunit. This is the situation where I hoped the rule would be particularly useful.

When dealing with over 1000 messages (thanks to LC0068 🫣πŸ€ͺ) in VSCode, where more than 700 are similar to the following:

... I really don't care about these messages because I'm confident the users possess proper permission sets that fully cover that. Besides, I won't ask the devs and I won't invest time in adding this permission to codeunits πŸ˜‰

P.S: yes yes, it is a big app where a user can generate more than 20 orders with just 5 clicks 😁

I hope this better explains my perspective.

StefanMaron commented 1 month ago

RIMD Split: With the second rule, like I described, I simply do not see the need to create yet another 4 rules, or 8 if we want to split both rules.

And yes, if a user has direct permissions on everything, the Permissions property is not a concern at all.

The problem with the real world use case is, that its currenlty not really possible to leverage the indirect permisions model, because developers are not setting the Permissions property. This is the chicken egg problem. Devs dont see the reason in setting the property because indirect permisison are not used/usefull. But its not possible to build permission sets in the first place because it only works if the permissions properties are set everywhere.

And just for the record, I am not sure if everyone knows: While it is possible to set Permissions = tabledata SomeTable = RIMD; (direct permissions) it wont have any impact, its the same result as rimd indrect permissions.

The entire Idea of indirect permissions is, that a given user does not need to have direct permissions on every table. Its to enable scenarios like I described in my blog.

I hope that makes sense.