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

Dev debuglog #4

Open
wants to merge 7 commits into
base: master
Choose a base branch
from
Open

Dev debuglog #4

wants to merge 7 commits into from

Conversation

chakl
Copy link
Collaborator

@chakl chakl commented Sep 7, 2021

various improvements:

  • make log level configurable from config-exsaml.xml
  • do not store SAML request ids unless needed
  • remove console:log()
  • doc clarifications

@chakl chakl requested a review from line-o September 7, 2021 23:18
@chakl chakl self-assigned this Sep 7, 2021
content/exsaml.xqm Outdated Show resolved Hide resolved
content/exsaml.xqm Outdated Show resolved Hide resolved
Copy link
Member

@line-o line-o left a comment

Choose a reason for hiding this comment

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

I believe the custom logging logic introduced with $exsaml:logdebug is for convenience, right?
If this was set to "debug" level by default one would have to change the global setting of an existdb instance and then restart and with this feature it can be switched on and off while the db is running.
But you could argue that changing the level of these log entries should require a restart because it would log sensitive data. What is your though on this?

content/exsaml.xqm Outdated Show resolved Hide resolved
@@ -123,30 +130,34 @@ declare %private function exsaml:build-saml-authnreq() {
};

declare %private function exsaml:store-authnreqid-as-exsol-user($id as xs:string, $instant as xs:string) {
let $saml-coll-reqid := $exsaml:saml-coll-reqid-base || "/" || $exsaml:saml-coll-reqid-name
Copy link
Member

Choose a reason for hiding this comment

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

Since this value is rather static I would argue it is better to have it built once as declared variable of the module.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

That would mean

declare %private variable $exsaml:saml-coll-reqid-base := "/db/apps/existdb-saml";
declare %private variable $exsaml:saml-coll-reqid := $exsaml:saml-coll-reqid-base || "/" || "saml-request-ids";

and that looks dirty to me.

Copy link
Member

Choose a reason for hiding this comment

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

I was thinking more like:

declare %private variable $exsaml:saml-coll-reqid-base := "/db/apps/existdb-saml";
declare %private variable $exsaml:saml-coll-reqid-name := "saml-request-ids";
declare %private variable $exsaml:saml-coll-reqid := $exsaml:saml-coll-reqid-base || "/" || $exsaml:saml-coll-reqid-name;

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I was arguing that declareing a variable value derived from a previously declared variable might be wrong. Does the Xquery standard specify that declarations get processed sequentially? Don't think so, but I may be wrong.

content/exsaml.xqm Show resolved Hide resolved
content/exsaml.xqm Show resolved Hide resolved
content/exsaml.xqm Show resolved Hide resolved
@chakl
Copy link
Collaborator Author

chakl commented Sep 8, 2021

I believe the custom logging logic introduced with $exsaml:logdebug is for convenience, right?

Actually, it was introduced to stop people editing the module code, just because they want to see a certain log message.
They can now enable "debug mode" and see all log messages from this module, including more detailed debug log messages that are not logged in normal operation.
Debug mode is for debugging the module's operation. It should not be enabled in normal setups.

This is controlled by the config-exsaml.xml:/config/@debug attribute. false (default) means standard logging, true means verbose debug logging.

But you could argue that changing the level of these log entries should require a restart because it would log sensitive data. What is your though on this?

  • the module must not log sensitive persistent data (passwords, secrets). It may log sensitive volatile data (SAML assertions) when in debug mode
  • debug mode is for developers and admins, who can access sensitive data anyway

@joewiz
Copy link
Member

joewiz commented Apr 12, 2023

@chakl @line-o Is this PR good to go?

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.

3 participants