Updating site-config to do permission/lock checks after initial load #2318
Conversation
| @@ -46,6 +46,9 @@ export class AuthzService { | |||
| return this._getLocks(resourceId) | |||
| .map(locks => { | |||
| return !!locks.find(l => l.properties.level === 'ReadOnly'); | |||
| }) | |||
| .catch(e => { | |||
| return Observable.of(false); | |||
| }); | |||
There was a problem hiding this comment.
Does this mean we assume there are no locks if the ARM call fails? #Resolved
There was a problem hiding this comment.
Yes. In the past we've had issues where ARM has an LSI going on for certain permission calls which would completely break our UI. Returning false would allow us to make a best effort to continue to load as much data as we can. If they actually have a lock, then the back-end would handle it properly.
In reply to: 167211993 [](ancestors = 167211993)
| @@ -128,7 +125,8 @@ export class ConnectionStringsComponent extends FeatureComponent<ResourceId> imp | |||
|
|
|||
| ngOnDestroy(): void { | |||
| super.ngOnDestroy(); | |||
| this._busyManager.clearBusy(); | |||
| // this._busyManager.clearBusy(); | |||
| @@ -151,7 +149,7 @@ export class ConnectionStringsComponent extends FeatureComponent<ResourceId> imp | |||
| this.hasWritePermissions = writePermission && !readOnlyLock; | |||
| } | |||
|
|
|||
| private _setupForm(connectionStringsArm: ArmObj<ConnectionStrings>, slotConfigNamesArm: ArmObj<SlotConfigNames>) { | |||
| private _setupForm(connectionStringsArm: ArmObj<ConnectionString>, slotConfigNamesArm: ArmObj<SlotConfigNames>) { | |||
There was a problem hiding this comment.
ConnectionString [](start = 52, length = 16)
Why did this need to change? It doesn't seem right. #Resolved
There was a problem hiding this comment.
Actually ConnectionString is the correct one. If I remember correctly, I think one of your callers were casting this to "any", so it compiled, but it wasn't correct.
In reply to: 167218627 [](ancestors = 167218627)
There was a problem hiding this comment.
Sorry for the long write-up below. I'm still convinced this should be "ConnectionStrings", but I don't actually feel to strongly because it ends up working either way.
(Regarding your comment about casting to . There is one place I can see where I'm casting the result of "JSON.parse(JSON.stringify(this._connectionStringsArm))" to ArmObj, but that is because I then go on to make changes to that object which make it inconsistent with type ArmObj.)
I pulled your branch and tried switching this to "ConnectionStrings" and found that it actually compiles and runs properly regardless of whether you use "ConnectionString" or "ConnectionStrings" here. (The return type for SiteService.getConnectionStrings() just needs to be updated accordingly).
This is because every time we use connectionStringsArm.properties in this method, we do so in a way that allows us to ignore its type:
(1) "for (const name in connectionStringsArm.properties) {"
--This just loops through the object keys, so it will work on any object.
(2) "if (connectionStringsArm.properties.hasOwnProperty(name)) {"
--The method hasOwnProperty() can be called on any object.
(3) "const connectionString = connectionStringsArm.properties[name];"
--Using bracket notation to access properties will work for any object.
However, if this really should be "ConnectionString", then "connectionStringsArm.properties" would be of type ConnectionString, which is defined as "{ value: string; type: ConnectionStringType }".
In that case, the for loop below doesn't make sense:
(1) Doing "for (const name in connectionStringsArm.properties)", would just be looping through two strings "value" and "type" because those are the keys for an object of type "{ value: string; type: ConnectionStringType }". However, we know that isn't the case, because this ends up looping through the actual names of the connection strings.
(2) Doing "const connectionString = connectionStringsArm.properties[name];" and subsequently using "connectionString.type" and "connectionString.value" wouldn't make sense. It would only make sense if "const connectionString" was of type ConnectionString, which means connectionStringsArm.properties could be of type "{ [key: string]: ConnectionString }", which is the definition of type "ConnectionStrings".
In reply to: 167256711 [](ancestors = 167256711,167218627)
There was a problem hiding this comment.
This actually even worked with an arbitrary type "ArmObj<{ foo: boolean }>". (I just had to make corresponding updates in two other places.)
In reply to: 167308138 [](ancestors = 167308138,167256711,167218627)
| .do(null, error => { | ||
| this._logService.error(LogCategories.defaultDocuments, '/default-documents', error); | ||
| this._setupForm(null); | ||
| this.loadingFailureMessage = this._translateService.instant(PortalResources.configLoadFailure); |
There was a problem hiding this comment.
this.loadingFailureMessage = this._translateService.instant(PortalResources.configLoadFailure); [](start = 16, length = 95)
We still need this. It should be included in the "do" below, similar to what you do in ConnectionStringsComponent. #Resolved
| .do(null, error => { | ||
| this._logService.error(LogCategories.generalSettings, '/general-settings', error); | ||
| this._setupForm(null, null); | ||
| this.loadingFailureMessage = this._translateService.instant(PortalResources.configLoadFailure); |
There was a problem hiding this comment.
this.loadingFailureMessage = this._translateService.instant(PortalResources.configLoadFailure); [](start = 16, length = 95)
Same as my other comment in DefaultDocumentsComponent. This is still needed. #Resolved
| .do(null, error => { | ||
| this._logService.error(LogCategories.handlerMappings, '/handler-mappings', error); | ||
| this._setupForm(null); | ||
| this.loadingFailureMessage = this._translateService.instant(PortalResources.configLoadFailure); |
There was a problem hiding this comment.
this.loadingFailureMessage = this._translateService.instant(PortalResources.configLoadFailure); [](start = 16, length = 95)
Same as my other comment in DefaultDocumentsComponent. This is still needed. #Resolved
| .do(null, error => { | ||
| this._logService.error(LogCategories.virtualDirectories, '/virtual-directories', error); | ||
| this._setupForm(null); | ||
| this.loadingFailureMessage = this._translateService.instant(PortalResources.configLoadFailure); |
There was a problem hiding this comment.
this.loadingFailureMessage = this._translateService.instant(PortalResources.configLoadFailure); [](start = 16, length = 95)
Same as my other comment in DefaultDocumentsComponent. This is still needed. #Resolved
| this._setupForm(this._webConfigArm); | ||
| this.loadingMessage = null; | ||
| this.showPermissionsMessage = true; | ||
|
|
||
| return Observable.zip( | ||
| Observable.of(this.hasWritePermissions), | ||
| this._cacheService.postArm(`${this.resourceId}/config/web`, true), | ||
| (h, w) => ({ hasWritePermissions: h, webConfigResponse: w }) | ||
| ); |
There was a problem hiding this comment.
This should be removed, as you did in DefaultDocumentsComponent and HandlerMappingsComponent. #Resolved
There was a problem hiding this comment.
side note:
The POST call here was actually supposed to be a GET. I just noticed this bug yesterday. It has been present here and in DefaultDocumentsComponent and HandlerMappingsComponent as well, but things have still been working somehow. It only surfaced when I was doing some testing with commented out from SiteConfigComponent. Therefore, I assume the proper GET call made from GeneralSettingsComponent was getting cached, and the cache service was returning that cached response to these invalid POST calls. If that's the case, it doesn't seem like the correct behavior for cache service.
In reply to: 167225843 [](ancestors = 167225843)
There was a problem hiding this comment.
That's a good point. The cache service should probably be caching to include the verb as well. Here's an issue to track: /~https://github.com/Azure/azure-functions-ux/issues/2320
In reply to: 167234023 [](ancestors = 167234023,167225843)
* Making sure selected option in DropDownComponent matches the selected option in the native select element (fixes #2060) * Add CRM headers (#2225) * Revert "Check referer before checking X-MS-CLIENT-PRINCIPAL-NAME (#2218)" This reverts commit 300746c. * add keyboard controls to download function app * CRM headers only on create and delete (#2226) * rp update * add headers only on create and delete calls * pr updates and add headers for get entities * update delete calls to include headers * Tentatively remove MSA login for Try Functions * update icon (#2236) fixes #2235 * Use ARM url specified from Ibiza for national clouds (#2239) * Revert "Revert "Check referer before checking X-MS-CLIENT-PRINCIPAL-NAME (#2218)"" This reverts commit 481a96b. * Use ARM url specified from Ibiza for national clouds * Fixing CR comments * Experimental Language Toggle (#2238) * Experimental Language Toggle * pr updates * Add TIP1 to accepted origins (#2242) * Handling namespace for embedded scenario (#2237) * Fixes #1592 - [Accessibility] Screen reader does not read tree node options when in focus (#2221) * Minor embedded updates (#2245) * update templates and filter out errors * remove retrieve option * Fixing runtime upgrade warning string * Updating Ibiza redirect URL's (#2247) * Minor fixes to disable some features that don't work in national clouds (#2249) * Fix all functions caching (#2250) * Fix all functions caching on create * update function create calls * nit update * Change SiteManage component to open the new App Service Plan blade * Change Destroy to Delete (#2252) * Fixing build * Add durable resources (#2251) * CICD Rework ( Deployment Center) (#2205) * CICD Rework * Fixing a build break and updating the portal-resources.ts file * Fixing Resources from a bad merge * Adding flag in tsc to not check lib typings * correcting import casing for linux build server * Fixing issues in build with unused variables * Polish adjustments dashboard tables and auto refreshing of data * adding state to bitbucket and github oauth * cleaned up deployment detail log and fixed some translate pipes * removing unused variables to make compiler happy * removing bad debug logging from server code * Made first pass at a basic summary page * adding some support for summary of Local Git Choice * first pass at PR Comments, any that are currently marked resolved should be fixed here * Adjustments based on PR comments * Fixing build break * Update travis.yml angular cli * Removing nodejs server auth and making pug use minified * adding production check for optimized js files * adding AI support on the server * fixing azure head pug * Fixing some bugs around loading * Fixing PR changes * Hiding Deployment Center behind a feature flag * Fixing build breaks from last commit * adjustments based on PR comments * Last of PR comments * Fixing file name casing issues for linux builds * securing session cookie * fixing file names to match our format * Addressing PR Comment * Stop showing invalid connection string types, except when already configured (fixes #2248) * Adding build badges * Fixing windows local server running (#2268) * Fixes #2267 and Fixes #2227 Adding mime type for static json files for iisnode (#2269) * Let NodeJS handle static file loading * Trying different fix * Embedded updates (#2271) * Add preview and update editor name for embedded * Add ibiza support for deployment center (#2272) * Let NodeJS handle static file loading * Trying different fix * Adding support for loading deployment center inside of ibiza * fixing build * build fixes * Updated settings file to format TS files on save * Fixing deployment center to load itself rather than app settings * Adding site service and generalizing conditional-http-client (#2266) * Adding site service and generalizing conditional-http-client * Removing arm-preconditions * Moving precondition.ts back to where it was originally * Removing deployment center auth state check from returning auth tokens * Adding telemetry for AppSettings and creating base FeatureComponent (#2280) * Adding telemetry for AppSettings and creating base FeatureComponent * Updating with code review comments * Added parent component concept to make sure we log feature timings only when a parent exists to represent the entire feature * Whitelist update and accessibility updates (#2273) * whitelist update and accessibility updates * remove comment * pr updates * Making sure the form control and displayed value are in sync for the AutoSwapSlotName dropdown (fixes #2275) * EventGrid UI support in function2.0. Fixes #2199 * Disable "enable/disable" and "delete" for VS functions * Add variable cdn url injected at build time * Setting view cache to true on node server to check performance increase in next * making cdn url in body.pug correct * Fixing typo in index.pug that is preventing the rendering in ng-min * Updating the referenced templates (#2291) * Fixing web config to add static file mime types * Make gulp ignore resources made specifically for Visual Studio that aren't used for portal * Updating template package, fixing resources * Updating the swagger editor to match the luminosity ratio (#2288) * manage blade, focus on input when adding key * [API definition blade] adding role for expand / collapse (#2286) * Create from Integrate Tab (#2296) * Embedded templates "type" update (#2295) * xrmwebhooktrigger replaces synctrigger * update binding type * Moving as many pieces as possible to go through CDN rather than directly to the app (#2300) * Add variable cdn url injected at build time * trigger build * Adding Woff Mime type to web.config * fixing mime map in web.config * Adding mime types to web config * moving more assets to load from cdn * fixing mime types * adding ttf mime type * get resources from cdn when possible * Moving pieces to load from CDN where available * fixing cdn image loading * Adding cache breaking cdn calls * focus on combo box when get function url clicked * Adding Cache break query for image loading (#2303) * add role and label to click to show/click to hide * undo template update (#2305) * Add redirects for people going directly to functions.azure.com (#2313) * correct fowarding address for try functions * Updated Travis CI link * xrmwebhooktrigger change (#2314) * Fixes #2267 - Updating conditional http client to handle complex error types being thrown (#2311) * Fixing TriggerFunctionAPIM api for ASP.Net controller * Fixing build * Implement /api/runtimetoken in node server. Closes #2253 (#2298) * Implement /api/runtimetoken in node server. Closes #2253 * CR 1 * Temporarily updating APIMTrigger API in node for debugging purposes * Updating site-config to do permission/lock checks after initial load (#2318) * Updating site-config to do permission/lock checks after initial load * Add error handling to several components * Fixing connection string type * Error handle updates (#2323) * creation error handle * update log error catch * type update * Support linux function apps that don't have scm endpoint (#2322) * Support linux function apps that don't have scm endpoint * Updating site summary and site manage tabs to inherit from FeatureComponent (#2326) * Updating site summary and site manage tabs to inherit from FeatureComponent * Updated SiteEnabledFeaturesComponent to be a FeatureComponent * Cleaning up based on PR feedback * PR feedback * Upgrading from Angular 4 to Angular 5 (#2335) * Update embedded API version (#2336) * add role link to revoke renew * removing vendor import script that is not there in Angular 5 * Fixing local loading * Update embedded logging (#2337) * update embedded logging * remove first do/retry * Only display error on HostStatus === 'Error'. Closes #2330 * Fixing bug in embedded logging
In order to get this to work properly, I had to update FeatureComponent to support clearing the busy state before the input observable completed. To do this, I did: