8000 Add a regex option for issuer validation by MrYawe · Pull Request #435 · erlef/oidcc · GitHub
[go: up one dir, main page]
More Web Proxy on the site http://driver.im/
Skip to content

Add a regex option for issuer validation #435

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 7 commits into from
Apr 2, 2025
Merged

Add a regex option for issuer validation #435

merged 7 commits into from
Apr 2, 2025

Conversation

MrYawe
Copy link
Contributor
@MrYawe MrYawe commented Apr 1, 2025

Fixes #401

maennchen
maennchen reviewed Apr 1, 2025
View reviewed changes
Copy link
Member
@maennchen maennchen left a comment

Choose a reason for hiding this comment

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

Thanks for your PR ❤️

I left a few specific comments.

We should also update the code here so that it uses the regex to validate the issuer of the configuration:

oidcc/src/oidcc_provider_configuration.erl

Lines 232 to 237 in 03d67d2

case ConfigIssuer of
Issuer ->
{ok, {Configuration, Expiry}};
_DifferentIssuer when AllowIssuerMismatch -> {ok, {Configuration, Expiry}};
DifferentIssuer when not AllowIssuerMismatch ->
{error, {issuer_mismatch, DifferentIssuer}}

@maennchen
Copy link
Member

@MrYawe Thanks for the change ❤️ It looks fine, but there's some issue around formatting. Please have a look at the CI output.

@maennchen
Copy link
Member

@MrYawe There seems to be an issue with the docs before OTP 27. Specifically the $ in the regex. Let's just remove it since it doesn't add much documentation-wise.

@maennchen
Copy link
Member

(src/oidcc_token.erl line 968)

@maennchen maennchen self-assigned this Apr 2, 2025
@maennchen
Copy link
Member

@MrYawe Sorry, I meant to just remove the $ itself, not the whole doc. The example is great otherwise :)

@maennchen
Copy link
Member

One last thing and we're ready to go:

Run rebar3 lint
===> Verifying dependencies...
===> Analyzing applications...
===> Compiling oidcc
===> elvis analysis starting, this may take a while...
# src/oidcc_provider_configuration.erl [FAIL]
  - no_trailing_whitespace (https://github.com/inaka/elvis_core/tree/main/doc_rules/elvis_text_style/no_trailing_whitespace.md)
    - Line 46 has 1 trailing whitespace characters.
    - Line 48 has 1 trailing whitespace characters.
# src/oidcc_token.erl [FAIL]
  - no_trailing_whitespace (https://github.com/inaka/elvis_core/tree/main/doc_rules/elvis_text_style/no_trailing_whitespace.md)
    - Line 870 has 1 trailing whitespace characters.
    - Line [9](https://github.com/erlef/oidcc/actions/runs/14224655806/job/39861120407?pr=435#step:7:10)61 has 1 trailing whitespace characters.
    - Line 965 has 1 trailing whitespace characters.
===> Linting failed

@maennchen maennchen enabled auto-merge (squash) April 2, 2025 16:29
@maennchen maennchen disabled auto-merge April 2, 2025 16:32
@maennchen maennchen merged commit 95f0f9d into erlef:main Apr 2, 2025
25 of 26 checks passed
@MrYawe
Copy link
Contributor Author
MrYawe commented Apr 2, 2025

@maennchen Thanks for the advice and the review!

@maennchen
Copy link
Member

@MrYawe Thanks a lot for the contribution. Released as v3.4.0

@coveralls
Copy link

Pull Request Test Coverage Report for Build 274

Warning: This coverage report may be inaccurate.

This pull request's base commit is no longer the HEAD commit of its target branch. This means it includes changes from outside the original pull request, including, potentially, unrelated coverage changes.

Details

  • 13 of 18 (72.22%) changed or added relevant lines in 3 files are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage decreased (-0.3%) to 91.504%
Changes Missing Coverage Covered Lines Changed/Added Lines %
src/oidcc_jwt_util.erl 7 9 77.78%
src/oidcc_provider_configuration.erl 2 5 40.0%
Totals Coverage Status
Change from base Build 242: -0.3%
Covered Lines: 1077
Relevant Lines: 1177
💛 - Coveralls

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@maennchen maennchen maennchen approved these changes

Assignees

@maennchen maennchen

Labels
enhancement
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Quirk for Dynamic (Tenant) based issuer of Microsoft Entra ID
3 participants
@MrYawe @maennchen @coveralls
0