Skip to content

Add a common implementation for the BrokerRestrictionsManager, Fixes AB#3181005 #2604

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

Open
wants to merge 21 commits into
base: dev
Choose a base branch
from

Conversation

p3dr0rv
Copy link
Contributor

@p3dr0rv p3dr0rv commented Mar 13, 2025

Context

Currently, for QR + PIN authentication, users are prompted for camera permission every time the camera is used, even if OS-level permissions are granted. This is a CELA requirement to ensure user consent. However, a new feature request aims to suppress this camera permission prompt if an admin enables a specific flag in the restriction manager of the authenticator app.

Changes

  • The current PR involves refactoring the RestrictionsManagerHelper class to make it reusable for reading the new flag.
  • Create a new class, CommonBrokerRestrictionsManager, that implements the IBrokerRestrictionsManager interface.

Upcoming PRs will leverage this flag to implement functionality that suppresses the camera permission prompt when enabled by the admin.

Related PR: https://github.com/AzureAD/ad-accounts-for-android/pull/3066
AB#3181005

Copy link

❌ Work item link check failed. Description does not contain AB#{ID}.

Click here to Learn more.

@p3dr0rv p3dr0rv changed the title Move restriction manager Move RestrictionsManagerHelper and BrokerProcessIpcUtils from broker to common Mar 13, 2025
@github-actions github-actions bot changed the title Move RestrictionsManagerHelper and BrokerProcessIpcUtils from broker to common Move RestrictionsManagerHelper and BrokerProcessIpcUtils from broker to common, Fixes AB#3181005 Mar 13, 2025
@p3dr0rv p3dr0rv marked this pull request as ready for review March 20, 2025 00:18
@p3dr0rv p3dr0rv requested a review from a team as a code owner March 20, 2025 00:18
fadidurah
fadidurah previously approved these changes Mar 20, 2025
/**
* Helper class to read the app restrictions manager of the current broker app or another broker app.
*/
class RestrictionsManagerHelper(
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

RestrictionsManagerHelper

If you're going to use this in WebViewAuthorizationFragment, how will it work in OneAuth only flow?

Copy link
Contributor Author

@p3dr0rv p3dr0rv Mar 26, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If we are in a non-broker flow, we do not call RestrictionsManagerHelper for this QR PIN scenario, checking the Activity type that contains the fragment.

However, in a hypothetical scenario where RestrictionsManagerHelper is used in a non-broker flow:

  1. Requesting data from a broker app: will make an IPC call to the broker app, read the Restrictions manager from there, and return the result.
  2. Requesting data from the current app: will read the Restrictions manager of the app, which will likely be null.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I raised this because I feel like this is broker only component is being put in common and can potentially run in non-brokered flow. It would be better if could inject an object in common layer (or WebViewAuthorizationFragment in this case) to customize behavior for broker vs other. It's general problem that is seen on other cases as well e.g. nonce redirect handling, flighting

I guess, it might be too much work if you do that in this PR?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Q: For what would be logic for non-broker flow?

Copy link
Contributor Author

@p3dr0rv p3dr0rv Apr 2, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

for non-broker flow we do not check the restriction manager, and we just grant the camera permission if we have it.
But even if the component were called in a non-broker flow it will keep working as I change it to only read RestrictionsManager from broker apps

@p3dr0rv p3dr0rv changed the title Move RestrictionsManagerHelper and BrokerProcessIpcUtils from broker to common, Fixes AB#3181005 Add a common implementation for the BrokerRestrictionsManager, Fixes AB#3181005 Apr 2, 2025
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.

3 participants