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

fixed bug the treat as same as no cookie if existing cookie vas invalid. #23915

Merged

Conversation

apc-kamezaki
Copy link
Contributor

Hey, I just made a Pull Request!

I've got error at /api/techdocs/cookie when I restarted backstage backend.

It was happend on the develop environment (using on-memory database).
It seemd it was happend because backend could not find the kid of JWT in the key store.

how to reproduce it.

  1. Signin and show to techdocs page
  2. Stop backend
  3. Restart backend
  4. Signin and try to show the same techdocs page.

This pr is for #23914

✔️ Checklist

  • A changeset describing the change and affected packages. (more info)
  • Added or updated documentation
  • Tests for new functionality and regression tests for bug fixes
  • Screenshots attached (for UI changes)
  • All your commits have a Signed-off-by line in the message. (more info)

@apc-kamezaki apc-kamezaki requested review from a team as code owners April 1, 2024 07:45
@backstage-goalie
Copy link
Contributor

Changed Packages

Package Name Package Path Changeset Bump Current Version
@backstage/backend-app-api packages/backend-app-api patch v0.6.2

Copy link
Contributor

github-actions bot commented Apr 1, 2024

Uffizzi Cluster pr-23915 was deleted.

Copy link
Member

@Rugvip Rugvip left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you! 👍

Ah, yep the current implementation here isn't right. I think we might change this up a bit more and possibly get rid of the optimization to not refresh cookies that aren't about to expire altogether. Either way we can ship this for now, with a small tweak

return existingCredentials.expiresAt;
return existingCredentials.expiresAt;
} catch (error) {
if (error instanceof AuthenticationError) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This check isn't safe, use error.name === 'AuthenticationError' instead, to be able to handle the case where @backstage/errors is duplicated.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'll fix it too.

'@backstage/backend-app-api': patch
---

fixed bug the treat as same as no cookie if existing cookie vas invalid.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
fixed bug the treat as same as no cookie if existing cookie vas invalid.
Fixed a bug where expired cookies would not be refreshed.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm sorry about my poor English.
I'll fix it.

Signed-off-by: Hitoshi Kamezaki <kamezaki@ap-com.co.jp>

update
@apc-kamezaki apc-kamezaki force-pushed the feature/handle-error-techdocs-cookie branch from f62d074 to 7e584d6 Compare April 2, 2024 09:45
@apc-kamezaki apc-kamezaki requested a review from Rugvip April 2, 2024 10:57
Copy link
Member

@Rugvip Rugvip left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you! 👍

Let's :shipit:

@Rugvip Rugvip merged commit 89a5b69 into backstage:master Apr 2, 2024
30 checks passed
Copy link
Contributor

github-actions bot commented Apr 2, 2024

Thank you for contributing to Backstage! The changes in this pull request will be part of the 1.26.0 release, scheduled for Tue, 14 May 2024.

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

Successfully merging this pull request may close these issues.

None yet

2 participants