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

[REF] OAuth - Move some API code to the BAO for better reusability #23725

Merged
merged 2 commits into from
Aug 4, 2022

Conversation

colemanw
Copy link
Member

@colemanw colemanw commented Jun 8, 2022

Overview

Moves some code out of the API into the BAO so we get more use out of it.

Before

Lots of overridden API actions, which is not standard practice.

After

Instead of overriding API actions, use the standard actions and put the other logic in the correct place in the BAOs so it gets picked up automatically.

Comments

I haven't used this extension or tested any of this code, this is just what I think the structure ought to look like.

@civibot
Copy link

civibot bot commented Jun 8, 2022

(Standard links)

@civibot civibot bot added the master label Jun 8, 2022
@colemanw
Copy link
Member Author

colemanw commented Jun 8, 2022

FYI @highfalutin this refactors your api code to move logic around for better reusability. Hopefully I've kept your functionality the same during the refactor.

@colemanw colemanw force-pushed the oAuth branch 3 times, most recently from 0ef8024 to 8af7e41 Compare June 11, 2022 22:46
@demeritcowboy
Copy link
Contributor

I did two simple tests. Except for the above comment:

Have not done code review.

@demeritcowboy
Copy link
Contributor

Also FYI when testing make sure your test site supports https and CIVICRM_BASEURL is https. And also Google has made the oauth prompt about unverified apps even scarier than before, but will still let you continue.

@demeritcowboy
Copy link
Contributor

This has a merge conflict: ext/oauth-client/Civi/Api4/Action/OAuthContactToken/Create.php

colemanw added 2 commits July 18, 2022 12:37
…I4 Get

Putting permission checks in the BAO ensures that they are always enforced
regardless of which layer accesses them (Api3, Api4, etc) and that they will be
enforced even if this is not the primary entity of the api call (e.g. using joins).
This ensures permissions are checked regardless of the api version or action being performed.
It also consolidates the code used to check access so that the API checkAccess action will
stay consistently and always perform the same access checks as the other api actions.
@colemanw
Copy link
Member Author

@demeritcowboy I've rebased to fix the MC

@highfalutin
Copy link
Contributor

Hi @colemanw, getting a chance to look at this now. When you say this "Moves some code out of the API into the BAO so we get more use out of it", what uses are you thinking of? Search Kit?

I support the code being refactored into a normal form as long as everything still works. I'm just curious about what the payoff will be.

@colemanw
Copy link
Member Author

The standard practice is to put logic into the BAO instead of the API because our APIs come and go. v2, v3, now v4 all rely on the BAO to do the actual work, the API is just a wrapper to that functionality, not the functionality itself.

@highfalutin
Copy link
Contributor

I did my own r-run/r-test, with good results.

I installed the code from this PR and ran the automated tests for a custom extension that depends on oauth-client. These tests don't directly test the permissions mechanisms, but the fact that they all passed is good. It tells me this PR doesn't break anything, as far as I can tell.

I also verified that the oauth-client tests DO test the permissions mechanisms, and they are all passing.

@colemanw
Copy link
Member Author

colemanw commented Aug 4, 2022

Merging based on review

@colemanw colemanw merged commit beb2d1e into civicrm:master Aug 4, 2022
@colemanw colemanw deleted the oAuth branch August 4, 2022 00:40
@demeritcowboy
Copy link
Contributor

Thanks @highfalutin @colemanw

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants