dspace-angular icon indicating copy to clipboard operation
dspace-angular copied to clipboard

Published 20 hours ago • verified publisher icon DSpace

Ensure auth methods are re-fetched after an authentication error.

Open philipprumpf opened this issue 3 years ago • 2 comments

References

  • Fixes #1793.

Description

Make sure a CheckAuthenticationTokenAction is triggered if authentication fails (this happens when the backend is restarted while a non-expired token is in storage). This, in turn, refreshes the auth methods and avoids an empty non-functional authentication menu.

Instructions for Reviewers

We must be careful not to trigger a CheckAuthenticationTokenAction recursively, which is why hasToken was added.

List of changes in this PR:

  • add AuthenticationService.hasToken to check existence of a token
  • make removeToken call conditional on the existence of a token
  • trigger CheckAuthenticationTokenAction if authentication fails and there was a valid-looking token

Checklist

  • [X] My PR is small in size (e.g. less than 1,000 lines of code, not including comments & specs/tests), or I have provided reasons as to why that's not possible.
  • [X] My PR passes TSLint validation using yarn run lint
  • [X] My PR doesn't introduce circular dependencies
  • [X] My PR includes TypeDoc comments for all new (or modified) public methods and classes. It also includes TypeDoc for large or complex private methods.
  • [ ] My PR passes all specs/tests and includes new/updated specs or tests based on the Code Testing Guide.

philipprumpf avatar Aug 29 '22 08:08 philipprumpf

Hmm. I've added some basic tests, but the 14.x tests appear to be failing in unrelated tests. Is this expected?

philipprumpf avatar Sep 01 '22 08:09 philipprumpf

@philipprumpf yes the e2e tests sometimes fail at random. I'll restart them

artlowel avatar Sep 01 '22 13:09 artlowel

@atarix83 Thanks! That makes sense, yes, I'll make the change.

philipprumpf avatar Sep 08 '22 10:09 philipprumpf

@philipprumpf : Just a friendly reminder that this PR is on your plate to do minor updates. If you'd like this included in 7.4, we'd need those updates done ideally in the next week... any PR not merged by the end of Sept will be rescheduled for the next release (7.5 due in Feb). So, the quicker you can make updates, the quicker we can get this re-reviewed and ready to go!

tdonohue avatar Sep 15 '22 17:09 tdonohue

@tdonohue Thanks!

@atarix83 Thanks again for the review. I've updated the PR.

philipprumpf avatar Sep 19 '22 08:09 philipprumpf

I agree this particular PR addresses a problem that no longer happens (or at least happens differently - if I notice it again, I'll open a new PR against 7.4)

philipprumpf avatar Sep 29 '22 07:09 philipprumpf