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

Show logged in user in header #244

Merged
merged 6 commits into from
Feb 22, 2022
Merged

Conversation

aanil
Copy link
Contributor

@aanil aanil commented Feb 18, 2022

Before submitting a pr:

  • Tests passing
  • Black formatting
  • Rebase/merge the dev branch
  • Note in the CHANGELOG

@i-oden
Copy link
Member

i-oden commented Feb 18, 2022

@worukan Is working on changing the token after at the moment and displaying information from the token in the CLI, so I think we'll wait a bit with this

@i-oden i-oden marked this pull request as draft February 18, 2022 11:13
@MatthiasZepper MatthiasZepper mentioned this pull request Feb 21, 2022
4 tasks
@worukan
Copy link
Contributor

worukan commented Feb 21, 2022

This can be handled by showing the username that the user has entered to login. There is no need to add this to the plaintext part of the token and extract it from there. So if cli can get the final token to use the endpoints, the entered username can be displayed as logged in user.

@aanil
Copy link
Contributor Author

aanil commented Feb 21, 2022

@worukan I'm not sure I understand. The user won't log in every time they use the CLI. They will login once and the token will be saved on their system and they'll use the token to run commands until the token expires?

@i-oden
Copy link
Member

i-oden commented Feb 21, 2022

I don't think displaying the username when during a command is a must have, more like a could have. If the user wants to know what username is authenticated in the current session, they can run the dds auth info command as @worukan said. So I think we can skip that for now and if we feel like this is something we want to work on in a few weeks or so, then we can do that.

@worukan
Copy link
Contributor

worukan commented Feb 21, 2022

Perhaps the cli can remember the username or store it with the token in the token file?

@MatthiasZepper
Copy link
Contributor

MatthiasZepper commented Feb 21, 2022

If the user wants to know what username is authenticated in the current session, they can run the dds auth info command as @worukan said.

Not anymore. This functionality will be scrapped entirely by Volkans PR.

Also we do not store anything locally except the .dds_cli_token file. So unless we update the format to let's say a JSON file that can locally cache more values than just the token, I don't see how to store the username for the lifetime of the token if not within the token itself.

@i-oden
Copy link
Member

i-oden commented Feb 21, 2022

@MatthiasZepper Since we're not able to test at the moment, I can't have a detailed discussion about this. But, the username is always required by the token, otherwise we would never know the current_user() in the web. So if this functionality is completely scrapped now, then we can add that at a later time, by for example calling the API, authenticating with the token as usual and just returning the username. This should be something fairly easy to implement too and we can even do that before the testing starts probably. So I think we can continue with the changes that Volkan is making and we'll see what possible additions and future improvements we can do later.

@worukan
Copy link
Contributor

worukan commented Feb 21, 2022

@MatthiasZepper Since we're not able to test at the moment, I can't have a detailed discussion about this. But, the username is always required by the token, otherwise we would never know the current_user() in the web. So if this functionality is completely scrapped now, then we can add that at a later time, by for example calling the API, authenticating with the token as usual and just returning the username. This should be something fairly easy to implement too and we can even do that before the testing starts probably. So I think we can continue with the changes that Volkan is making and we'll see what possible additions and future improvements we can do later.

Yes getting the username from the api by providing the token sounds like a good candidate! It is safer than storing locally assuming that all communication runs on TLS.

@aanil
Copy link
Contributor Author

aanil commented Feb 21, 2022

Changed the solution to get username from API.

@MatthiasZepper
Copy link
Contributor

MatthiasZepper commented Feb 21, 2022

Changed the solution to get username from API.

Ah nice. I completely forgot that there already is an existing endpoint to display the user information!

dds_cli/__main__.py Outdated Show resolved Hide resolved
@alneberg
Copy link
Contributor

@worukan what's wrong with putting the username in the readable token header? The solution as it stands now will have implications for authentication where an HOTP authentication will be initiated if the token has expired. And potentially one would hit the request limit if one does 10 --help commands with an expired token. I guess one could save the username in the tokenfile and change the format of the tokenfile but then we rely on unsigned information created by the CLI instead of the signed header of the token.

@worukan
Copy link
Contributor

worukan commented Feb 21, 2022

@worukan what's wrong with putting the username in the readable token header? The solution as it stands now will have implications for authentication where an HOTP authentication will be initiated if the token has expired. And potentially one would hit the request limit if one does 10 --help commands with an expired token. I guess one could save the username in the tokenfile and change the format of the tokenfile but then we rely on unsigned information created by the CLI instead of the signed header of the token.

Because it is the wrong place. That token header is mainly for being able to decrypt the token correctly. Just because we can put information in there does not justify doing so. The encrypted token is a special token. It should not be seen as an all-purpose token.

If you like I have a new suggestion @alneberg @MatthiasZepper @aanil @inaod568
We used to have a token endpoint which provided a signed token with sub and exp in it. While the endpoint is not available now, the functionality is still there which is jwt_token function. We could bring the endpoint back easily.

  1. Request jwt_token from the cli.
  2. Request encrypted_jwt_token from the cli. (Perhaps 1 and 2 can be done in one step)
  3. Save jwt_token and use the expiration time and the username from that.
  4. Save encrypted_jwt_token. If it simplifies, they can be saved to separate files.
  5. Use the encrypted_jwt_token for normal operation.

In this way the expiration time of the encrypted token will come slightly after the signed token, but that is a negligible difference.

@alneberg
Copy link
Contributor

Right, I see. Although I also realised that just skipping the API call for user info in case the token is expired would solve most of my issues as well.

@aanil aanil marked this pull request as ready for review February 22, 2022 09:54
Copy link
Contributor

@alneberg alneberg 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! I might have chosen a different color than red for the username, but that can be changed later.

Copy link
Member

@i-oden i-oden 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, just a small comment / change

dds_cli/__main__.py Outdated Show resolved Hide resolved
@aanil aanil requested a review from i-oden February 22, 2022 11:12
Copy link
Member

@i-oden i-oden 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!

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

5 participants