Control level get current user, site and web

Question

I have inherited fixes and enhancements on a SharePoint 2013 site and I'm a complete SP noob. I have a user control (.ascx file) that lives in the ControlTemplates folder that is referenced on a boatload of pages (which I think may be an issue in and of itself, but a different issue). One thing I've noticed is code like this:

protected void Page_Load(object sender, EventArgs e)
{
    SPUser CurrentLoggedInUser = SPContext.Current.Web.CurrentUser;

    Guid webID = SPContext.Current.Web.ID;
    Guid siteID = SPContext.Current.Site.ID;

    using (SPSite site = new SPSite(siteID))
    {
        site.AllowUnsafeUpdates = true;
        using (SPWeb web = site.OpenWeb(webID))
        {
              FunctionA("hello");
              // Do something here
        }
    }
}

protected void FunctionA(string message)
{
    SPSecurity.RunWithElevatedPrivileges(delegate()
    {
        SPUser CurrentLoggedInUser = SPContext.Current.Web.CurrentUser;
        Guid webID = SPContext.Current.Web.ID;
        Guid siteID = SPContext.Current.Site.ID;

        using (SPSite site = new SPSite(siteID))
        {
            site.AllowUnsafeUpdates = true;
            using (SPWeb web = site.OpenWeb(webID))
            {
                // Do something
            }
        }
     }
}

CurrentLoggedInUser, webID and siteID are declared and assigned in a large number of functions. What I want to know: is there some reason that would make it a bad idea to declare some page level variables and assign them in Page_Load and remove the redundant "CurrentLoggedInUser", "webID", "siteID" declarations and assignments? I'm thinking something like this:

Guid _webId;
Guid _siteId;
SPUser _currentLoggedInUser;

protected void Page_Load(object sender, EventArgs e)
{
    _webId = SPContext.Current.Web.ID;
    _siteId = SPContext.Current.Site.ID; 
    _currentLoggedInUser = SPContext.Current.Web.CurrentUser;

    // Do some work
}

protected void FunctionA(string message)
{
    // No longer need to get webID, siteID or CurrentLoggedInUser - can
    // use _webId, _siteId and _currentLoggedInUser
}

I figure it'd clean up the code a good bit and anything I can start to do to reduce this monster .ascx page and simplify the logic (I've found lots of code that does nothing) would be a good thing.

Answer 1

or

private Guid webID;
private Guid siteID;
private SPUser CurrentLoggedInUser;

protected void Page_Load(object sender, EventArgs e)
{
    CurrentLoggedInUser = SPContext.Current.Web.CurrentUser;

    webID = SPContext.Current.Web.ID;
    siteID = SPContext.Current.Site.ID;
    SPSite site;

    try
    {

       using (site = new SPSite(siteID))
       {
          using (SPWeb web = site.OpenWeb(webID))
          {
              FunctionA("hello");

          }
      }
   }
   catch(exception a)
   {
      //error logging
   }
}

protected void FunctionA(string message, SPWeb web)
{
    SPSecurity.RunWithElevatedPrivileges(delegate()
    {
      site.AllowUnsafeUpdates = true;
          // Do something to web object
          SPList list = web.Lists["myListExample"];
      site.AllowUnsafeUpdates = false;

    }
}

I don't quite know what the guy doing with the code for him to do it that way, From what I can see... quite possible you might need in some instance to open a new web object running on a separate thread hence the .OpenWeb() and then he is elevating privileges within a function....

as an example a specific user on his context might not be able to perform a specific task or that you want any action that meta data (modified by) to be recorded as system account as its elevated an on a separate thread other than what the user is logged on with. If the code is fully and what you shown is all he's doing than not its excessive and unnecessary so your solution would be correct! using site.AllowUnsafeUpdates = true; should only be used on the part of updating section rather than setting at the parent level of code, like updating a site web or list with a get request to sql database. This is in place to protect against cross site scripting attacks

Answer 2

That is perfectly fine, everything that is retrieved from the SPContext.Current can be put in a private.

What you absolutely should avoid is put any objects derived from a constructed new SPWeb or new SPSite in a private variable, these need to be properly disposed, preferably in a using statement. Stick to the SPContext.Current and you'll be fine.

Personally, I tend to stick to just keeping the structs.

EDIT:

RunWithElevatedPriviledges causes code to run under the application pool account the site is running under instead of the current user. However, it will only do this for objects that are created within it's scope, not objects that have already been created outside.

As the SPContext has already created the objects outside of the scope, these will not run under the application pool account. You can however re-use structs like guids from the SPContext.

In other words:

SPContext.Current.Web.Lists[0].DoesUserHavePermission(SPBasePermissions.ManageLists) -> False

loadedSPSiteObject.Lists[0].DoesUserHavePermission(SPBasePermissions.ManageLists) -> True

This:

CurrentLoggedInUser = SPContext.Current.Web.CurrentUser;

Will be the same object regardless where it's called.