Skip to content

Add two log injection applications with custom listeners #116

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

Merged
merged 19 commits into from
May 23, 2024

Conversation

jeongsoolee09
Copy link
Contributor

@jeongsoolee09 jeongsoolee09 commented May 22, 2024

What this PR contributes

This PR adds two examples of log injection in happening in a UI5 application:

Example 1: Log entries consumed by a custom log listener

sap.base.Log, a collection of static logging functions, provides an extension point for a developer to add arbitrary code that handles a log entry as it is created. The developer has to define an object that implements a interface method sap.base.LogListener.onLogEntry that is called if a log entry is created.

The example provides a custom log listener under webapp/utils/CustomLogListener.js whose onLogEntry sends the log entry via HTTP.

Example 2: Log entries consumed by sap.ui.vk.Notifications

sap.ui.vk.Notifications is a control provided in the namespace sap.ui.vk. The control implements Log.onLogEntry() and registers itself to Log using Log.addLogListener internally. Since it is a control provided by SAP themselves, it would be a good idea to include this as a separate example.

The example declares a use of sap.ui.vk.Notifications in its view/app.view.xml which automatically picks up log entries and displays it on screen as a notification entry.

Future work

If SAP requests for it, refine the existing log injection query to take into consideration the additional elements living in both examples.

1. Update usage of `sourceModel`, `sinkModel`, and `summaryModel`
2. Update usage of `XmlAttribute`, `XmlFile`
@jeongsoolee09 jeongsoolee09 requested a review from mbaluda May 22, 2024 00:44
Copy link
Contributor

@mbaluda mbaluda left a comment

Choose a reason for hiding this comment

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

@mbaluda
Copy link
Contributor

mbaluda commented May 22, 2024

@jeongsoolee09 see PR #119:

  • Extract CodeQL version from qlt.conf.json to be used in code_scanning workflow
  • Update CodeQL version to 2.17.3
  • Update qlpack files

@jeongsoolee09 jeongsoolee09 marked this pull request as ready for review May 22, 2024 22:04
@jeongsoolee09 jeongsoolee09 enabled auto-merge May 22, 2024 22:04
@jeongsoolee09 jeongsoolee09 requested a review from mbaluda May 22, 2024 22:05
Copy link
Contributor

@mbaluda mbaluda left a comment

Choose a reason for hiding this comment

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

It's good to go!

@@ -0,0 +1,3 @@
nodes
Copy link
Contributor

Choose a reason for hiding this comment

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

Empty expected file

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It's expected to be empty, since we don't yet have a query to update the .expected files.

Copy link
Contributor

Choose a reason for hiding this comment

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

I think in this case it's better to remove the file so that the unit test will be skipped

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That's better. I'll reopen the PR and remove the two .expected files.

Copy link
Contributor Author

@jeongsoolee09 jeongsoolee09 May 28, 2024

Choose a reason for hiding this comment

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

Instead of reopening the PR (I thought it was possible), I opened a new one that only removes the two files: #121.

@@ -0,0 +1,3 @@
nodes
Copy link
Contributor

Choose a reason for hiding this comment

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

Empty expected file

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It's expected to be empty, since we don't yet have a query to update the .expected files.

@jeongsoolee09 jeongsoolee09 merged commit ff8f4a5 into main May 23, 2024
5 checks passed
@jeongsoolee09 jeongsoolee09 deleted the jeongsoolee09/log-injection-fortified branch May 23, 2024 08:46
@jeongsoolee09 jeongsoolee09 restored the jeongsoolee09/log-injection-fortified branch May 28, 2024 19:46
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.

2 participants