umbraco-community / umbraco-analytics

Google Analytics for the Umbraco CMS
41 stars 31 forks source link

Package install Analytics_1.1.0-master in Umbraco 7.2.2 #27

Closed bjarnef closed 9 years ago

bjarnef commented 9 years ago

Installed package Analytics_1.1.0-master.zip in Umbraco 7.2.2 Debug was set to false during install.

It seems to have added language keys for both English and Danish. When clicking Authorize it open /App_Plugins/analytics/backoffice/OAuth.aspx in a new window (doesn't seem to include parameters? ) and returns the message "Sorry you do not have access."

2015-03-02_19-44-18

abjerner commented 9 years ago

Clicking "Authorize" will first open dialog with a local page. This page will initialize a few things for the authentication, and then redirect the user to the authentication site. It is when redirecting the user to this page the lang parameter is specified, so the language part should work fine. You just don't see it since you don't get that far :(

Regarding the "Sorry you do not have access." message, I haven't experienced that one yet. Perhaps it is related to http://issues.umbraco.org/issue/U4-6342#comment=67-19384 I will look into this a bit later tonight ;)

bjarnef commented 9 years ago

ahh okay :) .. yes, seems that there might be a bug somewhere related to UmbracoEnsuredPage

not sure if this have something to do with this issue in Analytics, but I noticed that the log contains this error from tonight:

2015-03-02 19:30:03,583 [18] ERROR umbraco.cms.businesslogic.packager.data - [Thread 8] An error occurred
System.NullReferenceException: Object reference not set to an instance of an object.
   ved umbraco.cms.businesslogic.packager.data.Save(PackageInstance package, String dataSource)
2015-03-02 19:30:06,893 [11] INFO  Umbraco.Core.UmbracoApplicationBase - [Thread 12] Application shutdown. Reason: BinDirChangeOrDirectoryRename
abjerner commented 9 years ago

I don't think the log entry is related to this issue.

The reason you're seeing the "Sorry you do not have access." message is because Warren changed something regarding UmbracoEnsuredPage in commit 94467e7, but it doesn't seems to fix the issue. I think we'll have to wait for Shannon to fix this for Umbraco 7.2.3.

bjarnef commented 9 years ago

yes, have seeen some reports about UmbracoContext.Current.Security.CurrentUser returning null :/

bjarnef commented 9 years ago

@abjerner did you see this comment https://github.com/warrenbuckley/Analytics/issues/4#issuecomment-76347977 if you want da-DKinstead of da?

abjerner commented 9 years ago

Parsing the string da will only give an instance of CultureInfo representing da, not da-DK. But for what we're using it for, it doesn't really matter whether it is da or da-DK.

bjarnef commented 9 years ago

okay, will GetCultureFromUserLanguage("da") then just return da ? .. okay, but can you distinguish between en English (UK) and en_us English (US) then? but probably quite similar.

abjerner commented 9 years ago

Yes, GetCultureFromUserLanguage("da") will simply return da since it has no way of knowing the country.

GetCultureFromUserLanguage("en") will return en, while GetCultureFromUserLanguage("en_us") will return en-US, so we can distinguish between the two.

As the current implementation is now, if the user has selected English (US), the lang parameter is sent to the authenticate site will be en_us, which isn't recognized by the authentication site since we only have translations for en and da. But en will always be used as fallback if nothing else is found, so it will still work.

warrenbuckley commented 9 years ago

Morning @abjerner & @bjarnef yes it was me who implemented an alternative whilst the 7.2.2 issue with UmbracoEnsuredPage for .aspx pages seems to be broken.

It's odd that the UmbracoContext.Current.Security.CurrentUser is null though I am pretty sure this worked for me. Does this fail for you both then?

abjerner commented 9 years ago

Yup, didn't work for me in 7.2.2. I think that is why UmbracoEnsuredPage is failing as well.

warrenbuckley commented 9 years ago

I wonder if it is because the page/URL is not inside the Umbraco backoffice section /umbraco/ maybe worth a quick test to see if we move this page into an Umbraco folder it will work? What do you think @abjerner ?

bjarnef commented 9 years ago

@abjerner @warrenbuckley could you try to make a test with a copy of the .aspx page - just locally from the plugin folder?

warrenbuckley commented 9 years ago

I dont have time this morning @bjarnef but if you get 5mins spare. Copy the .aspx page into say /umbraco/plugins and update the JS for opening the modal window to see if that works please.

@abjerner I implemented security for these pages due to the oAuth sensitive info but do you think there is a security risk/concern if anyone can visit this page/s without being logged into the Umbraco backoffice?

abjerner commented 9 years ago

Just tested this by creating an .aspx page in a fresh installation.

So we can fix this by moving the .aspx files :D

If Warren doesn't beat me to it, I can fix this when I get home from work ;)

warrenbuckley commented 9 years ago

Ah exactly what I thought. The security is based on the route of the page. Where before I believe it was a mix of route & or presence of auth cookie I believe. @abjerner I would update the Umbraco issue tracker with this info, so Shannon & gang are aware.

abjerner commented 9 years ago

I just did http://issues.umbraco.org/issue/U4-6342#comment=67-19384 ;)

I had tried creating the .aspx page in /App_Plugins/Whatever/backoffice, but that didn't seem to work either.

warrenbuckley commented 9 years ago

@abjerner so is the thought to move this then to /umbraco/plugins/analytics/ for the two .aspx pages for oAuth?

Just a shame we have to add custom files into the Umbraco folder for this to work, unless you do not think Umbraco auth is needed mate?

abjerner commented 9 years ago

I have now implemented the fix as suggested by Shannon. This seems to work for me. How about you guys?

warrenbuckley commented 9 years ago

Hi @abjerner I have not had a chance to test this yet. I may get some free time today to try it out and build out another test Umbraco package & NuGet to test with.

warrenbuckley commented 9 years ago

Will close this off guys as 1.2.0 is now out @bjarnef & @abjerner