microsoft / powerapps-tools

Unsupported PowerApps Tools & Apps
MIT License
1.04k stars 432 forks source link

[BUG]: App Archival is not getting deleted - default environments not writing correctly to Archive table. #666

Closed AkashPatel136-zz closed 3 years ago

AkashPatel136-zz commented 3 years ago

Describe the bug I am an admin and I received approval for an app that the owner does not exist, so I approved the request for archival. The record was created on 12/7/2020 on the Archival Approval table on Governance Components solution. Today I was checking few records and I came accord the record which was still Active on the PowerApp Apps table. If you can explain to me how this data get sync.

Based on my understanding is that if App status is active in the PowerApps App table that means the app still exists and it is not deleted.

Component (please tell us which flow or app you are experiencing issues with):

To Reproduce Steps to reproduce the behavior:

  1. Go to '...'
  2. Click on '....'
  3. Scroll down to '....'
  4. See error

Expected behavior IF application/flow is marched for approval for archival approval then it should delete the app/flow from the environment and update the record as deleted.

Screenshots

PowerApps App Table image image image image

Additional context Add any other context about the problem here.

JeneferM-MSFT commented 3 years ago

Did you set the Auto Delete On Archive environment variable to delete? We have added it as a fail proof to ensure admins are ready to start deleting from the environment. See Approval Clean Up or setup instructions

AkashPatel136-zz commented 3 years ago

Auto Delete On Archive Environment Variable is set to Yes

I was just checking one of the flows and it is failing. I am a system admin.

Error message - You are not permitted to make flows in this '68a7..........'. Please switch to the default environment, or to one of your own environment(s), where you have maker permissions.

JeneferM-MSFT commented 3 years ago

What identity is the action under? So if you go to edit the flow and see what identity has the connection reference image

If not yours, go and edit the Connection Reference from the flow details page image

AkashPatel136-zz commented 3 years ago

It is using my credentials for the connection reference

JeneferM-MSFT commented 3 years ago

What error message is being thrown from Check for Perms, and what is being thrown for Modify Flow Owners as Admin?

JeneferM-MSFT commented 3 years ago

I did find an issue locally where, if there is a broken connection in a flow, then the "Check for Perms" is failing with this error: image

I have added another catch for this to delete anyway. Let me know if that is the error you were seeing, or if something else.

AkashPatel136-zz commented 3 years ago

i have a different error message

image

JeneferM-MSFT commented 3 years ago

OK. That is an expected error when you arent an owner of a flow. Basically, the connector only allows you to check permissions if you own the flow, doesnt matter if you are a tenant admin. That is why we catch and do the call to Modify Flow Owners as Admin. Can you please see what error you get for this action: Modify Flow Owners as Admin

AkashPatel136-zz commented 3 years ago

image

JeneferM-MSFT commented 3 years ago

alright well I dont know why you would have that as forbidden if you have the permissions you state. The new version I have releasing in the future will try to delete in this case anyway. Can you see if you are able to delete this flow manually? If not, then you have an issue with your permissions. In order to get access, use the Set Flow Permissions app from the Core Solution

AkashPatel136-zz commented 3 years ago

when I tried to delete the below flow it throws an error message. Also, I am not the owner of these flows. IT is 142 flows which is not getting deleted.

{ "error": { "code": "ConnectionAuthorizationFailed", "message": "The caller with object id does not have permission for connection under Api 'shared_logicflows'." } }

AkashPatel136-zz commented 3 years ago

After my start approval flow is being successful I tried to run the clean-up flow but it still fails.

JeneferM-MSFT commented 3 years ago

I'm afraid the issue you are seeing is about your permissions then and not our tooling. One last thing to try, would be to grant yourself permission to the flow using the Set Flow Permissions app from Core, then go to the Flow Settings and Delete image

AkashPatel136-zz commented 3 years ago

I can do that but the concern for me is that I have 142 flows and for each, I have to do that and probably in the future also until a new version of COE does not come out.

JeneferM-MSFT commented 3 years ago

Yes, the ability to augment permissions is something that is needed. If you are able to augment them using that app, that will be interesting. It appears as though you do not have correct permissions right now. This flow works for most people to add permissions to the flow and delete

AkashPatel136-zz commented 3 years ago

What I found out is interesting that in the below images I created a flow that gets flow details from my default environment. In Get Flow as Admin action where it gave me an option to chose an environment from my tenant and I selected my default environment and it worked for me, but instead of selecting the environment if I just enter environment ID it throws the error message.

The issue I found is that my default environment ID is Default-prefix and in COE it says my default environment ID is Environment unique ID. so I am thinking it is looking for Default- at the beginning of environment ID which is stored in COE.

I did show this to @topness-msft today and he also thinks this is a really interesting issue.

image

JeneferM-MSFT commented 3 years ago

Phil's awesome! Thanks for investigating this deeper, we often fall into bugs with the default environment for this very reason, some connectors expect it with the "Default-" and others without. In this case though, we do already send as Default-, as shown here: image

I'm not sure what's happening on your side, but I think that the final catch I put in place will still work for you to get around it, as long as you do have sufficient permissions to modify and delete.

Let me try to get you a PR of that here to test

JeneferM-MSFT commented 3 years ago

CenterofExcellenceAuditComponents_1_26_managed.zip

AkashPatel136-zz commented 3 years ago

I want to leave this case open until next week and I will update based on the flow runs next week. Just want to see new version flows do not fail.

JeneferM-MSFT commented 3 years ago

Yes of course. I would like to leave it open until we ship the fix actually so that others can find it.

WouterF commented 3 years ago

@JeneferM-MSFT, can you show which changes you made in the flow? I cannot import the whole new solution again just as a test because we adapted the texts of all the emails/approval mails to fit better to our organization. There were also some spelling mistakes in it. I would like to add the fix manually to the flow. Thanks

JeneferM-MSFT commented 3 years ago

I'm not 100% sure I remember unfortunately as I made several changes in my attempt to fix. But I believe all I did in the end was change the Configure Run After of "Delete Flow" image image

AkashPatel136-zz commented 3 years ago

Hi @JeneferM-MSFT

I am having the same issue for the other flows where my Default Environment cannot be found because it does not start with Default- is there anywhere in the flow I can change this so it so it will store as a Default-65........... like that?

JeneferM-MSFT commented 3 years ago

Hi Akesh, Default-was not the cause for this case, as far as I can tell. Can you confirm that my fix above fixed this particular issue please?

Then if you have other instances of the CoE flows failing for default, please post them separately, and I'll need to know which flow and which call is failing. To our knowledge we have caught all these so happy to fix more individual cases if you find them.

WouterF commented 3 years ago

Hi @JeneferM-MSFT, I tried the fix above by changing the run-after condition, but the flows keeps returning the same error because there is no "Default-" in front of the environment id. Is there already a new version of the CoE governance solution that has this issue fixed? Don't understand me wrong, I'm very happy that the CoE exists, but is it maybe possible to mark versions of the CoE with beta/stable. I would prefer to wait for a month or longer to upgrade the CoE, if I'm sure that the version is stable. Until now, every time I upgrade the CoE, I have new unexpected issues. However, thanks for the time and effort you all put in all of this, because it is not easy. But if possible I would be very happy with CoE versions marked as stable so I can install them without a lot of issues. Thanks

JeneferM-MSFT commented 3 years ago

The CoE is a starter kit intended to help people understand how to monitor their tenants. It isn't officially supported by Microsoft, and it isn't for everyone.

This flow is old and this issue is new, and I dont repro locally. So I dont know what is going on. Might be a product bug or something.

When you say its not working because you dont have default-, can you sahre what you mean? My repros work for the default environment.

WouterF commented 3 years ago

Hi @JeneferM-MSFT, thanks for the fast response. I updated the flow as indicated: Changing the run-after conditions: image However, I still get the same error message: image Thanks

JeneferM-MSFT commented 3 years ago

Let me explain the flow and see if you can let me know where we are differing. I added some questions inline, if you can please answer them that might help figure out what you are seeing.

1) We pull the list of flows from our custom archive table instead of the product in order to reduce API calls.

image

2) In the ArchiveApproval Table, the envt name column is stored with the default- prefix for default envt. Can you confirm this is the case for you? image

3) We next do a call to Get Flow, we do this because we want to check to see if you already have permissions as a user. image

We expect this call to fail if you are not listed as an editor or user for that flow, even if you are all the levels of admin. Can you please confirm what error message you see for this call?

Here is the expected failure if you just dont have permissions image

This GitHub issue helped surface other reasons Get Flow might fail though. For example a current product bug (fix on the way) which causes it to fail for flows with certain connectors like the CDS Native (Current) connector will fail due to a product bug Here's what that failure looks like image

4) The expected forbidden error is why we catch this call at the next step and add your user Can you please validate that the values in this call make sense for your user? image

5) Now there may be other reasons that Get Flow fails. The example from above, where we fail for some connectors due to product bug, if you already had permissions to the flow then the call to modify them will fail. Can you please let me know what failure you see for this call? Please include downloaded output image

6) Which is why we now also run the delete call even if the above call failed

WouterF commented 3 years ago

Hi @JeneferM-MSFT, Please find below the answers to your questions: In the ArchiveApproval Table, the envt name column is stored with the default- prefix for default envt. Can you confirm this is the case for you? This is not the case. The default- prefix is not here, all without this prefix. image

We expect this call to fail if you are not listed as an editor or user for that flow, even if you are all the levels of admin. Can you please confirm what error message you see for this call? Same error as you. image

The expected forbidden error is why we catch this call at the next step and add your user Can you please validate that the values in this call make sense for your user? image

Now there may be other reasons that Get Flow fails. The example from above, where we fail for some connectors due to product bug, if you already had permissions to the flow then the call to modify them will fail. Can you please let me know what failure you see for this call? Please include downloaded output image

Thanks for the help. If you need more info, please let me know.

JeneferM-MSFT commented 3 years ago

Your image for the archive table did not come through, but I presume you looked for your default envt's GUID and found it to not have the Default-prefix?

WouterF commented 3 years ago

@JeneferM-MSFT, indeed the default environment does not have the default-prefix. In all of the records in the "Archive Approval" table, there is the same format of environment id's without prefix. This is probably the reason why this flow fails, a product change. I guess that the product team started removing the default prefix because it is too complex with different id's? Or am I wrong?

JeneferM-MSFT commented 3 years ago

Thank you Wouter. I would not have predicted that was the spot of error so this was very useful. I will investigate.

WouterF commented 3 years ago

Hi @JeneferM-MSFT, do you already have more news about the investigation with the default- prefix? Thanks

JeneferM-MSFT commented 3 years ago

The fix is in testing, expect to ship this week. Thanks

JeneferM-MSFT commented 3 years ago

Hello. this fix for this was released today. The fix is in the "start approval" flows in that we fetch the proper name (including pre-fix) to put in the archival table. image

Please be sure to remove any unmanaged layers before updating as described in installing upates.

And thanks for helping make CoE great!

WouterF commented 3 years ago

@JeneferM-MSFT , the flow is still failing after installing the new version of the CoE yesterday. Should I clean the "Archive Approvals" table because it contains the old records? image

JeneferM-MSFT commented 3 years ago

Yes thats correct. This will not go back and fix old entries, it will just correct new entries. Thank you for asking, I hadnt thought to mention that in this fix.