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

Supplying authorization to get slide notes for ottrpal #118

Merged
merged 12 commits into from
Aug 14, 2023

Conversation

cansavvy
Copy link
Contributor

@cansavvy cansavvy commented Aug 9, 2023

Purpose/implementation Section

What changes are being implemented in this Pull Request?

This was something we pair coded on. Basically if credentials aren't supplied we want to use a dummy credential to grab slides that are public anyway.

This code should do that with some encryption steps as well.

This is a PR based off the conversation here: #116 (comment)

@cansavvy cansavvy changed the base branch from main to cansavvy/fig.alt-2 August 9, 2023 19:23
@howardbaek howardbaek self-requested a review August 9, 2023 21:08
R/auth.R Outdated
}

credentials <- list(
access_token = unserialize(decrypted)[[1]]$access_token,
Copy link
Contributor

Choose a reason for hiding this comment

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

If user provides the access_token and refresh_token as arguments, then this line won't be able to find decrypted object.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oh right! We need to have this defined as access_token and refresh_token above in the if statement

R/auth.R Outdated
credentials <- list(
access_token = unserialize(decrypted)[[1]]$access_token,
expires_in = 3599L,
refresh_token = unserialize(decrypted)[[1]]$refresh_token,
Copy link
Contributor

@howardbaek howardbaek Aug 9, 2023

Choose a reason for hiding this comment

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

Same comment as above: If user provides the access_token and refresh_token as arguments, then this line won't be able to find decrypted object.

Copy link
Contributor Author

@cansavvy cansavvy left a comment

Choose a reason for hiding this comment

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

Does this comment make sense? @howardbaek Do you know how to fix this? If not I can send suggestions.

R/auth.R Outdated
}

credentials <- list(
access_token = unserialize(decrypted)[[1]]$access_token,
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oh right! We need to have this defined as access_token and refresh_token above in the if statement

@howardbaek
Copy link
Contributor

It seems like auth_from_secret() is a function that uses the "secrets" that we provided to the ottrpal R package to obtain a token. If so, should we just get rid of all the function arguments (access_token and refresh_token) so that when we run auth_from_secret(), we run this chunk no matter what:

decrypted <- openssl::aes_cbc_decrypt(
      readRDS(encrypt_creds_user_path()),
      key = readRDS(key_encrypt_creds_path())
)

and we create decrypted. When I supply a value to access_token and refresh_token, then the above code chunk doesn't run since its inside an if statement, and the following code chunk throws an error since it doesn't know what decrypted is:

  credentials <- list(
    access_token =  unserialize(decrypted)[[1]]$access_token,
    expires_in = 3599L,
    refresh_token = unserialize(decrypted)[[1]]$refresh_token,
    scope = scopes_list,
    token_type = "Bearer"
  )

Copy link
Contributor Author

@cansavvy cansavvy left a comment

Choose a reason for hiding this comment

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

@howardbaek this is what I was trying to describe to fix this. Let me know if this makes sense.

decrypted <- openssl::aes_cbc_decrypt(
readRDS(encrypt_creds_user_path()),
key = readRDS(key_encrypt_creds_path())
)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Suggested change
)
)
access_token <- unserialize(decrypted)[[1]]$access_token
refresh_token <- unserialize(decrypted)[[1]]$refresh_token

I'm doing this on my phone but this is what I'm suggesting you do. Bring the phrases from below up here (not sure if I typed these right) and declare the variables. Then below just specify access_token = access_token and same for refresh_token.

If this doesn't make sense, let me know and I'll send a commit later.

@howardbaek howardbaek marked this pull request as ready for review August 14, 2023 19:06
@howardbaek howardbaek merged commit c37aa3e into cansavvy/fig.alt-2 Aug 14, 2023
@howardbaek
Copy link
Contributor

@cansavvy Looks good. Merged.

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