Skip to content

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

Closed
p3dr0rv wants to merge 22 commits intodevfrom
pedroro/moveRestrictionManager
Closed

[WIP] Add a common implementation for the BrokerRestrictionsManager, Fixes AB#3181005#2604
p3dr0rv wants to merge 22 commits intodevfrom
pedroro/moveRestrictionManager

Conversation

@p3dr0rv
Copy link
Copy Markdown
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

@github-actions
Copy link
Copy Markdown

❌ 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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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
@p3dr0rv p3dr0rv requested a review from a team as a code owner April 21, 2025 22:12
@p3dr0rv p3dr0rv changed the title Add a common implementation for the BrokerRestrictionsManager, Fixes AB#3181005 [WIP] Add a common implementation for the BrokerRestrictionsManager, Fixes AB#3181005 Apr 21, 2025
@p3dr0rv p3dr0rv marked this pull request as draft April 21, 2025 22:13
@p3dr0rv p3dr0rv closed this Apr 25, 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