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 auth #2175

Open
wants to merge 22 commits into
base: master
Choose a base branch
from
Open

Azure auth #2175

wants to merge 22 commits into from

Conversation

not-night-but
Copy link
Contributor

@not-night-but not-night-but commented May 3, 2024

Fix azure sso auth

First pass at this, definitely still some stuff to work on, but I'm wanting some feedback.

TODO

  • Figure out what happens if perms expire/are revoked with an active connection. May need to add reauthentication to the error handler on the pool.

NOTES

  • I just have the clientid hardcoded in the azureAuthService (it is inserted as plaintext in the url, so not exactly a secret?)
  • To actually test this, you'll have to comment out the ultimate checking in SqlServerForm
  • I put the token cache in a new table to avoid updating the connection record while it's active (as we don't update that object in memory). This also makes it easier to grab as we have the id on the connection so its trivial, but trying to grab the connection record with the information available at that point is reasonably difficult (no id, no sense of whether its a used connection or saved connection, etc). This also prevents polluting that object, as the cache is one heck of a string

🚀 This description was created by Ellipsis for commit 656494e

Summary:

This PR integrates Azure SSO authentication for SQL Server connections in Beekeeper Studio, including UI updates, token caching, and handling of token refresh scenarios.

Key points:

  • Integrated Azure SSO authentication for SQL Server connections.
  • Added new columns for token caching in database models.
  • Updated connection handling to support Azure authentication.
  • Handled token refresh and reauthentication scenarios.
  • Updated UI components to allow Azure SSO configuration.
  • Added new migrations for database changes.

Generated with ❤️ by ellipsis.dev

@rathboma
Copy link
Collaborator

@ellipsis-dev code review please

Copy link
Contributor

ellipsis-dev bot commented May 10, 2024

OK! Reviewing this PR...


Responding to this comment by @rathboma. For more information about Ellipsis, check the documentation.

Copy link
Contributor

@ellipsis-dev ellipsis-dev bot left a comment

Choose a reason for hiding this comment

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

👍 Looks good to me! Reviewed everything up to 1217e28 in 1 minute and 44 seconds

More details
  • Looked at 1022 lines of code in 21 files
  • Skipped 1 files when reviewing.
  • Skipped posting 0 drafted comments based on config settings.

Workflow ID: wflow_hhp9lEyRQmqhobit


You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet mode, and more.

6 days left in your free trial, upgrade for $20/seat/month or contact us.

Copy link
Collaborator

@rathboma rathboma left a comment

Choose a reason for hiding this comment

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

Looks awesome! Super clean.

had a a about foreign keys, plus a couple of nits, but otherwise good

apps/studio/src/components/ConnectionInterface.vue Outdated Show resolved Hide resolved
apps/studio/src/lib/db/authentication/azure.ts Outdated Show resolved Hide resolved
apps/studio/src/lib/db/authentication/azure.ts Outdated Show resolved Hide resolved
Copy link
Contributor

@ellipsis-dev ellipsis-dev bot left a comment

Choose a reason for hiding this comment

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

👍 Looks good to me! Incremental review on 656494e in 3 minutes and 25 seconds

More details
  • Looked at 222 lines of code in 10 files
  • Skipped 0 files when reviewing.
  • Skipped posting 1 drafted comments based on config settings.
1. /apps/studio/src/lib/db/authentication/azure.ts:95
  • Draft comment:
clientId: process.env.AZURE_CLIENT_ID || globals.clientId

Consider fetching the clientId from environment variables to allow configuration flexibility and enhance security. This change should be applied wherever the clientId is used in the Azure authentication service.

  • Reason this comment was not posted:
    Confidence of 0% on close inspection, compared to threshold of 50%.

Workflow ID: wflow_hqpaME3tPAukH755


You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet mode, and more.

3 days left in your free trial, upgrade for $20/seat/month or contact us.

@rathboma
Copy link
Collaborator

@not-night-but if you fix merge conflicts, and then fix the migration, we can merge this and release it!

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.

None yet

2 participants