Various and sundry #332
Merged
Various and sundry #332
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Contributor
Author
|
I was going to write up change log entries, but all of these seem like internal improvements to me and not user-facing. The one or two that might be user-facing are only so when things are used in development contexts (e.g. with |
…able
Reading the OAuth 2.0 spec again¹, I noted that:
The authorization server MAY issue a new refresh token, in which
case the client MUST discard the old refresh token and replace it
with the new refresh token. The authorization server MAY revoke the
old refresh token after issuing a new refresh token to the client.
I had assumed the refresh token itself was never renewed as I never
observed AWS Cognito doing so in practice. I addressed this assumption
for nextstrain.org², which is using OAuth 2.0, but it also got me
thinking about Nextstrain CLI here, which isn't using OAuth 2.0 per se
but is using a Cognito API that's very similar.
Like the OAuth 2.0 spec, the documented Cognito response type
definitions include the possibility of an updated refresh token.³
Perhaps Cognito even does renew them, but only as necessary and we've
never happened to notice this issue!
Regardless, we're planning to move to generic OAuth 2.0 authentication
instead of using Cognito's API, or at least support the former in
addition to the latter. This change of logic will help smooth that
switch.
¹ <https://datatracker.ietf.org/doc/html/rfc6749#section-6>
² <nextstrain/nextstrain.org#736>
³ <https://docs.aws.amazon.com/cognito-user-identity-pools/latest/APIReference/API_AuthenticationResultType.html>
…esources… …like the other remote actions/commands do. Being precise about what's being acted upon is important and useful. Note that at the moment this is only not identical to the previous output when NEXTSTRAIN_DOT_ORG is set to something other than <https://nextstrain.org>.
…dling
There was a narrow space for bugs here since it was reconstructing what
the user associated with the request was *expected* to be, but not what
it necessarily *actually* was, e.g. what was used to send the actual
Authorization: Bearer …
header in the request.
From a given requests.Response object we can't access the auth provider
directly (by design), so we have to arrange a backchannel to pass along
to the error handler the actual user authn information used in the
request. A simple custom attribute on the request object does the job
nicely.
One alternative would be a custom request header, e.g.
Nextstrain-Username: trs
or some such, but I like the attribute because it's more targeted in
scope and doesn't send unnecessary information. Another alternative
would be for the error handler to parse the token in the Authorization
header—it doesn't really even need to verify the JWT, just parse it—and
extract the username that way. But that's a bit more involved and the
simplicity of a custom attribute is more compelling.
…is complete As of last spring! Related-to: <https://github.com/nextstrain/private/issues/20>
This avoids issues where nextstrain.cli.command.remote masks the
nextstrain.cli.remote module when attempting to use the former, e.g.
import nextstrain.cli.remote
will get you nextstrain.cli.command.remote because it was imported as
"remote" into nextstrain.cli.
I thought maybe you could work around this, e.g. with
import nextstrain.cli.remote as remote
but no dice.
This happens because importing x.y.z first imports x, then imports x.y,
and finally x.y.z and during this each nested module is made available
in its parent by design.
In any case, switching to nextstrain.cli.command.all_commands nicely
parallels the existing nextstrain.cli.runner.all_runners variable.
These are used in `nextstrain view --help` output, and setting them during development (e.g. for `make -C doc livehtml`) would otherwise influence the generated doc.
tsibley
force-pushed
the
trs/various-and-sundry
branch
from
ac2feff to
1c6a658
Compare
Based on `nextstrain view`'s interaction with the standard library's webbrowser module. Arranges to launch the browser in a separate thread, as this will be good enough for many contexts. `nextstrain view` itself still arranges to launch the browser in a separate _process_, since view's main thread/process exec's into a new program shortly after launching the browser.
4 tasks
joverlee521
approved these changes
Jan 11, 2024
Contributor
joverlee521
left a comment
joverlee521
left a comment
There was a problem hiding this comment.
I'm probably missing context from the ongoing CDC work, but changes LGTM from inspection.
I'm curious what prompted the NOBROWSER variable?
Contributor
Author
When doing browser work for #333 and Since |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
See commit messages. Done during work on the path towards OIDC/OAuth2-based login, but suitable for separate review and independent merge.
Checklist