-
Notifications
You must be signed in to change notification settings - Fork 250
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
Introduce universal/security library for AuthN and AuthZ #1486
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
2 tasks
This was referenced Mar 2, 2024
vitalii-honchar
requested changes
Mar 4, 2024
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Partially reviewed a PR. Major comments:
- Please use Java best practices and move newly created constants from the local declarations to
private static final
fields. - Please use single responsibility principle and don't create dependencies of some class inside it's constructor. If class depends on something, inject it as the dependency and leave the responsibility of the dependency creation to the client of the class.
- Please review code with Intelij Idea and remove highlighted unnecessary parts like
public
access modifier in the interface methods. - Please use Lombok
var
andSlf4j
annotations to create local variable or declare logger, let's try to use modern software engineering practices in Java 8 with Lombok.
...e/universal/src/main/java/com/pinterest/teletraan/universal/security/AuditLoggingFilter.java
Outdated
Show resolved
Hide resolved
...e/universal/src/main/java/com/pinterest/teletraan/universal/security/AuditLoggingFilter.java
Outdated
Show resolved
Hide resolved
...e/universal/src/main/java/com/pinterest/teletraan/universal/security/AuditLoggingFilter.java
Outdated
Show resolved
Hide resolved
...e/universal/src/main/java/com/pinterest/teletraan/universal/security/AuditLoggingFilter.java
Outdated
Show resolved
Hide resolved
...rvice/universal/src/main/java/com/pinterest/teletraan/universal/security/BaseAuthorizer.java
Show resolved
Hide resolved
...universal/src/main/java/com/pinterest/teletraan/universal/security/BasePastisAuthorizer.java
Outdated
Show resolved
Hide resolved
...universal/src/main/java/com/pinterest/teletraan/universal/security/BasePastisAuthorizer.java
Show resolved
Hide resolved
...universal/src/main/java/com/pinterest/teletraan/universal/security/BasePastisAuthorizer.java
Outdated
Show resolved
Hide resolved
...universal/src/main/java/com/pinterest/teletraan/universal/security/BasePastisAuthorizer.java
Outdated
Show resolved
Hide resolved
tylerwowen
force-pushed
the
spr/master/295e57c2
branch
from
March 4, 2024 22:09
cae348e
to
d443c62
Compare
tylerwowen
force-pushed
the
spr/master/17aa4dbb
branch
from
March 4, 2024 22:09
cba7ea9
to
d379af4
Compare
tylerwowen
force-pushed
the
spr/master/295e57c2
branch
from
March 4, 2024 22:20
d443c62
to
99117b0
Compare
tylerwowen
force-pushed
the
spr/master/17aa4dbb
branch
4 times, most recently
from
March 6, 2024 01:37
051c7c0
to
df4b162
Compare
tylerwowen
force-pushed
the
spr/master/295e57c2
branch
2 times, most recently
from
March 6, 2024 18:46
bdbf1df
to
e8c2163
Compare
tylerwowen
force-pushed
the
spr/master/17aa4dbb
branch
from
March 6, 2024 18:46
df4b162
to
46b0f23
Compare
tylerwowen
force-pushed
the
spr/master/295e57c2
branch
from
March 6, 2024 19:01
e8c2163
to
ef02f33
Compare
tylerwowen
force-pushed
the
spr/master/17aa4dbb
branch
from
March 6, 2024 19:01
46b0f23
to
8080061
Compare
tylerwowen
force-pushed
the
spr/master/17aa4dbb
branch
from
March 6, 2024 20:05
8080061
to
93d8636
Compare
...universal/src/main/java/com/pinterest/teletraan/universal/security/BasePastisAuthorizer.java
Outdated
Show resolved
Hide resolved
...oy-service/universal/src/main/java/com/pinterest/teletraan/universal/security/Constants.java
Outdated
Show resolved
Hide resolved
vitalii-honchar
previously approved these changes
Mar 6, 2024
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM. Leaved optional comments.
...ersal/src/test/java/com/pinterest/teletraan/universal/security/BasePastisAuthorizerTest.java
Show resolved
Hide resolved
...ersal/src/test/java/com/pinterest/teletraan/universal/security/BasePastisAuthorizerTest.java
Show resolved
Hide resolved
.../universal/src/test/java/com/pinterest/teletraan/universal/security/EnvoyAuthFilterTest.java
Show resolved
Hide resolved
...versal/src/test/java/com/pinterest/teletraan/universal/security/bean/ValueBasedRoleTest.java
Outdated
Show resolved
Hide resolved
tylerwowen
force-pushed
the
spr/master/17aa4dbb
branch
3 times, most recently
from
March 12, 2024 19:01
48a0a36
to
7aeecbd
Compare
tylerwowen
force-pushed
the
spr/master/17aa4dbb
branch
2 times, most recently
from
March 13, 2024 00:21
d693afd
to
88786fa
Compare
tylerwowen
force-pushed
the
spr/master/17aa4dbb
branch
from
March 13, 2024 00:39
88786fa
to
73b15b4
Compare
commit-id:17aa4dbb
tylerwowen
force-pushed
the
spr/master/17aa4dbb
branch
from
March 13, 2024 00:46
73b15b4
to
04079c0
Compare
vitalii-honchar
approved these changes
Mar 13, 2024
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This change reimplements the AuthN and AuthZ components.
Had to change the github workflows to include internal dependencies in order to get this pass PR validations.
Stack:
Things remaining