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

LC0068 on tableextension object #763

Open
Arthurvdv opened this issue Sep 12, 2024 · 25 comments
Open

LC0068 on tableextension object #763

Arthurvdv opened this issue Sep 12, 2024 · 25 comments
Labels
documentation Improvements or additions to documentation question Further information is requested

Comments

@Arthurvdv
Copy link
Collaborator

Arthurvdv commented Sep 12, 2024

To verify this, is there a way to assign permissions in the scenario below?

tableextension 50100 MyCustomer extends Customer
{
    procedure MyProcedure()
    var
        ShiptoAddress: Record "Ship-to Address";
    begin
        ShiptoAddress.SetRange("Customer No.", Rec."No.");
        ShiptoAddress.FindFirst(); // The current object is missing permission "r" for tabledata "Ship-to Address" (LC0068)
    end;
}

I thought of adding the InherentPermissions Attribute, to the method like this:
[InherentPermissions(PermissionObjectType::TableData, Database::"Ship-to Address", 'r', InherentPermissionsScope::Permissions)]
... but that raises another error An application object 'Table 222' could not be found in the current extension. Only application objects that belong to the current extensions can be used in this context. AL0720.

@StefanMaron
Copy link
Owner

No there is not, if you want to comply with the rule you have to move the code to a codeunit.

@Arthurvdv Arthurvdv added the documentation Improvements or additions to documentation label Sep 12, 2024
@Arthurvdv
Copy link
Collaborator Author

I'm a bit doubting here on what is the best approach is here. I initially thought of adding this to the documentation of this rule, where I now doubting on if excluding records that a not part of the App itself and inside a table(extension) object, from this rule makes more sense?

Potentially we could add an extra rule, something like "..move database operation outside table(extension) object for managing (indirect) permissions.." to catch these.

I've created an PR for this, would be great to have your thoughts on this.

@TKapitan
Copy link

TKapitan commented Oct 1, 2024

@Arthurvdv @StefanMaron Yes, I believe excluding the objects that are not part of the App is correct solution. We have now 150+ warnings in most of the apps for base app tables (Customer, Currency, Bank Account - standard tables where we can't add inherentopermissions).

@StefanMaron
Copy link
Owner

@TKapitan But still, just because the language doesn't support adding inherent permissions in extensions objects, doesn't mean you don't need them.

If you don't care, you can either disable the rule completely, or put a pragma at the top of the extension objects

@TKapitan
Copy link

TKapitan commented Oct 1, 2024

@StefanMaron what should this message for objects not under our control indicate? What should be the steps to fix the code without pragma?

I like the rule for my own objects, I don’t want to disable the whole rule, but also I can’t apply pragma in 300 places (another extension) as it would make code really messy

@StefanMaron
Copy link
Owner

@TKapitan I am not sure I understand the issue then.
In the example Arthur gave at the top he is creating a Table extension which he obviously has control over.

In order to comply with the rule to set permissions for all objects, you can not do the write/read/delete/modify on the extension object itself, you need to delegate this to a codeunit

@TKapitan
Copy link

TKapitan commented Oct 1, 2024

@StefanMaron What’s the difference in this case of having it in CU or in a tableextension?

@StefanMaron
Copy link
Owner

@TKapitan The difference is that the Codeunit supports the Permissions property

@Arthurvdv
Copy link
Collaborator Author

I would prefer to comply with this rule if it can be addressed using the Permissions property or the InherentPermissions decorator. Initially, I considered excluding objects in a (table)extension that aren’t part of the App itself in the PR, but this seems contrary to the spirit of the rule.

My idea was to introduce a second rule for greater flexibility:

  • LC0068 with exclude table(extension)
  • LC00xx move database operation outside table(extension) object for managing (indirect) permissions

This would provide more control to handle indirect permissions partially. Handling only part of the indirect permissions scenarios feels incomplete to be honest. And playing devil's advocate here: Would it really benefit users if indirect permissions generally work, but fails in some cases?

With that in mind, I’m considering canceling my PR and moving my code into a codeunit instead.

@StefanMaron
Copy link
Owner

Thanks @Arthurvdv this explains my point of view as well.
I would not mind to split this into two Rules, but like you described (and like I tried to describe before :D) this would still leave code in extension objects with potential issues.

I also want to remind again, what my initial motivation for this rule was:

For some tables and pages the Microsoft license dictates that we (developers) need to use the indirect permission assignment in code.
And since we have no way to find out at compile time which objects are affected by this, my solution was to just have the permissions always defined, as its better practice anyways.

But we might update the diagnistic description to give a hint on extension objects, that the code has to be moved?
How about that?

@StefanMaron
Copy link
Owner

Sorry pages is non-sense. Its reading thats sometimes restricted as well

@TKapitan
Copy link

TKapitan commented Oct 2, 2024

@StefanMaron still not sure that this is working completely (sorry for using this issue; it's probably not specifically related to table extensions.

This warning can be fixed by

  • Adding Permissions on the object level that add the object permissions (extends user's indirect permission)
  • Adding InherentPermissions that changes the user permissions but does not extend the user's indirect permission.

This mean, that adding indirect permission in InherentPermission should not fix the warning as developer must

  • Add direct permission in InherentPermissions
  • Add indirect permission in InherentPermissions and add the Permission property with indirect permission

@TKapitan
Copy link

TKapitan commented Oct 3, 2024

I was forced by our dev team to disable the rule as they don't agree with moving all code from reportextensions to codeunits.

I'll try to test a bit more examples once I'm back from holiday, but splitting to two rules (for own objects and for external objects) may be helpful for similar cases as ours.

@helenapi
Copy link

helenapi commented Oct 3, 2024

We fixed the issue by disabling this rule. It does not make the code any more readable to move it away to a codeunit and giving indirect permissions may cause the same kind of random error that not giving them does, according to my interpretation of the Note at the end of the documentation from microsoft... just in the opposite way where a user may lack some permission given at the top of the codeunit but not a permission specific to run a procedure in that codeunit: https://learn.microsoft.com/en-us/dynamics365/business-central/dev-itpro/developer/properties/devenv-permissions-property

image

@lvanvugt
Copy link

lvanvugt commented Oct 3, 2024

Wouldn't it be nice to add a code action that does set the Permissions property to cover the permissions? Or is this to be asked for in the AL Code Action extension.

@Arthurvdv
Copy link
Collaborator Author

Wouldn't it be nice to add a code action that does set the Permissions property to cover the permissions? Or is this to be asked for in the AL Code Action extension.

I believe it would be fantastic to have this feature, though unfortunately, it isn't available today. If you'd like to support this idea, we could use more votes on this BC Idea.

For now, the best option is indeed to request this through one of the VS Code extensions.

@Arthurvdv
Copy link
Collaborator Author

I'll try to test a bit more examples once I'm back from holiday, but splitting to two rules (for own objects and for external objects) may be helpful for similar cases as ours.

It would be great if you could share some examples so we can gain further insights on this.

@Arthurvdv
Copy link
Collaborator Author

We fixed the issue by disabling this rule. It does not make the code any more readable to move it away to a codeunit and giving indirect permissions may cause the same kind of random error that not giving them does

I'm not sure if I fully understand your point. The purpose of this rule is that the method or codeunit will function if the user has been granted indirect permissions to the tabledata, rather than full permissions.

With setup and configuration, you can always assign full read, write, insert, and delete permissions. However, assigning indirect read, write, insert, or delete permissions will only work if the developer has explicitly set the permission property in the code. The rule highlights all possible scenarios where the permission property must be set in order to enable this functionality.

@StefanMaron
Copy link
Owner

I think that note from the Docs is very confusing. Its like Arthur said. The Permissions property is necessary to make the Indirect Permissions of the users permission sets work.

Without the property, indirect permissions are simply useless.

And again, there are objects that are per Microsofts license indirect only.
Most commonly: writing to Ledger entry or Posted document tables.
But I recently noticed that its also sometimes set for reading data, like Sent Mails table.

Since we can never know during development if a table needs those permissions and since we enable the usage of indirect permissions for all other tables, I figured it would be good practice to always set Permissions for all Database operations.

@fridrichovsky
Copy link

fridrichovsky commented Nov 8, 2024

Wouldn't it be nice to add a code action that does set the Permissions property to cover the permissions? Or is this to be asked for in the AL Code Action extension.

Hello @lvanvugt and @Arthurvdv may be you now it but function exists. It is part of "AZ AL Dev Tools" extension in VSC. You have to click to the object name in code. Wait for light bulb and there is option for create permissions. It creates property permissions with required tables and their permissions.

image

@fridrichovsky
Copy link

I know that you mentioned here many times and I fully understand that permissions are required without exceptions. I agree with you. But I have same problem as @TKapitan. Developers ask me for disabling this rule because they are working on upgrades and they have to much warnings on all extension objects. I think when you split rules as prepared @Arthurvdv it will still check what do you want.
My point of view is that when I use your default rules I will get warning about permission for normal objects and second warning for extensions. It will follow your primary idea. Good think is that it gives me option disable rule for object extensions. If I disable it, it will be my bad (exists option that my code will not work because permissions.), but It gives me chance how to keep rule for normal objects. I don't want disable this rule because I feel that it is right way, but I can't fight with developers. So imagine my ask as looking for smaller evil ("Have one's cake and eat it too").
I hope that you understand what I mean.

@StefanMaron
Copy link
Owner

On Directions I talked to a few people about this rule as well and asked for feedback.
I got mixed opinions but the main point I took was, that this rule might not make sense for all projects and especially for Upgrade projects it might actually make sense to not have it enabled at the very beginning.

My problem with splitting is that I can not merge them back later, in case Microsoft would decide to introduce the Permissions property on Extension Objects, as removing a rule would be a breaking change.

However, I do understand the problem you have.

Did you try already to disable the warning on all extension objects with regex?
replace ^((pageextension|tableextension) \d+ [\s\S.\r\n]*?\{) with $1\n#pragma warning disable LC0068 globally across all files.

Maybe this can help you as it would be an instant solution.

Let me know!

@cegekaJG
Copy link

I was confused about this for a while since the official documentation is deceptive, but from my understanding, there's no way to request direct permissions using the Permissions property, correct?
If that's the case, I don't think we'll use this rule because satisfying the rule would mean allowing indirect permissions as well.

In any case, however, I think splitting the rule for table extensions is a good idea since there should be a degree of flexibility. And at the very least, table extensions should consider permissions set in the original object. There's no reason to either use a pragma or codeunit when reading a Sales Line in a Sales Header.

@fridrichovsky
Copy link

Hello @StefanMaron, thank you for your explanation and for replace regex. I discussed this solution with my developers team and they agree with this solution. So I can keep this rule set. For object extensions we are using pragma now. If MS allows permissions for extensions we can easily remove pragma by regex.

@ernestasjuska
Copy link

Hello @StefanMaron, How to deal with report extensions? The data items require the indirect read permission, but we cannot move them outside of the report.

image

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
documentation Improvements or additions to documentation question Further information is requested
Projects
None yet
8 participants