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(authentication): make cookie name unique between environments #2095
fix(authentication): make cookie name unique between environments #2095
Conversation
…-prod-domain-is-used-on-all-other-domains
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.
Looks good, but formatting test failed.
// TODO: add some default images: https://www.testcontainers.org/features/files/ | ||
val incunabulaImageDirPath = Paths.get("..", "sipi/images/0803/incunabula_0000000002.jp2") | ||
sipiContainer.withFileSystemBind(incunabulaImageDirPath.toString(), "/sipi/images/0803/incunabula_0000000002.jp2", BindMode.READ_ONLY) |
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.
This could be val sipi/images/0803/incunabula_0000000002.jp2
.
Also is above TODO fulfilled with these 2 lines?
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.
removed the comment. the val cannot be simplified, because the value is not the same then.
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, just added minor remarks
docker-compose.yml
Outdated
command: --config=/sipi/config/sipi.docker-test-config.lua ## command variant to start the sipi container with test routes enabled | ||
# command: --config=/sipi/config/sipi.docker-config.lua |
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.
Do we still need the commented out command? As far as I understand, for tests we need the sipi.docker-test-config.lua
. On test/staging/prod/project servers there is another lua config anyway, right?
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.
good catch. no, it is only needed for some manual testing. I reverted the change
|
||
-- | ||
-- maxcimal size of the cache | ||
-- maximal size of the cache | ||
-- | ||
cachesize = '100M', | ||
|
||
-- | ||
-- if the cache becomes full, the given percentage of file space is marked for reuase |
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.
-- if the cache becomes full, the given percentage of file space is marked for reuase | |
-- if the cache becomes full, the given percentage of file space is marked for reuse |
sipi/scripts/sipi.init.lua
Outdated
print("cookie key is invalid: " .. cookie) | ||
server.log("cookie key is invalid: " .. cookie, server.loglevel.LOG_ERR) |
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.
Do we need both?
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.
nope, removed the print
Kudos, SonarCloud Quality Gate passed!
|
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 overall, maybe we can get rid of some of the "Knora"s?
@@ -78,7 +78,6 @@ function pre_flight(prefix, identifier, cookie) | |||
|
|||
-- print("knora_url: " .. knora_url) |
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.
can be removed?
val config: Config = ConfigFactory.parseString(""" | ||
|akka.loglevel = "DEBUG" | ||
|akka.stdout-loglevel = "DEBUG" | ||
""".stripMargin) |
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.
this indentation looks wrong to me... have you auto-formatted?
assert(response.status === StatusCodes.OK) | ||
} | ||
|
||
"accept a token in Sipi that has been signed by Knora" in { |
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.
"accept a token in Sipi that has been signed by Knora" in { | |
"accept a token in Sipi that has been signed by DSP-API" in { |
assert(sipiResponse.status == StatusCodes.OK) | ||
} | ||
|
||
"not accept a token in Sipi that hasn't been signed by Knora" in { |
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.
"not accept a token in Sipi that hasn't been signed by Knora" in { | |
"not accept a token in Sipi that hasn't been signed by DSP-API" in { |
Knora is dead, long live Knora!
I would prefer not to delay this PR because of Knora's. |
Resolves DEV-994
PR Checklist
Please check if your PR fulfills the following requirements:
PR Type
What kind of change does this PR introduce?
What is the current behavior?
Issue Number: N/A
What is the new behavior?
Does this PR introduce a breaking change?
Other information