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

Flysystem Fedora Adapter's JWT Auth expiring during large batch loads #1030

Closed
seth-shaw-unlv opened this issue Feb 20, 2019 · 10 comments
Closed

Comments

@seth-shaw-unlv
Copy link
Contributor

During a large migration of files I started receiving "The file permissions could not be set" errors. After some sleuthing and conversation with @dannylamb on irc we determined that the error was a red herring. The JWT was expiring causing fedora to return a 401 "Unauthorized" error when PUT-ing the file. This resulted in:

  • a file entity with a NULL file size for the file,
  • without the corresponding Fedora resource, and
  • a migration map entry for the migration source id and file entity.

The permissions error is from Drupal's file_unamanged_copy calling chmod on the Fedora resource that we failed to create. The chmod function logs the error and returns false but file_unmanaged_copy tosses the returned value and returns success anyway.

Two things need to happen:

  1. Further debug why the 401 response didn't trigger a Migration Exception.
  2. Prevent the JWT token from expiring OR renew it when it does expire.

Debugging Success Despite 401 Response

The Drupal migrate process plugin file_copy calls file_unmanaged_copy (or file_unmanaged_move, as the case may be) which then calls the PHP copy function. Somehow Flysystem overrides that call. The FedoraAdapter uses the StreamedCopyTrait which then calls FedoraAdapter's write function which returns FALSE if the response isn't 201 or 204. This should result in copy returning false which cascades back resulting in a MigrationException. Except, everyone seems happy with it and I don't know why.

JWT Expiry

The JWT is generated in Drupal\islandora\Flysystem\Fedora using what appears to be the defaults. JWT currently uses a one-hour expiry by default, so any file migrations running that long will probably hit this.

Now, elsewhere we make our own events with a two-hour expiry but I'm not sure how (or if) we could incorporate those portions.

Besides, both cases are hard-coded. There is a JWT ticket for configuring expiry times, but it "needs review." We could make a configuration for for our own JWT claims, assuming we can integrate that part into the Flysystem adapter.

Alternatively, and related to the other sub-issue above, if we are able to catch the unauthorized error we could check the existing JWT for expiry, if it is, issue a new JWT and attempt the write again.

@seth-shaw-unlv
Copy link
Contributor Author

After looking at my most recent attempt, the errors are beginning after exactly two hours. So, it looks like we are using the Islandora claim settings. So, let me see if adjusting that limit helps...

@whikloj
Copy link
Member

whikloj commented Feb 21, 2019

Seems like each new object created should generate a new JWT? But I perhaps don't understand the JWT functions. @jonathangreen thoughts?

@seth-shaw-unlv
Copy link
Contributor Author

seth-shaw-unlv commented Feb 21, 2019

@whikloj Update: I don’t think it is the token expiring. Additional testing confirmed what you suggested. Each file migration is getting it’s own JWT with their own 2-hour expiry.

My new guess is that the account switcher (used by the userid flag) is reverting back to anonymous after two hours; although I don’t know how/why yet. Doing more testing, but each round takes two hours. 🙄

@seth-shaw-unlv
Copy link
Contributor Author

Well that doesn't seem to be it either. The setIslandoraClaims function in JwtEventSubscriber is still showing the same user and roles before and after the error is thrown; so the user is still the same, it appears, which means the account switcher probably hasn't reverted to anonymous.

I'll keep digging.

@seth-shaw-unlv
Copy link
Contributor Author

Okay, back to the token expiring. Earlier I had placed a debugging message in JwtEventSubscriber where the JWTs are created. These messages showed fresh tokens being created, so I dismissed the expiring JWT theory.

However, for this most recent pass I enabled the debug option on the Guzzle client request in Chullo. This showed the same JWT being used for every save, which did expire at the end of two hours.

What is happening: Flysystem\Fedora is creating a Chullo Fedora with a Guzzle client that includes a JWT with a two-hour expiration. That static Flysystem\Fedora is being passed to the Flysystem Fedora Adapter. Perhaps we shouldn't be using a static class to hold a time-limited authorization... OR we could modify the getAdapter function to check the Fedora client's authorization OR is this a job for Chullo?

Thoughts, @dannylamb ?

@whikloj
Copy link
Member

whikloj commented Feb 23, 2019

I'd vote for changing the Flysystem to refresh it's token as needed.

@jonathangreen
Copy link
Contributor

In my opinion we should be treating JWT tokens as time limited bearer tokens and requesting a new one each time we start a Fedora operation on the Drupal side. Currently we are treating them more like passwords, which they are not.

Maybe flysystem could request a new token before it makes each request? I think that would sort out this issue.

One thing I that will probably still hang up migrations and might necessitate another solution / additional solution is that in large migrations with slow derivatives it’s possible to have the queue backup to the point where derivatives are waiting to be created for more then two hours. I’ve see queues that take weeks in 7.x. I don’t think this is the problem here, but it may become a problem in the future.

Solving that case is harder and maybe should be broken out from this work as I think changing flysystem fixes the problem here, I just wanted to note it when I thought of it.

A possible solution is to implement something like a refresh token:
https://auth0.com/docs/tokens/refresh-token/current
https://stackoverflow.com/questions/26739167/jwt-json-web-token-automatic-prolongation-of-expiration

@seth-shaw-unlv
Copy link
Contributor Author

Okay, new PR up that shifts creating the client that Chullo uses (and thus generating a fresh token) to the FedoraAdpater as suggested by @jonathangreen. This is simpler than adding logic to check for JWT expiry and refreshing which would have required a bunch of this code change anyway.

@seth-shaw-unlv
Copy link
Contributor Author

Even newer PR is up that provides a simpler solution to issuing new JWT for each request.

@seth-shaw-unlv
Copy link
Contributor Author

We've done what we can for this issue. Future work should be done under #1036.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants