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

Authenticating / Refreshing Google API access token and avoiding a “file in use” exception? #1708

Closed
ajtruckle opened this issue Dec 1, 2020 · 24 comments
Assignees
Labels
priority: p2 Moderately-important priority. Fix may not be included in next release. type: question Request for information or clarification. Not an issue.

Comments

@ajtruckle
Copy link

I have been encouraged to ask my question that I raised on StackOverflow here.

In short, I have been having occasional issues when trying to sync with a Google Calendar.

The basic functionality is:

  • Authenticate
  • Delete existing calendar events
  • Add new calendar events

The delete / add tasks are done using batching.

As mentioned, sometimes I get an exception like:

2020-11-30 18:47:03.6762|ERROR|GoogleAuthandSync.Program|AddEventsToCalendarXML|System.IO.IOException: The process cannot access the file 'C:\Users\USERNAME\AppData\Roaming\XXXXX.Application\Google.Apis.Auth.OAuth2.Responses.TokenResponse-user' because it is being used by another process.
at System.IO.__Error.WinIOError(Int32 errorCode, String maybeFullPath)

This is how I authenticate:

Private Function DoAuthentication(ByRef rStrToken As String, ByRef rParameters As OAuth2Parameters) As Boolean
    Dim credential As UserCredential
    Dim Secrets = New ClientSecrets() With {
        .ClientId = m_strClientID,
        .ClientSecret = m_strClientSecret
    }

    m_Scopes.Add(CalendarService.Scope.Calendar)

    Try
        credential = GoogleWebAuthorizationBroker.AuthorizeAsync(Secrets, m_Scopes,
                                                                 "user", CancellationToken.None,
                                                                 New FileDataStore("XXXXX.Application")).Result()

        If credential.Token.IsExpired(Google.Apis.Util.SystemClock.Default) Then
            credential.RefreshTokenAsync(CancellationToken.None)
        End If


        ' Create the calendar service using an initializer instance
        Dim initializer As New BaseClientService.Initializer() With {
            .HttpClientInitializer = credential,
            .ApplicationName = "xxx"
        }
        m_Service = New CalendarService(initializer)

        rStrToken = credential.Token.AccessToken.ToString()
        rParameters.AccessToken = credential.Token.AccessToken
        rParameters.RefreshToken = credential.Token.RefreshToken
    Catch ex As Exception
        ' We encountered some kind of problem, perhaps they have not yet authenticated?
        ' Can we isolate that as the exception?
        m_logger.Error(ex, "DoAuthentication")

        Return False
    End Try

    Return True
End Function

I was wondering if this line is wrong:

credential.RefreshTokenAsync(CancellationToken.None)

Should it be proceeded with await? I wondered if it might still be refreshing the token file whilst we are continuing to try and use it. I can provide further code snippets as needed.

As mentioned, it usually works and this is a sporadic issue.

@amanda-tarafa amanda-tarafa self-assigned this Dec 1, 2020
@amanda-tarafa amanda-tarafa added priority: p2 Moderately-important priority. Fix may not be included in next release. type: question Request for information or clarification. Not an issue. labels Dec 1, 2020
@LindaLawton
Copy link
Collaborator

@amanda-tarafa i forwarded them over here. I have never seen this issue before with the library. In my experience the file lock is always released after it reads it so i'm not sure why the lock would remain. However my knowledge of this library in VB.net is very limited

@jskeet
Copy link
Collaborator

jskeet commented Dec 1, 2020

I suspect the problem may be caused by calling RefreshTokenAsync and then not waiting for it to complete. (You should also await the AuthorizeAsync call instead of using the Result property.) But more importantly, you shouldn't need to do any of that part yourself at all. It's not clear to me why you're storing the access token and refresh token - you're already creating the service with the credential appropriately, and the credential will just refresh its access token automatically as and when it needs to.

I would have hoped that the library would be thread-safe and not try to perform multiple operations on the file anyway, and we should look into that, but I'm also hoping that the code given can be simplified to remove the manual refresh.

@ajtruckle
Copy link
Author

Hi @jskeet. Thanks for your messages. I have simply coded it as best as I understood it. Sounds like you are proposing a better, more simpler way for me to handle authentication all together? Can I kindly ask for an example based around what I have at the moment?

I had to add the refresh approach in because users weren't using my calendar sync too regularly and the token was expiring. I think the majority of what I have show you I extracted from examples online.

@jskeet
Copy link
Collaborator

jskeet commented Dec 1, 2020

I had to add the refresh approach in because users weren't using my calendar sync too regularly and the token was expiring. I think the majority of what I have show you I extracted from examples online.

But if you're using m_Service, that should refresh automatically. We can't tell from the sample what you're doing with rStrToken or rParameters - in most cases you shouldn't need to care about the access/refresh token yourself at all. Just load the credential, create the service with it, and everything should happen transparently.

@ajtruckle
Copy link
Author

In my Main I start off with:

       Dim strPath As String = ""
        Dim strToken As String = ""
        Dim strRefreshToken As String = ""
        Dim iResult As Integer = 0

        Dim parameters As New OAuth2Parameters
        If (DoAuthentication(strToken, parameters)) Then
            iResult = RESULT_SUCCESS_OAUTH
        Else
            Return RESULT_FAILED_OAUTH
        End If

Then, after setting up some logging I do this:

            Dim settings As New RequestSettings("XXX", parameters) With {
                .AutoPaging = True
            }

Where XXX is the name of my tool. Was not sure if it was safe to expose the name of the tool on here?

My app is a console app and has some command line switches. So if I pass /authenticate it is supposed to trigger the user to authenticate and allow my tool to work with their account. If it is authenticating, it does this:

               If (Left(strArgument, 13) = "/authenticate") Then ' AJT v2017.11.18.01
                    strPath = ExtractArgumentData("/authenticate", strArgument)
                    If (strPath <> "") Then
                        Dim oIni As New Ini.IniFile(strPath)
                        oIni.IniWriteValue("Authenticate", "AccessToken", parameters.AccessToken)
                        oIni.IniWriteValue("Authenticate", "RefreshToken", parameters.RefreshToken)

                        Exit For
                    End If

But in our particular case, we had already authenticated and already had a data file in the user folder. And it was running my /addtocaledarxml command line switch:

                ElseIf (Left(strArgument, 17) = "/addtocalendarxml") Then
                    strPath = ExtractArgumentData("/addtocalendarxml", strArgument)
                    If (strPath <> "") Then
                        If (AddEventsToCalendarXML(strPath)) Then
                            iResult = RESULT_ADD_CALENDAR_OK
                        Else
                            iResult = RESULT_ADD_CALENDAR_ERROR
                        End If
                        System.IO.File.Delete(strPath) ' We are finished with it ...!
                    End If

What I don't understand is that you are telling it it is all supposed to work under the hood, and I admit that my Outlook Calendar tool which does the same stuff is much simpler and I don't have to worry about anything at all. But I always had problems with Google API.

Going back to basics then. My master application shows a window and wants to display a list of calendars from the user for them to choose one to sync with. They will be in one of two scenarios:

  1. It can already show the list of calendars, and we already know the id of the calendar they want to use so can pre select it in our list box.

  2. It can't show anything because the user has not authenticated. So we need to go through the motions of the authenticating wizard before we continue to read the list of calendars.

  3. It was authenticated and needed to refresh the token before proceeding.

It sounds like you are tell me that I don't need to worry about any of it?

@jskeet
Copy link
Collaborator

jskeet commented Dec 1, 2020

I would suggest avoiding writing out your own authentication file. You're already using an auth file here: New FileDataStore("XXXXX.Application"). Just use that, always.

That authentication flow will (IIRC) detect if the file exists and use the existing authentication if it's present and valid for the scopes you're requesting, or prompt the user if necessary. There's no need to do anything more, as far as I can see.

@amanda-tarafa
Copy link
Contributor

Your suspicion is right, definetely credential.RefreshTokenAsync(CancellationToken.None) should be awaited else, yes, there might be file clashes if the token had expired or was close to expiring, because when m_service requests a token from the credential the credential will also attempt to refresh the token to guarantee that m_service always gets a fresh one. And also, there's no guarantee that the token you get in this line rParameters.AccessToken = credential.Token.AccessToken is the fresh token if refreshing is not done yet. (And async methods should always be awaited).

You'll have to turn your method into an async method, like such:

Private Async Function DoAuthentication(ByRef rStrToken As String, ByRef rParameters As OAuth2Parameters) As Task (Of Boolean)

I would also recommend to await the call to AuthorizeAsync instead of calling .Result() which will block the thread (and in some cases could lead to deadlocks):

credential = Await GoogleWebAuthorizationBroker.AuthorizeAsync(Secrets, m_Scopes,
                                                                 "user", CancellationToken.None,
                                                                 New FileDataStore("XXXXX.Application"))

Also, I see that you are returning the tokens as part of your method, so I suppose you are using them somewhere else and need them fresh? If I may ask, what are you using them for, since you've already instantiated the CalendarService using the credential? If you don't need them for anything else, you don't need to refresh the token explicitly here, when the calendar service requests a token from the credential, the credential will check whether the token has expired or is close to expiring and will refresh it and always give you a fresh one.

@ajtruckle
Copy link
Author

ajtruckle commented Dec 1, 2020

Hi @amanda-tarafa

I just tried changing my code to use Async etc but now it says that my DoAuthentication function can't pass ByRef parameters.

I need to populate the passed in rParameters:

        rParameters.AccessToken = credential.Token.AccessToken
        rParameters.RefreshToken = credential.Token.RefreshToken

So that other calls can use it. Would it be better here to change DoAuthentication to return a UserCredential instead? And then I can fill in the rParameters in Main.

Also, I see that you are returning the tokens as part of your method, so I suppose you are using them somewhere else and need them fresh? If I may ask, what are you using them for, since you've already instantiated the CalendarService using the credential?

In short, I don't really remember why I did that. I started writing this tool along time ago and I most likely (obviously) didn't understand the mechanics of it. I am just getting a bit confused ....

@amanda-tarafa
Copy link
Contributor

I just tried changing my code to use Async etc but now it says that my DoAuthentication function can't pass ByRef parameters

Yes, Async methods can't have ByRef parameters, but than can be easily solved, if you needed it, by returning a more complex type.

But, as @jskeet suggested, I would advise you stop returning and writing in a file the tokens yourself, and also refreshing the tokens. Your authenticate method will turn into something like this:

Private Async Function DoAuthenticationAsync() As Task (Of Boolean)
    Dim credential As UserCredential
    Dim Secrets = New ClientSecrets() With {
        .ClientId = m_strClientID,
        .ClientSecret = m_strClientSecret
    }

    m_Scopes.Add(CalendarService.Scope.Calendar)

    Try
        credential = Await GoogleWebAuthorizationBroker.AuthorizeAsync(Secrets, m_Scopes,
                                                                 "user", CancellationToken.None,
                                                                 New FileDataStore("XXXXX.Application"))

        ' Create the calendar service using an initializer instance
        Dim initializer As New BaseClientService.Initializer() With {
            .HttpClientInitializer = credential,
            .ApplicationName = "xxx"
        }
        m_Service = New CalendarService(initializer)

    Catch ex As Exception
        ' We encountered some kind of problem, perhaps they have not yet authenticated?
        ' Can we isolate that as the exception?
        m_logger.Error(ex, "DoAuthentication")

        Return False
    End Try

    Return True
End Function

And then, as long as you are using m_service in methods such as AddEventsToCalendarXML you don't need to read the tokens from your own file. m_service has already been initialized with the credential, which stores the tokens in "XXXXX.Application" and will always provide m_service with a fresh token, because it checks the token in the file for freshnes and refreshes it if needed. So, although I'm not sure I have a full understanding of your app, I think you can remove the authenticate switch and always call await DoAuthenticationAsync() as a first step for all the other switches to guarantee that you have m_service correctly initialized with the credential.

You can see a C# example here in the method called Run (it uses the Book API but that's the same as using the Calendar API).

@ajtruckle
Copy link
Author

ajtruckle commented Dec 1, 2020

Thanks @amanda-tarafa ! I am taking this one step at a time. I have changed my function. As a result, I have two further issues (for now).

  1. I need to change the following bit of code because theDoAuthenticateAsync no longer returns a plain Boolean:

     Dim parameters As New OAuth2Parameters
     If DoAuthenticationAsync() Then
         iResult = RESULT_SUCCESS_OAUTH
     Else
         Return RESULT_FAILED_OAUTH
     End If
    

2, We no longer have a parameters variable of type OAuth2parameters so I can't do this:

        Dim settings As New RequestSettings("XXX", parameters) With {
            .AutoPaging = True
        }

Update

I assume that point 2 is redundant. It turns out that I don't actually use settings anywhere myself. So unless this RequestSettings call is required for calendar sync to work I assume it is safe for me to comment it out. Correct?

@ajtruckle
Copy link
Author

It seems that if I simply change this bit to:

        If DoAuthenticationAsync.Result Then
            iResult = RESULT_SUCCESS_OAUTH
        Else
            Return RESULT_FAILED_OAUTH
        End If

Then it will be Ok to use the async function because Result will apparently wait and fetch the result from the task. Correct?

And it is Ok to comment out that RequestSettings code? If so, I will make all the changes and remove all of my storing of token data in INI files (with exception of calendar list details for showing in my own listbox).

@jskeet
Copy link
Collaborator

jskeet commented Dec 1, 2020

Then it will be Ok to use the async function because Result will apparently wait and fetch the result from the task. Correct?

You should generally not use the Result property from tasks. That blocks the current thread until the task has completed - which means if the task is asynchronous and expects to be able to get back to the current thread (e.g. in a WinForms app) it creates a deadlock.

Instead, you should make this calling method async as well, and await the task.

@ajtruckle
Copy link
Author

@jskeet I am calling this from Main. Can that me made async? Or do I need to move all of my code from inside Main into a new async function then?

@amanda-tarafa
Copy link
Contributor

amanda-tarafa commented Dec 1, 2020

It's always better to do Await, calling Result will block the calling thread and could in some cases lead to deadlocks. Here is some starter docs on Asynchronous programming in .NET

If Await DoAuthenticationAsync Then

And it is Ok to comment out that RequestSettings code? If so, I will make all the changes and remove all of my storing of token data in INI files (with exception of calendar list details for showing in my own listbox).

You said yourself you are not using those, right? The following line is only creating an instance of the RequestSettings object, if you don't use settings anywhere it will have no effect, so in that case it is safe to delete them.

Dim settings As New RequestSettings("XXX", parameters) With {
            .AutoPaging = True
        }

@ajtruckle
Copy link
Author

@amanda-tarafa Thanks for the clarifications. I think the RequestSettings code is from an original sample of how to do things and I never took it out. Great - now removed.

And thanks for the link on async programming. I have now changed my Main to:

Public Async Function Main() As Task(Of Integer)

But it won't compile now:

bc : error BC30737: No accessible 'Main' method with an appropriate signature was found in 'Program'.

@ajtruckle
Copy link
Author

So I have now moved all of my main "main" code into a function:

Private Async Function Run() As Task(Of Integer)

And now my main looks like this:

    Public Function Main() As Integer
        Run.Wait()

        Return Run.Result
    End Function

I still have to move all the obsolete calls to logging of token info to the INI, but apart from that is the above correct? Thanks.

@ajtruckle
Copy link
Author

I realise Amanda said:

It's always better to do Await, calling Result will block the calling thread and could in some cases lead to deadlocks.

But eventually we end up in Main.

@amanda-tarafa
Copy link
Contributor

Yes, given that yours is a console application, it should be fine as it is now. Just calling Wait() and Result is redundant. You can just do:

Public Function Main() As Integer
        Return Run.Result
End Function

@ajtruckle
Copy link
Author

Thank you! Fantastic. :) I will give all this ago. I may end up asking a related question about obtaining the calendar list. But I won't bloat this discussion. I will give it all a try and see if my exceptions stop now.

@ajtruckle
Copy link
Author

Whilst I am in the middle of deleting all references to maintaining my own copies of tokens, I now see why. I don't know if this should be split into a new question.

In my main application it has a option to remove Google access. At the moment it is doing this:

bool CCalendarSettingsGooglePage::RevokeGoogleAccess()
{
	bool bOK = false;
	CStringW header;
	CStringA query;
	{
		CStringW queryW;
		queryW.Format(L"token=%s", (LPCTSTR)m_strGoogleToken);
		query = CW2A(queryW, CP_UTF8);
	}
	DWORD flag = INTERNET_FLAG_SECURE | INTERNET_FLAG_DONT_CACHE |
		INTERNET_FLAG_IGNORE_CERT_CN_INVALID | INTERNET_FLAG_IGNORE_CERT_DATE_INVALID;

	header.Format(L"Content-Type: application/x-www-form-urlencoded\r\nContent-length: %d\r\nConnection: Close\r\n\r\n", query.GetLength());

	CInternetSession session;
	CHttpConnection* connection = session.GetHttpConnection(L"accounts.google.com",
		(INTERNET_PORT)INTERNET_DEFAULT_HTTPS_PORT);
	if (connection)
	{
		CHttpFile* file = connection->OpenRequest(L"POST", L"o/oauth2/revoke", L"http://localhost", 1, NULL, NULL, flag);
		if (file)
		{
			if (file->SendRequest(header, query.GetBuffer(), query.GetLength()))
			{
				DWORD dwRet;
				if (file->QueryInfoStatusCode(dwRet))
				{
					if (dwRet == 200 || dwRet == 400)
					{
						//success!
						bOK = true;
					}
				}
			}
			delete file;
		}
		delete connection;
	}

	if (bOK)
	{
		CGoogleAuthandSync syncGoogle(theApp.GetProgramPath(), theApp.GetWorkingPath());
		syncGoogle.DeleteToken();
	}

	return bOK;
}

The above is a Visual C++ MFC project and as you can see, it needed the token.

Now, I also have another function:

bool CGoogleAuthandSync::DeleteToken()
{
	CString	strCommand = BuildCommandLine(_T("deletetoken"), _T(""));
	DWORD   dwExitCode;

	if (ExecuteProgram(strCommand, dwExitCode))
	{
		if (dwExitCode == 12)
		{
			CString strError;

			strError.Format(IDS_TPL_GOOGLE_AUTHENTICATE_FAILED, (LPCTSTR)g_strResultDesc[12]);
			AfxMessageBox(strError, MB_OK | MB_ICONINFORMATION);
			return false;
		}
		else
			return true;
	}

	return false;
}

This one is called at the bottom of the previous one.

The DeleteToken counterpart in my console app is:

   Private Function DeleteToken() As Boolean

        Try
            Dim strFolder As String = New FileDataStore("XXXXX.Application").FolderPath
            For Each foundFile As String In My.Computer.FileSystem.GetFiles(strFolder)
                Console.WriteLine(foundFile)
                If (foundFile.Contains("-user")) Then
                    File.Delete(foundFile)
                    Exit For

                End If
            Next

        Catch ex As Exception
            ' We encountered some kind of problem, perhaps they have not yet authenticated?
            ' Can we isolate that as the exception?
            m_logger.Error(ex, "DeleteToken")

            Return False
        End Try

        Return True
    End Function

If I am to remove any requirement for my main application to need knowledge of tokens I need to either migrate this code into VB.Net or find another solution. I am open to guidance as I am now in the middle of changes to a main app that no longer compiles. Thanks!

@amanda-tarafa
Copy link
Contributor

You don't need to do any of that, there's a Revoke method on the UserCredential. You can do something like the following (I'm writing directly on the comment so there might be some syntax errors).

Private Async Function DoRevokeAsync() As Task (Of Boolean)
    Dim credential As UserCredential
    Dim Secrets = New ClientSecrets() With {
        .ClientId = m_strClientID,
        .ClientSecret = m_strClientSecret
    }

    m_Scopes.Add(CalendarService.Scope.Calendar)

    Try
        credential = Await GoogleWebAuthorizationBroker.AuthorizeAsync(Secrets, m_Scopes,
                                                                 "user", CancellationToken.None,
                                                                 New FileDataStore("XXXXX.Application"))

        Await credential.RevokeTokenAsync(CacenllationToken.None)

    Catch ex As Exception
        ' We encountered some kind of problem, perhaps they have not yet authenticated?
        ' Can we isolate that as the exception?
        m_logger.Error(ex, "DoAuthentication")

        Return False
    End Try

    Return True
End Function

This will both revoke the token with the Google Auth platform and delete it from the local file.

@ajtruckle
Copy link
Author

ajtruckle commented Dec 1, 2020

Thanks. Since I call DoAuthenticationAsync before parsing the list of command line arguments, is it acceptable to store the UserCredentials from that method into a member variable of the app, so that when I do know that we need to revoke I can run it directly. No security risks in maintaining that credentials variable like that?

Or, if I omit the m_Scopes line, I suppose it does not matter building a new credential object ....

@ajtruckle
Copy link
Author

Just tried the latter and it functions OK. Great! I will continue testing with a test calendar before I close this discussion.

@ajtruckle
Copy link
Author

It seems to work great. You have been very helpful. I'll ask the other question shortly.

Thanks again!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
priority: p2 Moderately-important priority. Fix may not be included in next release. type: question Request for information or clarification. Not an issue.
Projects
None yet
Development

No branches or pull requests

4 participants