-
Notifications
You must be signed in to change notification settings - Fork 595
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
feat: add JWT and claim validation directives #4377
Conversation
7dcd7d7
to
aeff9ea
Compare
akka-http/src/main/scala/akka/http/scaladsl/server/util/JwtSprayJson.scala
Outdated
Show resolved
Hide resolved
akka-http/src/main/scala/akka/http/scaladsl/server/util/JwtKeyLoader.scala
Outdated
Show resolved
Hide resolved
akka-http/src/main/scala/akka/http/scaladsl/server/util/JwtSupport.scala
Outdated
Show resolved
Hide resolved
akka-http/src/main/scala/akka/http/scaladsl/server/directives/JwtDirectives.scala
Outdated
Show resolved
Hide resolved
akka-http/src/main/scala/akka/http/scaladsl/server/directives/JwtDirectives.scala
Outdated
Show resolved
Hide resolved
akka-http/src/main/scala/akka/http/scaladsl/server/directives/JwtDirectives.scala
Outdated
Show resolved
Hide resolved
akka-http/src/main/scala/akka/http/scaladsl/server/util/JwtSupport.scala
Outdated
Show resolved
Hide resolved
akka-http/src/main/scala/akka/http/scaladsl/server/util/JwtSupport.scala
Outdated
Show resolved
Hide resolved
8f046a0
to
0022b48
Compare
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.
Looking good!
akka-http-jwt/src/main/scala/akka/http/jwt/impl/settings/JwtSettingsImpl.scala
Outdated
Show resolved
Hide resolved
akka-http-jwt/src/main/scala/akka/http/jwt/scaladsl/JwtSettings.scala
Outdated
Show resolved
Hide resolved
akka-http-jwt/src/main/scala/akka/http/scaladsl/server/directives/JwtDirectives.scala
Outdated
Show resolved
Hide resolved
akka-http-jwt/src/main/scala/akka/http/scaladsl/server/directives/JwtDirectives.scala
Outdated
Show resolved
Hide resolved
akka-http-jwt/src/main/scala/akka/http/jwt/impl/settings/JwtSprayJson.scala
Outdated
Show resolved
Hide resolved
akka-http-jwt/src/main/scala/akka/http/scaladsl/server/directives/JwtDirectives.scala
Outdated
Show resolved
Hide resolved
akka-http-jwt/src/main/scala/akka/http/scaladsl/server/directives/JwtDirectives.scala
Outdated
Show resolved
Hide resolved
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.
Shaping up nicely, left a few smaller things except for those I think it's time to add a section to the docs.
akka-http-jwt/src/main/scala/akka/http/jwt/javadsl/server/directives/JwtClaims.scala
Outdated
Show resolved
Hide resolved
akka-http-jwt/src/main/scala/akka/http/jwt/javadsl/server/directives/JwtClaims.scala
Outdated
Show resolved
Hide resolved
akka-http-jwt/src/main/scala/akka/http/scaladsl/server/directives/JwtDirectives.scala
Outdated
Show resolved
Hide resolved
….scala Co-authored-by: Johan Andrén <johan@markatta.com>
akka-http-jwt/src/main/scala/akka/http/jwt/scaladsl/server/directives/JwtClaims.scala
Show resolved
Hide resolved
akka-http-jwt/src/main/scala/akka/http/jwt/javadsl/server/directives/JwtClaims.scala
Outdated
Show resolved
Hide resolved
akka-http-jwt/src/main/scala/akka/http/jwt/scaladsl/server/directives/JwtDirectives.scala
Outdated
Show resolved
Hide resolved
akka-http-jwt/src/test/scala/akka/http/jwt/scaladsl/server/directives/JwtDirectivesSpec.scala
Show resolved
Hide resolved
Co-authored-by: Johan Andrén <johan@markatta.com>
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 with one tiny fix to docs
docs/src/main/paradox/routing-dsl/directives/jwt-directives/jwt.md
Outdated
Show resolved
Hide resolved
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.
I think I've addressed all feedback now. Don't have any outstanding items on my list but left a few questions for final review/considerations. Feel free to send any additional feedback of course.
akka-http-jwt/src/main/scala/akka/http/jwt/javadsl/server/directives/JwtDirectives.scala
Show resolved
Hide resolved
docs/src/test/scala/docs/http/scaladsl/server/directives/JwtDirectivesExamplesSpec.scala
Outdated
Show resolved
Hide resolved
|
||
def fromConfig(jwtConfig: Config): JwtSupport = { | ||
val devSecret = if (jwtConfig.getBoolean("dev")) { | ||
Some(JwtSecret("dev", None, JwtNoneAlgorithmSecret)) |
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.
I wonder if we should print a log warning about dev
being on if that's the case? Trying to avoid people having it on in production without noticing..
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.
Yes, sounds like a good idea.
Somewhat tricky to implement though, we wouldn't want to log it for each request, and not sure we want to put a logger in the settings class. So in the directive with a flag to only log it one or a few times (flag would need to be volatile because cross-thread access, but if can be setting first so it is only ever touched if dev enabled)?
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.
Added with c65785d
Let me know what you think. It's logging once every minute.
…t.md Co-authored-by: Johan Andrén <johan@markatta.com>
@johanandren do you have a clue on what might be missing to make CI happy? Had a look at the failing pipelines but not obvious for me. |
One thing is a test failure:
the other is a check that any api annotated with |
Green at last. Thanks Johan for the tips. See what you think about c65785d .. otherwise I think this is good to go. |
With this PR, we want to:
Opening as an early version for feedback.