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

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

Merged

Conversation

Gearheads
Copy link
Contributor

@Gearheads Gearheads commented Mar 23, 2022

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
Copy link
Contributor Author

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 Gearheads force-pushed the feat/244-add-ntlm-support branch from 3d2ca91 to b7ff3ea Compare May 9, 2022 13:39
@Gearheads
Copy link
Contributor Author

Gearheads commented Jun 17, 2022

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

@wolfeidau
Copy link
Contributor

@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
Copy link
Contributor Author

@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
Copy link

@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
Copy link
Contributor Author

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?

@Gearheads 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 Gearheads force-pushed the feat/244-add-ntlm-support branch from 7c3258c to 1506cb7 Compare December 14, 2023 07:58
@mapkon
Copy link
Contributor

mapkon commented Jan 5, 2024

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

@codecov-commenter
Copy link

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
Copy link
Contributor Author

Yes, I will take look.

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

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

@mapkon
Copy link
Contributor

mapkon commented Jan 12, 2024

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

Copy link
Contributor

@mapkon mapkon left a comment

Choose a reason for hiding this comment

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

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

@Gearheads
Copy link
Contributor Author

@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.

@Gearheads Gearheads force-pushed the feat/244-add-ntlm-support branch from 04fb8c7 to e06b933 Compare January 12, 2024 04:39
@Gearheads Gearheads requested a review from mapkon January 12, 2024 05:09
@mapkon
Copy link
Contributor

mapkon commented Jan 12, 2024

@Gearheads Looks like the tests are still complaining

@Gearheads
Copy link
Contributor Author

@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
Copy link
Contributor

mapkon commented Jan 13, 2024

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

@mapkon
Copy link
Contributor

mapkon commented Jan 13, 2024

@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.

@mapkon mapkon merged commit 8d3843c into Versent:master Jan 14, 2024
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.

support of pre-authentication (basic,ntlm,ff)
5 participants