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
New feature json web tokens - related to old issue SK-16 #342
Conversation
I will review it soon SK-74 |
Is there any documentation on how to test this? |
No, since there is no docs for CLI, nor help string in click. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good, tested and it works.
I think we should change so that the token is used by default and can optionally be turned off.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM. I think for now it's best to have the token disabled by default. If we enable it we then need some way to pass the one time token to the quickstart compose deployments. I will look into it later when I'll refactor the compose yamls.
Good point @mcapuccini I did not think of that. A reasonable solution there though is to be able to just set the token in the config files. But I am ok having it disabled by default for now. Actually, that would also be a bit nicer to users, then they get a couple of versions to adapt their deployment automation also. |
@ahellander sure, we should have it set in the config files later on. We could have the token generation as an additional step before the deployment. If we have a default token pushed to the GitHub configs is like disabling the token by default. My thinking is that we could explicitly disable it instead :) |
Yes, agree, lets do it that way for now.
Best regards,
Andreas Hellander
Associate Professor, Scientific Computing
15 feb. 2022 kl. 11:28 skrev Marco Capuccini ***@***.******@***.***>>:
@ahellander<https://github.com/ahellander> sure, we should have it set in the config files later on. We could have the token generation as an additional step before the deployment. If we have a default token pushed to the GitHub configs is like disabling the token by default. My thinking is that we could explicitly disable it instead :)
—
Reply to this email directly, view it on GitHub<#342 (comment)>, or unsubscribe<https://github.com/notifications/unsubscribe-auth/AASBHZQNYQ4IDQPMXCYQ5ITU3ITFDANCNFSM5NXZYGCA>.
Triage notifications on the go with GitHub Mobile for iOS<https://apps.apple.com/app/apple-store/id1477376905?ct=notification-email&mt=8&pt=524675> or Android<https://play.google.com/store/apps/details?id=com.github.android&referrer=utm_campaign%3Dnotification-email%26utm_medium%3Demail%26utm_source%3Dgithub>.
You are receiving this because you were mentioned.Message ID: ***@***.***>
När du har kontakt med oss på Uppsala universitet med e-post så innebär det att vi behandlar dina personuppgifter. För att läsa mer om hur vi gör det kan du läsa här: http://www.uu.se/om-uu/dataskydd-personuppgifter/
E-mailing Uppsala University means that we will process your personal data. For more information on how this is performed, please read here: http://www.uu.se/en/about-uu/data-protection-policy
|
@ahellander you requested change, but I think you decided to keep it as is based on the discussion above? Please approve if so. |
Correct.
Best regards,
Andreas Hellander
Associate Professor, Scientific Computing
16 feb. 2022 kl. 15:26 skrev Fredrik Wrede ***@***.******@***.***>>:
@ahellander<https://github.com/ahellander> you requested change, but I think you decided to keep it as is based on the discussion above?
—
Reply to this email directly, view it on GitHub<#342 (comment)>, or unsubscribe<https://github.com/notifications/unsubscribe-auth/AASBHZTP6PALPZSUCEYSULTU3OXZZANCNFSM5NXZYGCA>.
Triage notifications on the go with GitHub Mobile for iOS<https://apps.apple.com/app/apple-store/id1477376905?ct=notification-email&mt=8&pt=524675> or Android<https://play.google.com/store/apps/details?id=com.github.android&referrer=utm_campaign%3Dnotification-email%26utm_medium%3Demail%26utm_source%3Dgithub>.
You are receiving this because you were mentioned.Message ID: ***@***.***>
När du har kontakt med oss på Uppsala universitet med e-post så innebär det att vi behandlar dina personuppgifter. För att läsa mer om hur vi gör det kan du läsa här: http://www.uu.se/om-uu/dataskydd-personuppgifter/
E-mailing Uppsala University means that we will process your personal data. For more information on how this is performed, please read here: http://www.uu.se/en/about-uu/data-protection-policy
|
@ahellander great, please approve this PR |
Forgot that we should probably remove token in combiner-settings.yaml.template. Will do ASAP tomorrow. |
Status
Description
Included in this pr:
Types of changes
What types of changes does your code introduce to FEDn?
Checklist
If you're unsure about any of the items below, don't hesitate to ask. We're here to help!
This is simply a reminder of what we are going to look for before merging your code.
Further comments
Anything else you think we should know before merging your code!