search

hubmapconsortium / commons

The common code supporting the HuBMAP web services
MIT License
1 stars 0 forks source link

Fix "has priv" methods #125

Closed shirey closed 5 months ago

shirey commented 5 months ago

Fixed issue when no or a blank token was sent to various privilege check methods in AuthHelper.

maxsibilla commented 5 months ago

@shirey Just to confirm, this PR is to address https://github.com/hubmapconsortium/commons/issues/115? I think the methods that utilize self.getUserInfo will also need to be updated (e.g. https://github.com/hubmapconsortium/commons/blob/514284effd124545aa8216a4f2ad9a46555e61ad/hubmap_commons/hm_auth.py#L273-L274) as an HTTPException and subsequent 500 is still returned to the user when an expire token is used

yuanzhou commented 5 months ago

@maxsibilla does Bill's fix address the issue #115 ? It was not clear to me based on the comments here. I'll do some testings too.

maxsibilla commented 5 months ago

@maxsibilla does Bill's fix address the issue #115 ? It was not clear to me based on the comments here. I'll do some testings too.

Yes this is done to address an issue where getUserInfo would just return if no token was present. For certain methods using this it would basically allow methods to continue with their processes instead of halting. I bet with Bill and discovered SenNet was using an incorrect method for a privilege check, which was my initial comment

yuanzhou commented 5 months ago

@maxsibilla good to know. Since it's a simple and straightforward fix, we don't need to go through the DEV->TEST->PROD workflow. I'll do some local testing before merging this PR. Then publish a new package to PyPI. We can just bump the dependency the version used in HuBMAP and SenNet API repos as we build and release new changes.

We'll need to coordinate for the search-adaptor and search-api updates and rebuild/release. Some of the Harvard and PSC codebase will also need to update possibly.

shirey commented 5 months ago

Based on a conversation with @maxsibilla yesterday, this fix wasn't really needed, so I'll close this PR as it is confusing the matter. @yuanzhou FYI