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

Go/Java/C#: Rename ThreatModelFlowSource to ActiveThreatModelSource #17424

Merged
merged 5 commits into from
Sep 26, 2024

Conversation

RasmusWL
Copy link
Member

@RasmusWL RasmusWL commented Sep 10, 2024

Go/Java/C#: Rename to ActiveThreatModelSource

Context for wanting this change

As part of adding support for threat-models to Python/JS (see #17203), we ran into some trouble with name clashes.

Naming in existing languages supporting threat-models:

  • SourceNode (for QL only modeling)
  • ThreatModelFlowSource (for active sources from QL or data-extensions)

However, since we use LocalSourceNode in Python, and SourceNode in JS (for local source nodes), it seems a bit confusing to follow the same naming convention as other languages, and we had to come up with new names.

Initially I used ThreatModelSource for the "QL only modeling", but that meant that we needed a new name to represent the active sources coming from either QL or data-extensions... for this I came up with ActiveThreatModelSource, and I really liked it. To me, it's much clearer that this class only contains the currently active threat model sources.

So to align languages, I got approval from @michaelnebel to rename the existing classes.

As part of adding support for threat-models to Python/JS (see
github#17203), we ran into some trouble
with name clashes.

Naming in existing languages supporting threat-models:
- `SourceNode` (for QL only modeling)
- `ThreatModelFlowSource` (for active sources from QL or data-extensions)

However, since we use `LocalSourceNode` in Python, and `SourceNode` in
JS (for local source nodes), it seems a bit confusing to follow the same
naming convention as other languages, and we had to come up with new names.

Initially I used `ThreatModelSource` for the "QL only modeling", but
that meant that we needed a new name to represent the active sources
coming from either QL or data-extensions... for this I came up with
`ActiveThreatModelSource`, and I really liked it. To me, it's much
clearer that this class only contains the currently active threat
model sources.

So to align languages, I got approval from @michaelnebel to rename the
existing classes.
Copy link
Contributor

@michaelnebel michaelnebel left a comment

Choose a reason for hiding this comment

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

Uh, thank very much for doing this @RasmusWL !

Copy link
Contributor

@egregius313 egregius313 left a comment

Choose a reason for hiding this comment

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

Looks good, but there's a formatting error

go/ql/src/experimental/CWE-090/LDAPInjection.qll would change by autoformatting.

Copy link
Contributor

@michaelnebel michaelnebel left a comment

Choose a reason for hiding this comment

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

C# LGTM!

@RasmusWL RasmusWL marked this pull request as ready for review September 16, 2024 08:45
@RasmusWL RasmusWL requested review from a team as code owners September 16, 2024 08:45
Copy link
Contributor

@aschackmull aschackmull left a comment

Choose a reason for hiding this comment

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

Java LGTM

Copy link
Contributor

@owen-mc owen-mc left a comment

Choose a reason for hiding this comment

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

Go LGTM

@RasmusWL RasmusWL merged commit 381ea93 into github:main Sep 26, 2024
59 checks passed
@RasmusWL RasmusWL deleted the active-threat-model-source branch September 26, 2024 11:08
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants