Feat/auth switch #507
Feat/auth switch #507
Conversation
| FORC_VERSION: str = '0.2' | ||
| DEBUG: bool = False | ||
| LOG_LEVEL: str = "INFO" | ||
| DEBUG: bool = True #temporary !!!!!!!!! |
There was a problem hiding this comment.
Set it back to False for the PR
| DEBUG: bool = False | ||
| LOG_LEVEL: str = "INFO" | ||
| DEBUG: bool = True #temporary !!!!!!!!! | ||
| LOG_LEVEL: str = "DEBUG" #temporary !!!!!!!!! |
There was a problem hiding this comment.
Set it back to INFO for the PR
| description="Inject the full url (with protocol) for the real location of the backend service in the template.", | ||
| example="http://192.168.0.1:8787/" | ||
| ) | ||
| only_allow_owner: bool = Field( |
There was a problem hiding this comment.
That is not true -- with this setting the owner can access the backend but also additional users (where the owner have added them --> id in the /users/.. folder)
So maybe we should rename this
| proxy_pass {{ location_url }}; | ||
| proxy_redirect {{ location_url }} $scheme://$http_host/{{ key_url }}/; | ||
| proxy_http_version 1.1; | ||
| proxy_http_version 1.1; @ reviewer: we have the same here. is this necessary? |
There was a problem hiding this comment.
Where do we have the same?
| summary="Set owner authorization to true/false for an existing backend." | ||
| ) | ||
| async def backend_update_auth(backend_id: int, enable_auth: bool, api_key: APIKey = Depends(get_api_key)): | ||
| backend_id = int(secure_filename(str(backend_id))) |
There was a problem hiding this comment.
Why secure_filename ?
The backend_int should just be a number (so you just could check if it is an int)
Ok this is also used in the other part of the code -- but i dont know why
|
|
||
|
|
||
| @router.post( | ||
| "/backends/{backend_id}/auth/{enable_auth}", |
There was a problem hiding this comment.
The {enable_auth} param should be moved to the payload rather than being part of the URL (as this is not restful).
| logger.info(f"Updating backend authorization for backend id: ${backend_id}") | ||
| ok = await backend_service.update_backend_authorization(backend_id, enable_auth) | ||
| if not ok: | ||
| raise HTTPException(status_code=404, detail="Backend not found or update failed.") |
There was a problem hiding this comment.
should differ in response:
404 -- if Backend with id not found
500 -- if something went wrong in the auth enable method itself (with the info what failed)
| suffix_number = await generate_suffix_number(payload.user_key_url) | ||
|
|
||
| payload.id = str(await random_with_n_digits(10)) | ||
| if 'id' in kwargs: # override id and suffix if provided |
…or different edge cases
dweinholz
left a comment
dweinholz
left a comment
There was a problem hiding this comment.
Please address all comments. Feel free to comment yourself if you think the requested change is unnecessary.
| return valid_backends | ||
|
|
||
|
|
||
| async def get_backends_by_id(backend_id: int) -> BackendOut: |
There was a problem hiding this comment.
small typo should be -> get_backend_by_id
| try: | ||
| await backend_service.update_backend_authorization(backend_id, enable_auth) | ||
| except NotFound: | ||
| raise HTTPException(status_code=404, detail="Backend not found.") |
There was a problem hiding this comment.
->
f"Backend with id {id} not found" (makes it easier to debug later via logs etc)
| except NotFound: | ||
| raise HTTPException(status_code=404, detail="Backend not found.") | ||
| except InternalServerError: | ||
| raise HTTPException(status_code=500, detail="Internal server error.") |
There was a problem hiding this comment.
Add the error msg to the detail:
("Could not extract proxy_pass from {backend.file_path}" (also add the backend id) etc..
| raise HTTPException(status_code=404, detail="Backend not found.") | ||
| except InternalServerError: | ||
| raise HTTPException(status_code=500, detail="Internal server error.") | ||
| return {"auth": f"{str(enable_auth).lower()}"} |
There was a problem hiding this comment.
Should also return BackendOut
Please also update the BackendOut so that the auth info is also present there
…refactors for readability, changes to upstream_url_regex
@dweinholz @qqmok
implemented way to switch the authorization layer of the NGINX snippet by adding a new parameter only_allow_owner and a new function that reloads the research environment when wanting to change authorization.