Skip to content

Implements the Authentication Configuration Checks #18377

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 4 commits into from
Mar 25, 2025

Conversation

mereghost
Copy link
Contributor

@mereghost mereghost commented Mar 19, 2025

Related Work Package: OP#62100

Implements the OIDC connection tests and some elementary OAuth2 tests.

@mereghost mereghost self-assigned this Mar 19, 2025
@mereghost mereghost requested a review from Kharonus March 20, 2025 14:35
@mereghost mereghost force-pushed the impl/62100-authentication-checks branch from e8ffa8a to 1a23138 Compare March 20, 2025 14:56
Copy link
Contributor

@NobodysNightmare NobodysNightmare left a comment

Choose a reason for hiding this comment

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

In addition to the 🔴 that I cleverly posted immediately, I added all other traffic light colors to round it off.

In general I like what those validators might look like soonish.

def validate = raise Errors::SubclassResponsibility

def register_checks(*keys)
@results = keys.to_h { [it, CheckResult.skipped(it)] }
Copy link
Contributor

Choose a reason for hiding this comment

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

🟢 The current implementation of this implies that I can only call register_checks once, never twice.

I think that's enough, but it limits the ways in which I can write my validator a bit. E.g. this layout would not work:

def validate
  register_checks(:common_check)
  if rand < 0.5
    register_checks(*a_checks)
    validate_a
  else
    register_checks(*b_checks)
    validate_b
  end
end

The other issue is that if I for whatever reason don't add any tests, we'd now return nil instead of returning {}. As indicated by the green traffic light, I think both of these problems are absolutely minor, but I wanted to explicitly write them down at least.

@mereghost mereghost merged commit 1f57cdc into dev Mar 25, 2025
14 checks passed
@mereghost mereghost deleted the impl/62100-authentication-checks branch March 25, 2025 09:04
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

3 participants