-
Notifications
You must be signed in to change notification settings - Fork 13
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
Add login functionality to ezBIDS #101
Add login functionality to ezBIDS #101
Conversation
LGTM, based on your testing instructions. |
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 some minor things but not obstructing
|
||
api/*.pub | ||
api/*.key | ||
api/ezbids.key |
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.
I guess the previous line should account for ezbids.key
ui/src/LandingPageHeader.vue
Outdated
style="font-size: var(--el-font-size-extra-large); font-family: unset; color: #3482e9;" | ||
>{{ hasJWT ? "GET STARTED" : "LOG IN / REGISTER"}}</el-button | ||
> | ||
<!-- <RouterLink style="text-decoration: none;" to="/convert">LOG IN / REGISTER</RouterLink> --> |
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.
safe to cleanup
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.
done
|
||
<br> | ||
<br> | ||
<br> | ||
|
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.
YES
Uploading | ||
<font-awesome-icon icon="spinner" pulse/> | ||
</h3> | ||
<small>Please do not close/refresh this page until all files are uploaded.</small> |
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.
For semantics I'd still wrap with a block element, like <p>
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.
ui/src/Upload.vue
Outdated
const res = await axios.post(`${this.config.apihost}/session`, { | ||
headers: { 'Content-Type': 'application/json' } | ||
}) | ||
console.log("created new session"); |
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.
cleanup
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.
im going to hold off on this for now, as cleaning this up will mean redoing how we manage configs in ezBIDS. (currently we store them in the store, which is quite annoying if we want to access them outside of the vuex context)
api/controllers.ts
Outdated
next(err); | ||
}); | ||
}); | ||
|
||
router.post('/upload-multi/:session_id', upload.any(), (req:any, res, next)=>{ | ||
router.post('/upload-multi/:session_id', validateWithJWTConfig(), upload.any(), (req: any, res, next) => { | ||
userCanAccessSession(req.params.session_id, req.auth.sub as unknown as number, false).then(async (session) => { |
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.
couldnt userCanAccessSession
be a router middleware?
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.
done, the req object will now hold a property ezBIDS
which contains the session
size="large" | ||
closable | ||
v-for="allowedUser in allowedUserProfiles" | ||
:key="allowedUser?.sub" |
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.
for some reason, github highlighting breaks at this point, I think it does not understand the ?.
yet
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.
Based on my testing, this should be fine
@nicoalee feel free to merge this PR after looking over Anibal's comments |
This PR aims to connect our login implementation for brainlife to ezBIDS.
NOTE: This PR is also connected to changes to generate_keys.sh.
HOW TO TEST
- This flag exists due to security vulnerabilities so make sure to enable again after testing
- ADMIN USER: email: [email protected] password: 30q49th
- GUEST USER: email: [email protected] password: 912r3iub
- login as another user in private browsing (you can create a user if you want or use the one you are not logged in with) and check that you cannot access the session
- in the original window, click the Share Session button to open the manage users dialog and add a user.
- use incognito mode and log in as the user you just added. Check that you can access the given session. This user should not be able to see the Share Session button.
SUMMARY OF CHANGES
- ezBIDS now issues its own JWTs from the
/download/:session_id/token
which will only be valid for 10 seconds.- The reason for this is because the
/download/:session_id/*
route often is used in a way that does not allow us to attach HTTP headers. We therefore send a JWT in the URL (albeit a short lived one).- The
/download/:session_id/*
route is the only valid route that expects and consumes these JWTs. Other routes will expect JWTs in the header.<a ...><img ... /></a>
HTML has been replaced with a custom<AsyncImageLink />
. This is because of the previously mentioned token change.- previously these elements were bound to the path directly, i.e.
<img :src="/route/to/backend.png" />.
This doesn't work anymore so this case needs to be handled.axios.instance.ts
. This attaches a JWT to every outgoing request.- people added to the session cannot add more users, only the owner can