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

18720 remove colin api auth on/off FF #2311

Merged
merged 5 commits into from
Nov 21, 2023
Merged

18720 remove colin api auth on/off FF #2311

merged 5 commits into from
Nov 21, 2023

Conversation

kzdev420
Copy link
Collaborator

Issue #: /bcgov/entity#17820

Description of changes:

By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of the lear license (Apache 2.0).

@kzdev420 kzdev420 requested a review from argush3 November 16, 2023 21:48
@kzdev420 kzdev420 self-assigned this Nov 16, 2023
Copy link

codecov bot commented Nov 16, 2023

Codecov Report

Merging #2311 (81349e7) into main (79511cd) will decrease coverage by 8.52%.
Report is 27 commits behind head on main.
The diff coverage is 100.00%.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #2311      +/-   ##
==========================================
- Coverage   77.37%   68.85%   -8.52%     
==========================================
  Files         202      235      +33     
  Lines       11602    14226    +2624     
  Branches     1961     1951      -10     
==========================================
+ Hits         8977     9796     +819     
- Misses       2043     3854    +1811     
+ Partials      582      576       -6     
Flag Coverage Δ
colinapi 31.97% <100.00%> (?)

Flags with carried forward coverage won't be shown. Click here to find out more.

Files Coverage Δ
colin-api/src/colin_api/resources/business.py 36.27% <100.00%> (ø)
colin-api/src/colin_api/resources/event.py 41.17% <100.00%> (ø)
colin-api/src/colin_api/resources/filing.py 22.12% <100.00%> (ø)
colin-api/src/colin_api/resources/office.py 47.22% <100.00%> (ø)
colin-api/src/colin_api/resources/parties.py 43.63% <100.00%> (ø)
...lin-api/src/colin_api/resources/program_account.py 58.62% <100.00%> (ø)
colin-api/src/colin_api/resources/reset.py 56.00% <100.00%> (ø)
colin-api/src/colin_api/resources/share_struct.py 48.57% <100.00%> (ø)
colin-api/src/colin_api/utils/__init__.py 19.29% <ø> (ø)
colin-api/src/colin_api/utils/util.py 100.00% <ø> (ø)
... and 18 more

... and 23 files with indirect coverage changes

@argush3
Copy link
Collaborator

argush3 commented Nov 16, 2023

I think we can remove the disable-colin-api-auth property from flags.json

"disable-colin-api-auth": true

@argush3
Copy link
Collaborator

argush3 commented Nov 16, 2023

please remove conditional_auth decorator altogether

def conditional_auth(auth_decorator, roles):
"""Authenticate an endpoint conditionally based off of the value of disable-colin-api-auth feature flag value.
When disable-colin-api-auth feature flag value is True, auth_decorator function will not be called resulting in
the endpoint using this decorator to run without authentication.
When disable-colin-api-auth feature flag value is False, auth_decorator function will be called resulting in
the endpoint using this decorator to authenticate the api consumer. In this scenario, the REST resource(endpoint)
should be decorated with "@conditional_auth(jwt.requires_roles, [COLIN_SVC_ROLE])".
"""
def decorator(f):
@wraps(f)
def wrapped(*args, **kwargs):
auth_is_disabled = flags.is_on('disable-colin-api-auth')
if auth_is_disabled: # pylint: disable=R1705;
return f(*args, **kwargs)
else:
return auth_decorator(roles)(f)(*args, **kwargs)
return wrapped
return decorator

@argush3
Copy link
Collaborator

argush3 commented Nov 16, 2023

we can remove test_util_conditional_auth.py

@argush3
Copy link
Collaborator

argush3 commented Nov 16, 2023

import statements for every resource file that used conditional_auth decorator can be deleted.

@kzdev420
Copy link
Collaborator Author

Hi all

pylint<=2.3.1

To fix the current lint issue, this line should be updated like this.
pylint. remove this <=2.3.1
Then it will install the pylint-3.0.2-py3-none-any.whl

I checked it gets rid of AttributeError: module 'astroid' has no attribute 'TryExcept'. but there are some new lint-errors that should be fixed.

@argush3
Copy link
Collaborator

argush3 commented Nov 20, 2023

@kzdev420 please update dev.txt as you had mentioned above and let's see what new linting errors you get.

For colin-api, we typically don't spend that much effort in fixing linting errors as colin-api will be going away.

Copy link

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 0 Code Smells

No Coverage information No Coverage information
0.0% 0.0% Duplication

@kzdev420 kzdev420 merged commit 4f43d91 into bcgov:main Nov 21, 2023
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.

2 participants