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

Refactor API Auth to support non X.509 identifiers #368

Merged
merged 1 commit into from
Sep 6, 2022

Conversation

gregcorbett
Copy link
Member

@gregcorbett gregcorbett commented Jul 8, 2022

We want the API to allow access based on OIDC Identifiers in the future., so need to generalise getAPIAuthentication.

Still a work in progress, I should rename the authByCert method at least and fix the tests.

I also wonder wether we should remove this check, maybe API access should be a function of the API Identifiers alone - not something a user can browse by virtue of their roles?

@gregcorbett gregcorbett added this to the 5.10.0 milestone Jul 8, 2022
@gregcorbett
Copy link
Member Author

... and fix the tests.

Wait a minute... the test passed... ah, you can throw as many arguement to a function as you like... Well I'll edit the tests anyway...

@gregcorbett gregcorbett force-pushed the typeless_API_Authentication branch from 7593956 to a7cd245 Compare July 8, 2022 14:57
@gregcorbett
Copy link
Member Author

I also wonder wether we should remove this check, maybe API access should be a function of the API Identifiers alone - not something a user can browse by virtue of their roles?

Admins should probably be able to access the API by virtue of being admins.

The message when an unauthorized request comes in should be changed. If it's a robot request it's not apporpriate to return "get a role", it should be along the lines of "a human should add your robot API identifier". This will probably be simplier if we remove the check the user ID check.

@gregcorbett
Copy link
Member Author

I also wonder wether we should remove this check, maybe API access should be a function of the API Identifiers alone - not something a user can browse by virtue of their roles?

Hmm... maybe not... we were vague and said we would "restrict the access to personal data to users with roles at sites or registered robots", no distinctions for access personal data in the portal or the API...

@gregcorbett gregcorbett force-pushed the typeless_API_Authentication branch from a7cd245 to a8dd918 Compare July 8, 2022 15:19
@gregcorbett gregcorbett marked this pull request as ready for review July 29, 2022 13:44
@gregcorbett gregcorbett requested a review from a team as a code owner July 29, 2022 13:44
@gregcorbett gregcorbett force-pushed the typeless_API_Authentication branch 3 times, most recently from d15bed2 to 2b7e112 Compare August 8, 2022 09:22
@gregcorbett
Copy link
Member Author

I've rebased this over the conflict, to avoid a 3rd pass of #324, I will merge that one first.

@gregcorbett gregcorbett marked this pull request as draft August 8, 2022 09:48
@gregcorbett
Copy link
Member Author

Converted to draft until #324 is merged.

- not just X.509, we want to allow OIDC based access to the
  API.
- change test cases appropriately
- generalise language used, i.e. identifiers not DNs.
@gregcorbett gregcorbett force-pushed the typeless_API_Authentication branch from 1d12bd4 to fd1aa88 Compare August 9, 2022 09:41
@gregcorbett
Copy link
Member Author

Rebased to fix the conflicts.

@gregcorbett gregcorbett marked this pull request as ready for review August 9, 2022 13:19
@gregcorbett gregcorbett merged commit 14e4722 into GOCDB:dev Sep 6, 2022
@gregcorbett gregcorbett deleted the typeless_API_Authentication branch September 6, 2022 16:28
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants