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

Azure Active Directory authentication #548

Closed
wants to merge 8 commits into from

Conversation

paulmey
Copy link
Contributor

@paulmey paulmey commented Jan 22, 2020

Merging #547 and #546 to fully fix #446

@codecov
Copy link

codecov bot commented Jan 23, 2020

Codecov Report

Merging #548 into master will decrease coverage by 2%.
The diff coverage is 47.69%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #548      +/-   ##
==========================================
- Coverage    68.9%   66.89%   -2.01%     
==========================================
  Files          22       24       +2     
  Lines        5068     5450     +382     
==========================================
+ Hits         3492     3646     +154     
- Misses       1370     1582     +212     
- Partials      206      222      +16
Impacted Files Coverage Δ
fedauth.go 0% <0%> (ø)
log_conn.go 0% <0%> (ø)
conn_str.go 100% <100%> (ø) ⬆️
tds.go 55.94% <50.23%> (-4.4%) ⬇️
token.go 58.59% <76.82%> (+2.67%) ⬆️
accesstokenconnector.go 80% <80%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 1d7a30a...ec77a0c. Read the comment docs.

@kardianos
Copy link
Collaborator

@paulmey
What's the current status on this PR? I see it merges two other PRs, one from you and another from @wrosenuance . I'm assuming it isn't ready yet for review due to draft status, correct?

Lastly, how would you propose we test (CI) this code so it doesn't break? SQL Server (hosted) has a free dev license, but this looks like Azure specific?

@kardianos
Copy link
Collaborator

Okay, I just read through the Issue history. I think I understand the situation now.

I assume we will keep this PR a s a draft and NOT merge it, but rather merge each contribution by itself.

@wrosenuance
Copy link
Contributor

wrosenuance commented Jan 30, 2020

@kardianos I was not sure which way to go for CI. I don't know of services that would allow for running actual tests against an Azure AD authenticated service, let alone run with access to the managed service identity endpoint used for some of the scenarios.

I see that @paulmey has added some tests at a protocol level, which is one avenue, the other might be to try to bring in some sort of mocking framework to help by abstracting the Azure-specific code.

I ran most of my tests by spinning up actual Azure infrastructure, using the Terraform example and a Pay-As-You-Go account, and I am also using the code at my company where they are using Azure infrastructure that supports the MSI and AD user logins.

I'm happy to try to adopt an approach that works for you!

@paulmey
Copy link
Contributor Author

paulmey commented Jan 31, 2020

@kardianos, I mainly started this PR to merge the approaches and bring the code coverage up with some more "unit" tests that don't need a real instance. I'm also fine with seeing if we can provide a SQL Azure instance for CI, but it will be a bit of work in appveyor etc. to keep credentials secret and fresh...?
@wrosenuance please feel free to steal the test code as your own if it seems useful for your PR, I can rebase the access token connector on top of it.

@wrosenuance
Copy link
Contributor

wrosenuance commented Feb 3, 2020

Hi @paulmey, as your original changes were merged first, I rebased #547 on top of them. So far I have not tried to add in your new tests from this PR while waiting for confirmation that the revisions I'm proposing are on the right track. I am happy to do the work to bring them over, though! I think I have broken the merge for this PR after rebasing. 🙈

@paulmey
Copy link
Contributor Author

paulmey commented Feb 3, 2020

Feel free to bring over the tests, this branch will stay around. Aborting this PR.

@paulmey paulmey closed this Feb 3, 2020
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.

Azure Active Directory Authentication is not supported?
3 participants