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

[uss_qualifier] new resource for cleanly exposing the client identity #362

Merged

Conversation

Shastick
Copy link
Contributor

@Shastick Shastick commented Nov 22, 2023

This is the proper fix for #306:

The client identity (or more colloquially the sub field of its JWT) is now exposed to any resource or scenario that needs it via a ClientIdentityResource, which now holds the whoami_* audience and scope definitions that were formerly passed to the IDGeneratorResource.

The IDGeneratorResource now depends on the new resource to obtain the client_identity.

It should be noted that the whoami_* audience and scope will be used to request a token only if no other token had yet been requested by the auth_adapter:

The IDGeneratorResource will wait until subscriber() is called for the first time before checking if get_sub() returns something.

@Shastick Shastick force-pushed the clean-sub-determination-fix branch 2 times, most recently from 08d0b1b to 8653a50 Compare November 22, 2023 11:10
@Shastick Shastick marked this pull request as ready for review November 22, 2023 11:36
@Shastick
Copy link
Contributor Author

Shastick commented Nov 22, 2023

edit indeed, a problem in the config caused the DSS tests to be skipped and this got caught by the aggregate check. This is ready for review.

This would be ready for review: It is unclear to me what causes the failure. Could it be due to a subtle change in the configuration files?

The logs show this:

 2023-11-22 11:30:52.233 | ERROR    | monitoring.uss_qualifier.reports.validation.report_validation:validate_report:306 - Validation criterion 2 failed to validate.  Criterion definition:
applicability:
  skipped_actions: {}
pass_condition:
  elements:
    count:
      equal_to: 0.0

But both the F3411-19 and F3411-22a test suites indicate a SUCCESS so I'm confused.

edit2: Now running into this warning:

2023-11-22 11:27:44.056 | WARNING  | monitoring.uss_qualifier.suites.suite:__init__:259 - Skipping action 1 (test_suite suites.astm.utm.f3548_21) because Test suite "ASTM F3548-21" is missing resource invalid_flight_intents (resources.flight_planning.FlightIntentsResource)

edit3: a resource was indeed missing.

@Shastick Shastick force-pushed the clean-sub-determination-fix branch from 2119cb1 to 5896ee2 Compare November 27, 2023 09:57
This is a function and not a field, to possibly profit from a token that would have been requested earlier
"""

sub = self._adapter.get_sub()
Copy link
Member

Choose a reason for hiding this comment

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

Most AuthAdapter subclasses do not have this method, so this line will fail when using most types of AuthAdapter

Copy link
Contributor Author

@Shastick Shastick Nov 29, 2023

Choose a reason for hiding this comment

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

get_sub seems to be defined on the AuthAdapter parent class:

it returns a None if no token with a sub can be found, and the constructor properly initializes self._tokens to an empty Dict so I assume this will at worst always return a None?

That would indeed seem to be a problem in situations where the adapters implement issue_token() (which raises a NotImplementedError in the superclass) and then do not update the adapter's token map for some reason: in such a case get_sub would return a None again after issue_token() has been called below.

(although, possibly for such situations the sub field value would be known and get_sub() could be overridden, such as has been done in DummyOAuth in this PR? This way we would not even call issue_token())

Now, if some implementations would effectively fail on get_sub, can I assume issue_token will always be available?

Copy link
Member

Choose a reason for hiding this comment

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

Oops, entirely my mistake; my original comment here was entirely wrong. Sorry about that!

The thing that was throwing me off was the new get_sub on just one subclass, but of course this makes sense as there is a particularly easy/better way to implement that pre-existing base-class method on that specific subclass. In any case, entirely my bad.

@BenjaminPelletier BenjaminPelletier merged commit e5b983d into interuss:main Nov 29, 2023
8 checks passed
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