feat: add pingntlm provider support for pre-authentication issue #794

Gearheads · 2022-03-23T19:48:55Z

This PR will add support for the pingntlm provider which is needed for pre-authentication issues.

Fixes #244

Thank you to @richardcase for the solution, and help.

I didn't see a CONTRIBUTING.md, so please let me know if there is anything that needs to be changed.
make mod and make test both completed successfully.

Gearheads · 2022-04-21T03:22:30Z

hi @wolfeidau, I am new here, so apologies in advance if I am mistaken, but would you or someone else be able to review this?

Gearheads · 2022-06-17T21:21:37Z

hi @wolfeidau, apologies to bother you, just checking in to see if you or someone else would be able to review this?

wolfeidau · 2022-09-12T00:12:00Z

@Gearheads hey is there a reason this needs to be a new provider entirely?

I would love some background on the configuration and setup for Ping.

Gearheads · 2022-09-13T18:09:44Z

@Gearheads hey is there a reason this needs to be a new provider entirely?

I would love some background on the configuration and setup for Ping.

hi @wolfeidau, apologies, unfortunately I do not have the full context for this change. 😞

@richardcase, would you be able to provide more background?

richardcase · 2022-09-16T09:25:14Z

@Gearheads @wolfeidau - its such a long time ago since i made this change...details are a bit fuzzy. At a high level the reason is that the existing ping provider doesn't support NTLM. At the time it felt cleaner and quicker to create a separate provider.

Gearheads · 2023-10-14T02:25:39Z

Hi @wolfeidau, apologies to bother, do you think this PR will be helpful, or do you think there is a better solution to support this type of provider?

mapkon · 2024-01-05T09:23:22Z

@Gearheads This is failing the build. Can you take a look?

codecov-commenter · 2024-01-05T09:24:39Z

Codecov Report

Attention: 174 lines in your changes are missing coverage. Please review.

Comparison is base (6047196) 39.28% compared to head (1506cb7) 38.74%.
Report is 34 commits behind head on master.

Files	Patch %	Lines
pkg/provider/pingntlm/pingntlm.go	18.75%	164 Missing and 5 partials ⚠️
saml2aws.go	0.00%	5 Missing ⚠️

❗ Your organization needs to install the Codecov GitHub app to enable full functionality.

Additional details and impacted files

@@            Coverage Diff             @@
##           master     #794      +/-   ##
==========================================
- Coverage   39.28%   38.74%   -0.55%     
==========================================
  Files          53       54       +1     
  Lines        8025     8238     +213     
==========================================
+ Hits         3153     3192      +39     
- Misses       4449     4618     +169     
- Partials      423      428       +5

Flag	Coverage Δ
unittests	`38.74% <18.30%> (-0.55%)`	⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Gearheads · 2024-01-09T14:06:04Z

Yes, I will take look.

Gearheads · 2024-01-11T22:02:16Z

Hi @mapkon, I have updated the PR with more unit tests. Is there a way for me to re-run the Codecov Report?

mapkon · 2024-01-12T00:45:50Z

Hi @mapkon, I have updated the PR with more unit tests. Is there a way for me to re-run the Codecov Report?

Done! I will review this today

mapkon

@Gearheads Can you please take a look at the lint issues.

Gearheads · 2024-01-12T04:39:36Z

@Gearheads Can you please take a look at the lint issues.

Hi @mapkon, yes of course. I have ran the lint command locally, and it should be fixed now.

Signed-off-by: Casale, Robert <[email protected]>

mapkon · 2024-01-12T12:32:05Z

@Gearheads Looks like the tests are still complaining

Gearheads · 2024-01-12T17:36:52Z

@Gearheads Looks like the tests are still complaining

Hi @mapkon, strange. It appears that the issue is with a different provide:

=== RUN   TestVerifyEndpointHealth
    okta_test.go:821: 
        	Error Trace:	/Users/runner/work/saml2aws/saml2aws/pkg/provider/okta/okta_test.go:821
        	            				/Users/runner/work/saml2aws/saml2aws/pkg/provider/okta/okta_test.go:728
        	            				/Users/runner/hostedtoolcache/go/1.21.5/x64/src/net/http/server.go:2938
        	            				/Users/runner/hostedtoolcache/go/1.21.5/x64/src/net/http/server.go:2009
        	            				/Users/runner/hostedtoolcache/go/1.21.5/x64/src/runtime/asm_amd64.s:1650
        	Error:      	Not equal: 
        	            	expected: "/report"
        	            	actual  : "/frame/check_endpoint_app_status"

I tried running make test locally, and I did not get the same error message:

make test
--- test all the things
?       github.com/versent/saml2aws/v2/cmd/saml2aws     [no test files]
ok      github.com/versent/saml2aws/v2  (cached)        coverage: 48.6% of statements
?       github.com/versent/saml2aws/v2/helper/credentials       [no test files]
?       github.com/versent/saml2aws/v2/mocks    [no test files]
?       github.com/versent/saml2aws/v2/pkg/creds        [no test files]
?       github.com/versent/saml2aws/v2/pkg/dump [no test files]
ok      github.com/versent/saml2aws/v2/cmd/saml2aws/commands    (cached)        coverage: 14.7% of statements
ok      github.com/versent/saml2aws/v2/helper/osxkeychain       (cached)        coverage: 75.4% of statements
ok      github.com/versent/saml2aws/v2/pkg/awsconfig    (cached)        coverage: 43.0% of statements
ok      github.com/versent/saml2aws/v2/pkg/cfg  (cached)        coverage: 60.9% of statements
ok      github.com/versent/saml2aws/v2/pkg/cookiejar    (cached)        coverage: 93.8% of statements
ok      github.com/versent/saml2aws/v2/pkg/flags        (cached)        coverage: 66.7% of statements
ok      github.com/versent/saml2aws/v2/pkg/page (cached)        coverage: 50.0% of statements
ok      github.com/versent/saml2aws/v2/pkg/prompter     (cached)        coverage: 36.5% of statements
?       github.com/versent/saml2aws/v2/pkg/provider/adfs        [no test files]
?       github.com/versent/saml2aws/v2/pkg/provider/akamai      [no test files]
ok      github.com/versent/saml2aws/v2/pkg/provider     (cached)        coverage: 49.3% of statements
ok      github.com/versent/saml2aws/v2/pkg/provider/aad (cached)        coverage: 77.1% of statements
ok      github.com/versent/saml2aws/v2/pkg/provider/adfs2       (cached)        coverage: 56.2% of statements
ok      github.com/versent/saml2aws/v2/pkg/provider/auth0       (cached)        coverage: 68.7% of statements
ok      github.com/versent/saml2aws/v2/pkg/provider/authentik   (cached)        coverage: 77.8% of statements
ok      github.com/versent/saml2aws/v2/pkg/provider/browser     (cached)        coverage: 58.8% of statements
ok      github.com/versent/saml2aws/v2/pkg/provider/f5apm       (cached)        coverage: 42.5% of statements
ok      github.com/versent/saml2aws/v2/pkg/provider/googleapps  (cached)        coverage: 34.3% of statements
ok      github.com/versent/saml2aws/v2/pkg/provider/jumpcloud   (cached)        coverage: 17.7% of statements
ok      github.com/versent/saml2aws/v2/pkg/provider/keycloak    (cached)        coverage: 47.7% of statements
?       github.com/versent/saml2aws/v2/pkg/provider/onelogin/mock       [no test files]
ok      github.com/versent/saml2aws/v2/pkg/provider/netiq       (cached)        coverage: 49.0% of statements
ok      github.com/versent/saml2aws/v2/pkg/provider/okta        (cached)        coverage: 45.7% of statements
ok      github.com/versent/saml2aws/v2/pkg/provider/onelogin    (cached)        coverage: 54.4% of statements
ok      github.com/versent/saml2aws/v2/pkg/provider/pingfed     (cached)        coverage: 39.9% of statements
ok      github.com/versent/saml2aws/v2/pkg/provider/pingntlm    (cached)        coverage: 60.3% of statements
?       github.com/versent/saml2aws/v2/pkg/provider/shell       [no test files]
?       github.com/versent/saml2aws/v2/pkg/provider/shibboleth  [no test files]
ok      github.com/versent/saml2aws/v2/pkg/provider/pingone     (cached)        coverage: 10.2% of statements
ok      github.com/versent/saml2aws/v2/pkg/provider/shibbolethecp       (cached)        coverage: 38.6% of statements
ok      github.com/versent/saml2aws/v2/pkg/samlcache    (cached)        coverage: 67.1% of statements
ok      github.com/versent/saml2aws/v2/pkg/shell        (cached)        coverage: 100.0% of statements

I am wondering if maybe the order of these handlers is important?
https://github.com/Versent/saml2aws/blob/master/pkg/provider/okta/okta_test.go#L812

Any ideas as to what I can check out, or if it is worth re-running the GitHub action to see if it was a one-time issue?

mapkon · 2024-01-13T10:49:23Z

@Gearheads I re-ran the github actions and got a similar result to the the one above.

mapkon · 2024-01-13T12:32:35Z

@Gearheads I re-ran the github actions and got a similar result to the the one above.

I just re-ran it and it passed. Something weird is going on.

Gearheads force-pushed the feat/244-add-ntlm-support branch from 3d2ca91 to b7ff3ea Compare May 9, 2022 13:39

Gearheads mentioned this pull request Jun 15, 2023

Resolves #349: Add Browser IDP & upgrade to saml2aws v2.32.0 fidelity/kconnect#350

Closed

Gearheads force-pushed the feat/244-add-ntlm-support branch from 8e10f5c to 2dcc346 Compare October 14, 2023 02:23

Gearheads force-pushed the feat/244-add-ntlm-support branch 2 times, most recently from 4a89e20 to 7c3258c Compare November 21, 2023 06:06

Gearheads force-pushed the feat/244-add-ntlm-support branch from 7c3258c to 1506cb7 Compare December 14, 2023 07:58

Gearheads force-pushed the feat/244-add-ntlm-support branch from 1506cb7 to aa183ef Compare January 11, 2024 02:18

mapkon suggested changes Jan 12, 2024

View reviewed changes

Gearheads added 4 commits January 11, 2024 23:39

feat: add pingntlm provider support for pre-authentication issue

4767386

feat: fix spelling typo for example directory

6be3346

feat: add unit tests, update deprecated packages

6fb8467

Signed-off-by: Casale, Robert <[email protected]>

feat: fix the lint issue

e06b933

Signed-off-by: Casale, Robert <[email protected]>

Gearheads force-pushed the feat/244-add-ntlm-support branch from 04fb8c7 to e06b933 Compare January 12, 2024 04:39

Gearheads requested a review from mapkon January 12, 2024 05:09

mapkon approved these changes Jan 13, 2024

View reviewed changes

mapkon merged commit 8d3843c into Versent:master Jan 14, 2024
8 checks passed

0rax mentioned this pull request Mar 27, 2024

fix: Browser IDP & Browser MFA support using saml2aws/v2 fidelity/kconnect#650

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: add pingntlm provider support for pre-authentication issue #794

feat: add pingntlm provider support for pre-authentication issue #794

Gearheads commented Mar 23, 2022 •

edited

Loading

Gearheads commented Apr 21, 2022

Gearheads commented Jun 17, 2022 •

edited

Loading

wolfeidau commented Sep 12, 2022

Gearheads commented Sep 13, 2022

richardcase commented Sep 16, 2022

Gearheads commented Oct 14, 2023

mapkon commented Jan 5, 2024

codecov-commenter commented Jan 5, 2024

Gearheads commented Jan 9, 2024

Gearheads commented Jan 11, 2024

mapkon commented Jan 12, 2024

mapkon left a comment

Gearheads commented Jan 12, 2024

mapkon commented Jan 12, 2024

Gearheads commented Jan 12, 2024

mapkon commented Jan 13, 2024

mapkon commented Jan 13, 2024

feat: add pingntlm provider support for pre-authentication issue #794

feat: add pingntlm provider support for pre-authentication issue #794

Conversation

Gearheads commented Mar 23, 2022 • edited Loading

Gearheads commented Apr 21, 2022

Gearheads commented Jun 17, 2022 • edited Loading

wolfeidau commented Sep 12, 2022

Gearheads commented Sep 13, 2022

richardcase commented Sep 16, 2022

Gearheads commented Oct 14, 2023

mapkon commented Jan 5, 2024

codecov-commenter commented Jan 5, 2024

Codecov Report

Gearheads commented Jan 9, 2024

Gearheads commented Jan 11, 2024

mapkon commented Jan 12, 2024

mapkon left a comment

Choose a reason for hiding this comment

Gearheads commented Jan 12, 2024

mapkon commented Jan 12, 2024

Gearheads commented Jan 12, 2024

mapkon commented Jan 13, 2024

mapkon commented Jan 13, 2024

Gearheads commented Mar 23, 2022 •

edited

Loading

Gearheads commented Jun 17, 2022 •

edited

Loading