Skip to content

wip: demo that uses Looker as an OAUTH provider for private embed #218

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

Draft
wants to merge 1 commit into
base: master
Choose a base branch
from
Draft

wip: demo that uses Looker as an OAUTH provider for private embed #218

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
49 changes: 48 additions & 1 deletion demo/demo_single_frame.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -525,6 +525,52 @@ const initializeContentControls = () => {
}
}

async function oauth_login() {
const base_url = 'https://self-signed.looker.com:9999/auth'
const code_verifier = secure_random(32)
const code_challenge = await sha256_hash(code_verifier)
const params = {
response_type: 'code',
client_id: 'embed-server',
redirect_uri: `${location.origin}`,
scope: 'cors_api',
Comment on lines +529 to +536
Copy link

Choose a reason for hiding this comment

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

medium

The base_url (line 529), client_id (line 534), and scope (line 536) are hardcoded here.

While this might be acceptable for an initial WIP demo, would it be beneficial to make these configurable? For instance, they could be sourced from demo_config.ts or environment variables, similar to how other configurations are handled in demo_config.ts. This would make the demo more adaptable and easier to configure for different environments or use cases.

state: '1235813',
Copy link

Choose a reason for hiding this comment

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

high

The state parameter is currently hardcoded. For OAuth 2.0 flows, the state parameter is crucial for preventing Cross-Site Request Forgery (CSRF) attacks. It must be a unique, unpredictable value that is generated by the client before initiating the authentication request and then validated when the authorization server redirects the user back.

Could you consider implementing a mechanism to:

  1. Generate a cryptographically random string for state before redirecting.
  2. Store this state value temporarily (e.g., in sessionStorage or a short-lived cookie).
  3. When the user is redirected back to redirect_uri, retrieve the stored state and compare it with the state parameter received from the authorization server. If they don't match, the request should be rejected.

Using a hardcoded state significantly undermines the security of the OAuth flow. For more details, you can refer to RFC 6749, Section 10.12.

As this is a WIP, the suggestion below is a minimal marker. A full implementation would involve more logic.

Suggested change
state: '1235813',
state: '1235813', // TODO: Replace with a dynamically generated, unpredictable value for CSRF protection.

Copy link

Choose a reason for hiding this comment

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

something like "rubber baby buggy bumpers"?

code_challenge_method: 'S256',
code_challenge: code_challenge,
}
const url = `${base_url}?${new URLSearchParams(params).toString()}`
document.location.assign(url)
}

function array_to_hex(array: any) {
return Array.from(array)
.map((b: any) => b.toString(16).padStart(2, '0'))
.join('')
Comment on lines +545 to +548
Copy link

Choose a reason for hiding this comment

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

medium

The array parameter and the mapped item b are currently typed as any. Using more specific types would improve type safety and code readability.

Considering its usage with crypto.getRandomValues (which produces a Uint8Array), could array be typed as Uint8Array and b as number?

Suggested change
function array_to_hex(array: any) {
return Array.from(array)
.map((b: any) => b.toString(16).padStart(2, '0'))
.join('')
function array_to_hex(array: Uint8Array): string { // Type 'array' more specifically, e.g., Uint8Array
return Array.from(array)
.map((b: number) => b.toString(16).padStart(2, '0')) // Type 'b' as number
.join('');

}

function secure_random(byte_count: any) {
Copy link

Choose a reason for hiding this comment

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

medium

The byte_count parameter is typed as any. Would it be more appropriate to type it as number for better type safety?

Suggested change
function secure_random(byte_count: any) {
function secure_random(byte_count: number): string {

const array = new Uint8Array(byte_count)
crypto.getRandomValues(array)
return array_to_hex(array)
}

async function sha256_hash(message: any) {
Copy link

Choose a reason for hiding this comment

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

medium

The message parameter is typed as any. To improve type safety, could this be typed as string? Also, explicitly typing the return promise as Promise<string> would enhance clarity.

Suggested change
async function sha256_hash(message: any) {
async function sha256_hash(message: string): Promise<string> {

const msgUint8 = new TextEncoder().encode(message)
const hashBuffer = await crypto.subtle.digest('SHA-256', msgUint8)
const hashArray = Array.from(new Uint8Array(hashBuffer))
const hashHex = hashArray.map((b) => b.toString(16).padStart(2, '0')).join('')
return hashHex
Comment on lines +560 to +562
Copy link

Choose a reason for hiding this comment

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

medium

The logic here to convert the hash buffer to a hex string seems to duplicate the functionality of the array_to_hex function.

Could we reuse array_to_hex here to avoid redundancy and adhere to the DRY (Don't Repeat Yourself) principle? This would also make the code more concise.

Suggested change
const hashArray = Array.from(new Uint8Array(hashBuffer))
const hashHex = hashArray.map((b) => b.toString(16).padStart(2, '0')).join('')
return hashHex
// const hashArray = Array.from(new Uint8Array(hashBuffer)); // This line can be removed
// const hashHex = hashArray.map((b) => b.toString(16).padStart(2, '0')).join(''); // This line can be removed
return array_to_hex(new Uint8Array(hashBuffer)); // Reuse array_to_hex for conciseness and to apply DRY principle

}

const initializeLoginControl = () => {
const b = document.getElementById('oauth-login')
if (b) {
b.addEventListener('click', () => {
oauth_login()
})
}
}

/**
* Initialize controls.
*/
Expand All @@ -535,6 +581,7 @@ const initializeControls = () => {
initializeUseDynamicHeightsCheckbox()
initializeTabs()
initializeContentControls()
initializeLoginControl()
}

/**
Expand Down Expand Up @@ -649,7 +696,7 @@ const createEmbed = (runtimeConfig: RuntimeConfig, sdk: ILookerEmbedSDK) => {
// Applicable to private embed only. If the user is not logged in,
// the Looker login page will be displayed. Note that this will not
// in Looker core.
.withAllowLoginScreen()
// .withAllowLoginScreen()
// Append to the #dashboard element
.appendTo('#embed-container')
.on('page:changed', (event: PageChangedEvent) => {
Expand Down
1 change: 1 addition & 0 deletions demo/index.html
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -37,6 +37,7 @@ <h6 class="main-heading">Looker Embed SDK Single Frame Demo</h6>
<input type="checkbox" id="preventNavigation">
<span>Prevent navigation</span>
</label>
<button id="oauth-login" class="small-round">Login</button>
</div>
</div>
<div class="s6">
Expand Down