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

fix(frontend): update cookie module #8862

Merged
merged 13 commits into from
Oct 17, 2023

Conversation

RyanHolstien
Copy link
Collaborator

NOTE: This is a breaking change for the session cookies. All previously created session cookies will be invalid regardless of their expiration time and users will have to login again. There should be no other issues with this change.

Updates session cookie configuration to utilize better security practices.

Checklist

  • The PR conforms to DataHub's Contributing Guideline (particularly Commit Message Format)
  • Links to related issues (if applicable)
  • Tests for the changes have been added/updated (if applicable)
  • Docs related to the changes have been added/updated (if applicable). If a new feature has been added a Usage Guide has been added for the same.
  • For any breaking change/potential downtime/deprecation/big changes an entry has been made in Updating DataHub

@RyanHolstien RyanHolstien added bug Bug report platform PR-s that make changes to core parts of the platform labels Sep 19, 2023
@RyanHolstien RyanHolstien self-assigned this Sep 19, 2023
docker/build.gradle Outdated Show resolved Hide resolved
HttpServletResponse servletResponse = mock(HttpServletResponse.class);
FilterChain filterChain = mock(FilterChain.class);
Actor actor = new Actor(ActorType.USER, "datahub");
// String token = _statefulTokenService.generateAccessToken(TokenType.SESSION, actor, 0L, System.currentTimeMillis(), "token",
Copy link
Collaborator

Choose a reason for hiding this comment

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

Why is this commented out? If it's not used might as well delete.
Is there no need to test the stateful token service?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This shows how the token was generated, I left in to make it more obvious

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

It's not super intuitive to know how the token is generated otherwise so I'd prefer to leave it in for anyone who wants to make a similar test and looks through this file if it doesn't bother you too much :)

@github-actions github-actions bot added docs Issues and Improvements to docs product PR or Issue related to the DataHub UI/UX devops PR or Issue related to DataHub backend & deployment labels Sep 19, 2023
@david-leifker
Copy link
Collaborator

Still has some checkstyle issues

@RyanHolstien RyanHolstien added the wip Work in progress, don't review yet label Sep 21, 2023
@RyanHolstien RyanHolstien removed the wip Work in progress, don't review yet label Sep 27, 2023
@RyanHolstien RyanHolstien merged commit 60c1aab into datahub-project:master Oct 17, 2023
39 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Bug report devops PR or Issue related to DataHub backend & deployment docs Issues and Improvements to docs platform PR-s that make changes to core parts of the platform product PR or Issue related to the DataHub UI/UX
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants